From 4488126b96337b43eda2e479beb67df4a4fd9338 Mon Sep 17 00:00:00 2001 From: Enyr Date: Fri, 8 May 2026 12:03:52 +0200 Subject: [PATCH] fix(chat): route /api/repos/{repo_id}/chat/messages by workspace_sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In workspace mode every non-primary repo's chat returns ``404 Repository not found`` even though the same id is listed in both ``GET /api/repos`` and ``GET /api/workspace.repos[].repo_id``. The chat router was the only ``/api/repos/{repo_id}/...`` endpoint not honouring ``app.state.workspace_sessions``. The conversation endpoints (``GET /chat/conversations``, etc.) already work correctly because they use ``Depends(get_db_session)``, which routes by ``repo_id`` automatically. ``chat_messages`` couldn't use that dep — it needs the *factory* (not a single session) so it can open multiple sessions over the SSE lifetime — so it was hand-wiring ``request.app.state.session_factory`` and missing the workspace branch. Fix: * Extract the factory-resolution logic from ``get_db_session`` into two reusable helpers in ``deps.py``: - ``resolve_session_factory(app_state, repo_id)`` — pure function, same logic as the existing private helper in ``routers/repos.py``. - ``resolve_request_session_factory(request)`` — request-scoped sibling that reads ``repo_id`` from path/query params (mirrors what ``get_db_session`` encodes). * ``chat_messages`` calls ``resolve_request_session_factory(request)`` instead of ``request.app.state.session_factory`` directly. * ``routers/repos.py::_resolve_repo_session_factory`` is now a one-line alias around the shared helper, removing the duplicate logic. Test: * New regression test ``tests/unit/server/test_chat_workspace.py``: - ``test_chat_messages_resolves_non_primary_repo_in_workspace_mode`` — POST to a non-primary ``repo_id`` no longer returns 404. Verified to fail on the unpatched source with the exact ``Repository not found`` message. - ``test_chat_messages_still_finds_primary_repo`` — the single-factory fallback still works when ``workspace_sessions`` is empty (single-repo mode and the primary repo of a workspace). * All 126 existing ``tests/unit/server/`` tests pass with the patch. --- packages/server/src/repowise/server/deps.py | 41 +++-- .../src/repowise/server/routers/chat.py | 13 +- .../src/repowise/server/routers/repos.py | 15 +- tests/unit/server/test_chat_workspace.py | 155 ++++++++++++++++++ 4 files changed, 203 insertions(+), 21 deletions(-) create mode 100644 tests/unit/server/test_chat_workspace.py diff --git a/packages/server/src/repowise/server/deps.py b/packages/server/src/repowise/server/deps.py index 831b6679b..1531d2892 100644 --- a/packages/server/src/repowise/server/deps.py +++ b/packages/server/src/repowise/server/deps.py @@ -36,23 +36,42 @@ ) +def resolve_session_factory(app_state, repo_id: str | None): + """Pick the session factory whose database contains ``repo_id``. + + In workspace mode each repo has its own ``wiki.db`` registered under + ``app_state.workspace_sessions[repo_id]``. The primary + ``app_state.session_factory`` does NOT see those rows. When ``repo_id`` + is ``None`` or unknown, falls back to the primary factory (single-repo + mode, or the primary repo of a workspace). + """ + if repo_id: + ws_sessions = getattr(app_state, "workspace_sessions", None) + if ws_sessions and repo_id in ws_sessions: + return ws_sessions[repo_id] + return app_state.session_factory + + +def resolve_request_session_factory(request: Request): + """Return the session factory for the active route's ``repo_id``. + + Reads ``repo_id`` from the path (e.g. ``/api/repos/{repo_id}/...``) + or query string (e.g. ``/api/pages?repo_id=xxx``), then delegates to + :func:`resolve_session_factory`. Used by streaming endpoints that + can't take a single ``Depends(get_db_session)`` because they need to + open multiple sessions over the request lifetime. + """ + repo_id = request.path_params.get("repo_id") or request.query_params.get("repo_id") + return resolve_session_factory(request.app.state, repo_id) + + async def get_db_session(request: Request) -> AsyncGenerator[AsyncSession, None]: """Yield an async DB session with auto-commit on success, rollback on error. In workspace mode, routes to the correct repo's DB based on the ``repo_id`` path parameter when a matching session factory exists. """ - factory = request.app.state.session_factory - - # In workspace mode, check if the request targets a specific repo DB - # Check both path params (e.g. /api/repos/{repo_id}/stats) and - # query params (e.g. /api/pages?repo_id=xxx) for the repo_id - repo_id = request.path_params.get("repo_id") or request.query_params.get("repo_id") - if repo_id: - ws_sessions = getattr(request.app.state, "workspace_sessions", None) - if ws_sessions and repo_id in ws_sessions: - factory = ws_sessions[repo_id] - + factory = resolve_request_session_factory(request) async with get_session(factory) as session: yield session diff --git a/packages/server/src/repowise/server/routers/chat.py b/packages/server/src/repowise/server/routers/chat.py index 5e1ee757b..2ea16f2b7 100644 --- a/packages/server/src/repowise/server/routers/chat.py +++ b/packages/server/src/repowise/server/routers/chat.py @@ -17,7 +17,11 @@ get_artifact_type, get_tool_schemas_for_llm, ) -from repowise.server.deps import get_db_session, verify_api_key +from repowise.server.deps import ( + get_db_session, + resolve_request_session_factory, + verify_api_key, +) from repowise.server.provider_config import get_chat_provider_instance, set_active_provider from repowise.server.schemas import ( ChatMessageResponse, @@ -70,7 +74,12 @@ async def _get_repo_info(factory: Any, repo_id: str) -> tuple[str, str]: @router.post("/api/repos/{repo_id}/chat/messages") async def chat_messages(repo_id: str, body: ChatRequest, request: Request): """Stream an agentic chat response via SSE.""" - factory = request.app.state.session_factory + # In workspace mode each repo has its own ``wiki.db``; the primary + # ``app.state.session_factory`` does NOT contain non-primary repos' + # rows, so resolving by ``repo_id`` is required for the + # ``_get_repo_info`` lookup (and every subsequent ``get_session`` + # call inside ``event_stream``) to land in the right database. + factory = resolve_request_session_factory(request) # Resolve repo repo_name, repo_path = await _get_repo_info(factory, repo_id) diff --git a/packages/server/src/repowise/server/routers/repos.py b/packages/server/src/repowise/server/routers/repos.py index da5f3375b..4ea2208fa 100644 --- a/packages/server/src/repowise/server/routers/repos.py +++ b/packages/server/src/repowise/server/routers/repos.py @@ -271,16 +271,15 @@ async def full_resync( def _resolve_repo_session_factory(app_state, repo_id: str): - """Return the session_factory whose database contains this repo's row. + """Backward-compatible alias for :func:`deps.resolve_session_factory`. - In workspace mode each repo has its own ``wiki.db`` registered under - ``app_state.workspace_sessions[repo_id]``. The primary session_factory - (``app_state.session_factory``) does NOT see those rows. + Kept to avoid churn at call sites; new code should call + ``resolve_session_factory`` (or its request-scoped sibling + ``resolve_request_session_factory``) directly. """ - ws_sessions = getattr(app_state, "workspace_sessions", None) - if ws_sessions and repo_id in ws_sessions: - return ws_sessions[repo_id] - return app_state.session_factory + from repowise.server.deps import resolve_session_factory # noqa: PLC0415 + + return resolve_session_factory(app_state, repo_id) def _launch_job_task(request: Request, job_id: str, repo_id: str) -> None: diff --git a/tests/unit/server/test_chat_workspace.py b/tests/unit/server/test_chat_workspace.py new file mode 100644 index 000000000..4eb464cb8 --- /dev/null +++ b/tests/unit/server/test_chat_workspace.py @@ -0,0 +1,155 @@ +"""Regression test: chat endpoints honour ``workspace_sessions`` routing. + +Bug (pre-fix): in workspace mode each repo has its own ``wiki.db`` +registered under ``app.state.workspace_sessions[repo_id]``. The +chat endpoint resolved ``repo_id`` against the primary's session +factory (``app.state.session_factory``), which doesn't contain the +non-primary repos' rows. Result: every chat request to a +non-primary repo 404'd with ``Repository {repo_id} not found``, +even though the same id is listed in ``GET /api/repos`` and +``GET /api/workspace.repos[].repo_id``. + +Fix: ``chat_messages`` now uses +:func:`repowise.server.deps.resolve_request_session_factory`, +which mirrors the routing logic that ``get_db_session`` (used by +the conversation endpoints) already encoded. + +The test below builds a minimal FastAPI app with one primary repo +in the global session factory and a non-primary repo in +``workspace_sessions``, and asserts that POST /chat/messages on +the *non-primary* id passes the lookup (i.e. does not 404 on the +``Repository ... not found`` branch). +""" + +from __future__ import annotations + +from contextlib import asynccontextmanager +from datetime import UTC, datetime +from unittest.mock import AsyncMock, patch + +import pytest +from httpx import ASGITransport, AsyncClient +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine +from sqlalchemy.pool import StaticPool + +from fastapi import FastAPI +from fastapi.responses import JSONResponse +from repowise.core.persistence.database import init_db +from repowise.core.persistence.models import Repository +from repowise.server.routers import chat + + +_NOW = datetime(2026, 4, 12, 10, 0, 0, tzinfo=UTC) + + +async def _make_factory_with_repo(*, repo_id: str, name: str): + """Build an in-memory async session factory containing one repo row.""" + engine = create_async_engine( + "sqlite+aiosqlite:///:memory:", + connect_args={"check_same_thread": False}, + poolclass=StaticPool, + ) + await init_db(engine) + factory = async_sessionmaker(engine, expire_on_commit=False, class_=AsyncSession) + async with factory() as session: + session.add( + Repository( + id=repo_id, + name=name, + url=f"https://example.com/{name}", + local_path=f"/workspace/{name}", + default_branch="main", + settings_json="{}", + created_at=_NOW, + updated_at=_NOW, + ) + ) + await session.commit() + return factory + + +def _build_app(*, primary_factory, workspace_sessions: dict) -> FastAPI: + @asynccontextmanager + async def noop_lifespan(app: FastAPI): + yield + + app = FastAPI(title="chat-workspace-test", lifespan=noop_lifespan) + + @app.exception_handler(LookupError) + async def _lookup(_request, exc): + return JSONResponse(status_code=404, content={"detail": str(exc)}) + + app.state.session_factory = primary_factory + app.state.workspace_sessions = workspace_sessions + app.include_router(chat.router) + return app + + +@pytest.mark.asyncio +async def test_chat_messages_resolves_non_primary_repo_in_workspace_mode(): + """Pin the workspace-routing fix: the non-primary id must NOT 404.""" + primary = await _make_factory_with_repo(repo_id="primary-id", name="primary") + non_primary = await _make_factory_with_repo( + repo_id="non-primary-id", name="non-primary" + ) + + app = _build_app( + primary_factory=primary, + workspace_sessions={"non-primary-id": non_primary}, + ) + + # Stop after the chat handler has resolved the repo by replacing + # the chat-provider factory; if the lookup 404s, this never runs. + fake_provider = AsyncMock() + fake_provider.provider_name = "openai" + + with ( + patch( + "repowise.server.routers.chat.get_chat_provider_instance", + return_value=fake_provider, + ), + ): + transport = ASGITransport(app=app) + async with AsyncClient( + transport=transport, base_url="http://testserver" + ) as client: + response = await client.post( + "/api/repos/non-primary-id/chat/messages", + json={"message": "hi"}, + ) + + # Pre-fix this returned 404 "Repository non-primary-id not found". + # Post-fix the lookup succeeds — the 422 here is the next branch + # ("Provider does not support streaming chat") because our fake + # provider isn't a ChatProvider. Either 200 (full happy path) or + # 422 (provider check) proves the workspace routing worked. + assert response.status_code != 404, response.text + + +@pytest.mark.asyncio +async def test_chat_messages_still_finds_primary_repo(): + """Single-factory fallback: when ``repo_id`` isn't in + ``workspace_sessions``, the resolver falls back to + ``app.state.session_factory`` — covers single-repo mode AND the + primary repo of a workspace, both of which keep the row in the + global factory.""" + primary = await _make_factory_with_repo(repo_id="primary-id", name="primary") + app = _build_app(primary_factory=primary, workspace_sessions={}) + + fake_provider = AsyncMock() + fake_provider.provider_name = "openai" + + with patch( + "repowise.server.routers.chat.get_chat_provider_instance", + return_value=fake_provider, + ): + transport = ASGITransport(app=app) + async with AsyncClient( + transport=transport, base_url="http://testserver" + ) as client: + response = await client.post( + "/api/repos/primary-id/chat/messages", + json={"message": "hi"}, + ) + + assert response.status_code != 404, response.text