From bc357208bb4f71201cfa62d1a67fae2ab7cb3b22 Mon Sep 17 00:00:00 2001 From: rav-melisono Date: Sun, 29 Mar 2026 15:31:29 +0100 Subject: [PATCH 01/32] feat: add HTTP health endpoint on gateway port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Binds a lightweight asyncio HTTP server on the configured gateway port (default 18790) alongside the existing agent and channel tasks. Endpoints: GET / -> "nanobot" (plain text, for service discovery) GET /health -> JSON with service, version, status, uptime, channels Zero new dependencies — uses asyncio.start_server. --- nanobot/cli/commands.py | 62 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/nanobot/cli/commands.py b/nanobot/cli/commands.py index cacb61ae..9ddb46d7 100644 --- a/nanobot/cli/commands.py +++ b/nanobot/cli/commands.py @@ -674,6 +674,67 @@ def gateway( console.print(f"[green]✓[/green] Heartbeat: every {hb_cfg.interval_s}s") + async def _health_server(host: str, health_port: int): + """Lightweight HTTP health endpoint on the gateway port.""" + import json as _json + import time + + start_time = time.monotonic() + + async def handle(reader, writer): + try: + data = await asyncio.wait_for(reader.read(4096), timeout=5) + except (asyncio.TimeoutError, ConnectionError): + writer.close() + return + + request_line = data.split(b"\r\n", 1)[0].decode("utf-8", errors="replace") + method, path = "", "" + parts = request_line.split(" ") + if len(parts) >= 2: + method, path = parts[0], parts[1] + + if method == "GET" and path == "/health": + uptime_s = int(time.monotonic() - start_time) + body = _json.dumps({ + "service": "nanobot", + "version": __version__, + "status": "running", + "uptime_seconds": uptime_s, + "channels": channels.enabled_channels, + }) + resp = ( + f"HTTP/1.0 200 OK\r\n" + f"Content-Type: application/json\r\n" + f"Content-Length: {len(body)}\r\n" + f"\r\n{body}" + ) + elif method == "GET" and path == "/": + body = "nanobot" + resp = ( + f"HTTP/1.0 200 OK\r\n" + f"Content-Type: text/plain\r\n" + f"Content-Length: {len(body)}\r\n" + f"\r\n{body}" + ) + else: + body = "Not Found" + resp = ( + f"HTTP/1.0 404 Not Found\r\n" + f"Content-Type: text/plain\r\n" + f"Content-Length: {len(body)}\r\n" + f"\r\n{body}" + ) + + writer.write(resp.encode()) + await writer.drain() + writer.close() + + server = await asyncio.start_server(handle, host, health_port) + console.print(f"[green]✓[/green] Health endpoint: http://{host}:{health_port}/health") + async with server: + await server.serve_forever() + async def run(): try: await cron.start() @@ -681,6 +742,7 @@ def gateway( await asyncio.gather( agent.run(), channels.start_all(), + _health_server(config.gateway.host, port), ) except KeyboardInterrupt: console.print("\nShutting down...") From a068df5a79c41798311121afd7b31db1c6b15049 Mon Sep 17 00:00:00 2001 From: dengjingren Date: Wed, 8 Apr 2026 15:28:36 +0800 Subject: [PATCH 02/32] feat(api): support file uploads via JSON base64 and multipart/form-data --- README.md | 39 ++++ nanobot/agent/context.py | 53 +++-- nanobot/agent/loop.py | 6 +- nanobot/api/server.py | 175 +++++++++++---- nanobot/utils/document.py | 206 +++++++++++++++++ pyproject.toml | 4 + tests/test_api_attachment.py | 379 ++++++++++++++++++++++++++++++++ tests/test_context_documents.py | 66 ++++++ tests/test_document_parsing.py | 276 +++++++++++++++++++++++ tests/test_openai_api.py | 49 ++++- 10 files changed, 1188 insertions(+), 65 deletions(-) create mode 100644 nanobot/utils/document.py create mode 100644 tests/test_api_attachment.py create mode 100644 tests/test_context_documents.py create mode 100644 tests/test_document_parsing.py diff --git a/README.md b/README.md index a2ea20f8..d7890b88 100644 --- a/README.md +++ b/README.md @@ -1757,6 +1757,7 @@ By default, the API binds to `127.0.0.1:8900`. You can change this in `config.js - Single-message input: each request must contain exactly one `user` message - Fixed model: omit `model`, or pass the same model shown by `/v1/models` - No streaming: `stream=true` is not supported +- **File uploads**: supports images, PDF, Word (.docx), Excel (.xlsx), PowerPoint (.pptx) via JSON base64 or `multipart/form-data` (max 10MB per file) ### Endpoints @@ -1775,6 +1776,44 @@ curl http://127.0.0.1:8900/v1/chat/completions \ }' ``` +### File Upload (JSON base64) + +Send images inline using the OpenAI multimodal content format: + +```bash +curl http://127.0.0.1:8900/v1/chat/completions \ + -H "Content-Type: application/json" \ + -d '{ + "messages": [{"role": "user", "content": [ + {"type": "text", "text": "Describe this image"}, + {"type": "image_url", "image_url": {"url": "data:image/png;base64,iVBOR..."}} + ]}] + }' +``` + +### File Upload (multipart/form-data) + +Upload any supported file type (images, PDF, Word, Excel, PPT) via multipart: + +```bash +# Single file +curl http://127.0.0.1:8900/v1/chat/completions \ + -F "message=Summarize this report" \ + -F "files=@report.docx" + +# Multiple files with session isolation +curl http://127.0.0.1:8900/v1/chat/completions \ + -F "message=Compare these files" \ + -F "files=@chart.png" \ + -F "files=@data.xlsx" \ + -F "session_id=my-session" +``` + +Supported file types: +- **Images**: PNG, JPEG, GIF, WebP (sent to AI as base64 for vision analysis) +- **Documents**: PDF, Word (.docx), Excel (.xlsx), PowerPoint (.pptx) (text extracted and sent to AI) +- **Text**: TXT, Markdown, CSV, JSON, etc. (read directly) + ### Python (`requests`) ```python diff --git a/nanobot/agent/context.py b/nanobot/agent/context.py index 3ac19e7f..5c0a8c80 100644 --- a/nanobot/agent/context.py +++ b/nanobot/agent/context.py @@ -144,31 +144,56 @@ class ContextBuilder: messages.append({"role": current_role, "content": merged}) return messages - def _build_user_content(self, text: str, media: list[str] | None) -> str | list[dict[str, Any]]: - """Build user message content with optional base64-encoded images.""" + def _build_user_content( + self, text: str, media: list[str] | None + ) -> str | list[dict[str, Any]]: + """Build user message content with optional media. + + Images are converted to base64 vision blocks. + Documents (PDF, Word, Excel, PPT) have their text extracted and appended. + """ if not media: return text - images = [] + images: list[dict[str, Any]] = [] + doc_texts: list[str] = [] + for path in media: p = Path(path) if not p.is_file(): continue raw = p.read_bytes() - # Detect real MIME type from magic bytes; fallback to filename guess mime = detect_image_mime(raw) or mimetypes.guess_type(path)[0] - if not mime or not mime.startswith("image/"): - continue - b64 = base64.b64encode(raw).decode() - images.append({ - "type": "image_url", - "image_url": {"url": f"data:{mime};base64,{b64}"}, - "_meta": {"path": str(p)}, - }) - if not images: + if mime and mime.startswith("image/"): + b64 = base64.b64encode(raw).decode() + images.append({ + "type": "image_url", + "image_url": {"url": f"data:{mime};base64,{b64}"}, + "_meta": {"path": str(p)}, + }) + else: + # Try document text extraction + from nanobot.utils.document import extract_text + extracted = extract_text(p) + if extracted and not extracted.startswith("Error"): + doc_texts.append(f"[File: {p.name}]\n{extracted}") + + # Build final content + parts: list[dict[str, Any]] = [] + parts.extend(images) + + combined_text = text + if doc_texts: + combined_text = text + "\n\n" + "\n\n".join(doc_texts) + + if images: + parts.append({"type": "text", "text": combined_text}) + return parts + elif doc_texts: + return combined_text + else: return text - return images + [{"type": "text", "text": text}] def add_tool_result( self, messages: list[dict[str, Any]], diff --git a/nanobot/agent/loop.py b/nanobot/agent/loop.py index 66d765d0..a3d0960f 100644 --- a/nanobot/agent/loop.py +++ b/nanobot/agent/loop.py @@ -765,13 +765,17 @@ class AgentLoop: session_key: str = "cli:direct", channel: str = "cli", chat_id: str = "direct", + media: list[str] | None = None, on_progress: Callable[[str], Awaitable[None]] | None = None, on_stream: Callable[[str], Awaitable[None]] | None = None, on_stream_end: Callable[..., Awaitable[None]] | None = None, ) -> OutboundMessage | None: """Process a message directly and return the outbound payload.""" await self._connect_mcp() - msg = InboundMessage(channel=channel, sender_id="user", chat_id=chat_id, content=content) + msg = InboundMessage( + channel=channel, sender_id="user", chat_id=chat_id, + content=content, media=media or [], + ) return await self._process_message( msg, session_key=session_key, on_progress=on_progress, on_stream=on_stream, on_stream_end=on_stream_end, diff --git a/nanobot/api/server.py b/nanobot/api/server.py index 2bfeddd0..8c9c9776 100644 --- a/nanobot/api/server.py +++ b/nanobot/api/server.py @@ -7,15 +7,28 @@ All requests route to a single persistent API session. from __future__ import annotations import asyncio +import base64 +import mimetypes +import re import time import uuid +from pathlib import Path from typing import Any from aiohttp import web from loguru import logger +from nanobot.config.paths import get_media_dir +from nanobot.utils.helpers import safe_filename from nanobot.utils.runtime import EMPTY_FINAL_RESPONSE_MESSAGE +MAX_FILE_SIZE = 10 * 1024 * 1024 # 10 MB +_DATA_URL_RE = re.compile(r"^data:([^;]+);base64,(.+)$", re.DOTALL) + + +class _FileSizeExceeded(Exception): + """Raised when an uploaded file exceeds the size limit.""" + API_SESSION_KEY = "api:default" API_CHAT_ID = "default" @@ -57,48 +70,134 @@ def _response_text(value: Any) -> str: return str(value) +# --------------------------------------------------------------------------- +# Upload helpers +# --------------------------------------------------------------------------- + +def _save_base64_data_url(data_url: str, media_dir: Path) -> str | None: + """Decode a data:...;base64,... URL and save to disk.""" + m = _DATA_URL_RE.match(data_url) + if not m: + return None + mime_type, b64_payload = m.group(1), m.group(2) + try: + raw = base64.b64decode(b64_payload) + except Exception: + return None + ext = mimetypes.guess_extension(mime_type) or ".bin" + filename = f"{uuid.uuid4().hex[:12]}{ext}" + dest = media_dir / safe_filename(filename) + dest.write_bytes(raw) + return str(dest) + + +def _parse_json_content(body: dict) -> tuple[str, list[str]]: + """Parse JSON request body. Returns (text, media_paths).""" + messages = body.get("messages") + if not isinstance(messages, list) or len(messages) != 1: + raise ValueError("Only a single user message is supported") + message = messages[0] + if not isinstance(message, dict) or message.get("role") != "user": + raise ValueError("Only a single user message is supported") + + user_content = message.get("content", "") + media_dir = get_media_dir("api") + media_paths: list[str] = [] + + if isinstance(user_content, list): + text_parts: list[str] = [] + for part in user_content: + if not isinstance(part, dict): + continue + if part.get("type") == "text": + text_parts.append(part.get("text", "")) + elif part.get("type") == "image_url": + url = part.get("image_url", {}).get("url", "") + if url.startswith("data:"): + saved = _save_base64_data_url(url, media_dir) + if saved: + media_paths.append(saved) + text = " ".join(text_parts) + elif isinstance(user_content, str): + text = user_content + else: + raise ValueError("Invalid content format") + + return text, media_paths + + +async def _parse_multipart(request: web.Request) -> tuple[str, list[str], str | None]: + """Parse multipart/form-data. Returns (text, media_paths, session_id).""" + media_dir = get_media_dir("api") + reader = await request.multipart() + text = "" + session_id = None + media_paths: list[str] = [] + + while True: + part = await reader.next() + if part is None: + break + if part.name == "message": + text = (await part.read()).decode("utf-8") + elif part.name == "session_id": + session_id = (await part.read()).decode("utf-8").strip() + elif part.name == "files": + raw = await part.read() + if len(raw) > MAX_FILE_SIZE: + raise _FileSizeExceeded(f"File '{part.filename}' exceeds {MAX_FILE_SIZE // (1024*1024)}MB limit") + filename = safe_filename(part.filename or f"{uuid.uuid4().hex[:12]}.bin") + dest = media_dir / filename + dest.write_bytes(raw) + media_paths.append(str(dest)) + + if not text: + text = "请分析上传的文件" + + return text, media_paths, session_id + + # --------------------------------------------------------------------------- # Route handlers # --------------------------------------------------------------------------- async def handle_chat_completions(request: web.Request) -> web.Response: - """POST /v1/chat/completions""" - - # --- Parse body --- - try: - body = await request.json() - except Exception: - return _error_json(400, "Invalid JSON body") - - messages = body.get("messages") - if not isinstance(messages, list) or len(messages) != 1: - return _error_json(400, "Only a single user message is supported") - - # Stream not yet supported - if body.get("stream", False): - return _error_json(400, "stream=true is not supported yet. Set stream=false or omit it.") - - message = messages[0] - if not isinstance(message, dict) or message.get("role") != "user": - return _error_json(400, "Only a single user message is supported") - user_content = message.get("content", "") - if isinstance(user_content, list): - # Multi-modal content array — extract text parts - user_content = " ".join( - part.get("text", "") for part in user_content if part.get("type") == "text" - ) + """POST /v1/chat/completions — supports JSON and multipart/form-data.""" + content_type = request.content_type or "" + if not isinstance(content_type, str): + content_type = "" agent_loop = request.app["agent_loop"] timeout_s: float = request.app.get("request_timeout", 120.0) model_name: str = request.app.get("model_name", "nanobot") - if (requested_model := body.get("model")) and requested_model != model_name: - return _error_json(400, f"Only configured model '{model_name}' is available") - session_key = f"api:{body['session_id']}" if body.get("session_id") else API_SESSION_KEY + try: + if content_type.startswith("multipart/"): + text, media_paths, session_id = await _parse_multipart(request) + else: + try: + body = await request.json() + except Exception: + return _error_json(400, "Invalid JSON body") + if body.get("stream", False): + return _error_json(400, "stream=true is not supported yet. Set stream=false or omit it.") + if (requested_model := body.get("model")) and requested_model != model_name: + return _error_json(400, f"Only configured model '{model_name}' is available") + text, media_paths = _parse_json_content(body) + session_id = body.get("session_id") + except ValueError as e: + return _error_json(400, str(e)) + except _FileSizeExceeded as e: + return _error_json(413, str(e), err_type="invalid_request_error") + except Exception: + logger.exception("Error parsing upload") + return _error_json(413, "File too large or invalid upload") + + session_key = f"api:{session_id}" if session_id else API_SESSION_KEY session_locks: dict[str, asyncio.Lock] = request.app["session_locks"] session_lock = session_locks.setdefault(session_key, asyncio.Lock()) - logger.info("API request session_key={} content={}", session_key, user_content[:80]) + logger.info("API request session_key={} media={} text={}", session_key, len(media_paths), text[:80]) _FALLBACK = EMPTY_FINAL_RESPONSE_MESSAGE @@ -107,7 +206,8 @@ async def handle_chat_completions(request: web.Request) -> web.Response: try: response = await asyncio.wait_for( agent_loop.process_direct( - content=user_content, + content=text, + media=media_paths if media_paths else None, session_key=session_key, channel="api", chat_id=API_CHAT_ID, @@ -117,13 +217,11 @@ async def handle_chat_completions(request: web.Request) -> web.Response: response_text = _response_text(response) if not response_text or not response_text.strip(): - logger.warning( - "Empty response for session {}, retrying", - session_key, - ) + logger.warning("Empty response for session {}, retrying", session_key) retry_response = await asyncio.wait_for( agent_loop.process_direct( - content=user_content, + content=text, + media=media_paths if media_paths else None, session_key=session_key, channel="api", chat_id=API_CHAT_ID, @@ -132,10 +230,7 @@ async def handle_chat_completions(request: web.Request) -> web.Response: ) response_text = _response_text(retry_response) if not response_text or not response_text.strip(): - logger.warning( - "Empty response after retry for session {}, using fallback", - session_key, - ) + logger.warning("Empty response after retry, using fallback") response_text = _FALLBACK except asyncio.TimeoutError: @@ -183,7 +278,7 @@ def create_app(agent_loop, model_name: str = "nanobot", request_timeout: float = model_name: Model name reported in responses. request_timeout: Per-request timeout in seconds. """ - app = web.Application() + app = web.Application(client_max_size=20 * 1024 * 1024) # 20MB for base64 images app["agent_loop"] = agent_loop app["model_name"] = model_name app["request_timeout"] = request_timeout diff --git a/nanobot/utils/document.py b/nanobot/utils/document.py new file mode 100644 index 00000000..23e8eeee --- /dev/null +++ b/nanobot/utils/document.py @@ -0,0 +1,206 @@ +"""Document text extraction utilities for nanobot.""" + +from pathlib import Path + +from loguru import logger + +try: + from pypdf import PdfReader +except ImportError: + PdfReader = None # type: ignore + +try: + from docx import Document as DocxDocument +except ImportError: + DocxDocument = None # type: ignore + +try: + from openpyxl import load_workbook +except ImportError: + load_workbook = None # type: ignore + +try: + from pptx import Presentation as PptxPresentation +except ImportError: + PptxPresentation = None # type: ignore + + +# Supported file extensions for text extraction +SUPPORTED_EXTENSIONS: set[str] = { + # Document formats + ".pdf", + ".docx", + ".xlsx", + ".pptx", + # Text formats + ".txt", + ".md", + ".csv", + ".json", + ".xml", + ".html", + ".htm", + ".log", + ".yaml", + ".yml", + ".toml", + ".ini", + ".cfg", + # Image formats (for future OCR support) + ".png", + ".jpg", + ".jpeg", + ".gif", + ".webp", +} + +_MAX_TEXT_LENGTH = 200_000 + + +def extract_text(path: Path) -> str | None: + """Extract text from a file. + + Args: + path: Path to the file. + + Returns: + Extracted text as string, None for unsupported types, + or error string for failures. + """ + if not isinstance(path, Path): + path = Path(path) + + if not path.exists(): + return f"[error: file not found: {path}]" + + ext = path.suffix.lower() + + # Document formats + if ext == ".pdf": + if PdfReader is None: + return "[error: pypdf not installed]" + return _extract_pdf(path) + elif ext == ".docx": + if DocxDocument is None: + return "[error: python-docx not installed]" + return _extract_docx(path) + elif ext == ".xlsx": + if load_workbook is None: + return "[error: openpyxl not installed]" + return _extract_xlsx(path) + elif ext == ".pptx": + if PptxPresentation is None: + return "[error: python-pptx not installed]" + return _extract_pptx(path) + elif _is_text_extension(ext): + return _extract_text_file(path) + elif ext in {".png", ".jpg", ".jpeg", ".gif", ".webp"}: + # Image files - for future OCR support + return f"[image: {path.name}]" + else: + # Unsupported extension + return None + + +def _extract_pdf(path: Path) -> str: + """Extract text from PDF using pypdf.""" + try: + reader = PdfReader(path) + pages: list[str] = [] + for i, page in enumerate(reader.pages, 1): + text = page.extract_text() or "" + pages.append(f"--- Page {i} ---\n{text}") + return _truncate("\n\n".join(pages), _MAX_TEXT_LENGTH) + except Exception as e: + logger.error("Failed to extract PDF {}: {}", path, e) + return f"[error: failed to extract PDF: {e!s}]" + + +def _extract_docx(path: Path) -> str: + """Extract text from DOCX using python-docx.""" + try: + doc = DocxDocument(path) + paragraphs: list[str] = [p.text for p in doc.paragraphs if p.text.strip()] + return _truncate("\n\n".join(paragraphs), _MAX_TEXT_LENGTH) + except Exception as e: + logger.error("Failed to extract DOCX {}: {}", path, e) + return f"[error: failed to extract DOCX: {e!s}]" + + +def _extract_xlsx(path: Path) -> str: + """Extract text from XLSX using openpyxl.""" + try: + wb = load_workbook(path, read_only=True, data_only=True) + sheets: list[str] = [] + for sheet_name in wb.sheetnames: + ws = wb[sheet_name] + rows: list[str] = [] + for row in ws.iter_rows(values_only=True): + row_text = "\t".join(str(cell) if cell is not None else "" for cell in row) + if row_text.strip(): + rows.append(row_text) + if rows: + sheets.append(f"--- Sheet: {sheet_name} ---\n" + "\n".join(rows)) + wb.close() + return _truncate("\n\n".join(sheets), _MAX_TEXT_LENGTH) + except Exception as e: + logger.error("Failed to extract XLSX {}: {}", path, e) + return f"[error: failed to extract XLSX: {e!s}]" + + +def _extract_pptx(path: Path) -> str: + """Extract text from PPTX using python-pptx.""" + try: + prs = PptxPresentation(path) + slides: list[str] = [] + for i, slide in enumerate(prs.slides, 1): + slide_text: list[str] = [] + for shape in slide.shapes: + if hasattr(shape, "text") and shape.text: + slide_text.append(shape.text) + if slide_text: + slides.append(f"--- Slide {i} ---\n" + "\n".join(slide_text)) + return _truncate("\n\n".join(slides), _MAX_TEXT_LENGTH) + except Exception as e: + logger.error("Failed to extract PPTX {}: {}", path, e) + return f"[error: failed to extract PPTX: {e!s}]" + + +def _extract_text_file(path: Path) -> str: + """Extract text from a plain text file.""" + try: + # Try UTF-8 first, then latin-1 fallback + try: + content = path.read_text(encoding="utf-8") + except UnicodeDecodeError: + content = path.read_text(encoding="latin-1") + return _truncate(content, _MAX_TEXT_LENGTH) + except Exception as e: + logger.error("Failed to read text file {}: {}", path, e) + return f"[error: failed to read file: {e!s}]" + + +def _truncate(text: str, max_length: int) -> str: + """Truncate text with a suffix indicating truncation.""" + if len(text) <= max_length: + return text + return text[:max_length] + f"... (truncated, {len(text)} chars total)" + + +def _is_text_extension(ext: str) -> bool: + """Check if extension is a text format.""" + return ext in { + ".txt", + ".md", + ".csv", + ".json", + ".xml", + ".html", + ".htm", + ".log", + ".yaml", + ".yml", + ".toml", + ".ini", + ".cfg", + } diff --git a/pyproject.toml b/pyproject.toml index a5807f96..290d06b2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -50,6 +50,10 @@ dependencies = [ "tiktoken>=0.12.0,<1.0.0", "jinja2>=3.1.0,<4.0.0", "dulwich>=0.22.0,<1.0.0", + "pypdf>=5.0.0,<6.0.0", + "python-docx>=1.1.0,<2.0.0", + "openpyxl>=3.1.0,<4.0.0", + "python-pptx>=1.0.0,<2.0.0", ] [project.optional-dependencies] diff --git a/tests/test_api_attachment.py b/tests/test_api_attachment.py new file mode 100644 index 00000000..9b29f3cb --- /dev/null +++ b/tests/test_api_attachment.py @@ -0,0 +1,379 @@ +"""Tests for API file upload functionality (JSON base64 + multipart).""" + +from __future__ import annotations + +import base64 +from io import BytesIO +from unittest.mock import AsyncMock, MagicMock + +import pytest +import pytest_asyncio + +from nanobot.api.server import ( + API_CHAT_ID, + API_SESSION_KEY, + _parse_json_content, + _save_base64_data_url, + create_app, +) + +try: + from aiohttp.test_utils import TestClient, TestServer + + HAS_AIOHTTP = True +except ImportError: + HAS_AIOHTTP = False + +pytest_plugins = ("pytest_asyncio",) + + +def _make_mock_agent(response_text: str = "mock response") -> MagicMock: + agent = MagicMock() + agent.process_direct = AsyncMock(return_value=response_text) + agent._connect_mcp = AsyncMock() + agent.close_mcp = AsyncMock() + return agent + + +@pytest.fixture +def mock_agent(): + return _make_mock_agent() + + +@pytest.fixture +def app(mock_agent): + return create_app(mock_agent, model_name="test-model", request_timeout=10.0) + + +@pytest_asyncio.fixture +async def aiohttp_client(): + clients: list[TestClient] = [] + + async def _make_client(app): + client = TestClient(TestServer(app)) + await client.start_server() + clients.append(client) + return client + + try: + yield _make_client + finally: + for client in clients: + await client.close() + + +# --------------------------------------------------------------------------- +# Helper function tests +# --------------------------------------------------------------------------- + +def test_save_base64_data_url_saves_png(tmp_path) -> None: + """Saving a base64 data URL creates a file with correct extension.""" + b64_data = base64.b64encode(b"fake png data").decode() + data_url = f"data:image/png;base64,{b64_data}" + result = _save_base64_data_url(data_url, tmp_path) + assert result is not None + assert result.endswith(".png") + assert (tmp_path / result.replace(str(tmp_path) + "/", "")).read_bytes() == b"fake png data" + + +def test_save_base64_data_url_handles_invalid_b64(tmp_path) -> None: + """Invalid base64 returns None.""" + result = _save_base64_data_url("data:image/png;base64,not-valid-base64!!!", tmp_path) + assert result is None + + +def test_save_base64_data_url_handles_unknown_mime(tmp_path) -> None: + """Unknown MIME type defaults to .bin.""" + b64_data = base64.b64encode(b"some data").decode() + data_url = f"data:unknown/type;base64,{b64_data}" + result = _save_base64_data_url(data_url, tmp_path) + assert result is not None + assert result.endswith(".bin") + + +def test_parse_json_content_extracts_text_and_media(tmp_path) -> None: + """Parse JSON with text + base64 image saves image and returns paths.""" + b64_data = base64.b64encode(b"img").decode() + body = { + "messages": [ + { + "role": "user", + "content": [ + {"type": "text", "text": "describe this"}, + {"type": "image_url", "image_url": {"url": f"data:image/png;base64,{b64_data}"}}, + ], + } + ] + } + import os + original_cwd = os.getcwd() + os.chdir(tmp_path) + + try: + text, media_paths = _parse_json_content(body) + assert text == "describe this" + assert len(media_paths) == 1 + finally: + os.chdir(original_cwd) + + +def test_parse_json_content_plain_text_only() -> None: + """Plain text string content returns no media.""" + body = {"messages": [{"role": "user", "content": "hello"}]} + text, media_paths = _parse_json_content(body) + assert text == "hello" + assert media_paths == [] + + +def test_parse_json_content_validates_single_message() -> None: + """Multiple messages raise ValueError.""" + body = { + "messages": [ + {"role": "user", "content": "first"}, + {"role": "user", "content": "second"}, + ] + } + with pytest.raises(ValueError, match="single user message"): + _parse_json_content(body) + + +def test_parse_json_content_validates_user_role() -> None: + """Non-user role raises ValueError.""" + body = {"messages": [{"role": "system", "content": "you are a bot"}]} + with pytest.raises(ValueError, match="single user message"): + _parse_json_content(body) + + +# --------------------------------------------------------------------------- +# Multipart upload tests +# --------------------------------------------------------------------------- + +@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") +@pytest.mark.asyncio +async def test_multipart_upload_saves_file(aiohttp_client, mock_agent, tmp_path) -> None: + """Multipart upload saves file to media dir and passes path to process_direct.""" + import os + original_cwd = os.getcwd() + os.chdir(tmp_path) + + try: + app = create_app(mock_agent, model_name="m") + client = await aiohttp_client(app) + + file_data = b"test file content" + data = BytesIO(file_data) + + resp = await client.post( + "/v1/chat/completions", + data={"message": "analyze this", "files": data}, + ) + assert resp.status == 200 + call_kwargs = mock_agent.process_direct.call_args.kwargs + assert call_kwargs["content"] == "analyze this" + assert len(call_kwargs.get("media", [])) == 1 + finally: + os.chdir(original_cwd) + + +@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") +@pytest.mark.asyncio +async def test_multipart_multiple_files(aiohttp_client, mock_agent, tmp_path) -> None: + """Multipart upload with multiple files saves all and passes paths.""" + import os + original_cwd = os.getcwd() + os.chdir(tmp_path) + + try: + app = create_app(mock_agent, model_name="m") + client = await aiohttp_client(app) + + # Note: aiohttp test client has limited multipart support + # This test verifies the basic flow + file_data = b"test content" + data = BytesIO(file_data) + + resp = await client.post( + "/v1/chat/completions", + data={"message": "analyze", "files": data}, + ) + assert resp.status == 200 + finally: + os.chdir(original_cwd) + + +@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") +@pytest.mark.asyncio +async def test_multipart_file_size_limit(aiohttp_client, mock_agent, tmp_path) -> None: + """File exceeding MAX_FILE_SIZE returns 413.""" + import os + original_cwd = os.getcwd() + os.chdir(tmp_path) + + try: + app = create_app(mock_agent, model_name="m") + client = await aiohttp_client(app) + + # Create a file larger than 10MB + large_data = b"x" * (11 * 1024 * 1024) + data = BytesIO(large_data) + + resp = await client.post( + "/v1/chat/completions", + data={"message": "analyze", "files": data}, + ) + assert resp.status == 413 + finally: + os.chdir(original_cwd) + + +@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") +@pytest.mark.asyncio +async def test_multipart_defaults_text_when_missing(aiohttp_client, mock_agent, tmp_path) -> None: + """Multipart without message field uses default text.""" + import os + original_cwd = os.getcwd() + os.chdir(tmp_path) + + try: + app = create_app(mock_agent, model_name="m") + client = await aiohttp_client(app) + + file_data = b"content" + data = BytesIO(file_data) + + resp = await client.post( + "/v1/chat/completions", + data={"files": data}, + ) + assert resp.status == 200 + call_kwargs = mock_agent.process_direct.call_args.kwargs + assert call_kwargs["content"] == "请分析上传的文件" + finally: + os.chdir(original_cwd) + + +@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") +@pytest.mark.asyncio +async def test_multipart_with_session_id(aiohttp_client, mock_agent, tmp_path) -> None: + """Multipart upload with session_id uses custom session key.""" + import os + original_cwd = os.getcwd() + os.chdir(tmp_path) + + try: + app = create_app(mock_agent, model_name="m") + client = await aiohttp_client(app) + + file_data = b"content" + data = BytesIO(file_data) + + resp = await client.post( + "/v1/chat/completions", + data={"message": "hello", "session_id": "my-session", "files": data}, + ) + assert resp.status == 200 + call_kwargs = mock_agent.process_direct.call_args.kwargs + assert call_kwargs["session_key"] == "api:my-session" + finally: + os.chdir(original_cwd) + + +# --------------------------------------------------------------------------- +# Backward compatibility tests +# --------------------------------------------------------------------------- + +@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") +@pytest.mark.asyncio +async def test_plain_text_backward_compat(aiohttp_client, mock_agent) -> None: + """Plain text JSON request (no media) works as before.""" + app = create_app(mock_agent, model_name="m") + client = await aiohttp_client(app) + resp = await client.post( + "/v1/chat/completions", + json={"messages": [{"role": "user", "content": "hello world"}]}, + ) + assert resp.status == 200 + body = await resp.json() + assert body["choices"][0]["message"]["content"] == "mock response" + call_kwargs = mock_agent.process_direct.call_args.kwargs + assert call_kwargs["content"] == "hello world" + assert call_kwargs.get("media") is None + + +@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") +@pytest.mark.asyncio +async def test_json_base64_image_upload(aiohttp_client, mock_agent, tmp_path) -> None: + """JSON request with base64 data URL saves file and passes path.""" + import os + original_cwd = os.getcwd() + os.chdir(tmp_path) + + try: + app = create_app(mock_agent, model_name="m") + client = await aiohttp_client(app) + + # Use valid base64 for a tiny PNG (1x1 transparent pixel) + tiny_png_b64 = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8DwHwAFBQIAX8jx0gAAAABJRU5ErkJggg==" + + resp = await client.post( + "/v1/chat/completions", + json={ + "messages": [ + { + "role": "user", + "content": [ + {"type": "text", "text": "what is this"}, + {"type": "image_url", "image_url": {"url": f"data:image/png;base64,{tiny_png_b64}"}}, + ], + } + ] + }, + ) + assert resp.status == 200 + call_kwargs = mock_agent.process_direct.call_args.kwargs + assert call_kwargs["content"] == "what is this" + assert len(call_kwargs.get("media", [])) == 1 + finally: + os.chdir(original_cwd) + + +# --------------------------------------------------------------------------- +# DOCX document extraction tests +# --------------------------------------------------------------------------- + +@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") +@pytest.mark.asyncio +async def test_docx_upload_extracted_and_sent(aiohttp_client, tmp_path) -> None: + """Uploaded DOCX should have its text extracted before being sent to AI.""" + from docx import Document + + agent = _make_mock_agent("This report shows $5M revenue") + import os + original_cwd = os.getcwd() + os.chdir(tmp_path) + + try: + app = create_app(agent, model_name="m") + client = await aiohttp_client(app) + + doc = Document() + doc.add_heading("Q1 Report", level=1) + doc.add_paragraph("Total revenue: $5,000,000") + buf = BytesIO() + doc.save(buf) + docx_bytes = buf.getvalue() + + import aiohttp + data = aiohttp.FormData() + data.add_field("message", "summarize the report") + data.add_field("files", docx_bytes, filename="report.docx", + content_type="application/vnd.openxmlformats-officedocument.wordprocessingml.document") + + resp = await client.post("/v1/chat/completions", data=data) + assert resp.status == 200 + call_kwargs = agent.process_direct.call_args.kwargs + media = call_kwargs.get("media", []) + assert len(media) == 1 + assert "report.docx" in media[0] + finally: + os.chdir(original_cwd) diff --git a/tests/test_context_documents.py b/tests/test_context_documents.py new file mode 100644 index 00000000..b6053f35 --- /dev/null +++ b/tests/test_context_documents.py @@ -0,0 +1,66 @@ +"""Tests for context builder document handling.""" + +from __future__ import annotations + +import pytest +from pathlib import Path + +from nanobot.agent.context import ContextBuilder + + +def _make_builder(tmp_path: Path) -> ContextBuilder: + """Create a minimal ContextBuilder for testing.""" + return ContextBuilder(workspace=tmp_path, timezone="UTC") + + +def test_build_user_content_with_no_media_returns_string(tmp_path: Path) -> None: + builder = _make_builder(tmp_path) + result = builder._build_user_content("hello", None) + assert result == "hello" + + +def test_build_user_content_with_image_returns_list(tmp_path: Path) -> None: + """Image files should produce base64 content blocks.""" + builder = _make_builder(tmp_path) + png = tmp_path / "test.png" + png.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) + result = builder._build_user_content("describe this", [str(png)]) + assert isinstance(result, list) + types = [b["type"] for b in result] + assert "image_url" in types + assert "text" in types + + +def test_build_user_content_with_docx_includes_extracted_text(tmp_path: Path) -> None: + """Document files should have their text extracted and included.""" + from docx import Document + + doc = Document() + doc.add_paragraph("Quarterly revenue is $5M") + docx_path = tmp_path / "report.docx" + doc.save(docx_path) + + builder = _make_builder(tmp_path) + result = builder._build_user_content("summarize this", [str(docx_path)]) + assert isinstance(result, str) + assert "Quarterly revenue" in result + + +def test_build_user_content_mixed_image_and_document(tmp_path: Path) -> None: + """Mix of images and documents: images as base64, docs as text.""" + from docx import Document + + png = tmp_path / "chart.png" + png.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) + + doc = Document() + doc.add_paragraph("Report text here") + docx = tmp_path / "report.docx" + doc.save(docx) + + builder = _make_builder(tmp_path) + result = builder._build_user_content("analyze both", [str(png), str(docx)]) + assert isinstance(result, list) + assert any(b["type"] == "image_url" for b in result) + text_parts = [b.get("text", "") for b in result if b.get("type") == "text"] + assert any("Report text here" in t for t in text_parts) diff --git a/tests/test_document_parsing.py b/tests/test_document_parsing.py new file mode 100644 index 00000000..a23c0db1 --- /dev/null +++ b/tests/test_document_parsing.py @@ -0,0 +1,276 @@ +"""Tests for document text extraction utilities.""" + +import io +from pathlib import Path + +import pytest + +from nanobot.utils.document import ( + SUPPORTED_EXTENSIONS, + _is_text_extension, + extract_text, +) + + +class TestSupportedExtensions: + """Test the SUPPORTED_EXTENSIONS constant.""" + + def test_supported_extensions_include_common_formats(self): + """Test that common document formats are included.""" + # Document formats + assert ".pdf" in SUPPORTED_EXTENSIONS + assert ".docx" in SUPPORTED_EXTENSIONS + assert ".xlsx" in SUPPORTED_EXTENSIONS + assert ".pptx" in SUPPORTED_EXTENSIONS + + # Text formats + assert ".txt" in SUPPORTED_EXTENSIONS + assert ".md" in SUPPORTED_EXTENSIONS + assert ".csv" in SUPPORTED_EXTENSIONS + assert ".json" in SUPPORTED_EXTENSIONS + assert ".yaml" in SUPPORTED_EXTENSIONS + assert ".yml" in SUPPORTED_EXTENSIONS + + # Image formats + assert ".png" in SUPPORTED_EXTENSIONS + assert ".jpg" in SUPPORTED_EXTENSIONS + assert ".jpeg" in SUPPORTED_EXTENSIONS + + +class TestExtractText: + """Test the extract_text function.""" + + def test_extract_text_unsupported_returns_none(self, tmp_path: Path): + """Test that unsupported file types return None.""" + unsupported_file = tmp_path / "file.xyz" + unsupported_file.write_text("content") + + result = extract_text(unsupported_file) + assert result is None + + def test_extract_text_file_not_found(self, tmp_path: Path): + """Test that non-existent files return error string.""" + missing_file = tmp_path / "nonexistent.txt" + + result = extract_text(missing_file) + assert result is not None + assert "[error: file not found:" in result + + def test_extract_text_txt_file(self, tmp_path: Path): + """Test extracting text from a .txt file.""" + txt_file = tmp_path / "test.txt" + content = "Hello, world!\nThis is a test." + txt_file.write_text(content, encoding="utf-8") + + result = extract_text(txt_file) + assert result == content + + def test_extract_text_txt_file_with_truncation(self, tmp_path: Path): + """Test that large text files are truncated.""" + txt_file = tmp_path / "large.txt" + # Create content larger than _MAX_TEXT_LENGTH + content = "x" * 300_000 + txt_file.write_text(content, encoding="utf-8") + + result = extract_text(txt_file) + assert len(result) < 300_000 + assert "(truncated," in result + assert "chars total)" in result + + def test_extract_text_md_file(self, tmp_path: Path): + """Test extracting text from a .md file.""" + md_file = tmp_path / "test.md" + content = "# Header\n\nSome markdown content." + md_file.write_text(content, encoding="utf-8") + + result = extract_text(md_file) + assert result == content + + def test_extract_text_csv_file(self, tmp_path: Path): + """Test extracting text from a .csv file.""" + csv_file = tmp_path / "test.csv" + content = "name,age\nAlice,30\nBob,25" + csv_file.write_text(content, encoding="utf-8") + + result = extract_text(csv_file) + assert result == content + + def test_extract_text_json_file(self, tmp_path: Path): + """Test extracting text from a .json file.""" + json_file = tmp_path / "test.json" + content = '{"key": "value", "number": 42}' + json_file.write_text(content, encoding="utf-8") + + result = extract_text(json_file) + assert result == content + + def test_extract_text_xlsx(self, tmp_path: Path): + """Test extracting text from an .xlsx file.""" + from openpyxl import Workbook + + xlsx_file = tmp_path / "test.xlsx" + wb = Workbook() + ws = wb.active + ws.title = "Sheet1" + ws["A1"] = "Name" + ws["B1"] = "Age" + ws["A2"] = "Alice" + ws["B2"] = 30 + ws["A3"] = "Bob" + ws["B3"] = 25 + + # Add a second sheet + ws2 = wb.create_sheet("Sheet2") + ws2["A1"] = "Product" + ws2["B1"] = "Price" + ws2["A2"] = "Widget" + ws2["B2"] = 9.99 + + wb.save(xlsx_file) + wb.close() + + result = extract_text(xlsx_file) + assert result is not None + assert "--- Sheet: Sheet1 ---" in result + assert "--- Sheet: Sheet2 ---" in result + assert "Alice" in result + assert "Bob" in result + assert "Widget" in result + assert "9.99" in result + + def test_extract_text_xlsx_empty_sheet(self, tmp_path: Path): + """Test extracting text from an .xlsx file with empty sheets.""" + from openpyxl import Workbook + + xlsx_file = tmp_path / "empty.xlsx" + wb = Workbook() + # Clear the default sheet + wb.remove(wb.active) + # Add an empty sheet + wb.create_sheet("EmptySheet") + wb.save(xlsx_file) + wb.close() + + result = extract_text(xlsx_file) + # Empty sheets should return empty string or header only + assert result == "--- Sheet: EmptySheet ---" or result == "" + + def test_extract_text_docx(self, tmp_path: Path): + """Test extracting text from a .docx file.""" + from docx import Document + + docx_file = tmp_path / "test.docx" + doc = Document() + doc.add_heading("Test Document", 0) + doc.add_paragraph("This is paragraph one.") + doc.add_paragraph("This is paragraph two.") + doc.save(docx_file) + + result = extract_text(docx_file) + assert result is not None + assert "Test Document" in result + assert "This is paragraph one." in result + assert "This is paragraph two." in result + + def test_extract_text_docx_empty(self, tmp_path: Path): + """Test extracting text from an empty .docx file.""" + from docx import Document + + docx_file = tmp_path / "empty.docx" + doc = Document() + doc.save(docx_file) + + result = extract_text(docx_file) + assert result == "" + + def test_extract_text_pptx(self, tmp_path: Path): + """Test extracting text from a .pptx file.""" + from pptx import Presentation + + pptx_file = tmp_path / "test.pptx" + prs = Presentation() + + # Slide 1 + slide1 = prs.slides.add_slide(prs.slide_layouts[0]) + for shape in slide1.shapes: + if hasattr(shape, "text"): + shape.text = "First Slide Title" + + # Slide 2 + slide2 = prs.slides.add_slide(prs.slide_layouts[5]) + left = top = width = height = 1000000 + textbox = slide2.shapes.add_textbox(left, top, width, height) + text_frame = textbox.text_frame + text_frame.text = "Bullet point content" + + prs.save(pptx_file) + + result = extract_text(pptx_file) + assert result is not None + assert "--- Slide 1 ---" in result + assert "--- Slide 2 ---" in result + # Text content may vary depending on PowerPoint layout defaults + assert len(result) > 0 + + def test_extract_text_pdf_not_found(self, tmp_path: Path): + """Test that missing PDF files return error string.""" + missing_pdf = tmp_path / "nonexistent.pdf" + + result = extract_text(missing_pdf) + assert result is not None + assert "[error: file not found:" in result + + def test_extract_text_image_files(self, tmp_path: Path): + """Test that image files return placeholder text.""" + # Create a minimal PNG file (1x1 pixel) + png_file = tmp_path / "test.png" + # Minimal valid PNG: 8-byte signature + IHDR + IDAT + IEND + png_data = ( + b"\x89PNG\r\n\x1a\n" + b"\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01" + b"\x08\x02\x00\x00\x00\x90wS\xde" + b"\x00\x00\x00\x0cIDATx\x9cc\x00\x01\x00\x00\x05\x00\x01" + b"\r\n-\xb4\x00\x00\x00\x00IEND\xaeB`\x82" + ) + png_file.write_bytes(png_data) + + result = extract_text(png_file) + assert result is not None + assert "[image:" in result + assert "test.png" in result + + +class TestIsTextExtension: + """Test the _is_text_extension helper.""" + + def test_text_extensions_return_true(self): + """Test that known text extensions return True.""" + assert _is_text_extension(".txt") is True + assert _is_text_extension(".md") is True + assert _is_text_extension(".csv") is True + assert _is_text_extension(".json") is True + assert _is_text_extension(".yaml") is True + assert _is_text_extension(".yml") is True + assert _is_text_extension(".xml") is True + assert _is_text_extension(".html") is True + assert _is_text_extension(".htm") is True + + def test_non_text_extensions_return_false(self): + """Test that non-text extensions return False.""" + assert _is_text_extension(".pdf") is False + assert _is_text_extension(".docx") is False + assert _is_text_extension(".xlsx") is False + assert _is_text_extension(".pptx") is False + assert _is_text_extension(".png") is False + assert _is_text_extension(".xyz") is False + + def test_case_sensitivity(self): + """Test that _is_text_extension requires lowercase extension. + + Note: The main extract_text function handles case-insensitivity by + converting extensions to lowercase before calling _is_text_extension. + """ + # _is_text_extension itself is case-sensitive (lowercase only) + assert _is_text_extension(".txt") is True + assert _is_text_extension(".TXT") is False + assert _is_text_extension(".pdf") is False diff --git a/tests/test_openai_api.py b/tests/test_openai_api.py index 2d4ae858..a6d019da 100644 --- a/tests/test_openai_api.py +++ b/tests/test_openai_api.py @@ -194,6 +194,7 @@ async def test_successful_request_uses_fixed_api_session(aiohttp_client, mock_ag assert body["model"] == "test-model" mock_agent.process_direct.assert_called_once_with( content="hello", + media=None, session_key=API_SESSION_KEY, channel="api", chat_id=API_CHAT_ID, @@ -205,7 +206,7 @@ async def test_successful_request_uses_fixed_api_session(aiohttp_client, mock_ag async def test_followup_requests_share_same_session_key(aiohttp_client) -> None: call_log: list[str] = [] - async def fake_process(content, session_key="", channel="", chat_id=""): + async def fake_process(content, session_key="", channel="", chat_id="", **kwargs): call_log.append(session_key) return f"reply to {content}" @@ -236,7 +237,7 @@ async def test_followup_requests_share_same_session_key(aiohttp_client) -> None: async def test_fixed_session_requests_are_serialized(aiohttp_client) -> None: order: list[str] = [] - async def slow_process(content, session_key="", channel="", chat_id=""): + async def slow_process(content, session_key="", channel="", chat_id="", **kwargs): order.append(f"start:{content}") await asyncio.sleep(0.1) order.append(f"end:{content}") @@ -307,12 +308,12 @@ async def test_multimodal_content_extracts_text(aiohttp_client, mock_agent) -> N }, ) assert resp.status == 200 - mock_agent.process_direct.assert_called_once_with( - content="describe this", - session_key=API_SESSION_KEY, - channel="api", - chat_id=API_CHAT_ID, - ) + call_kwargs = mock_agent.process_direct.call_args.kwargs + assert call_kwargs["content"] == "describe this" + assert call_kwargs["session_key"] == API_SESSION_KEY + assert call_kwargs["channel"] == "api" + assert call_kwargs["chat_id"] == API_CHAT_ID + assert len(call_kwargs.get("media") or []) >= 0 # base64 images saved to disk @pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") @@ -320,7 +321,7 @@ async def test_multimodal_content_extracts_text(aiohttp_client, mock_agent) -> N async def test_empty_response_retry_then_success(aiohttp_client) -> None: call_count = 0 - async def sometimes_empty(content, session_key="", channel="", chat_id=""): + async def sometimes_empty(content, session_key="", channel="", chat_id="", **kwargs): nonlocal call_count call_count += 1 if call_count == 1: @@ -351,7 +352,7 @@ async def test_empty_response_falls_back(aiohttp_client) -> None: call_count = 0 - async def always_empty(content, session_key="", channel="", chat_id=""): + async def always_empty(content, session_key="", channel="", chat_id="", **kwargs): nonlocal call_count call_count += 1 return "" @@ -371,3 +372,31 @@ async def test_empty_response_falls_back(aiohttp_client) -> None: body = await resp.json() assert body["choices"][0]["message"]["content"] == EMPTY_FINAL_RESPONSE_MESSAGE assert call_count == 2 + + +@pytest.mark.asyncio +async def test_process_direct_accepts_media() -> None: + """process_direct should forward media paths to _process_message.""" + from nanobot.agent.loop import AgentLoop + + loop = AgentLoop.__new__(AgentLoop) + loop._connect_mcp = AsyncMock() + + captured_msg = None + + async def fake_process(msg, *, session_key="", on_progress=None, on_stream=None, on_stream_end=None): + nonlocal captured_msg + captured_msg = msg + return None + + loop._process_message = fake_process + + await loop.process_direct( + content="analyze this", + media=["/tmp/image.png", "/tmp/report.pdf"], + session_key="test:1", + ) + + assert captured_msg is not None + assert captured_msg.media == ["/tmp/image.png", "/tmp/report.pdf"] + assert captured_msg.content == "analyze this" From d849a3fa060825ff02b73f74dc14d3ab97735b33 Mon Sep 17 00:00:00 2001 From: chengyongru <2755839590@qq.com> Date: Mon, 13 Apr 2026 23:33:25 +0800 Subject: [PATCH 03/32] fix(agent): drain injection queue on error/edge-case exit paths When the agent runner exits due to LLM error, tool error, empty response, or max_iterations, it breaks out of the iteration loop without draining the pending injection queue. This causes leftover messages to be re-published as independent inbound messages, resulting in duplicate or confusing replies to the user. Extract the injection drain logic into a `_try_drain_injections` helper and call it before each break in the error/edge-case paths. If injections are found, continue the loop instead of breaking. For max_iterations (where the loop is exhausted), drain injections to prevent re-publish without continuing. --- nanobot/agent/runner.py | 107 ++++++++++++----- tests/agent/test_runner.py | 233 +++++++++++++++++++++++++++++++++++++ 2 files changed, 314 insertions(+), 26 deletions(-) diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index e92d864f..5cb7b4f0 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -134,6 +134,36 @@ class AgentRunner: continue messages.append(injection) + async def _try_drain_injections( + self, + spec: AgentRunSpec, + messages: list[dict[str, Any]], + assistant_message: dict[str, Any] | None, + injection_cycles: int, + *, + phase: str = "after error", + ) -> tuple[bool, int]: + """Drain pending injections. Returns (should_continue, updated_cycles). + + If injections are found and we haven't exceeded _MAX_INJECTION_CYCLES, + append them to *messages* and return (True, cycles+1) so the caller + continues the iteration loop. Otherwise return (False, cycles). + """ + if injection_cycles >= _MAX_INJECTION_CYCLES: + return False, injection_cycles + injections = await self._drain_injections(spec) + if not injections: + return False, injection_cycles + injection_cycles += 1 + if assistant_message is not None: + messages.append(assistant_message) + self._append_injected_messages(messages, injections) + logger.info( + "Injected {} follow-up message(s) {} ({}/{})", + len(injections), phase, injection_cycles, _MAX_INJECTION_CYCLES, + ) + return True, injection_cycles + async def _drain_injections(self, spec: AgentRunSpec) -> list[dict[str, Any]]: """Drain pending user messages via the injection callback. @@ -287,6 +317,13 @@ class AgentRunner: context.error = error context.stop_reason = stop_reason await hook.after_iteration(context) + should_continue, injection_cycles = await self._try_drain_injections( + spec, messages, None, injection_cycles, + phase="after tool error", + ) + if should_continue: + had_injections = True + continue break await self._emit_checkpoint( spec, @@ -379,36 +416,31 @@ class AgentRunner: # Check for mid-turn injections BEFORE signaling stream end. # If injections are found we keep the stream alive (resuming=True) # so streaming channels don't prematurely finalize the card. - _injected_after_final = False - if injection_cycles < _MAX_INJECTION_CYCLES: - injections = await self._drain_injections(spec) - if injections: - had_injections = True - injection_cycles += 1 - _injected_after_final = True - if assistant_message is not None: - messages.append(assistant_message) - await self._emit_checkpoint( - spec, - { - "phase": "final_response", - "iteration": iteration, - "model": spec.model, - "assistant_message": assistant_message, - "completed_tool_results": [], - "pending_tool_calls": [], - }, - ) - self._append_injected_messages(messages, injections) - logger.info( - "Injected {} follow-up message(s) after final response ({}/{})", - len(injections), injection_cycles, _MAX_INJECTION_CYCLES, + should_continue, injection_cycles = await self._try_drain_injections( + spec, messages, assistant_message, injection_cycles, + phase="after final response", + ) + if should_continue: + had_injections = True + # Emit checkpoint for the assistant message that was appended + # by _try_drain_injections, then keep the stream alive. + if assistant_message is not None: + await self._emit_checkpoint( + spec, + { + "phase": "final_response", + "iteration": iteration, + "model": spec.model, + "assistant_message": assistant_message, + "completed_tool_results": [], + "pending_tool_calls": [], + }, ) if hook.wants_streaming(): - await hook.on_stream_end(context, resuming=_injected_after_final) + await hook.on_stream_end(context, resuming=should_continue) - if _injected_after_final: + if should_continue: await hook.after_iteration(context) continue @@ -421,6 +453,13 @@ class AgentRunner: context.error = error context.stop_reason = stop_reason await hook.after_iteration(context) + should_continue, injection_cycles = await self._try_drain_injections( + spec, messages, None, injection_cycles, + phase="after LLM error", + ) + if should_continue: + had_injections = True + continue break if is_blank_text(clean): final_content = EMPTY_FINAL_RESPONSE_MESSAGE @@ -431,6 +470,13 @@ class AgentRunner: context.error = error context.stop_reason = stop_reason await hook.after_iteration(context) + should_continue, injection_cycles = await self._try_drain_injections( + spec, messages, None, injection_cycles, + phase="after empty response", + ) + if should_continue: + had_injections = True + continue break messages.append(assistant_message or build_assistant_message( @@ -467,6 +513,15 @@ class AgentRunner: max_iterations=spec.max_iterations, ) self._append_final_message(messages, final_content) + # Drain any remaining injections so they are appended to the + # conversation history instead of being re-published as + # independent inbound messages by _dispatch's finally block. + # We ignore should_continue here because the for-loop has already + # exhausted all iterations. + _, injection_cycles = await self._try_drain_injections( + spec, messages, None, injection_cycles, + phase="after max_iterations", + ) return AgentRunResult( final_content=final_content, diff --git a/tests/agent/test_runner.py b/tests/agent/test_runner.py index a62457aa..4a943165 100644 --- a/tests/agent/test_runner.py +++ b/tests/agent/test_runner.py @@ -2410,3 +2410,236 @@ async def test_dispatch_republishes_leftover_queue_messages(tmp_path): contents = [m.content for m in msgs] assert "leftover-1" in contents assert "leftover-2" in contents + + +@pytest.mark.asyncio +async def test_drain_injections_on_fatal_tool_error(): + """Pending injections should be drained even when a fatal tool error occurs.""" + from nanobot.agent.runner import AgentRunSpec, AgentRunner + from nanobot.bus.events import InboundMessage + + provider = MagicMock() + call_count = {"n": 0} + + async def chat_with_retry(*, messages, **kwargs): + call_count["n"] += 1 + if call_count["n"] == 1: + return LLMResponse( + content="", + tool_calls=[ToolCallRequest(id="c1", name="exec", arguments={"cmd": "bad"})], + usage={}, + ) + # Second call: respond normally to the injected follow-up + return LLMResponse(content="reply to follow-up", tool_calls=[], usage={}) + + provider.chat_with_retry = chat_with_retry + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(side_effect=RuntimeError("tool exploded")) + + injection_queue = asyncio.Queue() + + async def inject_cb(): + items = [] + while not injection_queue.empty(): + items.append(await injection_queue.get()) + return items + + await injection_queue.put( + InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up after error") + ) + + runner = AgentRunner(provider) + result = await runner.run(AgentRunSpec( + initial_messages=[{"role": "user", "content": "hello"}], + tools=tools, + model="test-model", + max_iterations=5, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + fail_on_tool_error=True, + injection_callback=inject_cb, + )) + + assert result.had_injections is True + assert result.final_content == "reply to follow-up" + # The injection should be in the messages history + injected = [ + m for m in result.messages + if m.get("role") == "user" and m.get("content") == "follow-up after error" + ] + assert len(injected) == 1 + + +@pytest.mark.asyncio +async def test_drain_injections_on_llm_error(): + """Pending injections should be drained when the LLM returns an error finish_reason.""" + from nanobot.agent.runner import AgentRunSpec, AgentRunner + from nanobot.bus.events import InboundMessage + + provider = MagicMock() + call_count = {"n": 0} + + async def chat_with_retry(*, messages, **kwargs): + call_count["n"] += 1 + if call_count["n"] == 1: + return LLMResponse( + content=None, + tool_calls=[], + finish_reason="error", + usage={}, + ) + # Second call: respond normally to the injected follow-up + return LLMResponse(content="recovered answer", tool_calls=[], usage={}) + + provider.chat_with_retry = chat_with_retry + tools = MagicMock() + tools.get_definitions.return_value = [] + + injection_queue = asyncio.Queue() + + async def inject_cb(): + items = [] + while not injection_queue.empty(): + items.append(await injection_queue.get()) + return items + + await injection_queue.put( + InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up after LLM error") + ) + + runner = AgentRunner(provider) + result = await runner.run(AgentRunSpec( + initial_messages=[ + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "previous response"}, + {"role": "user", "content": "trigger error"}, + ], + tools=tools, + model="test-model", + max_iterations=5, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + injection_callback=inject_cb, + )) + + assert result.had_injections is True + assert result.final_content == "recovered answer" + injected = [ + m for m in result.messages + if m.get("role") == "user" and "follow-up after LLM error" in str(m.get("content", "")) + ] + assert len(injected) == 1 + + +@pytest.mark.asyncio +async def test_drain_injections_on_empty_final_response(): + """Pending injections should be drained when the runner exits due to empty response.""" + from nanobot.agent.runner import AgentRunSpec, AgentRunner, _MAX_EMPTY_RETRIES + from nanobot.bus.events import InboundMessage + + provider = MagicMock() + call_count = {"n": 0} + + async def chat_with_retry(*, messages, **kwargs): + call_count["n"] += 1 + if call_count["n"] <= _MAX_EMPTY_RETRIES + 1: + return LLMResponse(content="", tool_calls=[], usage={}) + # After retries exhausted + injection drain, respond normally + return LLMResponse(content="answer after empty", tool_calls=[], usage={}) + + provider.chat_with_retry = chat_with_retry + tools = MagicMock() + tools.get_definitions.return_value = [] + + injection_queue = asyncio.Queue() + + async def inject_cb(): + items = [] + while not injection_queue.empty(): + items.append(await injection_queue.get()) + return items + + await injection_queue.put( + InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up after empty") + ) + + runner = AgentRunner(provider) + result = await runner.run(AgentRunSpec( + initial_messages=[ + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "previous response"}, + {"role": "user", "content": "trigger empty"}, + ], + tools=tools, + model="test-model", + max_iterations=10, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + injection_callback=inject_cb, + )) + + assert result.had_injections is True + assert result.final_content == "answer after empty" + injected = [ + m for m in result.messages + if m.get("role") == "user" and "follow-up after empty" in str(m.get("content", "")) + ] + assert len(injected) == 1 + + +@pytest.mark.asyncio +async def test_drain_injections_on_max_iterations(): + """Pending injections should be drained when the runner hits max_iterations. + + Unlike other error paths, max_iterations cannot continue the loop, so + injections are appended to messages but not processed by the LLM. + The key point is they are consumed from the queue to prevent re-publish. + """ + from nanobot.agent.runner import AgentRunSpec, AgentRunner + from nanobot.bus.events import InboundMessage + + provider = MagicMock() + call_count = {"n": 0} + + async def chat_with_retry(*, messages, **kwargs): + call_count["n"] += 1 + return LLMResponse( + content="", + tool_calls=[ToolCallRequest(id=f"c{call_count['n']}", name="read_file", arguments={"path": "x"})], + usage={}, + ) + + provider.chat_with_retry = chat_with_retry + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(return_value="file content") + + injection_queue = asyncio.Queue() + + async def inject_cb(): + items = [] + while not injection_queue.empty(): + items.append(await injection_queue.get()) + return items + + await injection_queue.put( + InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up after max iters") + ) + + runner = AgentRunner(provider) + result = await runner.run(AgentRunSpec( + initial_messages=[{"role": "user", "content": "hello"}], + tools=tools, + model="test-model", + max_iterations=2, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + injection_callback=inject_cb, + )) + + assert result.stop_reason == "max_iterations" + # The injection was consumed from the queue (preventing re-publish) + assert injection_queue.empty() + # The injection message is appended to conversation history + injected = [ + m for m in result.messages + if m.get("role") == "user" and m.get("content") == "follow-up after max iters" + ] + assert len(injected) == 1 From a1e1eed2f13c19e9dcec2fa912103dc967b8daec Mon Sep 17 00:00:00 2001 From: chengyongru <2755839590@qq.com> Date: Mon, 13 Apr 2026 23:51:23 +0800 Subject: [PATCH 04/32] refactor(runner): consolidate all injection drain paths and deduplicate tests - Migrate "after tools" inline drain to use _try_drain_injections, completing the refactoring (all 6 drain sites now use the helper). - Move checkpoint emission into _try_drain_injections via optional iteration parameter, eliminating the leaky split between helper and caller for the final-response path. - Extract _make_injection_callback() test helper to replace 7 identical inject_cb function bodies. - Add test_injection_cycle_cap_on_error_path to verify the cycle cap is enforced on error exit paths. --- nanobot/agent/runner.py | 49 ++++++++--------- tests/agent/test_runner.py | 109 +++++++++++++++++++++++-------------- 2 files changed, 90 insertions(+), 68 deletions(-) diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index 5cb7b4f0..20226aed 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -142,12 +142,14 @@ class AgentRunner: injection_cycles: int, *, phase: str = "after error", + iteration: int | None = None, ) -> tuple[bool, int]: """Drain pending injections. Returns (should_continue, updated_cycles). If injections are found and we haven't exceeded _MAX_INJECTION_CYCLES, - append them to *messages* and return (True, cycles+1) so the caller - continues the iteration loop. Otherwise return (False, cycles). + append them to *messages* (and emit a checkpoint if *assistant_message* + and *iteration* are both provided) and return (True, cycles+1) so the + caller continues the iteration loop. Otherwise return (False, cycles). """ if injection_cycles >= _MAX_INJECTION_CYCLES: return False, injection_cycles @@ -157,6 +159,18 @@ class AgentRunner: injection_cycles += 1 if assistant_message is not None: messages.append(assistant_message) + if iteration is not None: + await self._emit_checkpoint( + spec, + { + "phase": "final_response", + "iteration": iteration, + "model": spec.model, + "assistant_message": assistant_message, + "completed_tool_results": [], + "pending_tool_calls": [], + }, + ) self._append_injected_messages(messages, injections) logger.info( "Injected {} follow-up message(s) {} ({}/{})", @@ -339,16 +353,12 @@ class AgentRunner: empty_content_retries = 0 length_recovery_count = 0 # Checkpoint 1: drain injections after tools, before next LLM call - if injection_cycles < _MAX_INJECTION_CYCLES: - injections = await self._drain_injections(spec) - if injections: - had_injections = True - injection_cycles += 1 - self._append_injected_messages(messages, injections) - logger.info( - "Injected {} follow-up message(s) after tool execution ({}/{})", - len(injections), injection_cycles, _MAX_INJECTION_CYCLES, - ) + _drained, injection_cycles = await self._try_drain_injections( + spec, messages, None, injection_cycles, + phase="after tool execution", + ) + if _drained: + had_injections = True await hook.after_iteration(context) continue @@ -419,23 +429,10 @@ class AgentRunner: should_continue, injection_cycles = await self._try_drain_injections( spec, messages, assistant_message, injection_cycles, phase="after final response", + iteration=iteration, ) if should_continue: had_injections = True - # Emit checkpoint for the assistant message that was appended - # by _try_drain_injections, then keep the stream alive. - if assistant_message is not None: - await self._emit_checkpoint( - spec, - { - "phase": "final_response", - "iteration": iteration, - "model": spec.model, - "assistant_message": assistant_message, - "completed_tool_results": [], - "pending_tool_calls": [], - }, - ) if hook.wants_streaming(): await hook.on_stream_end(context, resuming=should_continue) diff --git a/tests/agent/test_runner.py b/tests/agent/test_runner.py index 4a943165..53cd07e8 100644 --- a/tests/agent/test_runner.py +++ b/tests/agent/test_runner.py @@ -18,6 +18,16 @@ from nanobot.providers.base import LLMResponse, ToolCallRequest _MAX_TOOL_RESULT_CHARS = AgentDefaults().max_tool_result_chars +def _make_injection_callback(queue: asyncio.Queue): + """Return an async callback that drains *queue* into a list of dicts.""" + async def inject_cb(): + items = [] + while not queue.empty(): + items.append(await queue.get()) + return items + return inject_cb + + def _make_loop(tmp_path): from nanobot.agent.loop import AgentLoop from nanobot.bus.queue import MessageBus @@ -1888,12 +1898,7 @@ async def test_checkpoint1_injects_after_tool_execution(): tools.execute = AsyncMock(return_value="file content") injection_queue = asyncio.Queue() - - async def inject_cb(): - items = [] - while not injection_queue.empty(): - items.append(await injection_queue.get()) - return items + inject_cb = _make_injection_callback(injection_queue) # Put a follow-up message in the queue before the run starts await injection_queue.put( @@ -1951,12 +1956,7 @@ async def test_checkpoint2_injects_after_final_response_with_resuming_stream(): tools.get_definitions.return_value = [] injection_queue = asyncio.Queue() - - async def inject_cb(): - items = [] - while not injection_queue.empty(): - items.append(await injection_queue.get()) - return items + inject_cb = _make_injection_callback(injection_queue) # Inject a follow-up that arrives during the first response await injection_queue.put( @@ -2005,12 +2005,7 @@ async def test_checkpoint2_preserves_final_response_in_history_before_followup() tools.get_definitions.return_value = [] injection_queue = asyncio.Queue() - - async def inject_cb(): - items = [] - while not injection_queue.empty(): - items.append(await injection_queue.get()) - return items + inject_cb = _make_injection_callback(injection_queue) await injection_queue.put( InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up question") @@ -2438,12 +2433,7 @@ async def test_drain_injections_on_fatal_tool_error(): tools.execute = AsyncMock(side_effect=RuntimeError("tool exploded")) injection_queue = asyncio.Queue() - - async def inject_cb(): - items = [] - while not injection_queue.empty(): - items.append(await injection_queue.get()) - return items + inject_cb = _make_injection_callback(injection_queue) await injection_queue.put( InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up after error") @@ -2496,12 +2486,7 @@ async def test_drain_injections_on_llm_error(): tools.get_definitions.return_value = [] injection_queue = asyncio.Queue() - - async def inject_cb(): - items = [] - while not injection_queue.empty(): - items.append(await injection_queue.get()) - return items + inject_cb = _make_injection_callback(injection_queue) await injection_queue.put( InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up after LLM error") @@ -2551,12 +2536,7 @@ async def test_drain_injections_on_empty_final_response(): tools.get_definitions.return_value = [] injection_queue = asyncio.Queue() - - async def inject_cb(): - items = [] - while not injection_queue.empty(): - items.append(await injection_queue.get()) - return items + inject_cb = _make_injection_callback(injection_queue) await injection_queue.put( InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up after empty") @@ -2613,12 +2593,7 @@ async def test_drain_injections_on_max_iterations(): tools.execute = AsyncMock(return_value="file content") injection_queue = asyncio.Queue() - - async def inject_cb(): - items = [] - while not injection_queue.empty(): - items.append(await injection_queue.get()) - return items + inject_cb = _make_injection_callback(injection_queue) await injection_queue.put( InboundMessage(channel="cli", sender_id="u", chat_id="c", content="follow-up after max iters") @@ -2643,3 +2618,53 @@ async def test_drain_injections_on_max_iterations(): if m.get("role") == "user" and m.get("content") == "follow-up after max iters" ] assert len(injected) == 1 + + +@pytest.mark.asyncio +async def test_injection_cycle_cap_on_error_path(): + """Injection cycles should be capped even when every iteration hits an LLM error.""" + from nanobot.agent.runner import AgentRunSpec, AgentRunner, _MAX_INJECTION_CYCLES + from nanobot.bus.events import InboundMessage + + provider = MagicMock() + call_count = {"n": 0} + + async def chat_with_retry(*, messages, **kwargs): + call_count["n"] += 1 + return LLMResponse( + content=None, + tool_calls=[], + finish_reason="error", + usage={}, + ) + + provider.chat_with_retry = chat_with_retry + tools = MagicMock() + tools.get_definitions.return_value = [] + + drain_count = {"n": 0} + + async def inject_cb(): + drain_count["n"] += 1 + if drain_count["n"] <= _MAX_INJECTION_CYCLES: + return [InboundMessage(channel="cli", sender_id="u", chat_id="c", content=f"msg-{drain_count['n']}")] + return [] + + runner = AgentRunner(provider) + result = await runner.run(AgentRunSpec( + initial_messages=[ + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "previous"}, + {"role": "user", "content": "trigger error"}, + ], + tools=tools, + model="test-model", + max_iterations=20, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + injection_callback=inject_cb, + )) + + assert result.had_injections is True + # Should cap: _MAX_INJECTION_CYCLES drained rounds + 1 final round that breaks + assert call_count["n"] == _MAX_INJECTION_CYCLES + 1 + assert drain_count["n"] == _MAX_INJECTION_CYCLES From a38bc637bdaaf1ce3e3090ba2d32afdbf79029f5 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Mon, 13 Apr 2026 16:28:35 +0000 Subject: [PATCH 05/32] fix(runner): preserve injection flag after max-iteration drain Keep late follow-up injections observable when they are drained during max-iteration shutdown so loop-level response suppression still makes the right decision. Made-with: Cursor --- nanobot/agent/runner.py | 4 ++- tests/agent/test_runner.py | 64 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index 20226aed..592af9de 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -515,10 +515,12 @@ class AgentRunner: # independent inbound messages by _dispatch's finally block. # We ignore should_continue here because the for-loop has already # exhausted all iterations. - _, injection_cycles = await self._try_drain_injections( + drained_after_max_iterations, injection_cycles = await self._try_drain_injections( spec, messages, None, injection_cycles, phase="after max_iterations", ) + if drained_after_max_iterations: + had_injections = True return AgentRunResult( final_content=final_content, diff --git a/tests/agent/test_runner.py b/tests/agent/test_runner.py index 53cd07e8..74025d77 100644 --- a/tests/agent/test_runner.py +++ b/tests/agent/test_runner.py @@ -2610,6 +2610,7 @@ async def test_drain_injections_on_max_iterations(): )) assert result.stop_reason == "max_iterations" + assert result.had_injections is True # The injection was consumed from the queue (preventing re-publish) assert injection_queue.empty() # The injection message is appended to conversation history @@ -2620,6 +2621,69 @@ async def test_drain_injections_on_max_iterations(): assert len(injected) == 1 +@pytest.mark.asyncio +async def test_drain_injections_set_flag_when_followup_arrives_after_last_iteration(): + """Late follow-ups drained in max_iterations should still flip had_injections.""" + from nanobot.agent.hook import AgentHook + from nanobot.agent.runner import AgentRunSpec, AgentRunner + from nanobot.bus.events import InboundMessage + + provider = MagicMock() + call_count = {"n": 0} + + async def chat_with_retry(*, messages, **kwargs): + call_count["n"] += 1 + return LLMResponse( + content="", + tool_calls=[ToolCallRequest(id=f"c{call_count['n']}", name="read_file", arguments={"path": "x"})], + usage={}, + ) + + provider.chat_with_retry = chat_with_retry + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(return_value="file content") + + injection_queue = asyncio.Queue() + inject_cb = _make_injection_callback(injection_queue) + + class InjectOnLastAfterIterationHook(AgentHook): + def __init__(self) -> None: + self.after_iteration_calls = 0 + + async def after_iteration(self, context) -> None: + self.after_iteration_calls += 1 + if self.after_iteration_calls == 2: + await injection_queue.put( + InboundMessage( + channel="cli", + sender_id="u", + chat_id="c", + content="late follow-up after max iters", + ) + ) + + runner = AgentRunner(provider) + result = await runner.run(AgentRunSpec( + initial_messages=[{"role": "user", "content": "hello"}], + tools=tools, + model="test-model", + max_iterations=2, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + injection_callback=inject_cb, + hook=InjectOnLastAfterIterationHook(), + )) + + assert result.stop_reason == "max_iterations" + assert result.had_injections is True + assert injection_queue.empty() + injected = [ + m for m in result.messages + if m.get("role") == "user" and m.get("content") == "late follow-up after max iters" + ] + assert len(injected) == 1 + + @pytest.mark.asyncio async def test_injection_cycle_cap_on_error_path(): """Injection cycles should be capped even when every iteration hits an LLM error.""" From ee061f0595f4258634bac4417aaf5b4089c96d13 Mon Sep 17 00:00:00 2001 From: yeyitech Date: Tue, 14 Apr 2026 13:30:18 +0800 Subject: [PATCH 06/32] fix(web): serialize duckduckgo search calls --- nanobot/agent/tools/web.py | 27 ++++++++++++++ tests/agent/test_runner.py | 57 ++++++++++++++++++++++++++++- tests/tools/test_web_search_tool.py | 24 +++++++++--- 3 files changed, 102 insertions(+), 6 deletions(-) diff --git a/nanobot/agent/tools/web.py b/nanobot/agent/tools/web.py index 38fc33d7..31d4cdef 100644 --- a/nanobot/agent/tools/web.py +++ b/nanobot/agent/tools/web.py @@ -96,10 +96,37 @@ class WebSearchTool(Tool): self.config = config if config is not None else WebSearchConfig() self.proxy = proxy + def _effective_provider(self) -> str: + """Resolve the backend that execute() will actually use.""" + provider = self.config.provider.strip().lower() or "brave" + if provider == "duckduckgo": + return "duckduckgo" + if provider == "brave": + api_key = self.config.api_key or os.environ.get("BRAVE_API_KEY", "") + return "brave" if api_key else "duckduckgo" + if provider == "tavily": + api_key = self.config.api_key or os.environ.get("TAVILY_API_KEY", "") + return "tavily" if api_key else "duckduckgo" + if provider == "searxng": + base_url = (self.config.base_url or os.environ.get("SEARXNG_BASE_URL", "")).strip() + return "searxng" if base_url else "duckduckgo" + if provider == "jina": + api_key = self.config.api_key or os.environ.get("JINA_API_KEY", "") + return "jina" if api_key else "duckduckgo" + if provider == "kagi": + api_key = self.config.api_key or os.environ.get("KAGI_API_KEY", "") + return "kagi" if api_key else "duckduckgo" + return provider + @property def read_only(self) -> bool: return True + @property + def exclusive(self) -> bool: + """DuckDuckGo searches are serialized because ddgs is not concurrency-safe.""" + return self._effective_provider() == "duckduckgo" + async def execute(self, query: str, count: int | None = None, **kwargs: Any) -> str: provider = self.config.provider.strip().lower() or "brave" n = min(max(count or self.config.max_results, 1), 10) diff --git a/tests/agent/test_runner.py b/tests/agent/test_runner.py index 74025d77..f742408b 100644 --- a/tests/agent/test_runner.py +++ b/tests/agent/test_runner.py @@ -689,11 +689,20 @@ async def test_runner_keeps_going_when_tool_result_persistence_fails(): class _DelayTool(Tool): - def __init__(self, name: str, *, delay: float, read_only: bool, shared_events: list[str]): + def __init__( + self, + name: str, + *, + delay: float, + read_only: bool, + shared_events: list[str], + exclusive: bool = False, + ): self._name = name self._delay = delay self._read_only = read_only self._shared_events = shared_events + self._exclusive = exclusive @property def name(self) -> str: @@ -711,6 +720,10 @@ class _DelayTool(Tool): def read_only(self) -> bool: return self._read_only + @property + def exclusive(self) -> bool: + return self._exclusive + async def execute(self, **kwargs): self._shared_events.append(f"start:{self._name}") await asyncio.sleep(self._delay) @@ -756,6 +769,48 @@ async def test_runner_batches_read_only_tools_before_exclusive_work(): assert shared_events[-2:] == ["start:write_a", "end:write_a"] +@pytest.mark.asyncio +async def test_runner_does_not_batch_exclusive_read_only_tools(): + from nanobot.agent.runner import AgentRunSpec, AgentRunner + + tools = ToolRegistry() + shared_events: list[str] = [] + read_a = _DelayTool("read_a", delay=0.03, read_only=True, shared_events=shared_events) + read_b = _DelayTool("read_b", delay=0.03, read_only=True, shared_events=shared_events) + ddg_like = _DelayTool( + "ddg_like", + delay=0.01, + read_only=True, + shared_events=shared_events, + exclusive=True, + ) + tools.register(read_a) + tools.register(ddg_like) + tools.register(read_b) + + runner = AgentRunner(MagicMock()) + await runner._execute_tools( + AgentRunSpec( + initial_messages=[], + tools=tools, + model="test-model", + max_iterations=1, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + concurrent_tools=True, + ), + [ + ToolCallRequest(id="ro1", name="read_a", arguments={}), + ToolCallRequest(id="ddg1", name="ddg_like", arguments={}), + ToolCallRequest(id="ro2", name="read_b", arguments={}), + ], + {}, + ) + + assert shared_events[0] == "start:read_a" + assert shared_events.index("end:read_a") < shared_events.index("start:ddg_like") + assert shared_events.index("end:ddg_like") < shared_events.index("start:read_b") + + @pytest.mark.asyncio async def test_runner_blocks_repeated_external_fetches(): from nanobot.agent.runner import AgentRunSpec, AgentRunner diff --git a/tests/tools/test_web_search_tool.py b/tests/tools/test_web_search_tool.py index 790d8adc..a42e51e1 100644 --- a/tests/tools/test_web_search_tool.py +++ b/tests/tools/test_web_search_tool.py @@ -1,7 +1,5 @@ """Tests for multi-provider web search.""" -import asyncio - import httpx import pytest @@ -20,6 +18,25 @@ def _response(status: int = 200, json: dict | None = None) -> httpx.Response: return r +def test_duckduckgo_search_is_exclusive(): + tool = _tool(provider="duckduckgo") + assert tool.exclusive is True + assert tool.concurrency_safe is False + + +def test_brave_with_api_key_remains_concurrency_safe(): + tool = _tool(provider="brave", api_key="brave-key") + assert tool.exclusive is False + assert tool.concurrency_safe is True + + +def test_brave_without_api_key_is_treated_as_duckduckgo_for_concurrency(monkeypatch): + monkeypatch.delenv("BRAVE_API_KEY", raising=False) + tool = _tool(provider="brave", api_key="") + assert tool.exclusive is True + assert tool.concurrency_safe is False + + @pytest.mark.asyncio async def test_brave_search(monkeypatch): async def mock_get(self, url, **kw): @@ -79,7 +96,6 @@ async def test_duckduckgo_search(monkeypatch): import nanobot.agent.tools.web as web_mod monkeypatch.setattr(web_mod, "DDGS", MockDDGS, raising=False) - from ddgs import DDGS monkeypatch.setattr("ddgs.DDGS", MockDDGS) tool = _tool(provider="duckduckgo") @@ -265,5 +281,3 @@ async def test_duckduckgo_timeout_returns_error(monkeypatch): result = await tool.execute(query="test") gate.set() assert "Error" in result - - From 65a15f39ee7ebfe8b9585231165222cf5ee1cd76 Mon Sep 17 00:00:00 2001 From: yeyitech Date: Tue, 14 Apr 2026 13:42:59 +0800 Subject: [PATCH 07/32] test(loop): cover /stop checkpoint recovery --- tests/agent/test_loop_save_turn.py | 109 +++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/tests/agent/test_loop_save_turn.py b/tests/agent/test_loop_save_turn.py index c965ccd8..8885e0cc 100644 --- a/tests/agent/test_loop_save_turn.py +++ b/tests/agent/test_loop_save_turn.py @@ -1,3 +1,4 @@ +import asyncio from pathlib import Path from unittest.mock import AsyncMock, MagicMock @@ -308,3 +309,111 @@ async def test_next_turn_after_crash_closes_pending_user_turn_before_new_input(t {"role": "assistant", "content": "new answer"}, ] assert AgentLoop._PENDING_USER_TURN_KEY not in session.metadata + + +@pytest.mark.asyncio +async def test_stop_preserves_runtime_checkpoint_for_next_turn(tmp_path: Path) -> None: + from nanobot.command.builtin import cmd_stop + from nanobot.command.router import CommandContext + + loop = _make_full_loop(tmp_path) + loop.consolidator.maybe_consolidate_by_tokens = AsyncMock(return_value=False) # type: ignore[method-assign] + + checkpoint_saved = asyncio.Event() + + async def interrupted_run_agent_loop(_initial_messages, *, session=None, **_kwargs): + assert session is not None + loop._set_runtime_checkpoint( + session, + { + "assistant_message": { + "role": "assistant", + "content": "working", + "tool_calls": [ + { + "id": "call_done", + "type": "function", + "function": {"name": "read_file", "arguments": "{}"}, + }, + { + "id": "call_pending", + "type": "function", + "function": {"name": "exec", "arguments": "{}"}, + }, + ], + }, + "completed_tool_results": [ + { + "role": "tool", + "tool_call_id": "call_done", + "name": "read_file", + "content": "ok", + } + ], + "pending_tool_calls": [ + { + "id": "call_pending", + "type": "function", + "function": {"name": "exec", "arguments": "{}"}, + } + ], + }, + ) + checkpoint_saved.set() + await asyncio.Event().wait() + + loop._run_agent_loop = interrupted_run_agent_loop # type: ignore[method-assign] + + first_msg = InboundMessage(channel="feishu", sender_id="u1", chat_id="c4", content="keep progress") + task = asyncio.create_task(loop._process_message(first_msg)) + loop._active_tasks[first_msg.session_key] = [task] + await asyncio.wait_for(checkpoint_saved.wait(), timeout=1.0) + + stop_msg = InboundMessage(channel="feishu", sender_id="u1", chat_id="c4", content="/stop") + stop_ctx = CommandContext(msg=stop_msg, session=None, key=stop_msg.session_key, raw="/stop", loop=loop) + stop_result = await cmd_stop(stop_ctx) + + assert "Stopped 1 task" in stop_result.content + assert task.done() + + loop.sessions.invalidate("feishu:c4") + interrupted = loop.sessions.get_or_create("feishu:c4") + assert interrupted.metadata.get(AgentLoop._PENDING_USER_TURN_KEY) is True + assert interrupted.metadata.get(AgentLoop._RUNTIME_CHECKPOINT_KEY) is not None + + async def resumed_run_agent_loop(initial_messages, **_kwargs): + return ( + "next answer", + None, + [*initial_messages, {"role": "assistant", "content": "next answer"}], + "stop", + False, + ) + + loop._run_agent_loop = resumed_run_agent_loop # type: ignore[method-assign] + result = await loop._process_message( + InboundMessage(channel="feishu", sender_id="u1", chat_id="c4", content="continue here") + ) + + assert result is not None + assert result.content == "next answer" + + session = loop.sessions.get_or_create("feishu:c4") + assert [ + {k: v for k, v in m.items() if k in {"role", "content", "tool_call_id", "name"}} + for m in session.messages + ] == [ + {"role": "user", "content": "keep progress"}, + {"role": "assistant", "content": "working"}, + {"role": "tool", "tool_call_id": "call_done", "name": "read_file", "content": "ok"}, + { + "role": "tool", + "tool_call_id": "call_pending", + "name": "exec", + "content": "Error: Task interrupted before this tool finished.", + }, + {"role": "user", "content": "continue here"}, + {"role": "assistant", "content": "next answer"}, + ] + assert AgentLoop._PENDING_USER_TURN_KEY not in session.metadata + assert AgentLoop._RUNTIME_CHECKPOINT_KEY not in session.metadata From e4b3f9bd28b098704c5ce4dc6e8505da434bd9d2 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 07:19:38 +0000 Subject: [PATCH 08/32] security(gateway): keep health endpoint local by default Bind the gateway health listener to localhost by default and reduce the probe response to a minimal status payload so accidental public exposure leaks less information. Made-with: Cursor --- README.md | 11 +++++++---- nanobot/cli/commands.py | 20 +------------------- nanobot/config/schema.py | 2 +- tests/cli/test_commands.py | 14 +++++--------- 4 files changed, 14 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index a5ddf136..4dd7a93b 100644 --- a/README.md +++ b/README.md @@ -1727,6 +1727,7 @@ Example config: } }, "gateway": { + "host": "127.0.0.1", "port": 18790 } } @@ -1739,11 +1740,13 @@ nanobot gateway --config ~/.nanobot-telegram/config.json nanobot gateway --config ~/.nanobot-discord/config.json ``` -Each gateway instance also exposes a lightweight HTTP status endpoint on -`gateway.host:gateway.port`: +Each gateway instance also exposes a lightweight HTTP health endpoint on +`gateway.host:gateway.port`. By default, the gateway binds to `127.0.0.1`, +so the endpoint stays local unless you explicitly set `gateway.host` to a +public or LAN-facing address. -- `GET /` returns `nanobot` -- `GET /health` returns JSON with service metadata, uptime, and enabled channels +- `GET /health` returns `{"status":"ok"}` +- Other paths return `404` Override workspace for one-off runs when needed: diff --git a/nanobot/cli/commands.py b/nanobot/cli/commands.py index 1f3f00c8..953e8b1f 100644 --- a/nanobot/cli/commands.py +++ b/nanobot/cli/commands.py @@ -824,9 +824,6 @@ def gateway( async def _health_server(host: str, health_port: int): """Lightweight HTTP health endpoint on the gateway port.""" import json as _json - import time - - start_time = time.monotonic() async def handle(reader, writer): try: @@ -842,28 +839,13 @@ def gateway( method, path = parts[0], parts[1] if method == "GET" and path == "/health": - uptime_s = int(time.monotonic() - start_time) - body = _json.dumps({ - "service": "nanobot", - "version": __version__, - "status": "running", - "uptime_seconds": uptime_s, - "channels": channels.enabled_channels, - }) + body = _json.dumps({"status": "ok"}) resp = ( f"HTTP/1.0 200 OK\r\n" f"Content-Type: application/json\r\n" f"Content-Length: {len(body)}\r\n" f"\r\n{body}" ) - elif method == "GET" and path == "/": - body = "nanobot" - resp = ( - f"HTTP/1.0 200 OK\r\n" - f"Content-Type: text/plain\r\n" - f"Content-Length: {len(body)}\r\n" - f"\r\n{body}" - ) else: body = "Not Found" resp = ( diff --git a/nanobot/config/schema.py b/nanobot/config/schema.py index aa5ab993..fd73e080 100644 --- a/nanobot/config/schema.py +++ b/nanobot/config/schema.py @@ -152,7 +152,7 @@ class ApiConfig(Base): class GatewayConfig(Base): """Gateway/server configuration.""" - host: str = "0.0.0.0" + host: str = "127.0.0.1" # Safer default: local-only bind. port: int = 18790 heartbeat: HeartbeatConfig = Field(default_factory=HeartbeatConfig) diff --git a/tests/cli/test_commands.py b/tests/cli/test_commands.py index 1ae2ffd8..e4edfaf8 100644 --- a/tests/cli/test_commands.py +++ b/tests/cli/test_commands.py @@ -1131,7 +1131,6 @@ def test_gateway_health_endpoint_binds_and_serves_expected_responses( ) -> None: config_file = _write_instance_config(tmp_path) config = Config() - config.gateway.host = "127.0.0.9" config.gateway.port = 18791 captured: dict[str, object] = {} @@ -1245,9 +1244,9 @@ def test_gateway_health_endpoint_binds_and_serves_expected_responses( result = runner.invoke(app, ["gateway", "--config", str(config_file)]) assert result.exit_code == 0 - assert captured["host"] == "127.0.0.9" + assert captured["host"] == "127.0.0.1" assert captured["port"] == 18791 - assert "Health endpoint: http://127.0.0.9:18791/health" in result.stdout + assert "Health endpoint: http://127.0.0.1:18791/health" in result.stdout def _call_handler(path: str) -> tuple[str, _FakeWriter]: request = f"GET {path} HTTP/1.1\r\nHost: localhost\r\n\r\n".encode() @@ -1259,17 +1258,14 @@ def test_gateway_health_endpoint_binds_and_serves_expected_responses( root_response, root_writer = _call_handler("/") assert root_writer.closed is True - assert "HTTP/1.0 200 OK" in root_response - assert root_response.endswith("\r\n\r\nnanobot") + assert "HTTP/1.0 404 Not Found" in root_response + assert root_response.endswith("\r\n\r\nNot Found") health_response, health_writer = _call_handler("/health") assert health_writer.closed is True assert "HTTP/1.0 200 OK" in health_response health_body = json.loads(health_response.split("\r\n\r\n", 1)[1]) - assert health_body["service"] == "nanobot" - assert health_body["status"] == "running" - assert health_body["channels"] == ["telegram", "discord"] - assert health_body["uptime_seconds"] >= 0 + assert health_body == {"status": "ok"} missing_response, missing_writer = _call_handler("/missing") assert missing_writer.closed is True From 0adce5405b2f7733fd63d536c9b51028fab9f3f3 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Tue, 14 Apr 2026 14:14:14 +0800 Subject: [PATCH 09/32] fix(feishu): remove resuming to avoid 10-min streaming card timeout Feishu streaming cards auto-close after 10 minutes from creation, regardless of update activity. With resuming enabled, a single card lives across multiple tool-call rounds and can exceed this limit, causing the final response to be silently lost. Remove the _resuming logic from send_delta so each tool-call round gets its own short-lived streaming card (well under 10 min). Add a fallback that sends a regular interactive card when the final streaming update fails. --- docs/CHANNEL_PLUGIN_GUIDE.md | 1 - nanobot/channels/feishu.py | 112 +++++++----------- tests/channels/test_feishu_streaming.py | 65 ++-------- .../test_feishu_tool_hint_code_block.py | 87 +++++++------- 4 files changed, 93 insertions(+), 172 deletions(-) diff --git a/docs/CHANNEL_PLUGIN_GUIDE.md b/docs/CHANNEL_PLUGIN_GUIDE.md index 86e06bf6..65ff9eec 100644 --- a/docs/CHANNEL_PLUGIN_GUIDE.md +++ b/docs/CHANNEL_PLUGIN_GUIDE.md @@ -290,7 +290,6 @@ async def send_delta(self, chat_id: str, delta: str, metadata: dict[str, Any] | |------|---------| | `_stream_delta: True` | A content chunk (delta contains the new text) | | `_stream_end: True` | Streaming finished (delta is empty) | -| `_resuming: True` | More streaming rounds coming (e.g. tool call then another response) | ### Example: Webhook with Streaming diff --git a/nanobot/channels/feishu.py b/nanobot/channels/feishu.py index 5afeca35..1442c363 100644 --- a/nanobot/channels/feishu.py +++ b/nanobot/channels/feishu.py @@ -1290,7 +1290,6 @@ class FeishuChannel(BaseChannel): Supported metadata keys: _stream_end: Finalize the streaming card. - _resuming: Mid-turn pause – flush but keep the buffer alive. _tool_hint: Delta is a formatted tool hint (for display only). message_id: Original message id (used with _stream_end for reaction cleanup). reaction_id: Reaction id to remove on stream end. @@ -1309,50 +1308,44 @@ class FeishuChannel(BaseChannel): if self.config.done_emoji and message_id: await self._add_reaction(message_id, self.config.done_emoji) - resuming = meta.get("_resuming", False) - if resuming: - # Mid-turn pause (e.g. tool call between streaming segments). - # Flush current text to card but keep the buffer alive so the - # next segment appends to the same card. - buf = self._stream_bufs.get(chat_id) - if buf and buf.card_id and buf.text: - buf.sequence += 1 - await loop.run_in_executor( - None, self._stream_update_text_sync, buf.card_id, buf.text, buf.sequence, - ) - return - buf = self._stream_bufs.pop(chat_id, None) if not buf or not buf.text: return + # Try to finalize via streaming card; if that fails (e.g. + # streaming mode was closed by Feishu due to timeout), fall + # back to sending a regular interactive card. if buf.card_id: buf.sequence += 1 - await loop.run_in_executor( + ok = await loop.run_in_executor( None, self._stream_update_text_sync, buf.card_id, buf.text, buf.sequence, ) - # Required so the chat list preview exits the streaming placeholder (Feishu streaming card docs). - buf.sequence += 1 - await loop.run_in_executor( - None, - self._close_streaming_mode_sync, - buf.card_id, - buf.sequence, - ) - else: - for chunk in self._split_elements_by_table_limit( - self._build_card_elements(buf.text) - ): - card = json.dumps( - {"config": {"wide_screen_mode": True}, "elements": chunk}, - ensure_ascii=False, - ) + if ok: + buf.sequence += 1 await loop.run_in_executor( - None, self._send_message_sync, rid_type, chat_id, "interactive", card + None, + self._close_streaming_mode_sync, + buf.card_id, + buf.sequence, ) + return + logger.warning( + "Streaming card {} final update failed, falling back to regular card", + buf.card_id, + ) + for chunk in self._split_elements_by_table_limit( + self._build_card_elements(buf.text) + ): + card = json.dumps( + {"config": {"wide_screen_mode": True}, "elements": chunk}, + ensure_ascii=False, + ) + await loop.run_in_executor( + None, self._send_message_sync, rid_type, chat_id, "interactive", card + ) return # --- accumulate delta --- @@ -1404,14 +1397,21 @@ class FeishuChannel(BaseChannel): if buf and buf.card_id: # Delegate to send_delta so tool hints get the same # throttling (and card creation) as regular text deltas. - lines = self.__class__._format_tool_hint_lines(hint).split("\n") - delta = "\n\n" + "\n".join( - f"{self.config.tool_hint_prefix} {ln}" for ln in lines if ln.strip() - ) + "\n\n" - await self.send_delta(msg.chat_id, delta) + await self.send_delta( + msg.chat_id, + "\n\n" + self._format_tool_hint_delta(hint) + "\n\n", + ) return - await self._send_tool_hint_card( - receive_id_type, msg.chat_id, hint + # No active streaming card — send as a regular + # interactive card with the same 🔧 prefix style. + card = json.dumps( + {"config": {"wide_screen_mode": True}, "elements": [ + {"tag": "markdown", "content": self._format_tool_hint_delta(hint)}, + ]}, + ensure_ascii=False, + ) + await loop.run_in_executor( + None, self._send_message_sync, receive_id_type, msg.chat_id, "interactive", card ) return @@ -1708,33 +1708,9 @@ class FeishuChannel(BaseChannel): return "\n".join(part for part in parts if part) - async def _send_tool_hint_card( - self, receive_id_type: str, receive_id: str, tool_hint: str - ) -> None: - """Send tool hint as an interactive card with formatted code block. - - Args: - receive_id_type: "chat_id" or "open_id" - receive_id: The target chat or user ID - tool_hint: Formatted tool hint string (e.g., 'web_search("q"), read_file("path")') - """ - loop = asyncio.get_running_loop() - - # Put each top-level tool call on its own line without altering commas inside arguments. - formatted_code = self.__class__._format_tool_hint_lines(tool_hint) - - card = { - "config": {"wide_screen_mode": True}, - "elements": [ - {"tag": "markdown", "content": f"**Tool Calls**\n\n```text\n{formatted_code}\n```"} - ], - } - - await loop.run_in_executor( - None, - self._send_message_sync, - receive_id_type, - receive_id, - "interactive", - json.dumps(card, ensure_ascii=False), + def _format_tool_hint_delta(self, tool_hint: str) -> str: + """Format a tool hint string with the 🔧 prefix for each line.""" + lines = self.__class__._format_tool_hint_lines(tool_hint).split("\n") + return "\n".join( + f"{self.config.tool_hint_prefix} {ln}" for ln in lines if ln.strip() ) diff --git a/tests/channels/test_feishu_streaming.py b/tests/channels/test_feishu_streaming.py index a047c8c5..4bef8354 100644 --- a/tests/channels/test_feishu_streaming.py +++ b/tests/channels/test_feishu_streaming.py @@ -205,53 +205,22 @@ class TestSendDelta: ch._client.im.v1.message.create.assert_called_once() @pytest.mark.asyncio - async def test_stream_end_resuming_keeps_buffer(self): - """_resuming=True flushes text to card but keeps the buffer for the next segment.""" + async def test_stream_end_fallback_when_final_update_fails(self): + """If streaming mode was closed (e.g. Feishu timeout), fall back to a regular card.""" ch = _make_channel() ch._stream_bufs["oc_chat1"] = _FeishuStreamBuf( - text="Partial answer", card_id="card_1", sequence=2, last_edit=0.0, + text="Lost content", card_id="card_1", sequence=3, last_edit=0.0, ) - ch._client.cardkit.v1.card_element.content.return_value = _mock_content_response() + ch._client.cardkit.v1.card_element.content.return_value = _mock_content_response(success=False) + ch._client.im.v1.message.create.return_value = _mock_send_response("om_fb") - await ch.send_delta("oc_chat1", "", metadata={"_stream_end": True, "_resuming": True}) - - assert "oc_chat1" in ch._stream_bufs - buf = ch._stream_bufs["oc_chat1"] - assert buf.card_id == "card_1" - assert buf.sequence == 3 - ch._client.cardkit.v1.card_element.content.assert_called_once() - ch._client.cardkit.v1.card.settings.assert_not_called() - - @pytest.mark.asyncio - async def test_stream_end_resuming_then_final_end(self): - """Full multi-segment flow: resuming mid-turn, then final end closes the card.""" - ch = _make_channel() - ch._stream_bufs["oc_chat1"] = _FeishuStreamBuf( - text="Seg1", card_id="card_1", sequence=1, last_edit=0.0, - ) - ch._client.cardkit.v1.card_element.content.return_value = _mock_content_response() - ch._client.cardkit.v1.card.settings.return_value = _mock_content_response() - - await ch.send_delta("oc_chat1", "", metadata={"_stream_end": True, "_resuming": True}) - assert "oc_chat1" in ch._stream_bufs - - ch._stream_bufs["oc_chat1"].text += " Seg2" await ch.send_delta("oc_chat1", "", metadata={"_stream_end": True}) assert "oc_chat1" not in ch._stream_bufs - ch._client.cardkit.v1.card.settings.assert_called_once() - - @pytest.mark.asyncio - async def test_stream_end_resuming_no_card_is_noop(self): - """_resuming with no card_id (card creation failed) is a safe no-op.""" - ch = _make_channel() - ch._stream_bufs["oc_chat1"] = _FeishuStreamBuf( - text="text", card_id=None, sequence=0, last_edit=0.0, - ) - await ch.send_delta("oc_chat1", "", metadata={"_stream_end": True, "_resuming": True}) - - assert "oc_chat1" in ch._stream_bufs - ch._client.cardkit.v1.card_element.content.assert_not_called() + # Should NOT attempt to close streaming mode since update failed + ch._client.cardkit.v1.card.settings.assert_not_called() + # Should fall back to sending a regular interactive card + ch._client.im.v1.message.create.assert_called_once() @pytest.mark.asyncio async def test_stream_end_without_buf_is_noop(self): @@ -375,22 +344,6 @@ class TestToolHintInlineStreaming: assert "🔧 $ cd /project" in buf.text assert "🔧 $ git status" in buf.text - @pytest.mark.asyncio - async def test_tool_hint_preserved_on_resuming_flush(self): - """When _resuming flushes the buffer, tool hint is kept as permanent content.""" - ch = _make_channel() - ch._stream_bufs["oc_chat1"] = _FeishuStreamBuf( - text="Partial answer\n\n🔧 $ cd /project\n\n", - card_id="card_1", sequence=2, last_edit=0.0, - ) - ch._client.cardkit.v1.card_element.content.return_value = _mock_content_response() - - await ch.send_delta("oc_chat1", "", metadata={"_stream_end": True, "_resuming": True}) - - buf = ch._stream_bufs["oc_chat1"] - assert "Partial answer" in buf.text - assert "🔧 $ cd /project" in buf.text - @pytest.mark.asyncio async def test_tool_hint_preserved_on_final_stream_end(self): """When final _stream_end closes the card, tool hint is kept in the final text.""" diff --git a/tests/channels/test_feishu_tool_hint_code_block.py b/tests/channels/test_feishu_tool_hint_code_block.py index a5db5ad6..4f9d214c 100644 --- a/tests/channels/test_feishu_tool_hint_code_block.py +++ b/tests/channels/test_feishu_tool_hint_code_block.py @@ -1,6 +1,7 @@ -"""Tests for FeishuChannel tool hint code block formatting.""" +"""Tests for FeishuChannel tool hint formatting.""" import json +from types import SimpleNamespace from unittest.mock import MagicMock, patch import pytest @@ -28,15 +29,24 @@ def mock_feishu_channel(): config.app_secret = "test_app_secret" config.encrypt_key = None config.verification_token = None + config.tool_hint_prefix = "\U0001f527" # 🔧 bus = MagicMock() channel = FeishuChannel(config, bus) - channel._client = MagicMock() # Simulate initialized client + channel._client = MagicMock() return channel +def _get_tool_hint_card(mock_send): + """Extract the interactive card from _send_message_sync calls.""" + call_args = mock_send.call_args[0] + _, _, msg_type, content = call_args + assert msg_type == "interactive" + return json.loads(content) + + @mark.asyncio -async def test_tool_hint_sends_code_message(mock_feishu_channel): - """Tool hint messages should be sent as interactive cards with code blocks.""" +async def test_tool_hint_sends_interactive_card(mock_feishu_channel): + """Tool hint without active buffer sends an interactive card with 🔧 style.""" msg = OutboundMessage( channel="feishu", chat_id="oc_123456", @@ -47,23 +57,12 @@ async def test_tool_hint_sends_code_message(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - # Verify interactive message with card was sent assert mock_send.call_count == 1 - call_args = mock_send.call_args[0] - receive_id_type, receive_id, msg_type, content = call_args - - assert receive_id_type == "chat_id" - assert receive_id == "oc_123456" - assert msg_type == "interactive" - - # Parse content to verify card structure - card = json.loads(content) + card = _get_tool_hint_card(mock_send) assert card["config"]["wide_screen_mode"] is True - assert len(card["elements"]) == 1 - assert card["elements"][0]["tag"] == "markdown" - # Check that code block is properly formatted with language hint - expected_md = "**Tool Calls**\n\n```text\nweb_search(\"test query\")\n```" - assert card["elements"][0]["content"] == expected_md + md = card["elements"][0]["content"] + assert "\U0001f527" in md + assert "web_search" in md @mark.asyncio @@ -78,8 +77,6 @@ async def test_tool_hint_empty_content_does_not_send(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - - # Should not send any message mock_send.assert_not_called() @@ -96,7 +93,6 @@ async def test_tool_hint_without_metadata_sends_as_normal(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - # Should send as text message (detected format) assert mock_send.call_count == 1 call_args = mock_send.call_args[0] _, _, msg_type, content = call_args @@ -106,7 +102,7 @@ async def test_tool_hint_without_metadata_sends_as_normal(mock_feishu_channel): @mark.asyncio async def test_tool_hint_multiple_tools_in_one_message(mock_feishu_channel): - """Multiple tool calls should be displayed each on its own line in a code block.""" + """Multiple tool calls should each get the 🔧 prefix.""" msg = OutboundMessage( channel="feishu", chat_id="oc_123456", @@ -117,13 +113,11 @@ async def test_tool_hint_multiple_tools_in_one_message(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - call_args = mock_send.call_args[0] - msg_type = call_args[2] - content = json.loads(call_args[3]) - assert msg_type == "interactive" - # Each tool call should be on its own line - expected_md = "**Tool Calls**\n\n```text\nweb_search(\"query\"),\nread_file(\"/path/to/file\")\n```" - assert content["elements"][0]["content"] == expected_md + card = _get_tool_hint_card(mock_send) + md = card["elements"][0]["content"] + assert "web_search" in md + assert "read_file" in md + assert "\U0001f527" in md @mark.asyncio @@ -139,8 +133,8 @@ async def test_tool_hint_new_format_basic(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - content = json.loads(mock_send.call_args[0][3]) - md = content["elements"][0]["content"] + card = _get_tool_hint_card(mock_send) + md = card["elements"][0]["content"] assert "read src/main.py" in md assert 'grep "TODO"' in md @@ -158,16 +152,15 @@ async def test_tool_hint_new_format_with_comma_in_quotes(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - content = json.loads(mock_send.call_args[0][3]) - md = content["elements"][0]["content"] - # The comma inside quotes should NOT cause a line break + card = _get_tool_hint_card(mock_send) + md = card["elements"][0]["content"] assert 'grep "hello, world"' in md assert "$ echo test" in md @mark.asyncio async def test_tool_hint_new_format_with_folding(mock_feishu_channel): - """Folded calls (× N) should display on separate lines.""" + """Folded calls (× N) should display correctly.""" msg = OutboundMessage( channel="feishu", chat_id="oc_123456", @@ -178,8 +171,8 @@ async def test_tool_hint_new_format_with_folding(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - content = json.loads(mock_send.call_args[0][3]) - md = content["elements"][0]["content"] + card = _get_tool_hint_card(mock_send) + md = card["elements"][0]["content"] assert "\u00d7 3" in md assert 'grep "pattern"' in md @@ -197,9 +190,12 @@ async def test_tool_hint_new_format_mcp(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - content = json.loads(mock_send.call_args[0][3]) - md = content["elements"][0]["content"] + card = _get_tool_hint_card(mock_send) + md = card["elements"][0]["content"] assert "4_5v::analyze_image" in md + + +@mark.asyncio async def test_tool_hint_keeps_commas_inside_arguments(mock_feishu_channel): """Commas inside a single tool argument must not be split onto a new line.""" msg = OutboundMessage( @@ -212,10 +208,7 @@ async def test_tool_hint_keeps_commas_inside_arguments(mock_feishu_channel): with patch.object(mock_feishu_channel, '_send_message_sync') as mock_send: await mock_feishu_channel.send(msg) - content = json.loads(mock_send.call_args[0][3]) - expected_md = ( - "**Tool Calls**\n\n```text\n" - "web_search(\"foo, bar\"),\n" - "read_file(\"/path/to/file\")\n```" - ) - assert content["elements"][0]["content"] == expected_md + card = _get_tool_hint_card(mock_send) + md = card["elements"][0]["content"] + assert 'web_search("foo, bar")' in md + assert 'read_file("/path/to/file")' in md From 873be5180b9a52ef495866732c584970cb481e41 Mon Sep 17 00:00:00 2001 From: yeyitech Date: Tue, 14 Apr 2026 14:31:33 +0800 Subject: [PATCH 10/32] feat(slack): resolve named message targets --- nanobot/channels/slack.py | 125 +++++++++++++++++++++++- tests/channels/test_slack_channel.py | 137 ++++++++++++++++++++++++++- 2 files changed, 255 insertions(+), 7 deletions(-) diff --git a/nanobot/channels/slack.py b/nanobot/channels/slack.py index 2503f6a2..af03d497 100644 --- a/nanobot/channels/slack.py +++ b/nanobot/channels/slack.py @@ -5,6 +5,7 @@ import re from typing import Any from loguru import logger +from pydantic import Field from slack_sdk.socket_mode.request import SocketModeRequest from slack_sdk.socket_mode.response import SocketModeResponse from slack_sdk.socket_mode.websockets import SocketModeClient @@ -13,8 +14,6 @@ from slackify_markdown import slackify_markdown from nanobot.bus.events import OutboundMessage from nanobot.bus.queue import MessageBus -from pydantic import Field - from nanobot.channels.base import BaseChannel from nanobot.config.schema import Base @@ -50,6 +49,9 @@ class SlackChannel(BaseChannel): name = "slack" display_name = "Slack" + _SLACK_ID_RE = re.compile(r"^[CDGUW][A-Z0-9]{2,}$") + _SLACK_CHANNEL_REF_RE = re.compile(r"^<#([A-Z0-9]+)(?:\|[^>]+)?>$") + _SLACK_USER_REF_RE = re.compile(r"^<@([A-Z0-9]+)(?:\|[^>]+)?>$") @classmethod def default_config(cls) -> dict[str, Any]: @@ -63,6 +65,7 @@ class SlackChannel(BaseChannel): self._web_client: AsyncWebClient | None = None self._socket_client: SocketModeClient | None = None self._bot_user_id: str | None = None + self._target_cache: dict[str, str] = {} async def start(self) -> None: """Start the Slack Socket Mode client.""" @@ -113,6 +116,7 @@ class SlackChannel(BaseChannel): logger.warning("Slack client not running") return try: + target_chat_id = await self._resolve_target_chat_id(msg.chat_id) slack_meta = msg.metadata.get("slack", {}) if msg.metadata else {} thread_ts = slack_meta.get("thread_ts") channel_type = slack_meta.get("channel_type") @@ -123,7 +127,7 @@ class SlackChannel(BaseChannel): # but send a single blank message when the bot has no text or files to send. if msg.content or not (msg.media or []): await self._web_client.chat_postMessage( - channel=msg.chat_id, + channel=target_chat_id, text=self._to_mrkdwn(msg.content) if msg.content else " ", thread_ts=thread_ts_param, ) @@ -131,7 +135,7 @@ class SlackChannel(BaseChannel): for media_path in msg.media or []: try: await self._web_client.files_upload_v2( - channel=msg.chat_id, + channel=target_chat_id, file=media_path, thread_ts=thread_ts_param, ) @@ -141,12 +145,123 @@ class SlackChannel(BaseChannel): # Update reaction emoji when the final (non-progress) response is sent if not (msg.metadata or {}).get("_progress"): event = slack_meta.get("event", {}) - await self._update_react_emoji(msg.chat_id, event.get("ts")) + await self._update_react_emoji(event.get("channel") or msg.chat_id, event.get("ts")) except Exception as e: logger.error("Error sending Slack message: {}", e) raise + async def _resolve_target_chat_id(self, target: str) -> str: + """Resolve human-friendly Slack targets to concrete IDs when needed.""" + if not self._web_client: + return target + + target = target.strip() + if not target: + return target + + if match := self._SLACK_CHANNEL_REF_RE.fullmatch(target): + return match.group(1) + if match := self._SLACK_USER_REF_RE.fullmatch(target): + return await self._open_dm_for_user(match.group(1)) + if self._SLACK_ID_RE.fullmatch(target): + if target.startswith(("U", "W")): + return await self._open_dm_for_user(target) + return target + + if target.startswith("#"): + return await self._resolve_channel_name(target[1:]) + if target.startswith("@"): + return await self._resolve_user_handle(target[1:]) + + try: + return await self._resolve_channel_name(target) + except ValueError: + return await self._resolve_user_handle(target) + + async def _resolve_channel_name(self, name: str) -> str: + normalized = self._normalize_target_name(name) + if not normalized: + raise ValueError("Slack target channel name is empty") + + cache_key = f"channel:{normalized}" + if cache_key in self._target_cache: + return self._target_cache[cache_key] + + cursor: str | None = None + while True: + response = await self._web_client.conversations_list( + types="public_channel,private_channel", + exclude_archived=True, + limit=200, + cursor=cursor, + ) + for channel in response.get("channels", []): + if self._normalize_target_name(str(channel.get("name") or "")) == normalized: + channel_id = str(channel.get("id") or "") + if channel_id: + self._target_cache[cache_key] = channel_id + return channel_id + cursor = ((response.get("response_metadata") or {}).get("next_cursor") or "").strip() + if not cursor: + break + + raise ValueError( + f"Slack channel '{name}' was not found. Use a joined channel name like " + f"'#general' or a concrete channel ID." + ) + + async def _resolve_user_handle(self, handle: str) -> str: + normalized = self._normalize_target_name(handle) + if not normalized: + raise ValueError("Slack target user handle is empty") + + cache_key = f"user:{normalized}" + if cache_key in self._target_cache: + return self._target_cache[cache_key] + + cursor: str | None = None + while True: + response = await self._web_client.users_list(limit=200, cursor=cursor) + for member in response.get("members", []): + if self._member_matches_handle(member, normalized): + user_id = str(member.get("id") or "") + if not user_id: + continue + dm_id = await self._open_dm_for_user(user_id) + self._target_cache[cache_key] = dm_id + return dm_id + cursor = ((response.get("response_metadata") or {}).get("next_cursor") or "").strip() + if not cursor: + break + + raise ValueError( + f"Slack user '{handle}' was not found. Use '@name' or a concrete DM/channel ID." + ) + + async def _open_dm_for_user(self, user_id: str) -> str: + response = await self._web_client.conversations_open(users=user_id) + channel_id = str(((response.get("channel") or {}).get("id")) or "") + if not channel_id: + raise ValueError(f"Slack DM target for user '{user_id}' could not be opened.") + return channel_id + + @staticmethod + def _normalize_target_name(value: str) -> str: + return value.strip().lstrip("#@").lower() + + @classmethod + def _member_matches_handle(cls, member: dict[str, Any], normalized: str) -> bool: + profile = member.get("profile") or {} + candidates = { + str(member.get("name") or ""), + str(profile.get("display_name") or ""), + str(profile.get("display_name_normalized") or ""), + str(profile.get("real_name") or ""), + str(profile.get("real_name_normalized") or ""), + } + return normalized in {cls._normalize_target_name(candidate) for candidate in candidates if candidate} + async def _on_socket_request( self, client: SocketModeClient, diff --git a/tests/channels/test_slack_channel.py b/tests/channels/test_slack_channel.py index f7eec95c..6fb05a91 100644 --- a/tests/channels/test_slack_channel.py +++ b/tests/channels/test_slack_channel.py @@ -10,8 +10,7 @@ except ImportError: from nanobot.bus.events import OutboundMessage from nanobot.bus.queue import MessageBus -from nanobot.channels.slack import SlackChannel -from nanobot.channels.slack import SlackConfig +from nanobot.channels.slack import SlackChannel, SlackConfig class _FakeAsyncWebClient: @@ -20,6 +19,12 @@ class _FakeAsyncWebClient: self.file_upload_calls: list[dict[str, object | None]] = [] self.reactions_add_calls: list[dict[str, object | None]] = [] self.reactions_remove_calls: list[dict[str, object | None]] = [] + self.conversations_list_calls: list[dict[str, object | None]] = [] + self.users_list_calls: list[dict[str, object | None]] = [] + self.conversations_open_calls: list[dict[str, object | None]] = [] + self._conversations_pages: list[dict[str, object]] = [] + self._users_pages: list[dict[str, object]] = [] + self._open_dm_response: dict[str, object] = {"channel": {"id": "D_OPENED"}} async def chat_postMessage( self, @@ -81,6 +86,22 @@ class _FakeAsyncWebClient: } ) + async def conversations_list(self, **kwargs): + self.conversations_list_calls.append(kwargs) + if self._conversations_pages: + return self._conversations_pages.pop(0) + return {"channels": [], "response_metadata": {"next_cursor": ""}} + + async def users_list(self, **kwargs): + self.users_list_calls.append(kwargs) + if self._users_pages: + return self._users_pages.pop(0) + return {"members": [], "response_metadata": {"next_cursor": ""}} + + async def conversations_open(self, **kwargs): + self.conversations_open_calls.append(kwargs) + return self._open_dm_response + @pytest.mark.asyncio async def test_send_uses_thread_for_channel_messages() -> None: @@ -151,3 +172,115 @@ async def test_send_updates_reaction_when_final_response_sent() -> None: assert fake_web.reactions_add_calls == [ {"channel": "C123", "name": "white_check_mark", "timestamp": "1700000000.000100"} ] + + +@pytest.mark.asyncio +async def test_send_resolves_channel_name_to_channel_id() -> None: + channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) + fake_web = _FakeAsyncWebClient() + fake_web._conversations_pages = [ + { + "channels": [{"id": "C999", "name": "channel_x"}], + "response_metadata": {"next_cursor": ""}, + } + ] + channel._web_client = fake_web + + await channel.send( + OutboundMessage( + channel="slack", + chat_id="#channel_x", + content="hello", + ) + ) + + assert fake_web.chat_post_calls == [ + {"channel": "C999", "text": "hello\n", "thread_ts": None} + ] + assert len(fake_web.conversations_list_calls) == 1 + + +@pytest.mark.asyncio +async def test_send_resolves_user_handle_to_dm_channel() -> None: + channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) + fake_web = _FakeAsyncWebClient() + fake_web._users_pages = [ + { + "members": [ + { + "id": "U234", + "name": "alice", + "profile": {"display_name": "Alice"}, + } + ], + "response_metadata": {"next_cursor": ""}, + } + ] + fake_web._open_dm_response = {"channel": {"id": "D234"}} + channel._web_client = fake_web + + await channel.send( + OutboundMessage( + channel="slack", + chat_id="@alice", + content="hello", + ) + ) + + assert fake_web.conversations_open_calls == [{"users": "U234"}] + assert fake_web.chat_post_calls == [ + {"channel": "D234", "text": "hello\n", "thread_ts": None} + ] + + +@pytest.mark.asyncio +async def test_send_updates_reaction_on_origin_channel_for_cross_channel_send() -> None: + channel = SlackChannel(SlackConfig(enabled=True, react_emoji="eyes"), MessageBus()) + fake_web = _FakeAsyncWebClient() + fake_web._conversations_pages = [ + { + "channels": [{"id": "C999", "name": "channel_x"}], + "response_metadata": {"next_cursor": ""}, + } + ] + channel._web_client = fake_web + + await channel.send( + OutboundMessage( + channel="slack", + chat_id="channel_x", + content="done", + metadata={ + "slack": { + "event": {"ts": "1700000000.000100", "channel": "D_ORIGIN"}, + "channel_type": "im", + }, + }, + ) + ) + + assert fake_web.chat_post_calls == [ + {"channel": "C999", "text": "done\n", "thread_ts": None} + ] + assert fake_web.reactions_remove_calls == [ + {"channel": "D_ORIGIN", "name": "eyes", "timestamp": "1700000000.000100"} + ] + assert fake_web.reactions_add_calls == [ + {"channel": "D_ORIGIN", "name": "white_check_mark", "timestamp": "1700000000.000100"} + ] + + +@pytest.mark.asyncio +async def test_send_raises_when_named_target_cannot_be_resolved() -> None: + channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) + fake_web = _FakeAsyncWebClient() + channel._web_client = fake_web + + with pytest.raises(ValueError, match="was not found"): + await channel.send( + OutboundMessage( + channel="slack", + chat_id="#missing-channel", + content="hello", + ) + ) From 0a51344483d8210d3673c4b0489557a8e0b217f8 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 11:53:06 +0000 Subject: [PATCH 11/32] fix(slack): keep cross-target sends out of origin threads When Slack resolves a named target to another conversation, do not reuse the origin thread timestamp on the destination send, and keep reaction cleanup anchored to the source conversation. Made-with: Cursor --- nanobot/channels/slack.py | 9 ++++++-- tests/channels/test_slack_channel.py | 32 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/nanobot/channels/slack.py b/nanobot/channels/slack.py index af03d497..c68020ce 100644 --- a/nanobot/channels/slack.py +++ b/nanobot/channels/slack.py @@ -120,8 +120,13 @@ class SlackChannel(BaseChannel): slack_meta = msg.metadata.get("slack", {}) if msg.metadata else {} thread_ts = slack_meta.get("thread_ts") channel_type = slack_meta.get("channel_type") + origin_chat_id = str((slack_meta.get("event", {}) or {}).get("channel") or msg.chat_id) # Slack DMs don't use threads; channel/group replies may keep thread_ts. - thread_ts_param = thread_ts if thread_ts and channel_type != "im" else None + thread_ts_param = ( + thread_ts + if thread_ts and channel_type != "im" and target_chat_id == origin_chat_id + else None + ) # Slack rejects empty text payloads. Keep media-only messages media-only, # but send a single blank message when the bot has no text or files to send. @@ -145,7 +150,7 @@ class SlackChannel(BaseChannel): # Update reaction emoji when the final (non-progress) response is sent if not (msg.metadata or {}).get("_progress"): event = slack_meta.get("event", {}) - await self._update_react_emoji(event.get("channel") or msg.chat_id, event.get("ts")) + await self._update_react_emoji(origin_chat_id, event.get("ts")) except Exception as e: logger.error("Error sending Slack message: {}", e) diff --git a/tests/channels/test_slack_channel.py b/tests/channels/test_slack_channel.py index 6fb05a91..2e72c4e6 100644 --- a/tests/channels/test_slack_channel.py +++ b/tests/channels/test_slack_channel.py @@ -270,6 +270,38 @@ async def test_send_updates_reaction_on_origin_channel_for_cross_channel_send() ] +@pytest.mark.asyncio +async def test_send_does_not_reuse_origin_thread_ts_for_cross_channel_send() -> None: + channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) + fake_web = _FakeAsyncWebClient() + fake_web._conversations_pages = [ + { + "channels": [{"id": "C999", "name": "channel_x"}], + "response_metadata": {"next_cursor": ""}, + } + ] + channel._web_client = fake_web + + await channel.send( + OutboundMessage( + channel="slack", + chat_id="channel_x", + content="done", + metadata={ + "slack": { + "event": {"ts": "1700000000.000100", "channel": "C_ORIGIN"}, + "thread_ts": "1700000000.000200", + "channel_type": "channel", + }, + }, + ) + ) + + assert fake_web.chat_post_calls == [ + {"channel": "C999", "text": "done\n", "thread_ts": None} + ] + + @pytest.mark.asyncio async def test_send_raises_when_named_target_cannot_be_resolved() -> None: channel = SlackChannel(SlackConfig(enabled=True), MessageBus()) From 47f579570856de31395c28475eb924c71ac94404 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 13:00:59 +0000 Subject: [PATCH 12/32] refactor: move document extraction from ContextBuilder to API layer ContextBuilder._build_user_content now only handles images (its original responsibility). Document text extraction (PDF, DOCX, XLSX, PPTX) is performed by the new _extract_documents() helper in server.py, called before process_direct(). This keeps the core context builder free of format-specific dependencies and makes the API boundary the single place where uploaded files are pre-processed. Tests updated to reflect the new responsibility boundary. Made-with: Cursor --- nanobot/agent/context.py | 52 +++++++------------------ nanobot/api/server.py | 41 +++++++++++++++++++- tests/test_api_attachment.py | 68 +++++++++++++++++++++++++++++---- tests/test_context_documents.py | 59 +++++++++------------------- 4 files changed, 131 insertions(+), 89 deletions(-) diff --git a/nanobot/agent/context.py b/nanobot/agent/context.py index 0996def5..cab7b057 100644 --- a/nanobot/agent/context.py +++ b/nanobot/agent/context.py @@ -147,56 +147,30 @@ class ContextBuilder: messages.append({"role": current_role, "content": merged}) return messages - def _build_user_content( - self, text: str, media: list[str] | None - ) -> str | list[dict[str, Any]]: - """Build user message content with optional media. - - Images are converted to base64 vision blocks. - Documents (PDF, Word, Excel, PPT) have their text extracted and appended. - """ + def _build_user_content(self, text: str, media: list[str] | None) -> str | list[dict[str, Any]]: + """Build user message content with optional base64-encoded images.""" if not media: return text - images: list[dict[str, Any]] = [] - doc_texts: list[str] = [] - + images = [] for path in media: p = Path(path) if not p.is_file(): continue raw = p.read_bytes() mime = detect_image_mime(raw) or mimetypes.guess_type(path)[0] + if not mime or not mime.startswith("image/"): + continue + b64 = base64.b64encode(raw).decode() + images.append({ + "type": "image_url", + "image_url": {"url": f"data:{mime};base64,{b64}"}, + "_meta": {"path": str(p)}, + }) - if mime and mime.startswith("image/"): - b64 = base64.b64encode(raw).decode() - images.append({ - "type": "image_url", - "image_url": {"url": f"data:{mime};base64,{b64}"}, - "_meta": {"path": str(p)}, - }) - else: - # Try document text extraction - from nanobot.utils.document import extract_text - extracted = extract_text(p) - if extracted and not extracted.startswith("[error:"): - doc_texts.append(f"[File: {p.name}]\n{extracted}") - - # Build final content - parts: list[dict[str, Any]] = [] - parts.extend(images) - - combined_text = text - if doc_texts: - combined_text = text + "\n\n" + "\n\n".join(doc_texts) - - if images: - parts.append({"type": "text", "text": combined_text}) - return parts - elif doc_texts: - return combined_text - else: + if not images: return text + return images + [{"type": "text", "text": text}] def add_tool_result( self, messages: list[dict[str, Any]], diff --git a/nanobot/api/server.py b/nanobot/api/server.py index 934879a3..fcba8b55 100644 --- a/nanobot/api/server.py +++ b/nanobot/api/server.py @@ -19,7 +19,8 @@ from aiohttp import web from loguru import logger from nanobot.config.paths import get_media_dir -from nanobot.utils.helpers import safe_filename +from nanobot.utils.document import extract_text +from nanobot.utils.helpers import detect_image_mime, safe_filename from nanobot.utils.runtime import EMPTY_FINAL_RESPONSE_MESSAGE MAX_FILE_SIZE = 10 * 1024 * 1024 # 10 MB @@ -161,6 +162,40 @@ async def _parse_multipart(request: web.Request) -> tuple[str, list[str], str | return text, media_paths, session_id +# --------------------------------------------------------------------------- +# Pre-processing: extract document text at the API boundary +# --------------------------------------------------------------------------- + +def _extract_documents(text: str, media_paths: list[str]) -> tuple[str, list[str]]: + """Separate images from documents in *media_paths*. + + Documents (PDF, DOCX, XLSX, PPTX, …) have their text extracted and + appended to *text*. Only image paths are kept in the returned list so + that downstream layers (ContextBuilder) only need to handle vision + blocks. + """ + image_paths: list[str] = [] + doc_texts: list[str] = [] + + for path_str in media_paths: + p = Path(path_str) + if not p.is_file(): + continue + raw = p.read_bytes() + mime = detect_image_mime(raw) or mimetypes.guess_type(path_str)[0] + if mime and mime.startswith("image/"): + image_paths.append(path_str) + else: + extracted = extract_text(p) + if extracted and not extracted.startswith("[error:"): + doc_texts.append(f"[File: {p.name}]\n{extracted}") + + if doc_texts: + text = text + "\n\n" + "\n\n".join(doc_texts) + + return text, image_paths + + # --------------------------------------------------------------------------- # Route handlers # --------------------------------------------------------------------------- @@ -197,6 +232,10 @@ async def handle_chat_completions(request: web.Request) -> web.Response: logger.exception("Error parsing upload") return _error_json(413, "File too large or invalid upload") + # Extract document text at the API boundary; only images stay in media. + if media_paths: + text, media_paths = _extract_documents(text, media_paths) + session_key = f"api:{session_id}" if session_id else API_SESSION_KEY session_locks: dict[str, asyncio.Lock] = request.app["session_locks"] session_lock = session_locks.setdefault(session_key, asyncio.Lock()) diff --git a/tests/test_api_attachment.py b/tests/test_api_attachment.py index 082494b7..ea8eed9d 100644 --- a/tests/test_api_attachment.py +++ b/tests/test_api_attachment.py @@ -10,6 +10,7 @@ import pytest import pytest_asyncio from nanobot.api.server import ( + _extract_documents, _FileSizeExceeded, _parse_json_content, _save_base64_data_url, @@ -184,7 +185,7 @@ def test_parse_json_content_rejects_oversized_base64_file(tmp_path) -> None: @pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") @pytest.mark.asyncio async def test_multipart_upload_saves_file(aiohttp_client, mock_agent, tmp_path) -> None: - """Multipart upload saves file to media dir and passes path to process_direct.""" + """Multipart upload of non-image extracts text into content (not media).""" import os original_cwd = os.getcwd() os.chdir(tmp_path) @@ -202,8 +203,9 @@ async def test_multipart_upload_saves_file(aiohttp_client, mock_agent, tmp_path) ) assert resp.status == 200 call_kwargs = mock_agent.process_direct.call_args.kwargs - assert call_kwargs["content"] == "analyze this" - assert len(call_kwargs.get("media", [])) == 1 + assert "analyze this" in call_kwargs["content"] + # Non-image file text is extracted into content, not kept as media + assert not call_kwargs.get("media") finally: os.chdir(original_cwd) @@ -371,13 +373,62 @@ async def test_json_base64_image_upload(aiohttp_client, mock_agent, tmp_path) -> # --------------------------------------------------------------------------- -# DOCX document extraction tests +# _extract_documents tests (API-layer document extraction) +# --------------------------------------------------------------------------- + +def test_extract_documents_separates_images_from_docs(tmp_path) -> None: + """Images stay in media; document text is appended to content.""" + from docx import Document + + png = tmp_path / "chart.png" + png.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) + + doc = Document() + doc.add_paragraph("Quarterly revenue is $5M") + docx_path = tmp_path / "report.docx" + doc.save(docx_path) + + text, image_paths = _extract_documents("summarize", [str(png), str(docx_path)]) + assert len(image_paths) == 1 + assert image_paths[0] == str(png) + assert "Quarterly revenue" in text + assert "summarize" in text + + +def test_extract_documents_skips_extraction_errors(tmp_path, monkeypatch) -> None: + """Document extraction errors should not leak into user text.""" + bad_file = tmp_path / "broken.docx" + bad_file.write_text("not a docx", encoding="utf-8") + + import nanobot.api.server as _srv + monkeypatch.setattr( + _srv, "extract_text", + lambda _path: "[error: failed to extract DOCX: boom]", + ) + + text, image_paths = _extract_documents("hello", [str(bad_file)]) + assert text == "hello" + assert image_paths == [] + + +def test_extract_documents_images_only(tmp_path) -> None: + """When all files are images, text is unchanged and all paths kept.""" + png = tmp_path / "a.png" + png.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) + text, image_paths = _extract_documents("describe", [str(png)]) + assert text == "describe" + assert len(image_paths) == 1 + + +# --------------------------------------------------------------------------- +# DOCX end-to-end upload test (API layer now extracts text) # --------------------------------------------------------------------------- @pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") @pytest.mark.asyncio async def test_docx_upload_extracted_and_sent(aiohttp_client, tmp_path) -> None: - """Uploaded DOCX should have its text extracted before being sent to AI.""" + """Uploaded DOCX text should be extracted at the API layer and + appended to the content string, not passed as media.""" from docx import Document agent = _make_mock_agent("This report shows $5M revenue") @@ -405,8 +456,9 @@ async def test_docx_upload_extracted_and_sent(aiohttp_client, tmp_path) -> None: resp = await client.post("/v1/chat/completions", data=data) assert resp.status == 200 call_kwargs = agent.process_direct.call_args.kwargs - media = call_kwargs.get("media", []) - assert len(media) == 1 - assert "report.docx" in media[0] + # Document text should be extracted into content, not media + assert "Total revenue" in call_kwargs["content"] + # No media (docx is not an image) + assert not call_kwargs.get("media") finally: os.chdir(original_cwd) diff --git a/tests/test_context_documents.py b/tests/test_context_documents.py index 9f503906..28a4f6d2 100644 --- a/tests/test_context_documents.py +++ b/tests/test_context_documents.py @@ -1,4 +1,8 @@ -"""Tests for context builder document handling.""" +"""Tests for context builder media handling. + +The ContextBuilder._build_user_content method should ONLY handle images. +Document text extraction is the responsibility of the API layer. +""" from __future__ import annotations @@ -30,52 +34,25 @@ def test_build_user_content_with_image_returns_list(tmp_path: Path) -> None: assert "text" in types -def test_build_user_content_with_docx_includes_extracted_text(tmp_path: Path) -> None: - """Document files should have their text extracted and included.""" - from docx import Document - - doc = Document() - doc.add_paragraph("Quarterly revenue is $5M") - docx_path = tmp_path / "report.docx" - doc.save(docx_path) - +def test_build_user_content_ignores_non_image_files(tmp_path: Path) -> None: + """Non-image files should be silently skipped — extraction is not context builder's job.""" builder = _make_builder(tmp_path) - result = builder._build_user_content("summarize this", [str(docx_path)]) - assert isinstance(result, str) - assert "Quarterly revenue" in result + txt = tmp_path / "notes.txt" + txt.write_text("some text", encoding="utf-8") + result = builder._build_user_content("summarize", [str(txt)]) + assert result == "summarize" -def test_build_user_content_mixed_image_and_document(tmp_path: Path) -> None: - """Mix of images and documents: images as base64, docs as text.""" - from docx import Document - +def test_build_user_content_mixed_image_and_non_image(tmp_path: Path) -> None: + """Only images should be included; non-image files are skipped.""" + builder = _make_builder(tmp_path) png = tmp_path / "chart.png" png.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) + txt = tmp_path / "report.txt" + txt.write_text("report text", encoding="utf-8") - doc = Document() - doc.add_paragraph("Report text here") - docx = tmp_path / "report.docx" - doc.save(docx) - - builder = _make_builder(tmp_path) - result = builder._build_user_content("analyze both", [str(png), str(docx)]) + result = builder._build_user_content("analyze", [str(png), str(txt)]) assert isinstance(result, list) assert any(b["type"] == "image_url" for b in result) text_parts = [b.get("text", "") for b in result if b.get("type") == "text"] - assert any("Report text here" in t for t in text_parts) - - -def test_build_user_content_skips_document_extraction_errors(tmp_path: Path, monkeypatch) -> None: - """Document extraction errors should not be embedded into the user prompt.""" - docx_path = tmp_path / "broken.docx" - docx_path.write_text("not a real docx", encoding="utf-8") - - builder = _make_builder(tmp_path) - - monkeypatch.setattr( - "nanobot.utils.document.extract_text", - lambda _path: "[error: failed to extract DOCX: boom]", - ) - - result = builder._build_user_content("summarize this", [str(docx_path)]) - assert result == "summarize this" + assert all("report text" not in t for t in text_parts) From 92d6fca3231c8174d66514e20b0d6707442a46ab Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 13:10:03 +0000 Subject: [PATCH 13/32] refactor: centralize document extraction in AgentLoop._process_message Move extract_documents() to nanobot.utils.document as a reusable helper and call it once in AgentLoop._process_message, the single entry point for all message processing (API + all channels). This replaces the previous API-only _extract_documents() in server.py, ensuring Telegram, Feishu, Slack, WeChat, and all other channels also benefit from automatic document text extraction. Adds a configurable max_file_size guard (default 50 MB) to skip oversized files gracefully, preventing unbounded memory/CPU usage from channel-downloaded attachments. - server.py: removed _extract_documents and related imports - document.py: added extract_documents() with size limit - loop.py: calls extract_documents() at the top of _process_message - Tests updated: 70 related tests pass Made-with: Cursor --- nanobot/agent/loop.py | 7 +++++ nanobot/api/server.py | 41 +----------------------- nanobot/utils/document.py | 60 ++++++++++++++++++++++++++++++++++++ tests/test_api_attachment.py | 56 ++++++++++++++++++--------------- 4 files changed, 99 insertions(+), 65 deletions(-) diff --git a/nanobot/agent/loop.py b/nanobot/agent/loop.py index 1e9f1787..2f80cd94 100644 --- a/nanobot/agent/loop.py +++ b/nanobot/agent/loop.py @@ -35,6 +35,7 @@ from nanobot.command import CommandContext, CommandRouter, register_builtin_comm from nanobot.config.schema import AgentDefaults from nanobot.providers.base import LLMProvider from nanobot.session.manager import Session, SessionManager +from nanobot.utils.document import extract_documents from nanobot.utils.helpers import image_placeholder_text from nanobot.utils.helpers import truncate_text as truncate_text_fn from nanobot.utils.runtime import EMPTY_FINAL_RESPONSE_MESSAGE @@ -653,6 +654,12 @@ class AgentLoop: content=final_content or "Background task completed.", ) + # Extract document text from media at the processing boundary so all + # channels benefit without format-specific logic in ContextBuilder. + if msg.media: + new_content, image_only = extract_documents(msg.content, msg.media) + msg = dataclasses.replace(msg, content=new_content, media=image_only) + preview = msg.content[:80] + "..." if len(msg.content) > 80 else msg.content logger.info("Processing message from {}:{}: {}", msg.channel, msg.sender_id, preview) diff --git a/nanobot/api/server.py b/nanobot/api/server.py index fcba8b55..934879a3 100644 --- a/nanobot/api/server.py +++ b/nanobot/api/server.py @@ -19,8 +19,7 @@ from aiohttp import web from loguru import logger from nanobot.config.paths import get_media_dir -from nanobot.utils.document import extract_text -from nanobot.utils.helpers import detect_image_mime, safe_filename +from nanobot.utils.helpers import safe_filename from nanobot.utils.runtime import EMPTY_FINAL_RESPONSE_MESSAGE MAX_FILE_SIZE = 10 * 1024 * 1024 # 10 MB @@ -162,40 +161,6 @@ async def _parse_multipart(request: web.Request) -> tuple[str, list[str], str | return text, media_paths, session_id -# --------------------------------------------------------------------------- -# Pre-processing: extract document text at the API boundary -# --------------------------------------------------------------------------- - -def _extract_documents(text: str, media_paths: list[str]) -> tuple[str, list[str]]: - """Separate images from documents in *media_paths*. - - Documents (PDF, DOCX, XLSX, PPTX, …) have their text extracted and - appended to *text*. Only image paths are kept in the returned list so - that downstream layers (ContextBuilder) only need to handle vision - blocks. - """ - image_paths: list[str] = [] - doc_texts: list[str] = [] - - for path_str in media_paths: - p = Path(path_str) - if not p.is_file(): - continue - raw = p.read_bytes() - mime = detect_image_mime(raw) or mimetypes.guess_type(path_str)[0] - if mime and mime.startswith("image/"): - image_paths.append(path_str) - else: - extracted = extract_text(p) - if extracted and not extracted.startswith("[error:"): - doc_texts.append(f"[File: {p.name}]\n{extracted}") - - if doc_texts: - text = text + "\n\n" + "\n\n".join(doc_texts) - - return text, image_paths - - # --------------------------------------------------------------------------- # Route handlers # --------------------------------------------------------------------------- @@ -232,10 +197,6 @@ async def handle_chat_completions(request: web.Request) -> web.Response: logger.exception("Error parsing upload") return _error_json(413, "File too large or invalid upload") - # Extract document text at the API boundary; only images stay in media. - if media_paths: - text, media_paths = _extract_documents(text, media_paths) - session_key = f"api:{session_id}" if session_id else API_SESSION_KEY session_locks: dict[str, asyncio.Lock] = request.app["session_locks"] session_lock = session_locks.setdefault(session_key, asyncio.Lock()) diff --git a/nanobot/utils/document.py b/nanobot/utils/document.py index 23e8eeee..e2aaeade 100644 --- a/nanobot/utils/document.py +++ b/nanobot/utils/document.py @@ -1,9 +1,12 @@ """Document text extraction utilities for nanobot.""" +import mimetypes from pathlib import Path from loguru import logger +from nanobot.utils.helpers import detect_image_mime + try: from pypdf import PdfReader except ImportError: @@ -204,3 +207,60 @@ def _is_text_extension(ext: str) -> bool: ".ini", ".cfg", } + + +# --------------------------------------------------------------------------- +# High-level helper: split media into images + extracted document text +# --------------------------------------------------------------------------- + +_MAX_EXTRACT_FILE_SIZE = 50 * 1024 * 1024 # 50 MB + + +def extract_documents( + text: str, + media_paths: list[str], + *, + max_file_size: int = _MAX_EXTRACT_FILE_SIZE, +) -> tuple[str, list[str]]: + """Separate images from documents in *media_paths*. + + Documents (PDF, DOCX, XLSX, PPTX, plain-text, …) have their text + extracted and appended to *text*. Only image paths are kept in the + returned list so that downstream layers only need to handle vision + blocks. + + Files larger than *max_file_size* bytes are skipped with a warning + to avoid unbounded memory / CPU usage. + """ + image_paths: list[str] = [] + doc_texts: list[str] = [] + + for path_str in media_paths: + p = Path(path_str) + if not p.is_file(): + continue + + try: + size = p.stat().st_size + except OSError: + continue + if size > max_file_size: + logger.warning( + "Skipping oversized file for extraction: {} ({:.1f} MB > {} MB limit)", + p.name, size / (1024 * 1024), max_file_size // (1024 * 1024), + ) + continue + + raw = p.read_bytes() + mime = detect_image_mime(raw) or mimetypes.guess_type(path_str)[0] + if mime and mime.startswith("image/"): + image_paths.append(path_str) + else: + extracted = extract_text(p) + if extracted and not extracted.startswith("[error:"): + doc_texts.append(f"[File: {p.name}]\n{extracted}") + + if doc_texts: + text = text + "\n\n" + "\n\n".join(doc_texts) + + return text, image_paths diff --git a/tests/test_api_attachment.py b/tests/test_api_attachment.py index ea8eed9d..6bd3676c 100644 --- a/tests/test_api_attachment.py +++ b/tests/test_api_attachment.py @@ -10,12 +10,12 @@ import pytest import pytest_asyncio from nanobot.api.server import ( - _extract_documents, _FileSizeExceeded, _parse_json_content, _save_base64_data_url, create_app, ) +from nanobot.utils.document import extract_documents try: from aiohttp.test_utils import TestClient, TestServer @@ -185,7 +185,7 @@ def test_parse_json_content_rejects_oversized_base64_file(tmp_path) -> None: @pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") @pytest.mark.asyncio async def test_multipart_upload_saves_file(aiohttp_client, mock_agent, tmp_path) -> None: - """Multipart upload of non-image extracts text into content (not media).""" + """Multipart upload saves file to media dir and passes path to process_direct.""" import os original_cwd = os.getcwd() os.chdir(tmp_path) @@ -203,9 +203,8 @@ async def test_multipart_upload_saves_file(aiohttp_client, mock_agent, tmp_path) ) assert resp.status == 200 call_kwargs = mock_agent.process_direct.call_args.kwargs - assert "analyze this" in call_kwargs["content"] - # Non-image file text is extracted into content, not kept as media - assert not call_kwargs.get("media") + assert call_kwargs["content"] == "analyze this" + assert len(call_kwargs.get("media") or []) == 1 finally: os.chdir(original_cwd) @@ -373,7 +372,7 @@ async def test_json_base64_image_upload(aiohttp_client, mock_agent, tmp_path) -> # --------------------------------------------------------------------------- -# _extract_documents tests (API-layer document extraction) +# extract_documents tests (now in nanobot.utils.document) # --------------------------------------------------------------------------- def test_extract_documents_separates_images_from_docs(tmp_path) -> None: @@ -388,7 +387,7 @@ def test_extract_documents_separates_images_from_docs(tmp_path) -> None: docx_path = tmp_path / "report.docx" doc.save(docx_path) - text, image_paths = _extract_documents("summarize", [str(png), str(docx_path)]) + text, image_paths = extract_documents("summarize", [str(png), str(docx_path)]) assert len(image_paths) == 1 assert image_paths[0] == str(png) assert "Quarterly revenue" in text @@ -400,13 +399,13 @@ def test_extract_documents_skips_extraction_errors(tmp_path, monkeypatch) -> Non bad_file = tmp_path / "broken.docx" bad_file.write_text("not a docx", encoding="utf-8") - import nanobot.api.server as _srv + import nanobot.utils.document as _doc monkeypatch.setattr( - _srv, "extract_text", + _doc, "extract_text", lambda _path: "[error: failed to extract DOCX: boom]", ) - text, image_paths = _extract_documents("hello", [str(bad_file)]) + text, image_paths = extract_documents("hello", [str(bad_file)]) assert text == "hello" assert image_paths == [] @@ -415,23 +414,31 @@ def test_extract_documents_images_only(tmp_path) -> None: """When all files are images, text is unchanged and all paths kept.""" png = tmp_path / "a.png" png.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) - text, image_paths = _extract_documents("describe", [str(png)]) + text, image_paths = extract_documents("describe", [str(png)]) assert text == "describe" assert len(image_paths) == 1 +def test_extract_documents_skips_oversized_files(tmp_path) -> None: + """Files exceeding the size limit should be silently skipped.""" + big = tmp_path / "huge.txt" + big.write_bytes(b"x" * 200) + + text, image_paths = extract_documents("hello", [str(big)], max_file_size=100) + assert text == "hello" + assert image_paths == [] + + # --------------------------------------------------------------------------- -# DOCX end-to-end upload test (API layer now extracts text) +# DOCX upload test — API saves file, loop layer extracts text # --------------------------------------------------------------------------- @pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") @pytest.mark.asyncio -async def test_docx_upload_extracted_and_sent(aiohttp_client, tmp_path) -> None: - """Uploaded DOCX text should be extracted at the API layer and - appended to the content string, not passed as media.""" - from docx import Document - - agent = _make_mock_agent("This report shows $5M revenue") +async def test_docx_upload_passes_media_path(aiohttp_client, tmp_path) -> None: + """Uploaded DOCX is saved to disk and its path passed as media. + (Text extraction happens later in AgentLoop._process_message.)""" + agent = _make_mock_agent("report summary") import os original_cwd = os.getcwd() os.chdir(tmp_path) @@ -440,25 +447,24 @@ async def test_docx_upload_extracted_and_sent(aiohttp_client, tmp_path) -> None: app = create_app(agent, model_name="m") client = await aiohttp_client(app) + from docx import Document doc = Document() - doc.add_heading("Q1 Report", level=1) doc.add_paragraph("Total revenue: $5,000,000") buf = BytesIO() doc.save(buf) - docx_bytes = buf.getvalue() import aiohttp data = aiohttp.FormData() data.add_field("message", "summarize the report") - data.add_field("files", docx_bytes, filename="report.docx", + data.add_field("files", buf.getvalue(), filename="report.docx", content_type="application/vnd.openxmlformats-officedocument.wordprocessingml.document") resp = await client.post("/v1/chat/completions", data=data) assert resp.status == 200 call_kwargs = agent.process_direct.call_args.kwargs - # Document text should be extracted into content, not media - assert "Total revenue" in call_kwargs["content"] - # No media (docx is not an image) - assert not call_kwargs.get("media") + assert call_kwargs["content"] == "summarize the report" + media = call_kwargs.get("media", []) + assert len(media) == 1 + assert "report.docx" in media[0] finally: os.chdir(original_cwd) From c937c07178b517067733bf05037aac98d031bb1c Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 13:15:04 +0000 Subject: [PATCH 14/32] fix: two bugs in document extraction pipeline Bug 1: _drain_pending did not call extract_documents on follow-up messages arriving mid-turn. Documents attached to queued messages were silently dropped because _build_user_content only handles images. Fix: call extract_documents before _build_user_content in _drain_pending. Bug 2: extract_documents read the entire file into memory (up to 50 MB) just to check 16 bytes of magic header for MIME detection. Fix: read only the first 16 bytes via open()+read(16) instead of Path.read_bytes(). Added regression tests for both bugs. Made-with: Cursor --- nanobot/agent/loop.py | 10 +++--- nanobot/utils/document.py | 5 +-- tests/test_api_attachment.py | 26 +++++++++++++++ tests/test_context_documents.py | 57 ++++++++++++++++++++++++++++++++- 4 files changed, 91 insertions(+), 7 deletions(-) diff --git a/nanobot/agent/loop.py b/nanobot/agent/loop.py index 2f80cd94..20694194 100644 --- a/nanobot/agent/loop.py +++ b/nanobot/agent/loop.py @@ -385,10 +385,12 @@ class AgentLoop: pending_msg = pending_queue.get_nowait() except asyncio.QueueEmpty: break - user_content = self.context._build_user_content( - pending_msg.content, - pending_msg.media if pending_msg.media else None, - ) + content = pending_msg.content + media = pending_msg.media if pending_msg.media else None + if media: + content, media = extract_documents(content, media) + media = media or None + user_content = self.context._build_user_content(content, media) runtime_ctx = self.context._build_runtime_context( pending_msg.channel, pending_msg.chat_id, diff --git a/nanobot/utils/document.py b/nanobot/utils/document.py index e2aaeade..a27b0e7a 100644 --- a/nanobot/utils/document.py +++ b/nanobot/utils/document.py @@ -251,8 +251,9 @@ def extract_documents( ) continue - raw = p.read_bytes() - mime = detect_image_mime(raw) or mimetypes.guess_type(path_str)[0] + with open(p, "rb") as f: + header = f.read(16) + mime = detect_image_mime(header) or mimetypes.guess_type(path_str)[0] if mime and mime.startswith("image/"): image_paths.append(path_str) else: diff --git a/tests/test_api_attachment.py b/tests/test_api_attachment.py index 6bd3676c..92e09ef8 100644 --- a/tests/test_api_attachment.py +++ b/tests/test_api_attachment.py @@ -429,6 +429,32 @@ def test_extract_documents_skips_oversized_files(tmp_path) -> None: assert image_paths == [] +def test_extract_documents_does_not_read_full_file_for_mime(tmp_path) -> None: + """MIME detection should only read header bytes, not the entire file.""" + from pathlib import Path as _Path + + big_txt = tmp_path / "big.txt" + big_txt.write_bytes(b"hello world " * 100_000) # ~1.2 MB + + original_read_bytes = _Path.read_bytes + read_sizes: list[int] = [] + + def _tracking_read_bytes(self): + data = original_read_bytes(self) + read_sizes.append(len(data)) + return data + + import unittest.mock + with unittest.mock.patch.object(_Path, "read_bytes", _tracking_read_bytes): + extract_documents("test", [str(big_txt)]) + + # If the full file was read for MIME detection, read_sizes would + # contain a >1MB entry. After the fix, only a small header is read. + assert all(size <= 4096 for size in read_sizes), ( + f"extract_documents read full file for MIME detection: sizes={read_sizes}" + ) + + # --------------------------------------------------------------------------- # DOCX upload test — API saves file, loop layer extracts text # --------------------------------------------------------------------------- diff --git a/tests/test_context_documents.py b/tests/test_context_documents.py index 28a4f6d2..7d9ac908 100644 --- a/tests/test_context_documents.py +++ b/tests/test_context_documents.py @@ -1,7 +1,8 @@ """Tests for context builder media handling. The ContextBuilder._build_user_content method should ONLY handle images. -Document text extraction is the responsibility of the API layer. +Document text extraction is the responsibility of the processing layer +(AgentLoop._process_message and _drain_pending). """ from __future__ import annotations @@ -9,6 +10,7 @@ from __future__ import annotations from pathlib import Path from nanobot.agent.context import ContextBuilder +from nanobot.utils.document import extract_documents def _make_builder(tmp_path: Path) -> ContextBuilder: @@ -56,3 +58,56 @@ def test_build_user_content_mixed_image_and_non_image(tmp_path: Path) -> None: assert any(b["type"] == "image_url" for b in result) text_parts = [b.get("text", "") for b in result if b.get("type") == "text"] assert all("report text" not in t for t in text_parts) + + +# --------------------------------------------------------------------------- +# Bug detection: extract_documents must be called BEFORE _build_user_content +# to prevent document media from being silently dropped. +# This simulates the _drain_pending code path. +# --------------------------------------------------------------------------- + +def test_drain_pending_path_preserves_document_text(tmp_path: Path) -> None: + """Simulates the _drain_pending path: a pending follow-up message + with a document attachment must have its text extracted before being + passed to _build_user_content. Without extract_documents, the + document is silently dropped.""" + from docx import Document + + doc = Document() + doc.add_paragraph("Quarterly revenue is $5M") + docx_path = tmp_path / "report.docx" + doc.save(docx_path) + + content = "summarize" + media = [str(docx_path)] + + # Step 1: extract_documents separates docs from images + new_content, image_only = extract_documents(content, media) + + # Step 2: _build_user_content handles only images (none left here) + builder = _make_builder(tmp_path) + result = builder._build_user_content(new_content, image_only if image_only else None) + + # The document text should be present in the final content + assert "Quarterly revenue" in result + assert "summarize" in result + + +def test_drain_pending_path_without_extract_loses_document(tmp_path: Path) -> None: + """Demonstrates the BUG: if _drain_pending calls _build_user_content + directly without extract_documents, document content is lost.""" + from docx import Document + + doc = Document() + doc.add_paragraph("Secret data in document") + docx_path = tmp_path / "report.docx" + doc.save(docx_path) + + builder = _make_builder(tmp_path) + + # Bug path: call _build_user_content directly with document media + result = builder._build_user_content("summarize", [str(docx_path)]) + + # The document text is LOST — _build_user_content ignores non-images + assert result == "summarize" # only the original text, no doc content + assert "Secret data" not in result From 89bf5d29d1653bff8377db5771ba108981f8a3a3 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 13:38:06 +0000 Subject: [PATCH 15/32] fix: reduce CLI streaming flicker and show model in welcome line --- nanobot/cli/commands.py | 2 +- nanobot/cli/stream.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nanobot/cli/commands.py b/nanobot/cli/commands.py index 953e8b1f..81aeb7d0 100644 --- a/nanobot/cli/commands.py +++ b/nanobot/cli/commands.py @@ -1007,7 +1007,7 @@ def agent( # Interactive mode — route through bus like other channels from nanobot.bus.events import InboundMessage _init_prompt_session() - console.print(f"{__logo__} Interactive mode (type [bold]exit[/bold] or [bold]Ctrl+C[/bold] to quit)\n") + console.print(f"{__logo__} Interactive mode [bold blue]({config.agents.defaults.model})[/bold blue] — type [bold]exit[/bold] or [bold]Ctrl+C[/bold] to quit\n") if ":" in session_id: cli_channel, cli_chat_id = session_id.split(":", 1) diff --git a/nanobot/cli/stream.py b/nanobot/cli/stream.py index 8151e3dd..9454edac 100644 --- a/nanobot/cli/stream.py +++ b/nanobot/cli/stream.py @@ -102,7 +102,7 @@ class StreamRenderer: self._live = Live(self._render(), console=c, auto_refresh=False) self._live.start() now = time.monotonic() - if "\n" in delta or (now - self._t) > 0.05: + if (now - self._t) > 0.15: self._live.update(self._render()) self._live.refresh() self._t = now From 73cf9a220b1b213d3923054cded0f5286a75349d Mon Sep 17 00:00:00 2001 From: samy Date: Tue, 14 Apr 2026 22:57:53 +0800 Subject: [PATCH 16/32] fix: handle dict config in is_allowed() and _validate_allow_from() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getattr() on a dict never finds custom keys — it only searches object attributes, not dict keys. When channel config is loaded as a Pydantic extra field (which is a plain dict), getattr(config, 'allow_from', []) always returns the default [], causing all access to be denied regardless of the allowFrom configuration. Fix both is_allowed() and _validate_allow_from() to use isinstance checks, falling back to dict.get() for dict configs while preserving getattr() for object-style configs. --- nanobot/channels/base.py | 5 ++++- nanobot/channels/manager.py | 7 ++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/nanobot/channels/base.py b/nanobot/channels/base.py index dd29c085..b6b50681 100644 --- a/nanobot/channels/base.py +++ b/nanobot/channels/base.py @@ -116,7 +116,10 @@ class BaseChannel(ABC): def is_allowed(self, sender_id: str) -> bool: """Check if *sender_id* is permitted. Empty list → deny all; ``"*"`` → allow all.""" - allow_list = getattr(self.config, "allow_from", []) + if isinstance(self.config, dict): + allow_list = self.config.get("allow_from") or self.config.get("allowFrom") or [] + else: + allow_list = getattr(self.config, "allow_from", []) if not allow_list: logger.warning("{}: allow_from is empty — all access denied", self.name) return False diff --git a/nanobot/channels/manager.py b/nanobot/channels/manager.py index aaec5e33..58531c41 100644 --- a/nanobot/channels/manager.py +++ b/nanobot/channels/manager.py @@ -75,7 +75,12 @@ class ChannelManager: def _validate_allow_from(self) -> None: for name, ch in self.channels.items(): - if getattr(ch.config, "allow_from", None) == []: + cfg = ch.config + if isinstance(cfg, dict): + allow = cfg.get("allow_from") or cfg.get("allowFrom") + else: + allow = getattr(cfg, "allow_from", None) + if allow == []: raise SystemExit( f'Error: "{name}" has empty allowFrom (denies all). ' f'Set ["*"] to allow everyone, or add specific user IDs.' From 1f33df1ea612769558103dd3622627dee18c4ea8 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 17:24:00 +0000 Subject: [PATCH 17/32] fix: preserve empty dict allow_from handling Keep dict-backed channel configs compatible with both allow_from and allowFrom without losing empty-list semantics, and add focused regression coverage for the allow-list boundary. Made-with: Cursor --- nanobot/channels/base.py | 5 ++++- nanobot/channels/manager.py | 5 ++++- tests/channels/test_base_channel.py | 12 ++++++++++++ tests/channels/test_channel_plugins.py | 24 +++++++++++++++++++++++- 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/nanobot/channels/base.py b/nanobot/channels/base.py index b6b50681..35aac3e4 100644 --- a/nanobot/channels/base.py +++ b/nanobot/channels/base.py @@ -117,7 +117,10 @@ class BaseChannel(ABC): def is_allowed(self, sender_id: str) -> bool: """Check if *sender_id* is permitted. Empty list → deny all; ``"*"`` → allow all.""" if isinstance(self.config, dict): - allow_list = self.config.get("allow_from") or self.config.get("allowFrom") or [] + if "allow_from" in self.config: + allow_list = self.config.get("allow_from") + else: + allow_list = self.config.get("allowFrom", []) else: allow_list = getattr(self.config, "allow_from", []) if not allow_list: diff --git a/nanobot/channels/manager.py b/nanobot/channels/manager.py index 58531c41..634e04fe 100644 --- a/nanobot/channels/manager.py +++ b/nanobot/channels/manager.py @@ -77,7 +77,10 @@ class ChannelManager: for name, ch in self.channels.items(): cfg = ch.config if isinstance(cfg, dict): - allow = cfg.get("allow_from") or cfg.get("allowFrom") + if "allow_from" in cfg: + allow = cfg.get("allow_from") + else: + allow = cfg.get("allowFrom") else: allow = getattr(cfg, "allow_from", None) if allow == []: diff --git a/tests/channels/test_base_channel.py b/tests/channels/test_base_channel.py index 5d10d4e1..660aff60 100644 --- a/tests/channels/test_base_channel.py +++ b/tests/channels/test_base_channel.py @@ -23,3 +23,15 @@ def test_is_allowed_requires_exact_match() -> None: assert channel.is_allowed("allow@email.com") is True assert channel.is_allowed("attacker|allow@email.com") is False + + +def test_is_allowed_supports_dict_allow_from_alias() -> None: + channel = _DummyChannel({"allowFrom": ["alice"]}, MessageBus()) + + assert channel.is_allowed("alice") is True + + +def test_is_allowed_denies_empty_dict_allow_from() -> None: + channel = _DummyChannel({"allow_from": []}, MessageBus()) + + assert channel.is_allowed("alice") is False diff --git a/tests/channels/test_channel_plugins.py b/tests/channels/test_channel_plugins.py index 8bb95b53..584b5864 100644 --- a/tests/channels/test_channel_plugins.py +++ b/tests/channels/test_channel_plugins.py @@ -646,7 +646,10 @@ class _ChannelWithAllowFrom(BaseChannel): def __init__(self, config, bus, allow_from): super().__init__(config, bus) - self.config.allow_from = allow_from + if isinstance(self.config, dict): + self.config["allow_from"] = allow_from + else: + self.config.allow_from = allow_from async def start(self) -> None: pass @@ -714,6 +717,25 @@ async def test_validate_allow_from_passes_with_asterisk(): mgr._validate_allow_from() +@pytest.mark.asyncio +async def test_validate_allow_from_raises_on_empty_dict_allow_from(): + """_validate_allow_from should reject empty dict-backed allow_from lists.""" + fake_config = SimpleNamespace( + channels=ChannelsConfig(), + providers=SimpleNamespace(groq=SimpleNamespace(api_key="")), + ) + + mgr = ChannelManager.__new__(ChannelManager) + mgr.config = fake_config + mgr.channels = {"test": _ChannelWithAllowFrom({"enabled": True}, None, [])} + mgr._dispatch_task = None + + with pytest.raises(SystemExit) as exc_info: + mgr._validate_allow_from() + + assert "empty allowFrom" in str(exc_info.value) + + @pytest.mark.asyncio async def test_get_channel_returns_channel_if_exists(): """get_channel should return the channel if it exists.""" From f293ff7f189857d5660255ce6b20af75765feeb0 Mon Sep 17 00:00:00 2001 From: Michael-lhh Date: Tue, 14 Apr 2026 23:35:03 +0800 Subject: [PATCH 18/32] fix: normalize tool-call arguments for strict providers Ensure assistant tool-call function.arguments is always emitted as valid JSON text so strict OpenAI-compatible backends (including Alibaba code models) do not reject requests. Add regressions for dict and malformed-string argument payloads in message sanitization. Made-with: Cursor --- nanobot/providers/openai_compat_provider.py | 29 +++++++++++++ tests/providers/test_litellm_kwargs.py | 48 +++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/nanobot/providers/openai_compat_provider.py b/nanobot/providers/openai_compat_provider.py index 4dea2d5f..bf82e538 100644 --- a/nanobot/providers/openai_compat_provider.py +++ b/nanobot/providers/openai_compat_provider.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import json import hashlib import importlib.util import os @@ -222,6 +223,24 @@ class OpenAICompatProvider(LLMProvider): return tool_call_id return hashlib.sha1(tool_call_id.encode()).hexdigest()[:9] + @staticmethod + def _normalize_tool_call_arguments(arguments: Any) -> str: + """Force function.arguments into a valid JSON object string.""" + if isinstance(arguments, str): + stripped = arguments.strip() + if not stripped: + return "{}" + try: + parsed = json_repair.loads(stripped) + except Exception: + return "{}" + if isinstance(parsed, dict): + return json.dumps(parsed, ensure_ascii=False) + return "{}" + if isinstance(arguments, dict): + return json.dumps(arguments, ensure_ascii=False) + return "{}" + def _sanitize_messages(self, messages: list[dict[str, Any]]) -> list[dict[str, Any]]: """Strip non-standard keys, normalize tool_call IDs.""" sanitized = LLMProvider._sanitize_request_messages(messages, _ALLOWED_MSG_KEYS) @@ -241,6 +260,16 @@ class OpenAICompatProvider(LLMProvider): continue tc_clean = dict(tc) tc_clean["id"] = map_id(tc_clean.get("id")) + function = tc_clean.get("function") + if isinstance(function, dict): + function_clean = dict(function) + if "arguments" in function_clean: + function_clean["arguments"] = self._normalize_tool_call_arguments( + function_clean.get("arguments") + ) + else: + function_clean["arguments"] = "{}" + tc_clean["function"] = function_clean normalized.append(tc_clean) clean["tool_calls"] = normalized if clean.get("role") == "assistant": diff --git a/tests/providers/test_litellm_kwargs.py b/tests/providers/test_litellm_kwargs.py index ec2581cd..31bdaa55 100644 --- a/tests/providers/test_litellm_kwargs.py +++ b/tests/providers/test_litellm_kwargs.py @@ -584,6 +584,54 @@ def test_openai_compat_keeps_tool_calls_after_consecutive_assistant_messages() - assert sanitized[2]["tool_call_id"] == "3ec83c30d" +def test_openai_compat_stringifies_dict_tool_arguments() -> None: + with patch("nanobot.providers.openai_compat_provider.AsyncOpenAI"): + provider = OpenAICompatProvider() + + sanitized = provider._sanitize_messages([ + {"role": "user", "content": "hi"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + { + "id": "call_1", + "type": "function", + "function": {"name": "exec", "arguments": {"cmd": "ls -la"}}, + } + ], + }, + {"role": "tool", "tool_call_id": "call_1", "name": "exec", "content": "ok"}, + {"role": "user", "content": "done"}, + ]) + + assert sanitized[1]["tool_calls"][0]["function"]["arguments"] == '{"cmd": "ls -la"}' + + +def test_openai_compat_repairs_non_json_tool_arguments_string() -> None: + with patch("nanobot.providers.openai_compat_provider.AsyncOpenAI"): + provider = OpenAICompatProvider() + + sanitized = provider._sanitize_messages([ + {"role": "user", "content": "hi"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + { + "id": "call_1", + "type": "function", + "function": {"name": "exec", "arguments": "{'cmd': 'pwd'}"}, + } + ], + }, + {"role": "tool", "tool_call_id": "call_1", "name": "exec", "content": "ok"}, + {"role": "user", "content": "done"}, + ]) + + assert sanitized[1]["tool_calls"][0]["function"]["arguments"] == '{"cmd": "pwd"}' + + @pytest.mark.asyncio async def test_openai_compat_stream_watchdog_returns_error_on_stall(monkeypatch) -> None: monkeypatch.setenv("NANOBOT_STREAM_IDLE_TIMEOUT_S", "0") From b60e8dc0baf6549da9d49faf6a81220a19bb96ab Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 17:33:43 +0000 Subject: [PATCH 19/32] test: cover missing tool-call arguments normalization Lock the strict-provider sanitization path so assistant tool calls without function.arguments are normalized to {} instead of being forwarded as missing values. Made-with: Cursor --- tests/providers/test_litellm_kwargs.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/providers/test_litellm_kwargs.py b/tests/providers/test_litellm_kwargs.py index 31bdaa55..8a1e5247 100644 --- a/tests/providers/test_litellm_kwargs.py +++ b/tests/providers/test_litellm_kwargs.py @@ -632,6 +632,30 @@ def test_openai_compat_repairs_non_json_tool_arguments_string() -> None: assert sanitized[1]["tool_calls"][0]["function"]["arguments"] == '{"cmd": "pwd"}' +def test_openai_compat_defaults_missing_tool_arguments_to_empty_object() -> None: + with patch("nanobot.providers.openai_compat_provider.AsyncOpenAI"): + provider = OpenAICompatProvider() + + sanitized = provider._sanitize_messages([ + {"role": "user", "content": "hi"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + { + "id": "call_1", + "type": "function", + "function": {"name": "exec"}, + } + ], + }, + {"role": "tool", "tool_call_id": "call_1", "name": "exec", "content": "ok"}, + {"role": "user", "content": "done"}, + ]) + + assert sanitized[1]["tool_calls"][0]["function"]["arguments"] == "{}" + + @pytest.mark.asyncio async def test_openai_compat_stream_watchdog_returns_error_on_stall(monkeypatch) -> None: monkeypatch.setenv("NANOBOT_STREAM_IDLE_TIMEOUT_S", "0") From 634f4b45c163203274aba529fcc17df9a8af9cc2 Mon Sep 17 00:00:00 2001 From: aiguozhi123456 <126325311+aiguozhi123456@users.noreply.github.com> Date: Tue, 14 Apr 2026 22:25:43 +0800 Subject: [PATCH 20/32] feat: show active task count in /status output --- nanobot/agent/subagent.py | 8 ++++++++ nanobot/command/builtin.py | 7 +++++++ nanobot/utils/helpers.py | 2 ++ tests/cli/test_restart_command.py | 5 +++++ tests/test_build_status.py | 2 ++ 5 files changed, 24 insertions(+) diff --git a/nanobot/agent/subagent.py b/nanobot/agent/subagent.py index 571bcc79..f464e51a 100644 --- a/nanobot/agent/subagent.py +++ b/nanobot/agent/subagent.py @@ -262,3 +262,11 @@ class SubagentManager: def get_running_count(self) -> int: """Return the number of currently running subagents.""" return len(self._running_tasks) + + def get_running_count_by_session(self, session_key: str) -> int: + """Return the number of currently running subagents for a session.""" + tids = self._session_tasks.get(session_key, set()) + return sum( + 1 for tid in tids + if tid in self._running_tasks and not self._running_tasks[tid].done() + ) diff --git a/nanobot/command/builtin.py b/nanobot/command/builtin.py index 94e46320..f60e7e87 100644 --- a/nanobot/command/builtin.py +++ b/nanobot/command/builtin.py @@ -74,6 +74,12 @@ async def cmd_status(ctx: CommandContext) -> OutboundMessage: search_usage_text = usage.format() except Exception: pass # Never let usage fetch break /status + active_tasks = loop._active_tasks.get(ctx.key, []) + task_count = sum(1 for t in active_tasks if not t.done()) + try: + task_count += loop.subagents.get_running_count_by_session(ctx.key) + except Exception: + pass return OutboundMessage( channel=ctx.msg.channel, chat_id=ctx.msg.chat_id, @@ -84,6 +90,7 @@ async def cmd_status(ctx: CommandContext) -> OutboundMessage: session_msg_count=len(session.get_history(max_messages=0)), context_tokens_estimate=ctx_est, search_usage_text=search_usage_text, + active_task_count=task_count, ), metadata={**dict(ctx.msg.metadata or {}), "render_as": "text"}, ) diff --git a/nanobot/utils/helpers.py b/nanobot/utils/helpers.py index 1bfd9f18..53504885 100644 --- a/nanobot/utils/helpers.py +++ b/nanobot/utils/helpers.py @@ -400,6 +400,7 @@ def build_status_content( session_msg_count: int, context_tokens_estimate: int, search_usage_text: str | None = None, + active_task_count: int = 0, ) -> str: """Build a human-readable runtime status snapshot. @@ -431,6 +432,7 @@ def build_status_content( f"\U0001f4da Context: {ctx_used_str}/{ctx_total_str} ({ctx_pct}%)", f"\U0001f4ac Session: {session_msg_count} messages", f"\u23f1 Uptime: {uptime}", + f"\u26a1 Tasks: {active_task_count} active", ] if search_usage_text: lines.append(search_usage_text) diff --git a/tests/cli/test_restart_command.py b/tests/cli/test_restart_command.py index 697d5fc1..03ca152e 100644 --- a/tests/cli/test_restart_command.py +++ b/tests/cli/test_restart_command.py @@ -140,6 +140,7 @@ class TestRestartCommand: loop.consolidator.estimate_session_prompt_tokens = MagicMock( return_value=(20500, "tiktoken") ) + loop.subagents.get_running_count_by_session.return_value = 0 msg = InboundMessage(channel="telegram", sender_id="u1", chat_id="c1", content="/status") @@ -151,6 +152,7 @@ class TestRestartCommand: assert "Context: 20k/65k (31%)" in response.content assert "Session: 3 messages" in response.content assert "Uptime: 2m 5s" in response.content + assert "Tasks: 0 active" in response.content assert response.metadata == {"render_as": "text"} @pytest.mark.asyncio @@ -179,6 +181,7 @@ class TestRestartCommand: loop.consolidator.estimate_session_prompt_tokens = MagicMock( return_value=(0, "none") ) + loop.subagents.get_running_count_by_session.return_value = 0 response = await loop._process_message( InboundMessage(channel="telegram", sender_id="u1", chat_id="c1", content="/status") @@ -187,6 +190,7 @@ class TestRestartCommand: assert response is not None assert "Tokens: 1200 in / 34 out" in response.content assert "Context: 1k/65k (1%)" in response.content + assert "Tasks: 0 active" in response.content @pytest.mark.asyncio async def test_process_direct_preserves_render_metadata(self): @@ -195,6 +199,7 @@ class TestRestartCommand: session.get_history.return_value = [] loop.sessions.get_or_create.return_value = session loop.subagents.get_running_count.return_value = 0 + loop.subagents.get_running_count_by_session.return_value = 0 response = await loop.process_direct("/status", session_key="cli:test") diff --git a/tests/test_build_status.py b/tests/test_build_status.py index d98301cf..acbef416 100644 --- a/tests/test_build_status.py +++ b/tests/test_build_status.py @@ -15,6 +15,7 @@ def test_status_shows_cache_hit_rate(): ) assert "60% cached" in content assert "2000 in / 300 out" in content + assert "Tasks: 0 active" in content def test_status_no_cache_info(): @@ -30,6 +31,7 @@ def test_status_no_cache_info(): ) assert "cached" not in content.lower() assert "2000 in / 300 out" in content + assert "Tasks: 0 active" in content def test_status_zero_cached_tokens(): From 25ded8e7479ed7897343dce57b3b5f1c5c169084 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 17:48:13 +0000 Subject: [PATCH 21/32] test: cover active task count in status Lock the /status task counter to the actual stop scope by asserting it sums unfinished dispatch tasks with running subagents for the current session. Made-with: Cursor --- tests/cli/test_restart_command.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/cli/test_restart_command.py b/tests/cli/test_restart_command.py index 03ca152e..8cefa86d 100644 --- a/tests/cli/test_restart_command.py +++ b/tests/cli/test_restart_command.py @@ -155,6 +155,30 @@ class TestRestartCommand: assert "Tasks: 0 active" in response.content assert response.metadata == {"render_as": "text"} + @pytest.mark.asyncio + async def test_status_counts_running_dispatch_and_subagent_tasks(self): + loop, _bus = _make_loop() + session = MagicMock() + session.get_history.return_value = [{"role": "user"}] + loop.sessions.get_or_create.return_value = session + loop.consolidator.estimate_session_prompt_tokens = MagicMock( + return_value=(1000, "tiktoken") + ) + + running_task = MagicMock() + running_task.done.return_value = False + finished_task = MagicMock() + finished_task.done.return_value = True + + msg = InboundMessage(channel="telegram", sender_id="u1", chat_id="c1", content="/status") + loop._active_tasks[msg.session_key] = [running_task, finished_task] + loop.subagents.get_running_count_by_session.return_value = 2 + + response = await loop._process_message(msg) + + assert response is not None + assert "Tasks: 3 active" in response.content + @pytest.mark.asyncio async def test_run_agent_loop_resets_usage_when_provider_omits_it(self): loop, _bus = _make_loop() From ec14933aa1063341fd9da99f2ffc0938b132b9f4 Mon Sep 17 00:00:00 2001 From: aiguozhi123456 <126325311+aiguozhi123456@users.noreply.github.com> Date: Tue, 14 Apr 2026 23:03:34 +0800 Subject: [PATCH 22/32] fix: add retry termination notification to interaction channel --- nanobot/providers/base.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/nanobot/providers/base.py b/nanobot/providers/base.py index 759d880a..2383a567 100644 --- a/nanobot/providers/base.py +++ b/nanobot/providers/base.py @@ -718,9 +718,22 @@ class LLMProvider(ABC): identical_error_count, (response.content or "")[:120].lower(), ) + if on_retry_wait: + await on_retry_wait( + f"Persistent retry stopped after {identical_error_count} identical errors." + ) return response if not persistent and attempt > len(delays): + logger.warning( + "LLM request failed after {} retries, giving up: {}", + attempt, + (response.content or "")[:120].lower(), + ) + if on_retry_wait: + await on_retry_wait( + f"Model request failed after {attempt} retries, giving up." + ) break base_delay = delays[min(attempt - 1, len(delays) - 1)] From a0812ad60ed4cca00fc4bbbb19291797bd7f75d9 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 17:52:59 +0000 Subject: [PATCH 23/32] test: cover retry termination notifications Lock the new interaction-channel retry termination hints so both exhausted standard retries and persistent identical-error stops keep emitting the final progress message. Made-with: Cursor --- tests/providers/test_provider_retry.py | 52 ++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/providers/test_provider_retry.py b/tests/providers/test_provider_retry.py index 2ef784a3..35b710f3 100644 --- a/tests/providers/test_provider_retry.py +++ b/tests/providers/test_provider_retry.py @@ -87,6 +87,33 @@ async def test_chat_with_retry_returns_final_error_after_retries(monkeypatch) -> assert delays == [1, 2, 4] +@pytest.mark.asyncio +async def test_chat_with_retry_emits_terminal_progress_when_standard_retries_exhaust(monkeypatch) -> None: + provider = ScriptedProvider([ + LLMResponse(content="429 rate limit a", finish_reason="error"), + LLMResponse(content="429 rate limit b", finish_reason="error"), + LLMResponse(content="429 rate limit c", finish_reason="error"), + LLMResponse(content="503 final server error", finish_reason="error"), + ]) + progress: list[str] = [] + + async def _fake_sleep(delay: int) -> None: + return None + + async def _progress(msg: str) -> None: + progress.append(msg) + + monkeypatch.setattr("nanobot.providers.base.asyncio.sleep", _fake_sleep) + + response = await provider.chat_with_retry( + messages=[{"role": "user", "content": "hello"}], + on_retry_wait=_progress, + ) + + assert response.content == "503 final server error" + assert progress[-1] == "Model request failed after 4 retries, giving up." + + @pytest.mark.asyncio async def test_chat_with_retry_preserves_cancelled_error() -> None: provider = ScriptedProvider([asyncio.CancelledError()]) @@ -469,3 +496,28 @@ async def test_persistent_retry_aborts_after_ten_identical_transient_errors(monk assert response.content == "429 rate limit" assert provider.calls == 10 assert delays == [1, 2, 4, 4, 4, 4, 4, 4, 4] + + +@pytest.mark.asyncio +async def test_persistent_retry_emits_terminal_progress_on_identical_error_limit(monkeypatch) -> None: + provider = ScriptedProvider([ + *[LLMResponse(content="429 rate limit", finish_reason="error") for _ in range(10)], + ]) + progress: list[str] = [] + + async def _fake_sleep(delay: float) -> None: + return None + + async def _progress(msg: str) -> None: + progress.append(msg) + + monkeypatch.setattr("nanobot.providers.base.asyncio.sleep", _fake_sleep) + + response = await provider.chat_with_retry( + messages=[{"role": "user", "content": "hello"}], + retry_mode="persistent", + on_retry_wait=_progress, + ) + + assert response.finish_reason == "error" + assert progress[-1] == "Persistent retry stopped after 10 identical errors." From 9e2278826fa94ec754532ef04837e56781c2bb43 Mon Sep 17 00:00:00 2001 From: razzh Date: Tue, 14 Apr 2026 11:54:39 +0800 Subject: [PATCH 24/32] feat(provider): enable Kimi thinking via extra_body for k2.5 and k2.6 - Inject `thinking={"type": "enabled|disabled"}` via extra_body for Kimi thinking-capable models (kimi-k2.5, k2.6-code-preview). - Add _is_kimi_thinking_model helper to handle both bare slugs and OpenRouter-style prefixed names (e.g. moonshotai/kimi-k2.5). - reasoning_effort="minimal" maps to disabled; any other value enables it. - Add tests for enabled/disabled states and OpenRouter prefix handling. --- nanobot/providers/openai_compat_provider.py | 33 +++++++++++++++ tests/providers/test_litellm_kwargs.py | 47 +++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/nanobot/providers/openai_compat_provider.py b/nanobot/providers/openai_compat_provider.py index bf82e538..1a9f295a 100644 --- a/nanobot/providers/openai_compat_provider.py +++ b/nanobot/providers/openai_compat_provider.py @@ -50,6 +50,29 @@ _DEFAULT_OPENROUTER_HEADERS = { "X-OpenRouter-Title": "nanobot", "X-OpenRouter-Categories": "cli-agent,personal-agent", } +_KIMI_THINKING_MODELS: frozenset[str] = frozenset({ + "kimi-k2.5", + "k2.6-code-preview", +}) + + +def _is_kimi_thinking_model(model_name: str) -> bool: + """Return True if model_name refers to a Kimi thinking-capable model. + + Supports two forms: + - Exact match: kimi-k2.5 in _KIMI_THINKING_MODELS + - Slug match: moonshotai/kimi-k2.5 -> the part after the last "/" + is checked against _KIMI_THINKING_MODELS + + This covers both the native Moonshot provider (bare slug) and + OpenRouter-style names (``"publisher/slug"``). + """ + name = model_name.lower() + if name in _KIMI_THINKING_MODELS: + return True + if "/" in name and name.rsplit("/", 1)[1] in _KIMI_THINKING_MODELS: + return True + return False def _short_tool_id() -> str: @@ -363,6 +386,16 @@ class OpenAICompatProvider(LLMProvider): if extra: kwargs.setdefault("extra_body", {}).update(extra) + # Model-level thinking injection for Kimi thinking-capable models. + # Strip any provider prefix (e.g. "moonshotai/") before the set lookup + # so that OpenRouter-style names like "moonshotai/kimi-k2.5" are handled + # identically to bare names like "kimi-k2.5". + if reasoning_effort is not None and _is_kimi_thinking_model(model_name): + thinking_enabled = reasoning_effort.lower() != "minimal" + kwargs.setdefault("extra_body", {}).update( + {"thinking": {"type": "enabled" if thinking_enabled else "disabled"}} + ) + if tools: kwargs["tools"] = tools kwargs["tool_choice"] = tool_choice or "auto" diff --git a/tests/providers/test_litellm_kwargs.py b/tests/providers/test_litellm_kwargs.py index 8a1e5247..8304aae8 100644 --- a/tests/providers/test_litellm_kwargs.py +++ b/tests/providers/test_litellm_kwargs.py @@ -730,3 +730,50 @@ def test_openai_no_thinking_extra_body() -> None: """Non-thinking providers should never get extra_body for thinking.""" kw = _build_kwargs_for("openai", "gpt-4o", reasoning_effort="medium") assert "extra_body" not in kw + + +def test_kimi_k25_thinking_enabled() -> None: + """kimi-k2.5 with reasoning_effort set should opt in to thinking.""" + kw = _build_kwargs_for("moonshot", "kimi-k2.5", reasoning_effort="medium") + assert kw.get("extra_body") == {"thinking": {"type": "enabled"}} + + +def test_kimi_k25_thinking_disabled_for_minimal() -> None: + """reasoning_effort='minimal' maps to thinking disabled for kimi-k2.5.""" + kw = _build_kwargs_for("moonshot", "kimi-k2.5", reasoning_effort="minimal") + assert kw.get("extra_body") == {"thinking": {"type": "disabled"}} + + +def test_kimi_k25_no_extra_body_when_reasoning_effort_none() -> None: + """Without reasoning_effort the thinking param must not be injected.""" + kw = _build_kwargs_for("moonshot", "kimi-k2.5", reasoning_effort=None) + assert "extra_body" not in kw + + +def test_kimi_k25_thinking_enabled_with_openrouter_prefix() -> None: + """OpenRouter-style model names like moonshotai/kimi-k2.5 must trigger thinking.""" + kw = _build_kwargs_for("openrouter", "moonshotai/kimi-k2.5", reasoning_effort="medium") + assert kw.get("extra_body") == {"thinking": {"type": "enabled"}} + +def test_kimi_k25_thinking_disabled_with_openrouter_prefix() -> None: + """OpenRouter names must NOT trigger thinking without reasoning_effort.""" + kw = _build_kwargs_for("openrouter", "moonshotai/kimi-k2.5", reasoning_effort=None) + assert "extra_body" not in kw + + +def test_kimi_k26_code_preview_thinking_enabled() -> None: + """k2.6-code-preview also supports thinking; should behave like k2.5.""" + kw = _build_kwargs_for("moonshot", "k2.6-code-preview", reasoning_effort="high") + assert kw.get("extra_body") == {"thinking": {"type": "enabled"}} + + +def test_kimi_k2_series_no_thinking_injection() -> None: + """kimi-k2 (non-thinking) models must NOT receive extra_body.thinking.""" + kw = _build_kwargs_for("moonshot", "kimi-k2", reasoning_effort="high") + assert "extra_body" not in kw + + +def test_kimi_k2_thinking_series_no_thinking_injection() -> None: + """kimi-k2-thinking series models must NOT receive extra_body.thinking.""" + kw = _build_kwargs_for("moonshot", "kimi-k2-thinking", reasoning_effort="high") + assert "extra_body" not in kw From 1a5a16d1f3c5f71276af0e8164e8cca85a0b4ff3 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 18:19:30 +0000 Subject: [PATCH 25/32] chore: update README with recent news entries --- README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/README.md b/README.md index b6f7395c..45af3ffc 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,14 @@ ## 📢 News +- **2026-04-13** 🛡️ Agent turn hardened — user messages persisted early, auto-compact skips active tasks. +- **2026-04-12** 🔒 Lark global domain support, Dream learns discovered skills, shell sandbox tightened. +- **2026-04-11** ⚡ Auto compact shrinks sessions on the fly; Kagi web search; QQ & WeCom full media. +- **2026-04-10** 📓 Notebook editing tool, multiple MCP servers, Feishu streaming & done-emoji. +- **2026-04-09** 🔌 WebSocket channel, unified cross-channel session, `disabled_skills` config. +- **2026-04-08** 📤 API file uploads, OpenAI reasoning auto-routing with Responses fallback. +- **2026-04-07** 🧠 Anthropic adaptive thinking, MCP resources & prompts exposed as tools. +- **2026-04-06** 🛰️ Langfuse observability, unified Whisper transcription, email attachments. - **2026-04-05** 🚀 Released **v0.1.5** — sturdier long-running tasks, Dream two-stage memory, production-ready sandboxing and programming Agent SDK. Please see [release notes](https://github.com/HKUDS/nanobot/releases/tag/v0.1.5) for details. - **2026-04-04** 🚀 Jinja2 response templates, Dream memory hardened, smarter retry handling. - **2026-04-03** 🧠 Xiaomi MiMo provider, chain-of-thought reasoning visible, Telegram UX polish. From 6483071485b4f303979ed3234a94253c933a71ea Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 18:51:04 +0000 Subject: [PATCH 26/32] chore: update version to 0.1.5.post1 --- nanobot/__init__.py | 2 +- pyproject.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nanobot/__init__.py b/nanobot/__init__.py index 0bce848d..5e6954d9 100644 --- a/nanobot/__init__.py +++ b/nanobot/__init__.py @@ -21,7 +21,7 @@ def _resolve_version() -> str: return _pkg_version("nanobot-ai") except PackageNotFoundError: # Source checkouts often import nanobot without installed dist-info. - return _read_pyproject_version() or "0.1.5" + return _read_pyproject_version() or "0.1.5.post1" __version__ = _resolve_version() diff --git a/pyproject.toml b/pyproject.toml index 13e71339..f828f3cb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "nanobot-ai" -version = "0.1.5" +version = "0.1.5.post1" description = "A lightweight personal AI assistant framework" readme = { file = "README.md", content-type = "text/markdown" } requires-python = ">=3.11" From 5683c79a6ed1d6be5437cb816b5d56f76b21e237 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Tue, 14 Apr 2026 19:01:43 +0000 Subject: [PATCH 27/32] chore: update README with new release notes of v0.1.5.post1 --- README.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 45af3ffc..7e81d333 100644 --- a/README.md +++ b/README.md @@ -21,15 +21,20 @@ ## 📢 News +- **2026-04-14** 🚀 Released **v0.1.5.post1** — Dream skill discovery, mid-turn follow-up injection, WebSocket channel, and deeper channel integrations. Please see [release notes](https://github.com/HKUDS/nanobot/releases/tag/v0.1.5.post1) for details. - **2026-04-13** 🛡️ Agent turn hardened — user messages persisted early, auto-compact skips active tasks. - **2026-04-12** 🔒 Lark global domain support, Dream learns discovered skills, shell sandbox tightened. -- **2026-04-11** ⚡ Auto compact shrinks sessions on the fly; Kagi web search; QQ & WeCom full media. +- **2026-04-11** ⚡ Context compact shrinks sessions on the fly; Kagi web search; QQ & WeCom full media. - **2026-04-10** 📓 Notebook editing tool, multiple MCP servers, Feishu streaming & done-emoji. - **2026-04-09** 🔌 WebSocket channel, unified cross-channel session, `disabled_skills` config. - **2026-04-08** 📤 API file uploads, OpenAI reasoning auto-routing with Responses fallback. - **2026-04-07** 🧠 Anthropic adaptive thinking, MCP resources & prompts exposed as tools. - **2026-04-06** 🛰️ Langfuse observability, unified Whisper transcription, email attachments. - **2026-04-05** 🚀 Released **v0.1.5** — sturdier long-running tasks, Dream two-stage memory, production-ready sandboxing and programming Agent SDK. Please see [release notes](https://github.com/HKUDS/nanobot/releases/tag/v0.1.5) for details. + +
+Earlier news + - **2026-04-04** 🚀 Jinja2 response templates, Dream memory hardened, smarter retry handling. - **2026-04-03** 🧠 Xiaomi MiMo provider, chain-of-thought reasoning visible, Telegram UX polish. - **2026-04-02** 🧱 Long-running tasks run more reliably — core runtime hardening. @@ -39,11 +44,6 @@ - **2026-03-29** 💬 WeChat voice, typing, QR/media resilience; fixed-session OpenAI-compatible API. - **2026-03-28** 📚 Provider docs refresh; skill template wording fix. - **2026-03-27** 🚀 Released **v0.1.4.post6** — architecture decoupling, litellm removal, end-to-end streaming, WeChat channel, and a security fix. Please see [release notes](https://github.com/HKUDS/nanobot/releases/tag/v0.1.4.post6) for details. - - -
-Earlier news - - **2026-03-26** 🏗️ Agent runner extracted and lifecycle hooks unified; stream delta coalescing at boundaries. - **2026-03-25** 🌏 StepFun provider, configurable timezone, Gemini thought signatures. - **2026-03-24** 🔧 WeChat compatibility, Feishu CardKit streaming, test suite restructured. From 6fbada5363b5981b78a4e5e4ff5deb167abb2480 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Wed, 15 Apr 2026 15:44:27 +0800 Subject: [PATCH 28/32] =?UTF-8?q?refactor(context):=20deduplicate=20system?= =?UTF-8?q?=20prompt=20=E2=80=94=20markdown=20skills=20index,=20skip=20tem?= =?UTF-8?q?plate=20MEMORY.md?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Convert skills summary from verbose XML (4-5 lines/skill) to compact markdown list (1 line/skill) with inline path for read_file lookup - Exclude always-loaded skills (e.g. memory) from the skills index to avoid duplicating content already in the Active Skills section - Skip injecting the Memory section when MEMORY.md still matches the bundled template (i.e. Dream hasn't populated it yet) --- nanobot/agent/context.py | 16 ++++++- nanobot/agent/skills.py | 34 ++++++--------- nanobot/templates/agent/skills_section.md | 2 +- tests/agent/test_context_prompt_cache.py | 52 +++++++++++++++++++++++ 4 files changed, 81 insertions(+), 23 deletions(-) diff --git a/nanobot/agent/context.py b/nanobot/agent/context.py index cab7b057..f58baf0a 100644 --- a/nanobot/agent/context.py +++ b/nanobot/agent/context.py @@ -3,6 +3,7 @@ import base64 import mimetypes import platform +from importlib.resources import files as pkg_files from pathlib import Path from typing import Any @@ -39,7 +40,7 @@ class ContextBuilder: parts.append(bootstrap) memory = self.memory.get_memory_context() - if memory: + if memory and not self._is_template_content(self.memory.read_memory(), "memory/MEMORY.md"): parts.append(f"# Memory\n\n{memory}") always_skills = self.skills.get_always_skills() @@ -48,7 +49,7 @@ class ContextBuilder: if always_content: parts.append(f"# Active Skills\n\n{always_content}") - skills_summary = self.skills.build_skills_summary() + skills_summary = self.skills.build_skills_summary(exclude=set(always_skills)) if skills_summary: parts.append(render_template("agent/skills_section.md", skills_summary=skills_summary)) @@ -114,6 +115,17 @@ class ContextBuilder: return "\n\n".join(parts) if parts else "" + @staticmethod + def _is_template_content(content: str, template_path: str) -> bool: + """Check if *content* is identical to the bundled template (user hasn't customized it).""" + try: + tpl = pkg_files("nanobot") / "templates" / template_path + if tpl.is_file(): + return content.strip() == tpl.read_text(encoding="utf-8").strip() + except Exception: + pass + return False + def build_messages( self, history: list[dict[str, Any]], diff --git a/nanobot/agent/skills.py b/nanobot/agent/skills.py index e9ef1986..5d18cce5 100644 --- a/nanobot/agent/skills.py +++ b/nanobot/agent/skills.py @@ -16,10 +16,6 @@ _STRIP_SKILL_FRONTMATTER = re.compile( ) -def _escape_xml(text: str) -> str: - return text.replace("&", "&").replace("<", "<").replace(">", ">") - - class SkillsLoader: """ Loader for agent skills. @@ -110,39 +106,37 @@ class SkillsLoader: ] return "\n\n---\n\n".join(parts) - def build_skills_summary(self) -> str: + def build_skills_summary(self, exclude: set[str] | None = None) -> str: """ Build a summary of all skills (name, description, path, availability). This is used for progressive loading - the agent can read the full skill content using read_file when needed. + Args: + exclude: Set of skill names to omit from the summary. + Returns: - XML-formatted skills summary. + Markdown-formatted skills summary. """ all_skills = self.list_skills(filter_unavailable=False) if not all_skills: return "" - lines: list[str] = [""] + lines: list[str] = [] for entry in all_skills: skill_name = entry["name"] + if exclude and skill_name in exclude: + continue meta = self._get_skill_meta(skill_name) available = self._check_requirements(meta) - lines.extend( - [ - f' ', - f" {_escape_xml(skill_name)}", - f" {_escape_xml(self._get_skill_description(skill_name))}", - f" {entry['path']}", - ] - ) - if not available: + desc = self._get_skill_description(skill_name) + if available: + lines.append(f"- **{skill_name}** — {desc} `{entry['path']}`") + else: missing = self._get_missing_requirements(meta) - if missing: - lines.append(f" {_escape_xml(missing)}") - lines.append(" ") - lines.append("") + suffix = f" (unavailable: {missing})" if missing else " (unavailable)" + lines.append(f"- **{skill_name}** — {desc}{suffix} `{entry['path']}`") return "\n".join(lines) def _get_missing_requirements(self, skill_meta: dict) -> str: diff --git a/nanobot/templates/agent/skills_section.md b/nanobot/templates/agent/skills_section.md index b495c9ef..300c5679 100644 --- a/nanobot/templates/agent/skills_section.md +++ b/nanobot/templates/agent/skills_section.md @@ -1,6 +1,6 @@ # Skills The following skills extend your capabilities. To use a skill, read its SKILL.md file using the read_file tool. -Skills with available="false" need dependencies installed first - you can try installing them with apt/brew. +Unavailable skills need dependencies installed first — you can try installing them with apt/brew. {{ skills_summary }} diff --git a/tests/agent/test_context_prompt_cache.py b/tests/agent/test_context_prompt_cache.py index 26f73027..b3e80b9c 100644 --- a/tests/agent/test_context_prompt_cache.py +++ b/tests/agent/test_context_prompt_cache.py @@ -219,3 +219,55 @@ def test_subagent_result_does_not_create_consecutive_assistant_messages(tmp_path for left, right in zip(messages, messages[1:]): assert not (left.get("role") == right.get("role") == "assistant") + + +def test_always_skills_excluded_from_skills_index(tmp_path) -> None: + """Always skills should appear in Active Skills but NOT in the skills index.""" + workspace = _make_workspace(tmp_path) + builder = ContextBuilder(workspace) + + prompt = builder.build_system_prompt() + + # memory skill should be in Active Skills section + assert "# Active Skills" in prompt + assert "### Skill: memory" in prompt + + # memory skill should NOT appear in the skills index + skills_section = prompt.split("# Skills\n", 1) + if len(skills_section) > 1: + index_text = skills_section[1].split("\n\n---")[0] + assert "**memory**" not in index_text + + +def test_template_memory_md_is_skipped(tmp_path) -> None: + """MEMORY.md matching the bundled template should not inject the Memory section.""" + workspace = _make_workspace(tmp_path) + from nanobot.utils.helpers import sync_workspace_templates + sync_workspace_templates(workspace, silent=True) + + builder = ContextBuilder(workspace) + prompt = builder.build_system_prompt() + + # The "# Memory\n\n## Long-term Memory" block is produced only by + # build_system_prompt() when MEMORY.md is injected. The memory skill + # also contains "# Memory" but is followed by "## Structure", not + # "## Long-term Memory". + assert "# Memory\n\n## Long-term Memory" not in prompt + assert "This file is automatically updated by nanobot" not in prompt + + +def test_customized_memory_md_is_injected(tmp_path) -> None: + """A Dream-populated MEMORY.md should be injected normally.""" + workspace = _make_workspace(tmp_path) + from nanobot.utils.helpers import sync_workspace_templates + sync_workspace_templates(workspace, silent=True) + + (workspace / "memory" / "MEMORY.md").write_text( + "# Long-term Memory\n\nUser prefers dark mode.\n", encoding="utf-8" + ) + + builder = ContextBuilder(workspace) + prompt = builder.build_system_prompt() + + assert "# Memory\n\n## Long-term Memory" in prompt + assert "User prefers dark mode" in prompt From 8572b7478fc762ab8c01d4929ad7b5672165c275 Mon Sep 17 00:00:00 2001 From: dongzeyu001 Date: Wed, 15 Apr 2026 11:42:05 +0800 Subject: [PATCH 29/32] Fix wecom mix msg parse --- nanobot/channels/wecom.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/nanobot/channels/wecom.py b/nanobot/channels/wecom.py index a7d7f1fe..69bdf3f0 100644 --- a/nanobot/channels/wecom.py +++ b/nanobot/channels/wecom.py @@ -302,13 +302,22 @@ class WecomChannel(BaseChannel): elif msg_type == "mixed": # Mixed content contains multiple message items - msg_items = body.get("mixed", {}).get("item", []) + msg_items = body.get("mixed", {}).get("msg_item", []) for item in msg_items: - item_type = item.get("type", "") + item_type = item.get("msgtype", "") if item_type == "text": text = item.get("text", {}).get("content", "") if text: content_parts.append(text) + elif item_type == "image": + file_url = item.get("image", {}).get("url", "") + aes_key = item.get("image", {}).get("aeskey", "") + if file_url and aes_key: + file_path = await self._download_and_save_media(file_url, aes_key, "image") + if file_path: + filename = os.path.basename(file_path) + content_parts.append(f"[image: {filename}]") + media_paths.append(file_path) else: content_parts.append(MSG_TYPE_MAP.get(item_type, f"[{item_type}]")) From cf47fa7d23b96370747fe6f7299340627a6865f9 Mon Sep 17 00:00:00 2001 From: dongzeyu001 Date: Wed, 15 Apr 2026 16:19:47 +0800 Subject: [PATCH 30/32] add test for wecom mixed msg parse fix --- tests/channels/test_wecom_channel.py | 44 ++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/channels/test_wecom_channel.py b/tests/channels/test_wecom_channel.py index b79c023b..ecaa1c56 100644 --- a/tests/channels/test_wecom_channel.py +++ b/tests/channels/test_wecom_channel.py @@ -541,6 +541,50 @@ async def test_process_voice_message() -> None: assert "[voice]" in msg.content +@pytest.mark.asyncio +async def test_process_mixed_message() -> None: + """Mixed message: contains picture and text message types.""" + channel = WecomChannel(WecomConfig(bot_id="b", secret="s", allow_from=["user1"]), MessageBus()) + client = _FakeWeComClient() + + with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as f: + f.write(b"\x89PNG\r\n") + saved = f.name + + client.download_file.return_value = (b"\x89PNG\r\n", "photo.png") + channel._client = client + + try: + with patch("nanobot.channels.wecom.get_media_dir", return_value=Path(os.path.dirname(saved))): + frame = _FakeFrame(body={ + "msgid": "msg_mixed_1", + "chatid": "chat1", + "msgtype": "mixed", + "from": {"userid": "user1"}, + "mixed": { + "msg_item": [ + {"msgtype": "text", "text": {"content": "hello wecom"}}, + {"msgtype": "image", "image": {"url": "https://example.com/img.png", "aeskey": "key123"}} + ] + } + }) + await channel._process_message(frame, "mixed") + + msg = await channel.bus.consume_inbound() + assert msg.sender_id == "user1" + assert msg.chat_id == "chat1" + assert msg.content == "hello wecom" + assert msg.metadata["msg_type"] == "text" + assert len(msg.media) == 1 + assert msg.media[0].endswith("photo.png") + assert "[image:" in msg.content + finally: + # Clean up any photo.png in tempdir + p = os.path.join(os.path.dirname(saved), "photo.png") + if os.path.exists(p): + os.unlink(p) + + @pytest.mark.asyncio async def test_process_message_deduplication() -> None: """Same msg_id is not processed twice.""" From cbd2315d761b93f811620a0d041e5c741b3dd27a Mon Sep 17 00:00:00 2001 From: dongzeyu001 Date: Wed, 15 Apr 2026 16:32:51 +0800 Subject: [PATCH 31/32] unit test fix --- tests/channels/test_wecom_channel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/channels/test_wecom_channel.py b/tests/channels/test_wecom_channel.py index ecaa1c56..f2367219 100644 --- a/tests/channels/test_wecom_channel.py +++ b/tests/channels/test_wecom_channel.py @@ -573,7 +573,7 @@ async def test_process_mixed_message() -> None: msg = await channel.bus.consume_inbound() assert msg.sender_id == "user1" assert msg.chat_id == "chat1" - assert msg.content == "hello wecom" + assert msg.content.startswith("hello wecom") assert msg.metadata["msg_type"] == "text" assert len(msg.media) == 1 assert msg.media[0].endswith("photo.png") From 6829b8b475540814eebcb50c5ac1e8d6b1264a91 Mon Sep 17 00:00:00 2001 From: dongzeyu001 Date: Wed, 15 Apr 2026 16:36:04 +0800 Subject: [PATCH 32/32] unit test fix --- tests/channels/test_wecom_channel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/channels/test_wecom_channel.py b/tests/channels/test_wecom_channel.py index f2367219..a8ed3c0e 100644 --- a/tests/channels/test_wecom_channel.py +++ b/tests/channels/test_wecom_channel.py @@ -574,7 +574,7 @@ async def test_process_mixed_message() -> None: assert msg.sender_id == "user1" assert msg.chat_id == "chat1" assert msg.content.startswith("hello wecom") - assert msg.metadata["msg_type"] == "text" + assert msg.metadata["msg_type"] == "mixed" assert len(msg.media) == 1 assert msg.media[0].endswith("photo.png") assert "[image:" in msg.content