diff options
| author | tanaka takashi <isazaka@gmail.com> | 2026-08-16 21:34:13 +0900 |
|---|---|---|
| committer | tanaka takashi <isazaka@gmail.com> | 2026-08-16 21:34:13 +0900 |
| commit | a9f083785a7e961a291e5366013fc89c0b801b01 (patch) | |
| tree | cb456d3ee40b00b8b475429a6e29f05c93e0efad | |
| parent | e45e2dbf160445891ec765ffb3e0e14f106cfb56 (diff) | |
refactor: streamline chat flow and improve diagnostics
Store server.log inside OLLAMA_WEB_DATA_DIR and manage the ollama-web file handler through the Starlette lifespan. Close and detach the handler on shutdown to prevent duplicate records and Windows file locks when applications are recreated.
Stop converting DuckDuckGo provider failures into empty search results. Record a query-safe traceback, re-raise the original exception, and let ToolRegistry expose the exception type and message as a tool error without adding or spoofing Referer headers.
Remove the unused non-streaming chat_with_tools implementation and its private helpers. Move capability, options, standard tool, MCP tool, and final tools=[] regression coverage onto the production streaming path.
Add integration coverage for data/server.log placement, handler cleanup, repeated lifespans, and provider 403 diagnostics. Update the Unreleased changelog entry.
Validated with 118 tests, Ruff, mypy, diff checks, real DuckDuckGo searches, successful result-page fetches, and observable 401/403 responses from sites that reject automated retrieval.
| -rw-r--r-- | CHANGELOG.md | 9 | ||||
| -rw-r--r-- | src/ollama_web/app.py | 34 | ||||
| -rw-r--r-- | src/ollama_web/llm.py | 181 | ||||
| -rw-r--r-- | src/ollama_web/tools/search.py | 13 | ||||
| -rw-r--r-- | tests/test_llm.py | 101 | ||||
| -rw-r--r-- | tests/test_logging.py | 88 | ||||
| -rw-r--r-- | tests/test_settings.py | 25 | ||||
| -rw-r--r-- | tests/test_tools.py | 49 |
8 files changed, 248 insertions, 252 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 1381129..f7d0b69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ ## [Unreleased] +### Changed + +- 使用されていない非ストリーミングのチャット処理を削除し、ツール呼び出し関連のテストを実運用のストリーミング処理へ統合しました。 + +### Fixed + +- `server.log`を`OLLAMA_WEB_DATA_DIR`の親ではなくデータディレクトリ直下へ保存し、アプリケーションの起動・終了に合わせてログハンドラーを開閉するようにしました。 +- DuckDuckGo検索で発生した例外を空の検索結果として握りつぶさず、検索語を記録しない診断ログを残してツール実行層へ伝播するようにしました。 + ## [0.5.0] - 2026-08-16 ### Added diff --git a/src/ollama_web/app.py b/src/ollama_web/app.py index eeb3bae..9792c7b 100644 --- a/src/ollama_web/app.py +++ b/src/ollama_web/app.py @@ -3,6 +3,8 @@ from __future__ import annotations import logging +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager from pathlib import Path from typing import Any @@ -42,17 +44,29 @@ class NoCacheStaticFiles(StaticFiles): _STATIC_DIR = Path(__file__).resolve().parent / "static" +@asynccontextmanager +async def _lifespan(app: Starlette) -> AsyncIterator[None]: + """Keep the application log handler scoped to the ASGI lifespan.""" + log_dir = Path(app.state.settings.data_dir).resolve() + log_dir.mkdir(parents=True, exist_ok=True) + handler = logging.FileHandler(str(log_dir / "server.log"), encoding="utf-8") + handler.setFormatter( + logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s") + ) + logger = logging.getLogger("ollama_web") + previous_level = logger.level + logger.setLevel(logging.INFO) + logger.addHandler(handler) + try: + yield + finally: + logger.removeHandler(handler) + handler.close() + logger.setLevel(previous_level) + + def create_app() -> Starlette: """Build and return the configured Starlette application.""" - # Ensure application logs are written alongside uvicorn access logs. - log_path = Path(settings.data_dir).resolve().parent / "server.log" - log_path.parent.mkdir(parents=True, exist_ok=True) - handler = logging.FileHandler(str(log_path), encoding="utf-8") - handler.setFormatter(logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s")) - root = logging.getLogger() - root.setLevel(logging.INFO) - root.addHandler(handler) - routes = [ Route("/login", pages_route.login, name="login_page"), Route("/api/auth/login", auth_route.login, methods=["POST"], name="auth_login"), @@ -139,7 +153,7 @@ def create_app() -> Starlette: ) ) - app = Starlette(routes=routes, middleware=middleware) + app = Starlette(routes=routes, middleware=middleware, lifespan=_lifespan) app.state.settings = settings app.state.session_store = SessionStore(settings.data_dir) app.state.tool_names = ["web_search", "scrape_url", "search_and_fetch", "pdf_to_text"] diff --git a/src/ollama_web/llm.py b/src/ollama_web/llm.py index 4951b36..187b5e7 100644 --- a/src/ollama_web/llm.py +++ b/src/ollama_web/llm.py @@ -132,14 +132,6 @@ def _embed_images_in_content(message: Message) -> Message: return Message(**kwargs) -def _tool_calls(response: Any) -> list[dict[str, Any]]: - """Extract tool calls from an ollama response in a tolerant manner.""" - msg = getattr(response, "message", None) or ( - response.get("message") if isinstance(response, dict) else None - ) - return _tool_calls_from_message(msg) - - def _tool_calls_from_message(msg: Any) -> list[dict[str, Any]]: """Extract tool calls from a message object/dict.""" if msg is None: @@ -166,18 +158,6 @@ def _tool_calls_from_message(msg: Any) -> list[dict[str, Any]]: return out -def _assistant_message(response: Any) -> Message: - """Convert a non-streaming response into a Message for re-submission.""" - msg = getattr(response, "message", None) or ( - response.get("message") if isinstance(response, dict) else None - ) - if isinstance(msg, dict): - return Message(**msg) - if msg is not None: - return cast(Message, msg) - return Message(role="assistant", content="") - - def _token_counts(response: Any) -> dict[str, int] | None: """Extract prompt_eval_count and eval_count from an ollama response.""" if response is None: @@ -193,161 +173,6 @@ def _token_counts(response: Any) -> dict[str, int] | None: return {"prompt_eval_count": int(prompt_eval), "eval_count": int(eval_count)} -def chat_with_tools( - messages: list[Message], - model: str, - *, - think: bool | str | None = None, - registry: ToolRegistry | None = None, - host: str | None = None, - capabilities: set[str] | None = None, - options: dict[str, Any] | None = None, - language: str | None = None, -) -> Iterator[dict[str, Any]]: - """Run a multi-round tool-calling chat and yield progress events. - - Events are dicts with a ``type`` key: - - - ``{"type": "tool_start", "name": str, "arguments": dict}`` - - ``{"type": "tool_end", "name": str, "result": str}`` - - ``{"type": "delta", "content": str}`` - - ``{"type": "thinking", "content": str}`` - - ``{"type": "done", "prompt_eval_count": int, "eval_count": int}`` - - ``{"type": "error", "message": str}`` - - The final assistant message is streamed token-by-token via ``delta`` events. - """ - import logging - - logger = logging.getLogger("ollama_web.llm") - - caps = capabilities or set() - raw_client = get_client(host) - reg = registry or default_registry() - tools: list[ToolCallable] = [cast(Any, t) for t in reg.ollama_tools] - effective_tools = tools if "tools" in caps else [] - effective_think = think if "thinking" in caps else None - - # Non-vision models cannot use the ``images`` field; embed them as base64. - if "vision" not in caps: - messages = [_embed_images_in_content(m) if _is_image_message(m) else m for m in messages] - - cumulative_tool_chars = 0 - - rounds = 0 - while rounds < MAX_TOOL_ROUNDS: - rounds += 1 - logger.info( - "tool round %d/%d start messages=%d model=%s cumulative_tool_chars=%d", - rounds, - MAX_TOOL_ROUNDS, - len(messages), - model, - cumulative_tool_chars, - ) - try: - resp = cast(Any, raw_client).chat( - model=model, - messages=messages, - tools=effective_tools, - think=effective_think, - options=options or None, - stream=False, - ) - except Exception as exc: # noqa: BLE001 - logger.warning("ollama chat call failed: %s", exc) - yield {"type": "error", "message": str(exc)} - return - - calls = _tool_calls(resp) - tokens = _token_counts(resp) - if not calls: - # No tool calls: stream the final answer for UI effect. - content = _assistant_content(resp) - thinking = _assistant_thinking(resp) - if thinking: - yield {"type": "thinking", "content": thinking} - if content: - yield {"type": "delta", "content": content} - done_event: dict[str, Any] = {"type": "done"} - if tokens: - done_event.update(tokens) - yield done_event - return - - # Append the assistant message that requested tool calls. - messages.append(_assistant_message(resp)) - - # Execute each requested tool and append tool-role messages. - tool_results: list[tuple[str, str]] = [] - for call in calls: - name = call.get("name", "") - args = call.get("arguments", {}) - - yield {"type": "tool_start", "name": name, "arguments": _safe_tool_arguments(args)} - result = reg.execute(name, args if isinstance(args, dict) else json.dumps(args)) - result = _safe_tool_result(_trim_tool_result(result, settings.max_tool_result_chars)) - yield {"type": "tool_end", "name": name, "result": result} - tool_results.append((name, result)) - - if sum(len(r) for _, r in tool_results) > MAX_TOOL_CONTEXT_CHARS: - budget = MAX_TOOL_CONTEXT_CHARS - len(tool_results) * 200 - max_per_tool = budget // max(1, len(tool_results)) - logger.warning( - "tool results too large (%d chars), truncating to %d chars per tool", - sum(len(r) for _, r in tool_results), - max_per_tool, - ) - tool_results = [(n, _trim_tool_result(r, max_per_tool)) for n, r in tool_results] - - cumulative_tool_chars += sum(len(r) for _, r in tool_results) - for name, result in tool_results: - messages.append(Message(role="tool", tool_name=name, content=result)) - - # Exceeded tool rounds: ask for a final answer without tools. - logger.info( - ( - "max tool rounds exceeded, requesting final answer without tools " - "(cumulative_tool_chars=%d)" - ), - cumulative_tool_chars, - ) - yield { - "type": "status", - "message": t("status.tool_limit_reached", lang=language), - } - messages.append( - Message( - role="user", - content=get_prompt("tool_limit_instruction", lang=language), - ) - ) - try: - resp = cast(Any, raw_client).chat( - model=model, - messages=messages, - tools=[], - think=effective_think, - options=options or None, - stream=False, - ) - except Exception as exc: # noqa: BLE001 - logger.warning("final ollama chat call failed: %s", exc) - yield {"type": "error", "message": str(exc)} - return - tokens = _token_counts(resp) - content = _assistant_content(resp) - thinking = _assistant_thinking(resp) - if thinking: - yield {"type": "thinking", "content": thinking} - if content: - yield {"type": "delta", "content": content} - done_event_final: dict[str, Any] = {"type": "done"} - if tokens: - done_event_final.update(tokens) - yield done_event_final - - def _trim_tool_result(result: str, limit: int) -> str: """Truncate ``result`` to ``limit`` chars while keeping it readable. @@ -684,12 +509,6 @@ def _assistant_thinking(response: Any) -> str: return str(getattr(msg, "thinking", "") or "") -def _chunk_text(text: str, size: int) -> Iterator[str]: - """Yield successive chunks of ``text`` up to ``size`` characters.""" - for i in range(0, len(text), size): - yield text[i : i + size] - - def _assistant_message_chunk(full_message: dict[str, Any]) -> Message | None: """Build an assistant ``Message`` from the accumulated streamed message. diff --git a/src/ollama_web/tools/search.py b/src/ollama_web/tools/search.py index 761f182..773f7f3 100644 --- a/src/ollama_web/tools/search.py +++ b/src/ollama_web/tools/search.py @@ -2,10 +2,14 @@ from __future__ import annotations +import logging + from ddgs import DDGS from ..config import settings +logger = logging.getLogger(__name__) + def web_search(query: str, max_results: int | None = None) -> list[dict[str, str]]: """Search the web using DuckDuckGo and return compact result summaries. @@ -16,7 +20,8 @@ def web_search(query: str, max_results: int | None = None) -> list[dict[str, str Returns: A list of dictionaries with keys 'title', 'href' and 'body' for each - result. Returns an empty list if the search fails or no results are found. + result. Returns an empty list if no results are found. Provider failures + are logged and propagated to the tool execution layer. """ if not query or not query.strip(): return [] @@ -37,7 +42,9 @@ def web_search(query: str, max_results: int | None = None) -> list[dict[str, str } ) except Exception: # noqa: BLE001 - # Swallow network/runtime errors so the chat loop can continue. - return [] + # Preserve the provider exception for the tool layer while recording + # the traceback without logging the potentially sensitive query. + logger.exception("DuckDuckGo web search failed") + raise return results[:limit] diff --git a/tests/test_llm.py b/tests/test_llm.py index edd9d61..0e1b146 100644 --- a/tests/test_llm.py +++ b/tests/test_llm.py @@ -9,13 +9,11 @@ from typing import Any import pytest from ollama import Message from ollama._types import Image as OllamaImage -from ollama._types import ResponseError from ollama_web.llm import ( _embed_images_in_content, _is_image_message, _token_counts, - chat_with_tools, stream_chat_with_tools, ) from ollama_web.tools.registry import ToolRegistry @@ -44,46 +42,44 @@ def test_embed_images_in_content_adds_base64_markdown() -> None: assert not getattr(embedded, "images", None) -def test_chat_with_tools_skips_tools_for_non_tool_model() -> None: - """Tools are not passed when the model does not advertise tool support.""" - messages = [Message(role="user", content="hi")] - # Use an empty registry so any tool call would fail if requested. +def test_stream_chat_with_tools_disables_unsupported_capabilities( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Unsupported tools and thinking options are not sent to Ollama.""" + from ollama_web import llm + registry = ToolRegistry() - # Without tool capability, the iterator should finish immediately with no - # actual network request. We only verify it accepts the call and does not - # raise due to tool-related setup. - gen = chat_with_tools( - messages, - model="nonexistent-model", - capabilities=set(), - registry=registry, - ) - events = list(gen) - # We expect either an error (model not found) or a done event. - assert any(e.get("type") in {"error", "done"} for e in events) + def lookup(query: str) -> str: + return query + registry.register(lookup) -def test_chat_with_tools_forces_think_none_for_non_thinking_model() -> None: - messages = [Message(role="user", content="hi")] - gen = chat_with_tools( - messages, - model="nonexistent-model", - capabilities=set(), - ) - events = list(gen) - assert any(e.get("type") in {"error", "done"} for e in events) + class FakeClient: + def __init__(self) -> None: + self.calls: list[dict[str, Any]] = [] + def chat(self, **kwargs: Any) -> list[SimpleNamespace]: + self.calls.append(kwargs) + return [SimpleNamespace(message={"content": "done"})] + + client = FakeClient() + monkeypatch.setattr(llm, "get_client", lambda _host=None: client) -def test_stream_chat_with_tools_accepts_capability_set() -> None: - messages = [Message(role="user", content="hi")] - gen = stream_chat_with_tools( - messages, - model="nonexistent-model", - capabilities={"tools", "thinking"}, + events = list( + stream_chat_with_tools( + [Message(role="user", content="hi")], + model="test-model", + think=True, + capabilities=set(), + registry=registry, + ) ) - with pytest.raises(ResponseError): - list(gen) + + assert client.calls[0]["tools"] == [] + assert client.calls[0]["think"] is None + assert client.calls[0]["stream"] is True + assert events[-1] == {"type": "done"} def test_token_counts_extracts_from_dict() -> None: @@ -109,7 +105,7 @@ def test_token_counts_returns_none_when_missing() -> None: assert _token_counts(None) is None -def test_chat_with_tools_disables_tools_for_final_answer_after_limit( +def test_stream_chat_with_tools_disables_tools_for_final_answer_after_limit( monkeypatch: pytest.MonkeyPatch, ) -> None: from ollama_web import llm @@ -128,26 +124,33 @@ def test_chat_with_tools_disables_tools_for_final_answer_after_limit( def __init__(self) -> None: self.calls: list[dict[str, Any]] = [] - def chat(self, **kwargs: Any) -> dict[str, Any]: + def chat(self, **kwargs: Any) -> list[SimpleNamespace]: self.calls.append(kwargs) if len(self.calls) == 1: - return { - "message": { - "role": "assistant", - "content": "", - "tool_calls": [ - {"function": {"name": "lookup", "arguments": {"query": "test"}}} - ], - } - } - return {"message": {"role": "assistant", "content": "final answer"}} + return [ + SimpleNamespace( + message={ + "role": "assistant", + "content": "", + "tool_calls": [ + { + "function": { + "name": "lookup", + "arguments": {"query": "test"}, + } + } + ], + } + ) + ] + return [SimpleNamespace(message={"content": "final answer"})] client = FakeClient() monkeypatch.setattr(llm, "get_client", lambda _host=None: client) monkeypatch.setattr(llm, "MAX_TOOL_ROUNDS", 1) events = list( - llm.chat_with_tools( + llm.stream_chat_with_tools( [Message(role="user", content="research this")], model="test-model", registry=registry, @@ -160,6 +163,8 @@ def test_chat_with_tools_disables_tools_for_final_answer_after_limit( assert len(client.calls) == 2 assert client.calls[0]["tools"] assert client.calls[1]["tools"] == [] + assert client.calls[0]["stream"] is True + assert client.calls[1]["stream"] is True assert any(event.get("type") == "status" for event in events) assert any(event == {"type": "delta", "content": "final answer"} for event in events) assert events[-1] == {"type": "done"} diff --git a/tests/test_logging.py b/tests/test_logging.py new file mode 100644 index 0000000..ef6fbd2 --- /dev/null +++ b/tests/test_logging.py @@ -0,0 +1,88 @@ +"""Tests for application file logging.""" + +from __future__ import annotations + +import json +import logging +from pathlib import Path +from unittest.mock import patch + +import pytest +from starlette.testclient import TestClient + +from ollama_web.app import create_app +from ollama_web.config import settings +from ollama_web.tools.registry import default_registry + + +def test_server_log_is_scoped_to_data_dir_and_app_lifespan( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + data_dir = tmp_path / "data" + monkeypatch.setattr(settings, "data_dir", str(data_dir)) + app_logger = logging.getLogger("ollama_web") + original_handlers = tuple(app_logger.handlers) + original_level = app_logger.level + + app = create_app() + log_path = data_dir / "server.log" + assert not log_path.exists() + + with TestClient(app): + assert len(app_logger.handlers) == len(original_handlers) + 1 + logging.getLogger("ollama_web.test").info("lifespan log marker") + + assert tuple(app_logger.handlers) == original_handlers + assert app_logger.level == original_level + assert log_path.read_text(encoding="utf-8").count("lifespan log marker") == 1 + + # Closing the lifespan must release the file on Windows as well. + log_path.unlink() + assert not log_path.exists() + + +def test_repeated_app_lifespans_do_not_duplicate_log_handlers( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + data_dir = tmp_path / "data" + monkeypatch.setattr(settings, "data_dir", str(data_dir)) + + with TestClient(create_app()): + logging.getLogger("ollama_web.test").info("first app marker") + with TestClient(create_app()): + logging.getLogger("ollama_web.test").info("second app marker") + + content = (data_dir / "server.log").read_text(encoding="utf-8") + assert content.count("first app marker") == 1 + assert content.count("second app marker") == 1 + + +def test_web_search_provider_error_is_written_to_server_log( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + data_dir = tmp_path / "data" + monkeypatch.setattr(settings, "data_dir", str(data_dir)) + + class FailingDDGS: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def text(self, query, max_results=5): + raise RuntimeError("403 Forbidden") + + with patch("ollama_web.tools.search.DDGS", FailingDDGS), TestClient(create_app()): + result = json.loads( + default_registry().execute("web_search", {"query": "private search terms"}) + ) + + assert result == {"error": "RuntimeError: 403 Forbidden"} + content = (data_dir / "server.log").read_text(encoding="utf-8") + assert "DuckDuckGo web search failed" in content + assert "RuntimeError: 403 Forbidden" in content + assert "private search terms" not in content diff --git a/tests/test_settings.py b/tests/test_settings.py index 0f334f4..d19f64d 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -4,6 +4,7 @@ from __future__ import annotations import json from pathlib import Path +from types import SimpleNamespace from typing import Any import pytest @@ -151,24 +152,28 @@ def test_options_are_forwarded_to_tool_and_final_rounds(monkeypatch: pytest.Monk def __init__(self) -> None: self.calls: list[dict[str, Any]] = [] - def chat(self, **kwargs: Any) -> dict[str, Any]: + def chat(self, **kwargs: Any) -> list[SimpleNamespace]: self.calls.append(kwargs) if len(self.calls) == 1: - return { - "message": { - "role": "assistant", - "content": "", - "tool_calls": [{"function": {"name": "missing_tool", "arguments": {}}}], - } - } - return {"message": {"role": "assistant", "content": "done"}} + return [ + SimpleNamespace( + message={ + "role": "assistant", + "content": "", + "tool_calls": [ + {"function": {"name": "missing_tool", "arguments": {}}} + ], + } + ) + ] + return [SimpleNamespace(message={"content": "done"})] client = FakeClient() monkeypatch.setattr(llm, "get_client", lambda _host=None: client) monkeypatch.setattr(llm, "MAX_TOOL_ROUNDS", 1) options = {"temperature": 0.8, "num_ctx": 8192, "top_p": 0.9} events = list( - llm.chat_with_tools( + llm.stream_chat_with_tools( [Message(role="user", content="hello")], "test-model", capabilities={"tools"}, diff --git a/tests/test_tools.py b/tests/test_tools.py index a94ea52..387cbe2 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -114,6 +114,55 @@ def test_web_search_mocked(): assert result[0]["title"] == "T" +def test_web_search_returns_empty_list_for_no_provider_results() -> None: + from ollama_web.tools import search as search_mod + + class EmptyDDGS: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def text(self, query, max_results=5): + return [] + + with patch.object(search_mod, "DDGS", EmptyDDGS): + assert search_mod.web_search("nothing found") == [] + + +def test_web_search_logs_and_propagates_provider_errors( + caplog: pytest.LogCaptureFixture, +) -> None: + from ollama_web.tools import search as search_mod + + class FailingDDGS: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def text(self, query, max_results=5): + raise RuntimeError("403 Forbidden") + + with ( + patch.object(search_mod, "DDGS", FailingDDGS), + caplog.at_level("ERROR", logger="ollama_web.tools.search"), + ): + with pytest.raises(RuntimeError, match="403 Forbidden"): + search_mod.web_search("private search terms") + + assert "DuckDuckGo web search failed" in caplog.text + assert "private search terms" not in caplog.text + + registry = ToolRegistry() + registry.register(search_mod.web_search) + with patch.object(search_mod, "DDGS", FailingDDGS): + result = json.loads(registry.execute("web_search", {"query": "test"})) + assert result == {"error": "RuntimeError: 403 Forbidden"} + + # --------------------------------------------------------------------------- # scrape_url tests (existing + pagination) # --------------------------------------------------------------------------- |
