From 020dd4d6d851239d1260a20682e5e31aaf94a071 Mon Sep 17 00:00:00 2001 From: yanhe1 Date: Tue, 31 Mar 2026 12:19:10 +0800 Subject: [PATCH 1/3] docs(backend): add knowledge document read spec and plan --- ...-knowledge-document-read-implementation.md | 857 ++++++++++++++++++ ...0-wegent-knowledge-document-read-design.md | 339 +++++++ 2 files changed, 1196 insertions(+) create mode 100644 docs/superpowers/plans/2026-03-30-wegent-knowledge-document-read-implementation.md create mode 100644 docs/superpowers/specs/2026-03-30-wegent-knowledge-document-read-design.md diff --git a/docs/superpowers/plans/2026-03-30-wegent-knowledge-document-read-implementation.md b/docs/superpowers/plans/2026-03-30-wegent-knowledge-document-read-implementation.md new file mode 100644 index 00000000000..1fe46f7c90f --- /dev/null +++ b/docs/superpowers/plans/2026-03-30-wegent-knowledge-document-read-implementation.md @@ -0,0 +1,857 @@ +# Wegent Knowledge Document Read Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add a single-document content read capability to the Wegent Knowledge Base MCP skill, unify document-read logic through `KnowledgeOrchestrator`, and refactor existing REST `/detail` endpoints to reuse the unified read/detail flow without changing their public response shape. + +**Architecture:** Keep `app.services.rag.document_read_service` as the low-level paginator, add one orchestrator method for raw content paging and one orchestrator method for detail aggregation, then route MCP and REST callers through those two methods. Do not add a new public REST read endpoint; MCP gets raw `offset/limit/has_more`, while existing `/detail` routes remain compatibility adapters that map paging output back to `content_length` and `truncated`. + +**Tech Stack:** FastAPI, SQLAlchemy, Pydantic, pytest, unittest.mock, FastMCP + +--- + +## File Structure + +**Modify** +- `backend/app/schemas/knowledge.py` + - Add a typed schema for paginated document content reads so orchestrator and MCP share one contract. +- `backend/app/services/knowledge/orchestrator.py` + - Add `read_document_content(...)` and `get_document_detail(...)`. + - Centralize permission checks, argument validation, paging adaptation, and summary retrieval. +- `backend/app/mcp_server/tools/knowledge.py` + - Add the new `read_document_content` MCP tool that delegates to `knowledge_orchestrator`. +- `backend/app/api/endpoints/knowledge.py` + - Replace inline document-read logic in both `/detail` handlers with orchestrator calls. +- `backend/init_data/skills/wegent-knowledge/SKILL.md` + - Document the new MCP tool and its paging behavior. +- `backend/tests/services/knowledge/test_orchestrator.py` + - Add unit tests for raw content reads and detail aggregation. +- `backend/tests/mcp_server/test_tools.py` + - Add MCP tool tests for the new `read_document_content` function. + +**Create** +- `backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py` + - Add API tests for the standalone and KB-scoped `/detail` routes after refactor. + +## Task 1: Add Raw Content Read Contract And Orchestrator Paging + +**Files:** +- Modify: `backend/app/schemas/knowledge.py` +- Modify: `backend/app/services/knowledge/orchestrator.py` +- Modify: `backend/tests/services/knowledge/test_orchestrator.py` + +- [ ] **Step 1: Write the failing orchestrator tests for raw content reads** + +```python +def test_read_document_content_returns_paginated_payload( + self, orchestrator, mock_db, mock_user +): + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + + with ( + patch("app.services.knowledge.orchestrator.KnowledgeService") as mock_service, + patch( + "app.services.knowledge.orchestrator.document_read_service" + ) as mock_read_service, + ): + mock_service.get_knowledge_base.return_value = (MagicMock(id=77), True) + mock_db.query.return_value.filter.return_value.first.return_value = document + mock_read_service.read_documents.return_value = [ + { + "id": 9, + "name": "roadmap", + "content": "cdef", + "total_length": 10, + "offset": 2, + "returned_length": 4, + "has_more": True, + "kb_id": 77, + } + ] + + result = orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + offset=2, + limit=4, + ) + + assert result.document_id == 9 + assert result.content == "cdef" + assert result.total_length == 10 + assert result.returned_length == 4 + assert result.has_more is True + + +@pytest.mark.parametrize( + ("offset", "limit", "message"), + [ + (-1, 10, "offset must be greater than or equal to 0"), + (0, 0, "limit must be greater than 0"), + (0, 100001, "limit must be less than or equal to 100000"), + ], +) +def test_read_document_content_rejects_invalid_paging_args( + self, orchestrator, mock_db, mock_user, offset, limit, message +): + with pytest.raises(ValueError, match=message): + orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + offset=offset, + limit=limit, + ) + + +def test_read_document_content_raises_for_missing_document( + self, orchestrator, mock_db, mock_user +): + mock_db.query.return_value.filter.return_value.first.return_value = None + + with pytest.raises(ValueError, match="Document not found"): + orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=404, + ) +``` + +- [ ] **Step 2: Run the orchestrator tests to verify they fail** + +Run: + +```bash +uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py -k "read_document_content" -v +``` + +Expected: + +```text +FAIL ... AttributeError: 'KnowledgeOrchestrator' object has no attribute 'read_document_content' +``` + +- [ ] **Step 3: Add the paginated response schema** + +Add this model next to `DocumentDetailResponse` in `backend/app/schemas/knowledge.py`: + +```python +class DocumentContentReadResponse(BaseModel): + """Schema for paginated single-document content reads.""" + + document_id: int = Field(..., description="Document ID") + name: str = Field(..., description="Document name") + content: str = Field(..., description="Returned content slice") + total_length: int = Field(..., ge=0, description="Full content length") + offset: int = Field(..., ge=0, description="Actual returned offset") + returned_length: int = Field( + ..., ge=0, description="Length of the returned content slice" + ) + has_more: bool = Field(..., description="Whether more content is available") + kb_id: int = Field(..., description="Knowledge base ID") +``` + +- [ ] **Step 4: Implement `read_document_content(...)` in the orchestrator** + +Add imports in `backend/app/services/knowledge/orchestrator.py`: + +```python +from app.models.knowledge import KnowledgeDocument +from app.schemas.knowledge import DocumentContentReadResponse +from app.services.rag.document_read_service import document_read_service +``` + +Add helpers and method on `KnowledgeOrchestrator`: + +```python +MAX_DOCUMENT_READ_LIMIT = 100000 + + +def _validate_document_read_args(offset: int, limit: int) -> None: + if offset < 0: + raise ValueError("offset must be greater than or equal to 0") + if limit <= 0: + raise ValueError("limit must be greater than 0") + if limit > MAX_DOCUMENT_READ_LIMIT: + raise ValueError( + f"limit must be less than or equal to {MAX_DOCUMENT_READ_LIMIT}" + ) + + +def read_document_content( + self, + db: Session, + user: User, + document_id: int, + offset: int = 0, + limit: int = MAX_DOCUMENT_READ_LIMIT, +) -> DocumentContentReadResponse: + _validate_document_read_args(offset, limit) + + document = ( + db.query(KnowledgeDocument) + .filter(KnowledgeDocument.id == document_id) + .first() + ) + if not document: + raise ValueError("Document not found") + + knowledge_base, has_access = KnowledgeService.get_knowledge_base( + db=db, + knowledge_base_id=document.kind_id, + user_id=user.id, + ) + if not knowledge_base: + raise ValueError("Knowledge base not found") + if not has_access: + raise ValueError("Access denied to this document") + + result = document_read_service.read_documents( + db=db, + document_ids=[document_id], + offset=offset, + limit=limit, + knowledge_base_ids=[document.kind_id], + )[0] + + if result.get("error") == "Document not found": + raise ValueError("Document not found") + if result.get("error"): + raise ValueError(result["error"]) + + return DocumentContentReadResponse( + document_id=result["id"], + name=result.get("name", document.name), + content=result.get("content", ""), + total_length=result.get("total_length", 0), + offset=result.get("offset", offset), + returned_length=result.get("returned_length", 0), + has_more=result.get("has_more", False), + kb_id=result.get("kb_id", document.kind_id), + ) +``` + +- [ ] **Step 5: Run the orchestrator tests to verify they pass** + +Run: + +```bash +uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py -k "read_document_content" -v +``` + +Expected: + +```text +PASSED ... test_read_document_content_returns_paginated_payload +PASSED ... test_read_document_content_rejects_invalid_paging_args +PASSED ... test_read_document_content_raises_for_missing_document +``` + +- [ ] **Step 6: Commit the raw read foundation** + +```bash +git add backend/app/schemas/knowledge.py backend/app/services/knowledge/orchestrator.py backend/tests/services/knowledge/test_orchestrator.py +git commit -m "feat(backend): add orchestrated knowledge document reads" +``` + +## Task 2: Add Orchestrator Detail Aggregation + +**Files:** +- Modify: `backend/app/services/knowledge/orchestrator.py` +- Modify: `backend/tests/services/knowledge/test_orchestrator.py` + +- [ ] **Step 1: Write the failing tests for detail aggregation** + +Append these tests to `backend/tests/services/knowledge/test_orchestrator.py` and update the existing import to `from unittest.mock import AsyncMock, MagicMock, Mock, patch`: + +```python +@pytest.mark.asyncio +async def test_get_document_detail_maps_content_length_and_truncated( + self, orchestrator, mock_db, mock_user +): + paged = SimpleNamespace( + document_id=9, + name="roadmap", + content="abcd", + total_length=10, + offset=0, + returned_length=4, + has_more=True, + kb_id=77, + ) + + with ( + patch.object(orchestrator, "read_document_content", return_value=paged), + patch("app.services.knowledge.orchestrator.get_summary_service") as mock_get_summary, + ): + mock_get_summary.return_value.get_document_summary = AsyncMock(return_value={ + "summary": "hello" + }) + + result = await orchestrator.get_document_detail( + db=mock_db, + user=mock_user, + document_id=9, + include_content=True, + include_summary=True, + ) + + assert result.document_id == 9 + assert result.content == "abcd" + assert result.content_length == 10 + assert result.truncated is True + assert result.summary == {"summary": "hello"} + + +@pytest.mark.asyncio +async def test_get_document_detail_skips_content_when_disabled( + self, orchestrator, mock_db, mock_user +): + with ( + patch.object(orchestrator, "read_document_content") as mock_read, + patch("app.services.knowledge.orchestrator.get_summary_service") as mock_get_summary, + ): + result = await orchestrator.get_document_detail( + db=mock_db, + user=mock_user, + document_id=9, + include_content=False, + include_summary=False, + ) + + mock_read.assert_not_called() + mock_get_summary.assert_not_called() + assert result.content is None + assert result.content_length is None + assert result.truncated is None + assert result.summary is None +``` + +- [ ] **Step 2: Run the detail tests to verify they fail** + +Run: + +```bash +uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py -k "get_document_detail" -v +``` + +Expected: + +```text +FAIL ... AttributeError: 'KnowledgeOrchestrator' object has no attribute 'get_document_detail' +``` + +- [ ] **Step 3: Implement `get_document_detail(...)` in the orchestrator** + +Add the summary import: + +```python +from app.services.knowledge import get_summary_service +``` + +Add the async method: + +```python +async def get_document_detail( + self, + db: Session, + user: User, + document_id: int, + include_content: bool = True, + include_summary: bool = True, + offset: int = 0, + limit: int = MAX_DOCUMENT_READ_LIMIT, +) -> DocumentDetailResponse: + content = None + content_length = None + truncated = None + summary = None + + if include_content: + paged = self.read_document_content( + db=db, + user=user, + document_id=document_id, + offset=offset, + limit=limit, + ) + content = paged.content + content_length = paged.total_length + truncated = paged.has_more + + if include_summary: + summary_service = get_summary_service(db) + summary_obj = await summary_service.get_document_summary(document_id) + if hasattr(summary_obj, "model_dump"): + summary = summary_obj.model_dump() + else: + summary = summary_obj + + return DocumentDetailResponse( + document_id=document_id, + content=content, + content_length=content_length, + truncated=truncated, + summary=summary, + ) +``` + +- [ ] **Step 4: Run the detail tests to verify they pass** + +Run: + +```bash +uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py -k "get_document_detail" -v +``` + +Expected: + +```text +PASSED ... test_get_document_detail_maps_content_length_and_truncated +PASSED ... test_get_document_detail_skips_content_when_disabled +``` + +- [ ] **Step 5: Commit the detail aggregation layer** + +```bash +git add backend/app/services/knowledge/orchestrator.py backend/tests/services/knowledge/test_orchestrator.py +git commit -m "refactor(backend): centralize knowledge document detail aggregation" +``` + +## Task 3: Add MCP Tool And Skill Prompt Support + +**Files:** +- Modify: `backend/app/mcp_server/tools/knowledge.py` +- Modify: `backend/init_data/skills/wegent-knowledge/SKILL.md` +- Modify: `backend/tests/mcp_server/test_tools.py` + +- [ ] **Step 1: Write the failing MCP tool tests** + +Add a new test class to `backend/tests/mcp_server/test_tools.py`: + +```python +from app.mcp_server.auth import TaskTokenInfo +from app.mcp_server.tools.knowledge import read_document_content + + +class TestKnowledgeReadDocumentTool: + def test_read_document_content_returns_orchestrator_payload(self): + token_info = TaskTokenInfo( + task_id=1, + subtask_id=2, + user_id=3, + user_name="alice", + ) + mock_user = MagicMock(id=3) + mock_result = MagicMock( + document_id=9, + name="roadmap", + content="abcd", + total_length=10, + offset=0, + returned_length=4, + has_more=True, + kb_id=77, + ) + mock_result.model_dump.return_value = { + "document_id": 9, + "name": "roadmap", + "content": "abcd", + "total_length": 10, + "offset": 0, + "returned_length": 4, + "has_more": True, + "kb_id": 77, + } + + with ( + patch("app.mcp_server.tools.knowledge.SessionLocal") as mock_session_local, + patch( + "app.mcp_server.tools.knowledge._get_user_from_token", + return_value=mock_user, + ), + patch( + "app.mcp_server.tools.knowledge.knowledge_orchestrator.read_document_content", + return_value=mock_result, + ) as mock_read, + ): + mock_session_local.return_value = MagicMock() + + result = read_document_content( + token_info=token_info, + document_id=9, + offset=0, + limit=4, + ) + + assert result["document_id"] == 9 + assert result["has_more"] is True + mock_read.assert_called_once() + + def test_read_document_content_returns_error_dict_for_validation_failure(self): + token_info = TaskTokenInfo( + task_id=1, + subtask_id=2, + user_id=3, + user_name="alice", + ) + + with ( + patch("app.mcp_server.tools.knowledge.SessionLocal") as mock_session_local, + patch( + "app.mcp_server.tools.knowledge._get_user_from_token", + return_value=MagicMock(id=3), + ), + patch( + "app.mcp_server.tools.knowledge.knowledge_orchestrator.read_document_content", + side_effect=ValueError("limit must be greater than 0"), + ), + ): + mock_session_local.return_value = MagicMock() + result = read_document_content(token_info=token_info, document_id=9, limit=0) + + assert result == {"error": "limit must be greater than 0"} +``` + +- [ ] **Step 2: Run the MCP tests to verify they fail** + +Run: + +```bash +uv run --project backend --group dev pytest tests/mcp_server/test_tools.py -k "read_document_content" -v +``` + +Expected: + +```text +FAIL ... ImportError or AttributeError because read_document_content does not exist yet +``` + +- [ ] **Step 3: Implement the MCP tool** + +Add this function to `backend/app/mcp_server/tools/knowledge.py` before `update_document_content(...)`: + +```python +@mcp_tool( + name="read_document_content", + description="Read document content with offset/limit pagination.", + server="knowledge", + param_descriptions={ + "document_id": "Document ID to read", + "offset": "Start position in the extracted text (default: 0)", + "limit": "Maximum number of characters to return (default: 100000)", + }, +) +def read_document_content( + token_info: TaskTokenInfo, + document_id: int, + offset: int = 0, + limit: int = 100000, +) -> Dict[str, Any]: + db = SessionLocal() + try: + user = _get_user_from_token(db, token_info) + if not user: + return {"error": "User not found"} + + result = knowledge_orchestrator.read_document_content( + db=db, + user=user, + document_id=document_id, + offset=offset, + limit=limit, + ) + return result.model_dump() + except ValueError as e: + logger.warning(f"[MCP] read_document_content validation error: {e}") + return {"error": str(e)} + except Exception as e: + logger.error(f"[MCP] read_document_content error: {e}", exc_info=True) + return {"error": str(e)} + finally: + db.close() +``` + +- [ ] **Step 4: Update the skill prompt** + +Insert this section into `backend/init_data/skills/wegent-knowledge/SKILL.md` under `## Available Tools`: + +```markdown +- **read_document_content**: Read a document's extracted text with paging + - document_id: Document ID to read + - offset: Start character position (default: 0) + - limit: Maximum characters to return (default: 100000) + - returns: content slice, total_length, returned_length, has_more, kb_id +``` + +Add this note under `## Usage Notes`: + +```markdown +- Long documents should be read incrementally: start with the default limit, then continue with `offset = previous_offset + previous_returned_length` while `has_more=true` +``` + +- [ ] **Step 5: Run the MCP tests to verify they pass** + +Run: + +```bash +uv run --project backend --group dev pytest tests/mcp_server/test_tools.py -k "read_document_content" -v +``` + +Expected: + +```text +PASSED ... test_read_document_content_returns_orchestrator_payload +PASSED ... test_read_document_content_returns_error_dict_for_validation_failure +``` + +- [ ] **Step 6: Commit the MCP-facing feature** + +```bash +git add backend/app/mcp_server/tools/knowledge.py backend/init_data/skills/wegent-knowledge/SKILL.md backend/tests/mcp_server/test_tools.py +git commit -m "feat(backend): expose knowledge document reads to MCP" +``` + +## Task 4: Refactor REST `/detail` Endpoints To Use Orchestrator + +**Files:** +- Modify: `backend/app/api/endpoints/knowledge.py` +- Create: `backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py` + +- [ ] **Step 1: Write the failing endpoint tests** + +Create `backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py` with: + +```python +from unittest.mock import AsyncMock, patch + + +def test_standalone_detail_uses_orchestrator(test_client, test_token): + payload = { + "document_id": 9, + "content": "abcd", + "content_length": 10, + "truncated": True, + "summary": {"summary": "hello"}, + } + + with patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + return_value=payload, + ) as mock_detail: + response = test_client.get( + "/api/knowledge-documents/9/detail", + headers={"Authorization": f"Bearer {test_token}"}, + ) + + assert response.status_code == 200 + assert response.json() == payload + mock_detail.assert_awaited_once() + + +def test_kb_scoped_detail_uses_orchestrator(test_client, test_token): + payload = { + "document_id": 9, + "content": "abcd", + "content_length": 10, + "truncated": False, + "summary": {"summary": "hello"}, + } + + with patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + return_value=payload, + ) as mock_detail: + response = test_client.get( + "/api/knowledge-bases/77/documents/9/detail", + headers={"Authorization": f"Bearer {test_token}"}, + ) + + assert response.status_code == 200 + assert response.json() == payload + mock_detail.assert_awaited_once() + + +def test_standalone_detail_maps_not_found_error_to_404(test_client, test_token): + with patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + side_effect=ValueError("Document not found"), + ): + response = test_client.get( + "/api/knowledge-documents/404/detail", + headers={"Authorization": f"Bearer {test_token}"}, + ) + + assert response.status_code == 404 + assert response.json()["detail"] == "Document not found" +``` + +- [ ] **Step 2: Run the endpoint tests to verify they fail** + +Run: + +```bash +uv run --project backend --group dev pytest tests/api/endpoints/test_knowledge_document_detail_endpoints.py -v +``` + +Expected: + +```text +FAIL ... because the current endpoints still execute inline read logic and do not call knowledge_orchestrator.get_document_detail +``` + +- [ ] **Step 3: Refactor the standalone `/detail` handler** + +Convert the standalone route to async and replace the inline body in `backend/app/api/endpoints/knowledge.py` with an orchestrator call: + +```python +@document_router.get("/{document_id}/detail") +@trace_async("get_document_detail_standalone", "knowledge.api") +async def get_document_detail_standalone(...): + try: + return await knowledge_orchestrator.get_document_detail( + db=db, + user=current_user, + document_id=document_id, + include_content=include_content, + include_summary=include_summary, + ) + except ValueError as e: + error_msg = str(e) + if "not found" in error_msg.lower(): + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=error_msg) + if "access denied" in error_msg.lower(): + raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=error_msg) + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=error_msg) +``` + +- [ ] **Step 4: Refactor the KB-scoped `/detail` handler** + +Replace the inline body in the async KB-scoped route with: + +```python +@summary_router.get("/{kb_id}/documents/{doc_id}/detail", response_model=DocumentDetailResponse) +@trace_async("get_document_detail", "knowledge.api") +async def get_document_detail(...): + try: + return await knowledge_orchestrator.get_document_detail( + db=db, + user=current_user, + document_id=doc_id, + include_content=include_content, + include_summary=include_summary, + ) + except ValueError as e: + error_msg = str(e) + if "not found" in error_msg.lower(): + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=error_msg) + if "access denied" in error_msg.lower(): + raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=error_msg) + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=error_msg) +``` + +- [ ] **Step 5: Run the endpoint tests to verify they pass** + +Run: + +```bash +uv run --project backend --group dev pytest tests/api/endpoints/test_knowledge_document_detail_endpoints.py -v +``` + +Expected: + +```text +PASSED ... test_standalone_detail_uses_orchestrator +PASSED ... test_kb_scoped_detail_uses_orchestrator +PASSED ... test_standalone_detail_maps_not_found_error_to_404 +``` + +- [ ] **Step 6: Run the complete targeted regression suite** + +Run: + +```bash +uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py tests/mcp_server/test_tools.py tests/api/endpoints/test_knowledge_document_detail_endpoints.py tests/services/rag/test_document_read_service.py -v +``` + +Expected: + +```text +all selected tests PASS +``` + +- [ ] **Step 7: Commit the REST refactor** + +```bash +git add backend/app/api/endpoints/knowledge.py backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py +git commit -m "refactor(backend): reuse orchestrator in knowledge detail endpoints" +``` + +## Task 5: Final Verification And Cleanup + +**Files:** +- Modify: `backend/app/services/knowledge/orchestrator.py` +- Modify: `backend/app/mcp_server/tools/knowledge.py` +- Modify: `backend/app/api/endpoints/knowledge.py` +- Modify: `backend/app/schemas/knowledge.py` +- Modify: `backend/init_data/skills/wegent-knowledge/SKILL.md` +- Modify: `backend/tests/services/knowledge/test_orchestrator.py` +- Modify: `backend/tests/mcp_server/test_tools.py` +- Modify: `backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py` + +- [ ] **Step 1: Run formatting-safe compilation checks** + +Run: + +```bash +uv run --project backend --group dev python -m py_compile app/services/knowledge/orchestrator.py app/mcp_server/tools/knowledge.py app/api/endpoints/knowledge.py app/schemas/knowledge.py +``` + +Expected: + +```text +no output +``` + +- [ ] **Step 2: Re-run the backend verification commands** + +Run: + +```bash +uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py tests/mcp_server/test_tools.py tests/api/endpoints/test_knowledge_document_detail_endpoints.py tests/services/rag/test_document_read_service.py -v +``` + +Expected: + +```text +all selected tests PASS +``` + +- [ ] **Step 3: Review the skill documentation text in the repo** + +Confirm `backend/init_data/skills/wegent-knowledge/SKILL.md` now communicates: + +```markdown +- read_document_content is available +- default limit is 100000 +- continue reading by advancing offset +- use has_more to detect remaining content +``` + +- [ ] **Step 4: Capture final status without rewriting history** + +```bash +git status --short +git log --oneline -n 5 +``` + +Expected: + +```text +working tree clean except for any intentional uncommitted notes +recent commits show one commit per completed task +``` diff --git a/docs/superpowers/specs/2026-03-30-wegent-knowledge-document-read-design.md b/docs/superpowers/specs/2026-03-30-wegent-knowledge-document-read-design.md new file mode 100644 index 00000000000..84225adef02 --- /dev/null +++ b/docs/superpowers/specs/2026-03-30-wegent-knowledge-document-read-design.md @@ -0,0 +1,339 @@ +--- +sidebar_position: 1 +--- + +# Wegent Knowledge Base Skill 文档读取统一化设计 + +## 背景 + +当前 `backend/init_data/skills/wegent-knowledge` 只提供知识库和文档的列举、创建、更新能力，缺少“读取文档正文内容”的 MCP 工具。 + +与此同时，后端已经存在三套相近但未统一的读取路径： + +1. `backend/app/api/endpoints/knowledge.py` 中的 standalone `/detail` 接口 +2. `backend/app/api/endpoints/knowledge.py` 中的 KB-scoped `/detail` 接口 +3. `backend/app/services/rag/document_read_service.py` 与 `backend/app/api/endpoints/internal/rag.py` 中的 `offset/limit/has_more` 风格读取能力 + +这导致： + +- MCP 无法直接读取知识库文档正文 +- REST `/detail` 读取逻辑重复，且未复用 orchestrator +- 系统内同时存在“100k 预览截断语义”和“分页读取语义”，但缺少明确分层 + +## 目标 + +本次设计目标如下： + +- 为 `wegent-knowledge` skill 增加单文档正文读取能力 +- 统一通过 `backend/app/services/knowledge/orchestrator.py` 复用读取逻辑 +- 底层统一采用 `offset/limit/has_more` 语义 +- 保持现有 REST `/detail` 接口的对外行为不变 +- 让现有 `/detail` 内部改为复用统一读取链路 + +## 非目标 + +本次不做以下内容： + +- 不新增公开 REST 文档正文读取接口 +- 不支持批量读取多个 `document_id` +- 不修改现有 `/detail` 的响应结构 +- 不改变现有 skill 中除“读取正文”外的工具语义 + +## 方案对比 + +### 方案 1：只新增 MCP 读取工具 + +直接在 MCP 工具层新增 `read_document_content`，底层直接调用 `document_read_service`，REST `/detail` 保持原状。 + +优点： + +- 改动最少 + +缺点： + +- 无法满足统一经由 orchestrator 复用的要求 +- `/detail` 逻辑仍然重复 + +### 方案 2：经由 orchestrator 统一内容读取与详情聚合 + +在 orchestrator 中引入内容读取核心方法和详情聚合方法。MCP 新工具与现有 REST `/detail` 都调用 orchestrator。底层统一复用 `document_read_service`。 + +优点： + +- 读取核心只有一套实现 +- MCP 与 REST 共享权限校验、参数校验和读取语义 +- `/detail` 对外兼容，内部实现统一 + +缺点： + +- 需要增加少量 orchestrator 和 schema 适配代码 + +### 方案 3：彻底改造 `/detail` 为分页读取协议 + +将现有 `/detail` 也直接改成 `offset/limit/has_more` 风格。 + +优点： + +- 外部协议最统一 + +缺点： + +- 会破坏现有调用方 +- 不符合本次兼容性要求 + +## 选型 + +采用方案 2。 + +理由： + +- 满足“统一通过 orchestrator 复用”的核心要求 +- 不引入新的公开 REST 面 +- 底层语义统一，兼容层清晰 + +## 总体设计 + +本次改动分为三层： + +### 1. 底层读取层 + +继续复用 `backend/app/services/rag/document_read_service.py`。 + +该层负责： + +- 根据 `document_id` 读取文档正文 +- 按 `offset` 和 `limit` 返回正文片段 +- 返回 `total_length`、`returned_length`、`has_more` + +该层不负责： + +- 用户权限校验 +- REST `/detail` 兼容格式 +- summary 聚合 + +### 2. Orchestrator 统一业务层 + +在 `backend/app/services/knowledge/orchestrator.py` 中新增两个方法： + +#### `read_document_content(...)` + +职责： + +- 根据 `document_id` 找到文档 +- 校验当前用户对所属知识库的访问权限 +- 校验 `offset` 和 `limit` +- 调用 `document_read_service` +- 返回标准分页读取结果 + +该方法面向“正文读取”场景，供 MCP 新工具直接使用。 + +#### `get_document_detail(...)` + +职责： + +- 复用 `read_document_content(...)` +- 根据 `include_content` 决定是否读取正文 +- 根据 `include_summary` 决定是否聚合 summary +- 统一将分页读取结果适配为现有 detail 响应 + +该方法面向“文档详情”场景，供现有 REST `/detail` 接口复用。 + +之所以保留两个方法，是为了分离两类语义： + +- `read_document_content(...)` 是正文分页读取原子能力 +- `get_document_detail(...)` 是兼容现有 `/detail` 的详情聚合能力 + +### 3. 对外适配层 + +#### MCP + +在 `backend/app/mcp_server/tools/knowledge.py` 中新增： + +- `read_document_content(document_id, offset=0, limit=100000)` + +返回原生分页读取结果，不附带 summary。 + +#### REST + +不新增公开 REST 正文读取接口。 + +保留现有两个 `/detail`： + +- standalone document detail +- KB-scoped document detail + +这两个 endpoint 内部统一改为调用 orchestrator 的 `get_document_detail(...)`，对外返回结构保持不变。 + +## 对外行为 + +### MCP 新工具 + +新增 `read_document_content`，参数如下： + +- `document_id: int` +- `offset: int = 0` +- `limit: int = 100000` + +返回字段如下： + +- `document_id` +- `name` +- `content` +- `total_length` +- `offset` +- `returned_length` +- `has_more` +- `kb_id` + +适用场景： + +- 读取长文档前 100000 字符 +- 基于 `has_more=true` 再次读取后续片段 + +### 现有 `/detail` 接口 + +现有 `/detail` 接口的对外协议不变，继续返回： + +- `document_id` +- `content` +- `content_length` +- `truncated` +- `summary` + +内部固定读取： + +- `offset = 0` +- `limit = 100000` + +字段映射规则统一为： + +- `content_length = total_length` +- `truncated = has_more` + +这意味着现有 `/detail` 仍然表现为“最多返回前 100000 字符的详情预览”，但其底层实现不再独立维护截断逻辑。 + +## 权限与错误处理 + +权限和错误处理统一收敛到 orchestrator。 + +### 权限规则 + +- 先通过 `document_id` 获取文档 +- 再通过文档所属 `kb_id` 复用现有知识库访问校验逻辑 +- MCP 与 REST 使用同一套权限判断 + +### 参数规则 + +- `offset >= 0` +- `limit > 0` +- `limit <= 100000` + +### 错误语义 + +orchestrator 内部统一产生业务错误： + +- `Document not found` +- `Knowledge base not found` +- `Access denied to this document` +- `Access denied to this knowledge base` +- 参数非法错误 + +对外映射如下： + +- MCP：保持当前 knowledge MCP 工具风格，返回 `{ "error": "..." }` +- REST：继续映射为标准 HTTP 状态码 + - `404`：文档或知识库不存在 + - `403`：无权限 + - `400`：参数非法 + +### 空内容行为 + +如果文档存在但没有可读取的 `extracted_text`，则正文读取返回： + +- `content = ""` +- `total_length = 0` +- `returned_length = 0` +- `has_more = false` + +对应 `/detail` 的兼容结果为： + +- `content = ""` +- `content_length = 0` +- `truncated = false` + +## Summary 聚合策略 + +summary 不放入 `read_document_content(...)` 的返回中，而由 `get_document_detail(...)` 负责聚合。 + +原因： + +- MCP 新工具的目标是读取正文，不需要承担详情聚合职责 +- `/detail` 接口本质上是“详情视图”，应由专门的详情方法负责组装 +- 这样可以避免把 summary 逻辑污染到原子读取能力中 + +为消除现有两个 `/detail` endpoint 的 summary 来源差异，`get_document_detail(...)` +统一使用 `summary_service.get_document_summary(document_id)` 作为 summary 的标准读取方式。 +不再在 standalone `/detail` 中直接返回原始 `document.summary` 字段。 + +## 实现落点 + +本次改动涉及以下文件： + +- `backend/app/services/knowledge/orchestrator.py` + - 新增正文读取方法 + - 新增详情聚合方法 + +- `backend/app/mcp_server/tools/knowledge.py` + - 新增 `read_document_content` MCP 工具 + +- `backend/init_data/skills/wegent-knowledge/SKILL.md` + - 更新工具列表与使用说明 + +- `backend/app/api/endpoints/knowledge.py` + - 将现有两个 `/detail` endpoint 改为调用 orchestrator + +如需要新增独立 schema，可放在： + +- `backend/app/schemas/knowledge.py` + +## 测试设计 + +### Orchestrator 测试 + +覆盖以下场景： + +- 正常读取文档正文，默认 `offset=0`、`limit=100000` +- 指定 `offset` 和 `limit` 后，返回正确的 `returned_length` 与 `has_more` +- 文档不存在 +- 文档所属知识库不存在 +- 用户无权限 +- 文档没有正文内容 +- 非法参数：负数 `offset`、非正 `limit`、超出上限的 `limit` +- `get_document_detail(...)` 正确映射 `content_length` 和 `truncated` + +### MCP 测试 + +覆盖以下场景： + +- `read_document_content` 成功返回分页读取结果 +- orchestrator 抛出业务错误时，工具返回 `{ "error": "..." }` + +### REST 测试 + +覆盖以下场景： + +- 两个 `/detail` endpoint 都改为走 orchestrator +- 对外响应结构不变 +- 长文档时 `truncated = true` +- 短文档时 `truncated = false` +- 两个 `/detail` endpoint 的 summary 都统一来自 `summary_service.get_document_summary(document_id)` + +## 结果 + +本设计完成后，系统将形成清晰分层： + +- `document_read_service` 负责正文分页读取 +- `KnowledgeOrchestrator.read_document_content(...)` 负责统一正文读取业务 +- `KnowledgeOrchestrator.get_document_detail(...)` 负责 detail 兼容聚合 +- MCP 对外暴露真正的分页读取能力 +- 现有 REST `/detail` 保持兼容，但底层统一复用 orchestrator From 1eb5912d22a706a4865b08c12a18025f771eb546 Mon Sep 17 00:00:00 2001 From: yanhe1 Date: Tue, 31 Mar 2026 12:19:34 +0800 Subject: [PATCH 2/3] feat(backend): add orchestrated knowledge document reads --- .gitignore | 1 + backend/app/api/endpoints/knowledge.py | 226 +++++------ backend/app/mcp_server/tools/knowledge.py | 61 ++- backend/app/schemas/knowledge.py | 13 + .../app/services/knowledge/orchestrator.py | 175 +++++++++ .../skills/wegent-knowledge/SKILL.md | 17 +- ...est_knowledge_document_detail_endpoints.py | 224 +++++++++++ backend/tests/mcp_server/test_tools.py | 139 +++++++ .../services/knowledge/test_orchestrator.py | 368 +++++++++++++++++- 9 files changed, 1089 insertions(+), 135 deletions(-) create mode 100644 backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py diff --git a/.gitignore b/.gitignore index 2bae2960957..8c9d504a89f 100644 --- a/.gitignore +++ b/.gitignore @@ -94,6 +94,7 @@ coverage/ .claude/settings.local.json .history .wegentrc +.worktrees/ # PyInstaller *.spec diff --git a/backend/app/api/endpoints/knowledge.py b/backend/app/api/endpoints/knowledge.py index d04359cd562..deb8b01d11e 100644 --- a/backend/app/api/endpoints/knowledge.py +++ b/backend/app/api/endpoints/knowledge.py @@ -61,6 +61,70 @@ router = APIRouter() +def _serialize_standalone_document_detail( + detail: DocumentDetailResponse | dict, + *, + include_content: bool, + include_summary: bool, +) -> dict: + """Preserve the standalone endpoint's pre-refactor response shape.""" + payload = detail.model_dump() if hasattr(detail, "model_dump") else dict(detail) + response = { + "document_id": payload["document_id"], + } + if include_content: + response["content"] = payload.get("content") + response["content_length"] = payload.get("content_length") + response["truncated"] = payload.get("truncated") + if include_summary: + response["summary"] = payload.get("summary") + return response + + +def _raise_document_detail_http_error(error: ValueError) -> None: + """Map orchestrator detail errors to stable HTTP responses.""" + error_msg = str(error) + if "not found" in error_msg.lower(): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=error_msg, + ) + if "access denied" in error_msg.lower(): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=error_msg, + ) + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=error_msg, + ) + + +def _validate_knowledge_base_access_or_raise( + db: Session, + *, + knowledge_base_id: int, + user: User, +): + """Restore the KB-scoped endpoint's KB-level error semantics.""" + knowledge_base, has_access = KnowledgeService.get_knowledge_base( + db=db, + knowledge_base_id=knowledge_base_id, + user_id=user.id, + ) + if not knowledge_base: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail="Knowledge base not found", + ) + if not has_access: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied to this knowledge base", + ) + return knowledge_base + + # ============== Knowledge Base Endpoints ============== @@ -695,8 +759,8 @@ async def update_document_content( @document_router.get("/{document_id}/detail") -@trace_sync("get_document_detail_standalone", "knowledge.api") -def get_document_detail_standalone( +@trace_async("get_document_detail_standalone", "knowledge.api") +async def get_document_detail_standalone( document_id: int, include_content: bool = Query(True, description="Include document content"), include_summary: bool = Query(True, description="Include document summary"), @@ -709,76 +773,21 @@ def get_document_detail_standalone( This is a convenience endpoint for getting document content when the kb_id is not readily available (e.g., in citation tooltips). """ - from app.models.knowledge import KnowledgeDocument - - # Get document - document = ( - db.query(KnowledgeDocument).filter(KnowledgeDocument.id == document_id).first() - ) - - if not document: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="Document not found", - ) - - # Check access permission via knowledge base - kb, has_access = KnowledgeService.get_knowledge_base( - db=db, - knowledge_base_id=document.kind_id, - user_id=current_user.id, - ) - if not kb: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="Knowledge base not found", + try: + detail = await knowledge_orchestrator.get_document_detail( + db=db, + user=current_user, + document_id=document_id, + include_content=include_content, + include_summary=include_summary, ) - if not has_access: - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Access denied to this document", + return _serialize_standalone_document_detail( + detail, + include_content=include_content, + include_summary=include_summary, ) - - # Get content if requested - content = None - content_length = None - truncated = False - - if include_content and document.attachment_id: - try: - from app.services.context import context_service - - attachment = context_service.get_context_by_id( - db=db, - context_id=document.attachment_id, - ) - if attachment and attachment.extracted_text: - full_content = attachment.extracted_text - content_length = len(full_content) - # Truncate if too large - max_length = 100000 - if content_length > max_length: - content = full_content[:max_length] - truncated = True - else: - content = full_content - except Exception as e: - logger.warning(f"Failed to get document content: {e}") - - # Build response - response = { - "document_id": document_id, - } - - if include_content: - response["content"] = content - response["content_length"] = content_length - response["truncated"] = truncated - - if include_summary: - response["summary"] = document.summary - - return response + except ValueError as error: + _raise_document_detail_http_error(error) @document_router.post("/batch/delete", response_model=BatchOperationResult) @@ -1104,23 +1113,13 @@ async def get_document_detail( - summary: Document summary object (if include_summary=true) """ from app.models.knowledge import KnowledgeDocument - from app.models.subtask_context import SubtaskContext - from app.services.knowledge import get_summary_service - # Validate KB access permission first - kb, has_access = KnowledgeService.get_knowledge_base(db, kb_id, current_user.id) - if not kb: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail="Knowledge base not found", - ) - if not has_access: - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Access denied to this knowledge base", - ) + _validate_knowledge_base_access_or_raise( + db, + knowledge_base_id=kb_id, + user=current_user, + ) - # Validate document belongs to the specified knowledge base document = ( db.query(KnowledgeDocument) .filter( @@ -1135,53 +1134,16 @@ async def get_document_detail( detail="Document not found in the specified knowledge base", ) - # Initialize response data - content = None - content_length = None - truncated = None - summary = None - - # Get document content if requested - if include_content: - content = "" - truncated = False - max_length = 100000 # 100k characters limit for frontend display - - if document.attachment_id: - context = ( - db.query(SubtaskContext) - .filter(SubtaskContext.id == document.attachment_id) - .first() - ) - - if context and context.extracted_text: - content = context.extracted_text - # Truncate if too long - if len(content) > max_length: - content = content[:max_length] - truncated = True - - content_length = len(content) - - # Get document summary if requested - if include_summary: - summary_service = get_summary_service(db) - summary_obj = await summary_service.get_document_summary(doc_id) - # Convert DocumentSummary object to dict for response - if summary_obj: - summary = ( - summary_obj.model_dump() - if hasattr(summary_obj, "model_dump") - else summary_obj - ) - - return DocumentDetailResponse( - document_id=doc_id, - content=content, - content_length=content_length, - truncated=truncated, - summary=summary, - ) + try: + return await knowledge_orchestrator.get_document_detail( + db=db, + user=current_user, + document_id=doc_id, + include_content=include_content, + include_summary=include_summary, + ) + except ValueError as error: + _raise_document_detail_http_error(error) @summary_router.get("/{kb_id}/documents/{doc_id}/summary") diff --git a/backend/app/mcp_server/tools/knowledge.py b/backend/app/mcp_server/tools/knowledge.py index 1b02eebfa9b..ad6e230fd0d 100644 --- a/backend/app/mcp_server/tools/knowledge.py +++ b/backend/app/mcp_server/tools/knowledge.py @@ -28,7 +28,10 @@ from app.mcp_server.auth import TaskTokenInfo from app.mcp_server.tools.decorator import build_mcp_tools_dict, mcp_tool from app.models.user import User -from app.services.knowledge.orchestrator import knowledge_orchestrator +from app.services.knowledge.orchestrator import ( + MAX_DOCUMENT_READ_LIMIT, + knowledge_orchestrator, +) logger = logging.getLogger(__name__) @@ -302,6 +305,62 @@ def create_document( db.close() +@mcp_tool( + name="read_document_content", + description="Read document content with offset/limit pagination.", + server="knowledge", + param_descriptions={ + "document_id": "Document ID to read", + "offset": "Character offset to start reading from", + "limit": "Maximum number of characters to return", + }, +) +def read_document_content( + token_info: TaskTokenInfo, + document_id: int, + offset: int = 0, + limit: int = MAX_DOCUMENT_READ_LIMIT, +) -> Dict[str, Any]: + """ + Read raw document content with offset/limit pagination. + + Args: + token_info: Task token information containing user context + document_id: Document ID + offset: Character offset to start reading from + limit: Maximum number of characters to return (defaults to backend limit) + + Returns: + Dict with document content slice and pagination metadata + """ + db = SessionLocal() + try: + user = _get_user_from_token(db, token_info) + if not user: + return {"error": "User not found"} + + result = knowledge_orchestrator.read_document_content( + db=db, + user=user, + document_id=document_id, + offset=offset, + limit=limit, + ) + + return result.model_dump() + + except ValueError as e: + logger.warning(f"[MCP] read_document_content validation error: {e}") + return {"error": str(e)} + + except Exception as e: + logger.error(f"[MCP] read_document_content error: {e}", exc_info=True) + return {"error": str(e)} + + finally: + db.close() + + @mcp_tool( name="update_document_content", description="Update document content. Only supports TEXT type documents.", diff --git a/backend/app/schemas/knowledge.py b/backend/app/schemas/knowledge.py index c397d93b681..86dc729fd54 100644 --- a/backend/app/schemas/knowledge.py +++ b/backend/app/schemas/knowledge.py @@ -569,6 +569,19 @@ class DocumentDetailResponse(BaseModel): summary: Optional[dict] = Field(None, description="Document summary object") +class DocumentContentReadResponse(BaseModel): + """Schema for raw document content reads with pagination metadata.""" + + document_id: int = Field(..., description="Document ID") + name: str = Field(..., description="Document name") + content: str = Field(..., description="Document content (partial)") + total_length: int = Field(..., description="Total document length in characters") + offset: int = Field(..., description="Actual start position") + returned_length: int = Field(..., description="Number of characters returned") + has_more: bool = Field(..., description="Whether more content is available") + kb_id: int = Field(..., description="Knowledge base ID") + + class DocumentContentUpdate(BaseModel): """Schema for updating document content (TEXT type only).""" diff --git a/backend/app/services/knowledge/orchestrator.py b/backend/app/services/knowledge/orchestrator.py index 0607950aa09..fb41b83dfb6 100644 --- a/backend/app/services/knowledge/orchestrator.py +++ b/backend/app/services/knowledge/orchestrator.py @@ -26,9 +26,12 @@ from sqlalchemy.orm import Session from app.models.kind import Kind +from app.models.knowledge import KnowledgeDocument from app.models.task import TaskResource from app.models.user import User from app.schemas.knowledge import ( + DocumentContentReadResponse, + DocumentDetailResponse, KnowledgeBaseCreate, KnowledgeBaseListResponse, KnowledgeBaseResponse, @@ -38,10 +41,22 @@ ResourceScope, ) from app.services.knowledge.knowledge_service import KnowledgeService +from app.services.rag.document_read_service import document_read_service logger = logging.getLogger(__name__) DEFAULT_TEXT_FILE_EXTENSION = "txt" +MAX_DOCUMENT_READ_LIMIT = 100000 +REQUIRED_DOCUMENT_READ_KEYS = ( + "id", + "name", + "content", + "total_length", + "offset", + "returned_length", + "has_more", + "kb_id", +) def _normalize_file_extension(file_extension: Optional[str]) -> str: @@ -69,6 +84,27 @@ def _build_filename(name: str, file_extension: str) -> str: return f"{name}.{ext}" +def _validate_document_read_paging(offset: int, limit: int) -> None: + """Validate raw document read paging arguments.""" + if offset < 0: + raise ValueError("offset must be greater than or equal to 0") + if limit <= 0: + raise ValueError("limit must be greater than 0") + if limit > MAX_DOCUMENT_READ_LIMIT: + raise ValueError( + f"limit must be less than or equal to {MAX_DOCUMENT_READ_LIMIT}" + ) + + +def _validate_document_read_result_payload(result: Dict[str, Any]) -> None: + """Validate the reader payload contains all required paginator fields.""" + missing_keys = [key for key in REQUIRED_DOCUMENT_READ_KEYS if key not in result] + if missing_keys: + raise ValueError( + "Incomplete document read payload: missing " + ", ".join(missing_keys) + ) + + class KnowledgeOrchestrator: """ Knowledge base operations orchestrator. @@ -622,6 +658,145 @@ def list_documents( items=[KnowledgeDocumentResponse.model_validate(doc) for doc in documents], ) + def _get_document_with_access_or_raise( + self, + db: Session, + user: User, + document_id: int, + ) -> KnowledgeDocument: + """Fetch a document and validate its knowledge-base level access.""" + document = ( + db.query(KnowledgeDocument) + .filter(KnowledgeDocument.id == document_id) + .first() + ) + if not document: + raise ValueError("Document not found") + + knowledge_base, has_access = KnowledgeService.get_knowledge_base( + db=db, + knowledge_base_id=document.kind_id, + user_id=user.id, + ) + if not knowledge_base: + raise ValueError("Knowledge base not found") + if not has_access: + raise ValueError("Access denied to this document") + + return document + + def read_document_content( + self, + db: Session, + user: User, + document_id: int, + offset: int = 0, + limit: int = MAX_DOCUMENT_READ_LIMIT, + ) -> DocumentContentReadResponse: + """ + Read raw document content with offset/limit pagination. + + Args: + db: Database session + user: Current user + document_id: Document ID + offset: Read start offset + limit: Maximum number of characters to return + + Returns: + DocumentContentReadResponse + + Raises: + ValueError: If paging is invalid, the document is missing, or access fails + """ + _validate_document_read_paging(offset=offset, limit=limit) + + document = self._get_document_with_access_or_raise( + db=db, + user=user, + document_id=document_id, + ) + + results = document_read_service.read_documents( + db=db, + document_ids=[document_id], + offset=offset, + limit=limit, + knowledge_base_ids=[document.kind_id], + ) + result = results[0] if results else None + + if not result or result.get("error") == "Document not found": + raise ValueError("Document not found") + if result.get("error"): + raise ValueError(result["error"]) + _validate_document_read_result_payload(result) + + return DocumentContentReadResponse( + document_id=result["id"], + name=result["name"], + content=result["content"], + total_length=result["total_length"], + offset=result["offset"], + returned_length=result["returned_length"], + has_more=result["has_more"], + kb_id=result["kb_id"], + ) + + async def get_document_detail( + self, + db: Session, + user: User, + document_id: int, + include_content: bool = True, + include_summary: bool = True, + offset: int = 0, + limit: int = MAX_DOCUMENT_READ_LIMIT, + ) -> DocumentDetailResponse: + """Aggregate optional document content and summary into a detail response.""" + content = None + content_length = None + truncated = None + summary = None + + if include_content or include_summary: + self._get_document_with_access_or_raise( + db=db, + user=user, + document_id=document_id, + ) + + if include_content: + paged = self.read_document_content( + db=db, + user=user, + document_id=document_id, + offset=offset, + limit=limit, + ) + content = paged.content + content_length = paged.total_length + truncated = (paged.offset > 0) or paged.has_more + + if include_summary: + from app.services.knowledge.summary_service import get_summary_service + + summary_service = get_summary_service(db) + summary_result = await summary_service.get_document_summary(document_id) + summary = ( + summary_result.model_dump() + if hasattr(summary_result, "model_dump") + else summary_result + ) + + return DocumentDetailResponse( + document_id=document_id, + content=content, + content_length=content_length, + truncated=truncated, + summary=summary, + ) + def get_knowledge_base( self, db: Session, diff --git a/backend/init_data/skills/wegent-knowledge/SKILL.md b/backend/init_data/skills/wegent-knowledge/SKILL.md index 75f33467155..48aacb580a1 100644 --- a/backend/init_data/skills/wegent-knowledge/SKILL.md +++ b/backend/init_data/skills/wegent-knowledge/SKILL.md @@ -1,6 +1,6 @@ --- description: "Knowledge base management tools for Wegent. Provides capabilities to list, create, and update knowledge bases and documents. Use this skill when the user wants to manage knowledge bases or documents programmatically." -displayName: "Wegent Knowledge Base" +displayName: "知识库工具" version: "1.0.0" author: "Wegent Team" tags: ["knowledge", "knowledge-base", "document", "rag"] @@ -50,6 +50,12 @@ You now have access to Wegent Knowledge Base management tools. - trigger_indexing: Whether to trigger RAG indexing (default: true) - trigger_summary: Whether to trigger summary generation (default: true) +- **read_document_content**: Read raw document content with offset/limit pagination + - document_id: Document ID to read + - offset: Character offset to start reading from (default: 0) + - limit: Maximum number of characters to return (uses the backend default when omitted) + - returns: content slice, total_length, returned_length, has_more, kb_id + - **update_document_content**: Update a document's content (TEXT type documents only) - document_id: Document ID to update - content: New content (replaces existing content) @@ -63,6 +69,7 @@ You now have access to Wegent Knowledge Base management tools. - For web scraping, the URL content is fetched and stored as document content - Default behavior: if user doesn't specify scope, use `scope="all"` directly (no extra confirmation). - Avoid loops: if a tool call fails, report the error once and stop retrying/re-loading the skill unless the user changes inputs. +- Long documents should be read incrementally: start with the backend default limit, then continue with `offset = previous_offset + previous_returned_length` while `has_more=true` ## Example Workflow @@ -104,3 +111,11 @@ You now have access to Wegent Knowledge Base management tools. trigger_reindex=true ) ``` + +6. Read long document content incrementally: + ``` + read_document_content( + document_id=456, + offset=0 + ) + ``` diff --git a/backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py b/backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py new file mode 100644 index 00000000000..76a5edb3149 --- /dev/null +++ b/backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py @@ -0,0 +1,224 @@ +# SPDX-FileCopyrightText: 2026 Weibo, Inc. +# +# SPDX-License-Identifier: Apache-2.0 + +from unittest.mock import AsyncMock, patch + +from fastapi.testclient import TestClient + +from app.models.knowledge import KnowledgeDocument + + +def _auth_header(token: str) -> dict[str, str]: + return {"Authorization": f"Bearer {token}"} + + +def test_standalone_detail_uses_orchestrator( + test_client: TestClient, + test_token: str, +): + payload = { + "document_id": 9, + "content": "abcd", + "content_length": 10, + "truncated": True, + "summary": {"summary": "hello"}, + } + + with patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + return_value=payload, + ) as mock_detail: + response = test_client.get( + "/api/knowledge-documents/9/detail", + headers=_auth_header(test_token), + ) + + assert response.status_code == 200 + assert response.json() == payload + mock_detail.assert_awaited_once() + + +def test_kb_scoped_detail_uses_orchestrator( + test_client: TestClient, + test_db, + test_token: str, + test_user, +): + payload = { + "document_id": 9, + "content": "abcd", + "content_length": 10, + "truncated": False, + "summary": {"summary": "hello"}, + } + document = KnowledgeDocument( + id=9, + kind_id=77, + attachment_id=0, + name="kb-doc", + file_extension=".txt", + file_size=0, + user_id=test_user.id, + ) + test_db.add(document) + test_db.commit() + + with ( + patch( + "app.api.endpoints.knowledge.KnowledgeService.get_knowledge_base", + return_value=(object(), True), + ), + patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + return_value=payload, + ) as mock_detail, + ): + response = test_client.get( + "/api/knowledge-bases/77/documents/9/detail", + headers=_auth_header(test_token), + ) + + assert response.status_code == 200 + assert response.json() == payload + mock_detail.assert_awaited_once() + + +def test_standalone_detail_maps_not_found_error_to_404( + test_client: TestClient, + test_token: str, +): + with patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + side_effect=ValueError("Document not found"), + ): + response = test_client.get( + "/api/knowledge-documents/404/detail", + headers=_auth_header(test_token), + ) + + assert response.status_code == 404 + assert response.json()["detail"] == "Document not found" + + +def test_standalone_detail_omits_unrequested_fields( + test_client: TestClient, + test_token: str, +): + payload = { + "document_id": 9, + "content": None, + "content_length": None, + "truncated": None, + "summary": {"summary": "hello"}, + } + + with patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + return_value=payload, + ) as mock_detail: + response = test_client.get( + "/api/knowledge-documents/9/detail", + params={"include_content": "false", "include_summary": "true"}, + headers=_auth_header(test_token), + ) + + assert response.status_code == 200 + assert response.json() == { + "document_id": 9, + "summary": {"summary": "hello"}, + } + mock_detail.assert_awaited_once() + + +def test_kb_scoped_detail_rejects_document_outside_requested_kb( + test_client: TestClient, + test_db, + test_token: str, + test_user, +): + document = KnowledgeDocument( + kind_id=88, + attachment_id=0, + name="other-kb-doc", + file_extension=".txt", + file_size=0, + user_id=test_user.id, + ) + test_db.add(document) + test_db.commit() + test_db.refresh(document) + + with ( + patch( + "app.api.endpoints.knowledge.KnowledgeService.get_knowledge_base", + return_value=(object(), True), + ), + patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + ) as mock_detail, + ): + response = test_client.get( + f"/api/knowledge-bases/77/documents/{document.id}/detail", + headers=_auth_header(test_token), + ) + + assert response.status_code == 404 + assert ( + response.json()["detail"] + == "Document not found in the specified knowledge base" + ) + mock_detail.assert_not_awaited() + + +def test_kb_scoped_detail_maps_missing_kb_before_document_lookup( + test_client: TestClient, + test_token: str, +): + with ( + patch( + "app.api.endpoints.knowledge.KnowledgeService.get_knowledge_base", + return_value=(None, False), + ), + patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + ) as mock_detail, + ): + response = test_client.get( + "/api/knowledge-bases/77/documents/9/detail", + headers=_auth_header(test_token), + ) + + assert response.status_code == 404 + assert response.json()["detail"] == "Knowledge base not found" + mock_detail.assert_not_awaited() + + +def test_kb_scoped_detail_maps_kb_access_denied_before_document_lookup( + test_client: TestClient, + test_token: str, +): + with ( + patch( + "app.api.endpoints.knowledge.KnowledgeService.get_knowledge_base", + return_value=(object(), False), + ), + patch( + "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", + new_callable=AsyncMock, + ) as mock_detail, + ): + response = test_client.get( + "/api/knowledge-bases/77/documents/9/detail", + headers=_auth_header(test_token), + ) + + assert response.status_code == 403 + assert response.json()["detail"] == "Access denied to this knowledge base" + mock_detail.assert_not_awaited() diff --git a/backend/tests/mcp_server/test_tools.py b/backend/tests/mcp_server/test_tools.py index 9b2f56a0b8f..3fb5c113218 100644 --- a/backend/tests/mcp_server/test_tools.py +++ b/backend/tests/mcp_server/test_tools.py @@ -5,6 +5,7 @@ """Tests for MCP Server tools.""" import importlib +import inspect import json import sys from unittest.mock import MagicMock, patch @@ -23,6 +24,14 @@ def get_silent_exit_module(): return sys.modules[module_name] +def get_knowledge_module(): + """Get the knowledge module, handling import caching issues.""" + module_name = "app.mcp_server.tools.knowledge" + if module_name not in sys.modules: + importlib.import_module(module_name) + return sys.modules[module_name] + + class TestSilentExitTool: """Tests for silent_exit tool.""" @@ -105,3 +114,133 @@ def test_invalid_json_output(self): is_silent = False assert is_silent is False + + +class TestKnowledgeTool: + """Tests for knowledge MCP tools.""" + + def test_read_document_content_returns_orchestrator_payload(self): + """Test that read_document_content returns the orchestrator payload.""" + module = get_knowledge_module() + token_info = TaskTokenInfo( + task_id=1, + subtask_id=2, + user_id=3, + user_name="alice", + ) + mock_user = object() + mock_session = MagicMock() + mock_result = MagicMock( + document_id=9, + name="roadmap", + content="abcd", + total_length=10, + offset=0, + returned_length=4, + has_more=True, + kb_id=77, + ) + expected_payload = { + "document_id": 9, + "name": "roadmap", + "content": "abcd", + "total_length": 10, + "offset": 0, + "returned_length": 4, + "has_more": True, + "kb_id": 77, + } + mock_result.model_dump.return_value = expected_payload + + with ( + patch.object(module, "SessionLocal", return_value=mock_session), + patch.object(module, "_get_user_from_token", return_value=mock_user), + patch.object( + module.knowledge_orchestrator, + "read_document_content", + return_value=mock_result, + ) as mock_read, + ): + result = module.read_document_content( + token_info=token_info, + document_id=9, + offset=0, + limit=4, + ) + + assert result == expected_payload + mock_read.assert_called_once_with( + db=mock_session, + user=mock_user, + document_id=9, + offset=0, + limit=4, + ) + mock_session.close.assert_called_once() + + def test_read_document_content_returns_error_dict_for_validation_failure(self): + """Test that read_document_content converts validation failures to error dicts.""" + module = get_knowledge_module() + token_info = TaskTokenInfo( + task_id=1, + subtask_id=2, + user_id=3, + user_name="alice", + ) + mock_user = object() + mock_session = MagicMock() + + with ( + patch.object(module, "SessionLocal", return_value=mock_session), + patch.object(module, "_get_user_from_token", return_value=mock_user), + patch.object( + module.knowledge_orchestrator, + "read_document_content", + side_effect=ValueError("limit must be greater than 0"), + ), + ): + result = module.read_document_content( + token_info=token_info, + document_id=9, + offset=0, + limit=0, + ) + + assert result == {"error": "limit must be greater than 0"} + mock_session.close.assert_called_once() + + def test_read_document_content_returns_error_dict_for_unexpected_failure(self): + """Test that read_document_content converts unexpected failures to error dicts.""" + module = get_knowledge_module() + token_info = TaskTokenInfo( + task_id=1, + subtask_id=2, + user_id=3, + user_name="alice", + ) + mock_user = object() + mock_session = MagicMock() + + with ( + patch.object(module, "SessionLocal", return_value=mock_session), + patch.object(module, "_get_user_from_token", return_value=mock_user), + patch.object( + module.knowledge_orchestrator, + "read_document_content", + side_effect=RuntimeError("boom"), + ), + ): + result = module.read_document_content( + token_info=token_info, + document_id=9, + ) + + assert result == {"error": "boom"} + mock_session.close.assert_called_once() + + def test_read_document_content_uses_shared_default_limit_constant(self): + """Test that read_document_content reuses the shared default limit constant.""" + module = get_knowledge_module() + source = inspect.getsource(module.read_document_content) + + assert "limit: int = 100000" not in source diff --git a/backend/tests/services/knowledge/test_orchestrator.py b/backend/tests/services/knowledge/test_orchestrator.py index 1c2d89f4047..f5ff0ebf001 100644 --- a/backend/tests/services/knowledge/test_orchestrator.py +++ b/backend/tests/services/knowledge/test_orchestrator.py @@ -5,7 +5,7 @@ """Tests for KnowledgeOrchestrator service layer.""" from types import SimpleNamespace -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest @@ -224,6 +224,372 @@ def test_list_documents_raises_when_kb_not_found( with pytest.raises(ValueError, match="Knowledge base not found"): orchestrator.list_documents(mock_db, mock_user, knowledge_base_id=999) + def test_read_document_content_returns_paginated_payload( + self, orchestrator, mock_db, mock_user + ): + """Test read_document_content returns the paginated raw content payload.""" + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + mock_db.query.return_value.filter.return_value.first.return_value = document + + with patch( + "app.services.knowledge.orchestrator.KnowledgeService" + ) as mock_kb_service: + mock_kb_service.get_knowledge_base.return_value = (MagicMock(id=77), True) + + with patch( + "app.services.knowledge.orchestrator.document_read_service" + ) as mock_read_service: + mock_read_service.read_documents.return_value = [ + { + "id": 9, + "name": "roadmap", + "content": "cdef", + "total_length": 10, + "offset": 2, + "returned_length": 4, + "has_more": True, + "kb_id": 77, + } + ] + + result = orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + offset=2, + limit=4, + ) + + assert result.document_id == 9 + assert result.name == "roadmap" + assert result.content == "cdef" + assert result.total_length == 10 + assert result.offset == 2 + assert result.returned_length == 4 + assert result.has_more is True + assert result.kb_id == 77 + mock_kb_service.get_knowledge_base.assert_called_once_with( + db=mock_db, + knowledge_base_id=77, + user_id=mock_user.id, + ) + mock_read_service.read_documents.assert_called_once_with( + db=mock_db, + document_ids=[9], + offset=2, + limit=4, + knowledge_base_ids=[77], + ) + + @pytest.mark.parametrize( + ("offset", "limit", "message"), + [ + (-1, 1, "offset must be greater than or equal to 0"), + (0, 0, "limit must be greater than 0"), + (0, 100001, "limit must be less than or equal to 100000"), + ], + ) + def test_read_document_content_rejects_invalid_paging_args( + self, orchestrator, mock_db, mock_user, offset, limit, message + ): + """Test read_document_content validates offset and limit.""" + with pytest.raises(ValueError, match=message): + orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + offset=offset, + limit=limit, + ) + + mock_db.query.assert_not_called() + + def test_read_document_content_raises_for_missing_document( + self, orchestrator, mock_db, mock_user + ): + """Test read_document_content raises ValueError when document is missing.""" + mock_db.query.return_value.filter.return_value.first.return_value = None + + with pytest.raises(ValueError, match="Document not found"): + orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + ) + + def test_read_document_content_raises_when_reader_returns_empty_results( + self, orchestrator, mock_db, mock_user + ): + """Test read_document_content raises when the reader returns no rows.""" + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + mock_db.query.return_value.filter.return_value.first.return_value = document + + with patch( + "app.services.knowledge.orchestrator.KnowledgeService" + ) as mock_kb_service: + mock_kb_service.get_knowledge_base.return_value = (MagicMock(id=77), True) + + with patch( + "app.services.knowledge.orchestrator.document_read_service" + ) as mock_read_service: + mock_read_service.read_documents.return_value = [] + + with pytest.raises(ValueError, match="Document not found"): + orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + ) + + def test_read_document_content_raises_reader_error( + self, orchestrator, mock_db, mock_user + ): + """Test read_document_content surfaces reader access errors unchanged.""" + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + mock_db.query.return_value.filter.return_value.first.return_value = document + + with patch( + "app.services.knowledge.orchestrator.KnowledgeService" + ) as mock_kb_service: + mock_kb_service.get_knowledge_base.return_value = (MagicMock(id=77), True) + + with patch( + "app.services.knowledge.orchestrator.document_read_service" + ) as mock_read_service: + mock_read_service.read_documents.return_value = [ + { + "id": 9, + "error": "Access denied: document not in allowed knowledge bases", + } + ] + + with pytest.raises( + ValueError, + match="Access denied: document not in allowed knowledge bases", + ): + orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + ) + + def test_read_document_content_raises_for_incomplete_reader_payload( + self, orchestrator, mock_db, mock_user + ): + """Test read_document_content rejects incomplete reader payloads.""" + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + mock_db.query.return_value.filter.return_value.first.return_value = document + + with patch( + "app.services.knowledge.orchestrator.KnowledgeService" + ) as mock_kb_service: + mock_kb_service.get_knowledge_base.return_value = (MagicMock(id=77), True) + + with patch( + "app.services.knowledge.orchestrator.document_read_service" + ) as mock_read_service: + mock_read_service.read_documents.return_value = [ + { + "id": 9, + "name": "roadmap", + "content": "cdef", + "offset": 2, + "kb_id": 77, + } + ] + + with pytest.raises( + ValueError, + match="Incomplete document read payload: missing total_length, returned_length, has_more", + ): + orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + ) + + @pytest.mark.asyncio + async def test_get_document_detail_maps_content_length_and_truncated( + self, orchestrator, mock_db, mock_user + ): + """Test get_document_detail maps paged content and async summary.""" + paged = SimpleNamespace( + document_id=9, + name="roadmap", + content="abcd", + total_length=10, + offset=0, + returned_length=4, + has_more=True, + kb_id=77, + ) + summary_service = MagicMock() + summary_service.get_document_summary = AsyncMock( + return_value={"summary": "hello"} + ) + + with patch.object( + orchestrator, "read_document_content", return_value=paged + ) as mock_read_document_content: + with patch( + "app.services.knowledge.summary_service.get_summary_service", + return_value=summary_service, + ) as mock_get_summary_service: + result = await orchestrator.get_document_detail( + db=mock_db, + user=mock_user, + document_id=9, + include_content=True, + include_summary=True, + ) + + assert result.document_id == 9 + assert result.content == "abcd" + assert result.content_length == 10 + assert result.truncated is True + assert result.summary == {"summary": "hello"} + mock_read_document_content.assert_called_once_with( + db=mock_db, + user=mock_user, + document_id=9, + offset=0, + limit=100000, + ) + mock_get_summary_service.assert_called_once_with(mock_db) + summary_service.get_document_summary.assert_awaited_once_with(9) + + @pytest.mark.asyncio + async def test_get_document_detail_skips_content_when_disabled( + self, orchestrator, mock_db, mock_user + ): + """Test get_document_detail skips optional content and summary aggregation.""" + with patch.object( + orchestrator, "read_document_content" + ) as mock_read_document_content: + with patch( + "app.services.knowledge.summary_service.get_summary_service" + ) as mock_get_summary_service: + result = await orchestrator.get_document_detail( + db=mock_db, + user=mock_user, + document_id=9, + include_content=False, + include_summary=False, + ) + + assert result.document_id == 9 + assert result.content is None + assert result.content_length is None + assert result.truncated is None + assert result.summary is None + mock_read_document_content.assert_not_called() + mock_get_summary_service.assert_not_called() + + @pytest.mark.asyncio + async def test_get_document_detail_summary_only_validates_access_before_fetching( + self, orchestrator, mock_db, mock_user + ): + """Test summary-only detail requests still enforce document authorization.""" + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + mock_db.query.return_value.filter.return_value.first.return_value = document + + summary_service = MagicMock() + summary_service.get_document_summary = AsyncMock( + return_value={"summary": "hello"} + ) + + with patch( + "app.services.knowledge.orchestrator.KnowledgeService" + ) as mock_kb_service: + mock_kb_service.get_knowledge_base.return_value = (MagicMock(id=77), False) + + with patch( + "app.services.knowledge.summary_service.get_summary_service", + return_value=summary_service, + ): + with pytest.raises(ValueError, match="Access denied to this document"): + await orchestrator.get_document_detail( + db=mock_db, + user=mock_user, + document_id=9, + include_content=False, + include_summary=True, + ) + + summary_service.get_document_summary.assert_not_awaited() + + @pytest.mark.asyncio + async def test_get_document_detail_converts_summary_model_dump( + self, orchestrator, mock_db, mock_user + ): + """Test get_document_detail normalizes summary objects via model_dump.""" + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + mock_db.query.return_value.filter.return_value.first.return_value = document + + summary_result = Mock() + summary_result.model_dump.return_value = {"summary": "hello"} + + summary_service = MagicMock() + summary_service.get_document_summary = AsyncMock(return_value=summary_result) + + with patch( + "app.services.knowledge.orchestrator.KnowledgeService" + ) as mock_kb_service: + mock_kb_service.get_knowledge_base.return_value = (MagicMock(id=77), True) + + with patch( + "app.services.knowledge.summary_service.get_summary_service", + return_value=summary_service, + ): + result = await orchestrator.get_document_detail( + db=mock_db, + user=mock_user, + document_id=9, + include_content=False, + include_summary=True, + ) + + assert result.summary == {"summary": "hello"} + summary_result.model_dump.assert_called_once_with() + + @pytest.mark.asyncio + async def test_get_document_detail_marks_non_zero_offset_as_truncated( + self, orchestrator, mock_db, mock_user + ): + """Test get_document_detail treats non-zero offsets as truncated content.""" + paged = SimpleNamespace( + document_id=9, + name="roadmap", + content="cd", + total_length=10, + offset=2, + returned_length=2, + has_more=False, + kb_id=77, + ) + + with patch.object( + orchestrator, "read_document_content", return_value=paged + ) as mock_read_document_content: + result = await orchestrator.get_document_detail( + db=mock_db, + user=mock_user, + document_id=9, + include_content=True, + include_summary=False, + offset=2, + limit=2, + ) + + assert result.truncated is True + mock_read_document_content.assert_called_once_with( + db=mock_db, + user=mock_user, + document_id=9, + offset=2, + limit=2, + ) + def test_create_document_with_text_content(self, orchestrator, mock_db, mock_user): """Test create_document_with_content with text source type.""" with patch( From 63ae8f4dc0ffcaeedf4074e260498f6b975ee5e9 Mon Sep 17 00:00:00 2001 From: yanhe1 Date: Tue, 31 Mar 2026 14:33:50 +0800 Subject: [PATCH 3/3] fix(backend): tighten knowledge document read handling --- backend/app/api/endpoints/internal/rag.py | 13 +- backend/app/api/endpoints/knowledge.py | 9 +- backend/app/schemas/knowledge.py | 8 +- .../app/services/knowledge/orchestrator.py | 20 +- .../app/services/rag/document_read_service.py | 12 +- .../skills/wegent-knowledge/SKILL.md | 4 +- ...est_knowledge_document_detail_endpoints.py | 26 +- backend/tests/mcp_server/test_tools.py | 9 +- .../services/knowledge/test_orchestrator.py | 68 +- .../rag/test_document_read_service.py | 7 +- ...0-wegent-knowledge-document-read-design.md | 0 ...-knowledge-document-read-implementation.md | 857 ------------------ 12 files changed, 137 insertions(+), 896 deletions(-) rename docs/{superpowers/specs => specs/knowledge}/2026-03-30-wegent-knowledge-document-read-design.md (100%) delete mode 100644 docs/superpowers/plans/2026-03-30-wegent-knowledge-document-read-implementation.md diff --git a/backend/app/api/endpoints/internal/rag.py b/backend/app/api/endpoints/internal/rag.py index 796fc382b03..52d79e68017 100644 --- a/backend/app/api/endpoints/internal/rag.py +++ b/backend/app/api/endpoints/internal/rag.py @@ -737,7 +737,11 @@ async def read_document( Document content with pagination info """ try: - from app.services.rag.document_read_service import document_read_service + from app.services.rag.document_read_service import ( + DOCUMENT_READ_ERROR_ACCESS_DENIED, + DOCUMENT_READ_ERROR_NOT_FOUND, + document_read_service, + ) results = document_read_service.read_documents( db=db, @@ -748,12 +752,9 @@ async def read_document( ) result = results[0] if results else None - if not result or result.get("error") == "Document not found": + if not result or result.get("error_code") == DOCUMENT_READ_ERROR_NOT_FOUND: raise HTTPException(status_code=404, detail="Document not found") - if ( - result.get("error") - == "Access denied: document not in allowed knowledge bases" - ): + if result.get("error_code") == DOCUMENT_READ_ERROR_ACCESS_DENIED: raise HTTPException( status_code=403, detail="Access denied: document not in allowed knowledge bases", diff --git a/backend/app/api/endpoints/knowledge.py b/backend/app/api/endpoints/knowledge.py index deb8b01d11e..5a4d9f9db0e 100644 --- a/backend/app/api/endpoints/knowledge.py +++ b/backend/app/api/endpoints/knowledge.py @@ -47,7 +47,10 @@ KnowledgeService, knowledge_base_qa_service, ) -from app.services.knowledge.orchestrator import knowledge_orchestrator +from app.services.knowledge.orchestrator import ( + MAX_DOCUMENT_READ_LIMIT, + knowledge_orchestrator, +) from shared.telemetry.decorators import ( add_span_event, capture_trace_context, @@ -780,6 +783,8 @@ async def get_document_detail_standalone( document_id=document_id, include_content=include_content, include_summary=include_summary, + offset=0, + limit=MAX_DOCUMENT_READ_LIMIT, ) return _serialize_standalone_document_detail( detail, @@ -1141,6 +1146,8 @@ async def get_document_detail( document_id=doc_id, include_content=include_content, include_summary=include_summary, + offset=0, + limit=MAX_DOCUMENT_READ_LIMIT, ) except ValueError as error: _raise_document_detail_http_error(error) diff --git a/backend/app/schemas/knowledge.py b/backend/app/schemas/knowledge.py index 86dc729fd54..93b7c07b2b5 100644 --- a/backend/app/schemas/knowledge.py +++ b/backend/app/schemas/knowledge.py @@ -575,9 +575,11 @@ class DocumentContentReadResponse(BaseModel): document_id: int = Field(..., description="Document ID") name: str = Field(..., description="Document name") content: str = Field(..., description="Document content (partial)") - total_length: int = Field(..., description="Total document length in characters") - offset: int = Field(..., description="Actual start position") - returned_length: int = Field(..., description="Number of characters returned") + total_length: int = Field( + ..., ge=0, description="Total document length in characters" + ) + offset: int = Field(..., ge=0, description="Actual start position") + returned_length: int = Field(..., ge=0, description="Number of characters returned") has_more: bool = Field(..., description="Whether more content is available") kb_id: int = Field(..., description="Knowledge base ID") diff --git a/backend/app/services/knowledge/orchestrator.py b/backend/app/services/knowledge/orchestrator.py index fb41b83dfb6..0ff997455c0 100644 --- a/backend/app/services/knowledge/orchestrator.py +++ b/backend/app/services/knowledge/orchestrator.py @@ -41,7 +41,10 @@ ResourceScope, ) from app.services.knowledge.knowledge_service import KnowledgeService -from app.services.rag.document_read_service import document_read_service +from app.services.rag.document_read_service import ( + DOCUMENT_READ_ERROR_NOT_FOUND, + document_read_service, +) logger = logging.getLogger(__name__) @@ -726,7 +729,7 @@ def read_document_content( ) result = results[0] if results else None - if not result or result.get("error") == "Document not found": + if not result or result.get("error_code") == DOCUMENT_READ_ERROR_NOT_FOUND: raise ValueError("Document not found") if result.get("error"): raise ValueError(result["error"]) @@ -754,18 +757,17 @@ async def get_document_detail( limit: int = MAX_DOCUMENT_READ_LIMIT, ) -> DocumentDetailResponse: """Aggregate optional document content and summary into a detail response.""" + self._get_document_with_access_or_raise( + db=db, + user=user, + document_id=document_id, + ) + content = None content_length = None truncated = None summary = None - if include_content or include_summary: - self._get_document_with_access_or_raise( - db=db, - user=user, - document_id=document_id, - ) - if include_content: paged = self.read_document_content( db=db, diff --git a/backend/app/services/rag/document_read_service.py b/backend/app/services/rag/document_read_service.py index b2de653dd45..8e463bfd85c 100644 --- a/backend/app/services/rag/document_read_service.py +++ b/backend/app/services/rag/document_read_service.py @@ -14,6 +14,9 @@ logger = logging.getLogger(__name__) +DOCUMENT_READ_ERROR_NOT_FOUND = "DOCUMENT_NOT_FOUND" +DOCUMENT_READ_ERROR_ACCESS_DENIED = "DOCUMENT_ACCESS_DENIED" + class DocumentReadService: """Read knowledge documents and optionally persist kb_head usage.""" @@ -164,7 +167,13 @@ def read_documents( for document_id in document_ids: document = documents_by_id.get(document_id) if document is None: - results.append({"id": document_id, "error": "Document not found"}) + results.append( + { + "id": document_id, + "error": "Document not found", + "error_code": DOCUMENT_READ_ERROR_NOT_FOUND, + } + ) continue if allowed_kb_ids and document.kind_id not in allowed_kb_ids: @@ -178,6 +187,7 @@ def read_documents( { "id": document_id, "error": "Access denied: document not in allowed knowledge bases", + "error_code": DOCUMENT_READ_ERROR_ACCESS_DENIED, } ) continue diff --git a/backend/init_data/skills/wegent-knowledge/SKILL.md b/backend/init_data/skills/wegent-knowledge/SKILL.md index 48aacb580a1..0e014290e14 100644 --- a/backend/init_data/skills/wegent-knowledge/SKILL.md +++ b/backend/init_data/skills/wegent-knowledge/SKILL.md @@ -104,7 +104,7 @@ You now have access to Wegent Knowledge Base management tools. ``` 5. Update document content: - ``` + ```text update_document_content( document_id=456, content="Updated notes with new information...", @@ -113,7 +113,7 @@ You now have access to Wegent Knowledge Base management tools. ``` 6. Read long document content incrementally: - ``` + ```text read_document_content( document_id=456, offset=0 diff --git a/backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py b/backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py index 76a5edb3149..db09f595188 100644 --- a/backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py +++ b/backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py @@ -32,12 +32,26 @@ def test_standalone_detail_uses_orchestrator( ) as mock_detail: response = test_client.get( "/api/knowledge-documents/9/detail", + params={ + "include_content": "true", + "include_summary": "false", + }, headers=_auth_header(test_token), ) assert response.status_code == 200 - assert response.json() == payload + assert response.json() == { + "document_id": 9, + "content": "abcd", + "content_length": 10, + "truncated": True, + } mock_detail.assert_awaited_once() + assert mock_detail.await_args.kwargs["document_id"] == 9 + assert mock_detail.await_args.kwargs["include_content"] is True + assert mock_detail.await_args.kwargs["include_summary"] is False + assert mock_detail.await_args.kwargs["offset"] == 0 + assert mock_detail.await_args.kwargs["limit"] == 100000 def test_kb_scoped_detail_uses_orchestrator( @@ -84,6 +98,11 @@ def test_kb_scoped_detail_uses_orchestrator( assert response.status_code == 200 assert response.json() == payload mock_detail.assert_awaited_once() + assert mock_detail.await_args.kwargs["document_id"] == 9 + assert mock_detail.await_args.kwargs["include_content"] is True + assert mock_detail.await_args.kwargs["include_summary"] is True + assert mock_detail.await_args.kwargs["offset"] == 0 + assert mock_detail.await_args.kwargs["limit"] == 100000 def test_standalone_detail_maps_not_found_error_to_404( @@ -133,6 +152,11 @@ def test_standalone_detail_omits_unrequested_fields( "summary": {"summary": "hello"}, } mock_detail.assert_awaited_once() + assert mock_detail.await_args.kwargs["document_id"] == 9 + assert mock_detail.await_args.kwargs["include_content"] is False + assert mock_detail.await_args.kwargs["include_summary"] is True + assert mock_detail.await_args.kwargs["offset"] == 0 + assert mock_detail.await_args.kwargs["limit"] == 100000 def test_kb_scoped_detail_rejects_document_outside_requested_kb( diff --git a/backend/tests/mcp_server/test_tools.py b/backend/tests/mcp_server/test_tools.py index 3fb5c113218..a5507180fec 100644 --- a/backend/tests/mcp_server/test_tools.py +++ b/backend/tests/mcp_server/test_tools.py @@ -241,6 +241,11 @@ def test_read_document_content_returns_error_dict_for_unexpected_failure(self): def test_read_document_content_uses_shared_default_limit_constant(self): """Test that read_document_content reuses the shared default limit constant.""" module = get_knowledge_module() - source = inspect.getsource(module.read_document_content) + target = getattr( + module.read_document_content, + "__wrapped__", + module.read_document_content, + ) + default_limit = inspect.signature(target).parameters["limit"].default - assert "limit: int = 100000" not in source + assert default_limit == module.MAX_DOCUMENT_READ_LIMIT diff --git a/backend/tests/services/knowledge/test_orchestrator.py b/backend/tests/services/knowledge/test_orchestrator.py index f5ff0ebf001..ca84344f4dd 100644 --- a/backend/tests/services/knowledge/test_orchestrator.py +++ b/backend/tests/services/knowledge/test_orchestrator.py @@ -317,6 +317,36 @@ def test_read_document_content_raises_for_missing_document( document_id=9, ) + def test_read_document_content_uses_error_code_for_missing_document( + self, orchestrator, mock_db, mock_user + ): + """Test read_document_content maps stable missing-document error codes.""" + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + mock_db.query.return_value.filter.return_value.first.return_value = document + + with patch( + "app.services.knowledge.orchestrator.KnowledgeService" + ) as mock_kb_service: + mock_kb_service.get_knowledge_base.return_value = (MagicMock(id=77), True) + + with patch( + "app.services.knowledge.orchestrator.document_read_service" + ) as mock_read_service: + mock_read_service.read_documents.return_value = [ + { + "id": 9, + "error": "reader payload changed", + "error_code": "DOCUMENT_NOT_FOUND", + } + ] + + with pytest.raises(ValueError, match="Document not found"): + orchestrator.read_document_content( + db=mock_db, + user=mock_user, + document_id=9, + ) + def test_read_document_content_raises_when_reader_returns_empty_results( self, orchestrator, mock_db, mock_user ): @@ -462,26 +492,38 @@ async def test_get_document_detail_maps_content_length_and_truncated( async def test_get_document_detail_skips_content_when_disabled( self, orchestrator, mock_db, mock_user ): - """Test get_document_detail skips optional content and summary aggregation.""" + """Test get_document_detail still validates access when payload sections are disabled.""" + document = SimpleNamespace(id=9, name="roadmap", kind_id=77) + with patch.object( - orchestrator, "read_document_content" - ) as mock_read_document_content: - with patch( - "app.services.knowledge.summary_service.get_summary_service" - ) as mock_get_summary_service: - result = await orchestrator.get_document_detail( - db=mock_db, - user=mock_user, - document_id=9, - include_content=False, - include_summary=False, - ) + orchestrator, + "_get_document_with_access_or_raise", + return_value=document, + ) as mock_get_document: + with patch.object( + orchestrator, "read_document_content" + ) as mock_read_document_content: + with patch( + "app.services.knowledge.summary_service.get_summary_service" + ) as mock_get_summary_service: + result = await orchestrator.get_document_detail( + db=mock_db, + user=mock_user, + document_id=9, + include_content=False, + include_summary=False, + ) assert result.document_id == 9 assert result.content is None assert result.content_length is None assert result.truncated is None assert result.summary is None + mock_get_document.assert_called_once_with( + db=mock_db, + user=mock_user, + document_id=9, + ) mock_read_document_content.assert_not_called() mock_get_summary_service.assert_not_called() diff --git a/backend/tests/services/rag/test_document_read_service.py b/backend/tests/services/rag/test_document_read_service.py index 9c007641555..61c1c03da23 100644 --- a/backend/tests/services/rag/test_document_read_service.py +++ b/backend/tests/services/rag/test_document_read_service.py @@ -131,11 +131,16 @@ def test_read_documents_skips_invalid_results_during_persistence(self) -> None: ) assert results[0]["id"] == 11 - assert results[1] == {"id": 99, "error": "Document not found"} + assert results[1] == { + "id": 99, + "error": "Document not found", + "error_code": "DOCUMENT_NOT_FOUND", + } assert ( results[2]["error"] == "Access denied: document not in allowed knowledge bases" ) + assert results[2]["error_code"] == "DOCUMENT_ACCESS_DENIED" mock_get_context_map.assert_called_once_with( db=db, diff --git a/docs/superpowers/specs/2026-03-30-wegent-knowledge-document-read-design.md b/docs/specs/knowledge/2026-03-30-wegent-knowledge-document-read-design.md similarity index 100% rename from docs/superpowers/specs/2026-03-30-wegent-knowledge-document-read-design.md rename to docs/specs/knowledge/2026-03-30-wegent-knowledge-document-read-design.md diff --git a/docs/superpowers/plans/2026-03-30-wegent-knowledge-document-read-implementation.md b/docs/superpowers/plans/2026-03-30-wegent-knowledge-document-read-implementation.md deleted file mode 100644 index 1fe46f7c90f..00000000000 --- a/docs/superpowers/plans/2026-03-30-wegent-knowledge-document-read-implementation.md +++ /dev/null @@ -1,857 +0,0 @@ -# Wegent Knowledge Document Read Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Add a single-document content read capability to the Wegent Knowledge Base MCP skill, unify document-read logic through `KnowledgeOrchestrator`, and refactor existing REST `/detail` endpoints to reuse the unified read/detail flow without changing their public response shape. - -**Architecture:** Keep `app.services.rag.document_read_service` as the low-level paginator, add one orchestrator method for raw content paging and one orchestrator method for detail aggregation, then route MCP and REST callers through those two methods. Do not add a new public REST read endpoint; MCP gets raw `offset/limit/has_more`, while existing `/detail` routes remain compatibility adapters that map paging output back to `content_length` and `truncated`. - -**Tech Stack:** FastAPI, SQLAlchemy, Pydantic, pytest, unittest.mock, FastMCP - ---- - -## File Structure - -**Modify** -- `backend/app/schemas/knowledge.py` - - Add a typed schema for paginated document content reads so orchestrator and MCP share one contract. -- `backend/app/services/knowledge/orchestrator.py` - - Add `read_document_content(...)` and `get_document_detail(...)`. - - Centralize permission checks, argument validation, paging adaptation, and summary retrieval. -- `backend/app/mcp_server/tools/knowledge.py` - - Add the new `read_document_content` MCP tool that delegates to `knowledge_orchestrator`. -- `backend/app/api/endpoints/knowledge.py` - - Replace inline document-read logic in both `/detail` handlers with orchestrator calls. -- `backend/init_data/skills/wegent-knowledge/SKILL.md` - - Document the new MCP tool and its paging behavior. -- `backend/tests/services/knowledge/test_orchestrator.py` - - Add unit tests for raw content reads and detail aggregation. -- `backend/tests/mcp_server/test_tools.py` - - Add MCP tool tests for the new `read_document_content` function. - -**Create** -- `backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py` - - Add API tests for the standalone and KB-scoped `/detail` routes after refactor. - -## Task 1: Add Raw Content Read Contract And Orchestrator Paging - -**Files:** -- Modify: `backend/app/schemas/knowledge.py` -- Modify: `backend/app/services/knowledge/orchestrator.py` -- Modify: `backend/tests/services/knowledge/test_orchestrator.py` - -- [ ] **Step 1: Write the failing orchestrator tests for raw content reads** - -```python -def test_read_document_content_returns_paginated_payload( - self, orchestrator, mock_db, mock_user -): - document = SimpleNamespace(id=9, name="roadmap", kind_id=77) - - with ( - patch("app.services.knowledge.orchestrator.KnowledgeService") as mock_service, - patch( - "app.services.knowledge.orchestrator.document_read_service" - ) as mock_read_service, - ): - mock_service.get_knowledge_base.return_value = (MagicMock(id=77), True) - mock_db.query.return_value.filter.return_value.first.return_value = document - mock_read_service.read_documents.return_value = [ - { - "id": 9, - "name": "roadmap", - "content": "cdef", - "total_length": 10, - "offset": 2, - "returned_length": 4, - "has_more": True, - "kb_id": 77, - } - ] - - result = orchestrator.read_document_content( - db=mock_db, - user=mock_user, - document_id=9, - offset=2, - limit=4, - ) - - assert result.document_id == 9 - assert result.content == "cdef" - assert result.total_length == 10 - assert result.returned_length == 4 - assert result.has_more is True - - -@pytest.mark.parametrize( - ("offset", "limit", "message"), - [ - (-1, 10, "offset must be greater than or equal to 0"), - (0, 0, "limit must be greater than 0"), - (0, 100001, "limit must be less than or equal to 100000"), - ], -) -def test_read_document_content_rejects_invalid_paging_args( - self, orchestrator, mock_db, mock_user, offset, limit, message -): - with pytest.raises(ValueError, match=message): - orchestrator.read_document_content( - db=mock_db, - user=mock_user, - document_id=9, - offset=offset, - limit=limit, - ) - - -def test_read_document_content_raises_for_missing_document( - self, orchestrator, mock_db, mock_user -): - mock_db.query.return_value.filter.return_value.first.return_value = None - - with pytest.raises(ValueError, match="Document not found"): - orchestrator.read_document_content( - db=mock_db, - user=mock_user, - document_id=404, - ) -``` - -- [ ] **Step 2: Run the orchestrator tests to verify they fail** - -Run: - -```bash -uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py -k "read_document_content" -v -``` - -Expected: - -```text -FAIL ... AttributeError: 'KnowledgeOrchestrator' object has no attribute 'read_document_content' -``` - -- [ ] **Step 3: Add the paginated response schema** - -Add this model next to `DocumentDetailResponse` in `backend/app/schemas/knowledge.py`: - -```python -class DocumentContentReadResponse(BaseModel): - """Schema for paginated single-document content reads.""" - - document_id: int = Field(..., description="Document ID") - name: str = Field(..., description="Document name") - content: str = Field(..., description="Returned content slice") - total_length: int = Field(..., ge=0, description="Full content length") - offset: int = Field(..., ge=0, description="Actual returned offset") - returned_length: int = Field( - ..., ge=0, description="Length of the returned content slice" - ) - has_more: bool = Field(..., description="Whether more content is available") - kb_id: int = Field(..., description="Knowledge base ID") -``` - -- [ ] **Step 4: Implement `read_document_content(...)` in the orchestrator** - -Add imports in `backend/app/services/knowledge/orchestrator.py`: - -```python -from app.models.knowledge import KnowledgeDocument -from app.schemas.knowledge import DocumentContentReadResponse -from app.services.rag.document_read_service import document_read_service -``` - -Add helpers and method on `KnowledgeOrchestrator`: - -```python -MAX_DOCUMENT_READ_LIMIT = 100000 - - -def _validate_document_read_args(offset: int, limit: int) -> None: - if offset < 0: - raise ValueError("offset must be greater than or equal to 0") - if limit <= 0: - raise ValueError("limit must be greater than 0") - if limit > MAX_DOCUMENT_READ_LIMIT: - raise ValueError( - f"limit must be less than or equal to {MAX_DOCUMENT_READ_LIMIT}" - ) - - -def read_document_content( - self, - db: Session, - user: User, - document_id: int, - offset: int = 0, - limit: int = MAX_DOCUMENT_READ_LIMIT, -) -> DocumentContentReadResponse: - _validate_document_read_args(offset, limit) - - document = ( - db.query(KnowledgeDocument) - .filter(KnowledgeDocument.id == document_id) - .first() - ) - if not document: - raise ValueError("Document not found") - - knowledge_base, has_access = KnowledgeService.get_knowledge_base( - db=db, - knowledge_base_id=document.kind_id, - user_id=user.id, - ) - if not knowledge_base: - raise ValueError("Knowledge base not found") - if not has_access: - raise ValueError("Access denied to this document") - - result = document_read_service.read_documents( - db=db, - document_ids=[document_id], - offset=offset, - limit=limit, - knowledge_base_ids=[document.kind_id], - )[0] - - if result.get("error") == "Document not found": - raise ValueError("Document not found") - if result.get("error"): - raise ValueError(result["error"]) - - return DocumentContentReadResponse( - document_id=result["id"], - name=result.get("name", document.name), - content=result.get("content", ""), - total_length=result.get("total_length", 0), - offset=result.get("offset", offset), - returned_length=result.get("returned_length", 0), - has_more=result.get("has_more", False), - kb_id=result.get("kb_id", document.kind_id), - ) -``` - -- [ ] **Step 5: Run the orchestrator tests to verify they pass** - -Run: - -```bash -uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py -k "read_document_content" -v -``` - -Expected: - -```text -PASSED ... test_read_document_content_returns_paginated_payload -PASSED ... test_read_document_content_rejects_invalid_paging_args -PASSED ... test_read_document_content_raises_for_missing_document -``` - -- [ ] **Step 6: Commit the raw read foundation** - -```bash -git add backend/app/schemas/knowledge.py backend/app/services/knowledge/orchestrator.py backend/tests/services/knowledge/test_orchestrator.py -git commit -m "feat(backend): add orchestrated knowledge document reads" -``` - -## Task 2: Add Orchestrator Detail Aggregation - -**Files:** -- Modify: `backend/app/services/knowledge/orchestrator.py` -- Modify: `backend/tests/services/knowledge/test_orchestrator.py` - -- [ ] **Step 1: Write the failing tests for detail aggregation** - -Append these tests to `backend/tests/services/knowledge/test_orchestrator.py` and update the existing import to `from unittest.mock import AsyncMock, MagicMock, Mock, patch`: - -```python -@pytest.mark.asyncio -async def test_get_document_detail_maps_content_length_and_truncated( - self, orchestrator, mock_db, mock_user -): - paged = SimpleNamespace( - document_id=9, - name="roadmap", - content="abcd", - total_length=10, - offset=0, - returned_length=4, - has_more=True, - kb_id=77, - ) - - with ( - patch.object(orchestrator, "read_document_content", return_value=paged), - patch("app.services.knowledge.orchestrator.get_summary_service") as mock_get_summary, - ): - mock_get_summary.return_value.get_document_summary = AsyncMock(return_value={ - "summary": "hello" - }) - - result = await orchestrator.get_document_detail( - db=mock_db, - user=mock_user, - document_id=9, - include_content=True, - include_summary=True, - ) - - assert result.document_id == 9 - assert result.content == "abcd" - assert result.content_length == 10 - assert result.truncated is True - assert result.summary == {"summary": "hello"} - - -@pytest.mark.asyncio -async def test_get_document_detail_skips_content_when_disabled( - self, orchestrator, mock_db, mock_user -): - with ( - patch.object(orchestrator, "read_document_content") as mock_read, - patch("app.services.knowledge.orchestrator.get_summary_service") as mock_get_summary, - ): - result = await orchestrator.get_document_detail( - db=mock_db, - user=mock_user, - document_id=9, - include_content=False, - include_summary=False, - ) - - mock_read.assert_not_called() - mock_get_summary.assert_not_called() - assert result.content is None - assert result.content_length is None - assert result.truncated is None - assert result.summary is None -``` - -- [ ] **Step 2: Run the detail tests to verify they fail** - -Run: - -```bash -uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py -k "get_document_detail" -v -``` - -Expected: - -```text -FAIL ... AttributeError: 'KnowledgeOrchestrator' object has no attribute 'get_document_detail' -``` - -- [ ] **Step 3: Implement `get_document_detail(...)` in the orchestrator** - -Add the summary import: - -```python -from app.services.knowledge import get_summary_service -``` - -Add the async method: - -```python -async def get_document_detail( - self, - db: Session, - user: User, - document_id: int, - include_content: bool = True, - include_summary: bool = True, - offset: int = 0, - limit: int = MAX_DOCUMENT_READ_LIMIT, -) -> DocumentDetailResponse: - content = None - content_length = None - truncated = None - summary = None - - if include_content: - paged = self.read_document_content( - db=db, - user=user, - document_id=document_id, - offset=offset, - limit=limit, - ) - content = paged.content - content_length = paged.total_length - truncated = paged.has_more - - if include_summary: - summary_service = get_summary_service(db) - summary_obj = await summary_service.get_document_summary(document_id) - if hasattr(summary_obj, "model_dump"): - summary = summary_obj.model_dump() - else: - summary = summary_obj - - return DocumentDetailResponse( - document_id=document_id, - content=content, - content_length=content_length, - truncated=truncated, - summary=summary, - ) -``` - -- [ ] **Step 4: Run the detail tests to verify they pass** - -Run: - -```bash -uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py -k "get_document_detail" -v -``` - -Expected: - -```text -PASSED ... test_get_document_detail_maps_content_length_and_truncated -PASSED ... test_get_document_detail_skips_content_when_disabled -``` - -- [ ] **Step 5: Commit the detail aggregation layer** - -```bash -git add backend/app/services/knowledge/orchestrator.py backend/tests/services/knowledge/test_orchestrator.py -git commit -m "refactor(backend): centralize knowledge document detail aggregation" -``` - -## Task 3: Add MCP Tool And Skill Prompt Support - -**Files:** -- Modify: `backend/app/mcp_server/tools/knowledge.py` -- Modify: `backend/init_data/skills/wegent-knowledge/SKILL.md` -- Modify: `backend/tests/mcp_server/test_tools.py` - -- [ ] **Step 1: Write the failing MCP tool tests** - -Add a new test class to `backend/tests/mcp_server/test_tools.py`: - -```python -from app.mcp_server.auth import TaskTokenInfo -from app.mcp_server.tools.knowledge import read_document_content - - -class TestKnowledgeReadDocumentTool: - def test_read_document_content_returns_orchestrator_payload(self): - token_info = TaskTokenInfo( - task_id=1, - subtask_id=2, - user_id=3, - user_name="alice", - ) - mock_user = MagicMock(id=3) - mock_result = MagicMock( - document_id=9, - name="roadmap", - content="abcd", - total_length=10, - offset=0, - returned_length=4, - has_more=True, - kb_id=77, - ) - mock_result.model_dump.return_value = { - "document_id": 9, - "name": "roadmap", - "content": "abcd", - "total_length": 10, - "offset": 0, - "returned_length": 4, - "has_more": True, - "kb_id": 77, - } - - with ( - patch("app.mcp_server.tools.knowledge.SessionLocal") as mock_session_local, - patch( - "app.mcp_server.tools.knowledge._get_user_from_token", - return_value=mock_user, - ), - patch( - "app.mcp_server.tools.knowledge.knowledge_orchestrator.read_document_content", - return_value=mock_result, - ) as mock_read, - ): - mock_session_local.return_value = MagicMock() - - result = read_document_content( - token_info=token_info, - document_id=9, - offset=0, - limit=4, - ) - - assert result["document_id"] == 9 - assert result["has_more"] is True - mock_read.assert_called_once() - - def test_read_document_content_returns_error_dict_for_validation_failure(self): - token_info = TaskTokenInfo( - task_id=1, - subtask_id=2, - user_id=3, - user_name="alice", - ) - - with ( - patch("app.mcp_server.tools.knowledge.SessionLocal") as mock_session_local, - patch( - "app.mcp_server.tools.knowledge._get_user_from_token", - return_value=MagicMock(id=3), - ), - patch( - "app.mcp_server.tools.knowledge.knowledge_orchestrator.read_document_content", - side_effect=ValueError("limit must be greater than 0"), - ), - ): - mock_session_local.return_value = MagicMock() - result = read_document_content(token_info=token_info, document_id=9, limit=0) - - assert result == {"error": "limit must be greater than 0"} -``` - -- [ ] **Step 2: Run the MCP tests to verify they fail** - -Run: - -```bash -uv run --project backend --group dev pytest tests/mcp_server/test_tools.py -k "read_document_content" -v -``` - -Expected: - -```text -FAIL ... ImportError or AttributeError because read_document_content does not exist yet -``` - -- [ ] **Step 3: Implement the MCP tool** - -Add this function to `backend/app/mcp_server/tools/knowledge.py` before `update_document_content(...)`: - -```python -@mcp_tool( - name="read_document_content", - description="Read document content with offset/limit pagination.", - server="knowledge", - param_descriptions={ - "document_id": "Document ID to read", - "offset": "Start position in the extracted text (default: 0)", - "limit": "Maximum number of characters to return (default: 100000)", - }, -) -def read_document_content( - token_info: TaskTokenInfo, - document_id: int, - offset: int = 0, - limit: int = 100000, -) -> Dict[str, Any]: - db = SessionLocal() - try: - user = _get_user_from_token(db, token_info) - if not user: - return {"error": "User not found"} - - result = knowledge_orchestrator.read_document_content( - db=db, - user=user, - document_id=document_id, - offset=offset, - limit=limit, - ) - return result.model_dump() - except ValueError as e: - logger.warning(f"[MCP] read_document_content validation error: {e}") - return {"error": str(e)} - except Exception as e: - logger.error(f"[MCP] read_document_content error: {e}", exc_info=True) - return {"error": str(e)} - finally: - db.close() -``` - -- [ ] **Step 4: Update the skill prompt** - -Insert this section into `backend/init_data/skills/wegent-knowledge/SKILL.md` under `## Available Tools`: - -```markdown -- **read_document_content**: Read a document's extracted text with paging - - document_id: Document ID to read - - offset: Start character position (default: 0) - - limit: Maximum characters to return (default: 100000) - - returns: content slice, total_length, returned_length, has_more, kb_id -``` - -Add this note under `## Usage Notes`: - -```markdown -- Long documents should be read incrementally: start with the default limit, then continue with `offset = previous_offset + previous_returned_length` while `has_more=true` -``` - -- [ ] **Step 5: Run the MCP tests to verify they pass** - -Run: - -```bash -uv run --project backend --group dev pytest tests/mcp_server/test_tools.py -k "read_document_content" -v -``` - -Expected: - -```text -PASSED ... test_read_document_content_returns_orchestrator_payload -PASSED ... test_read_document_content_returns_error_dict_for_validation_failure -``` - -- [ ] **Step 6: Commit the MCP-facing feature** - -```bash -git add backend/app/mcp_server/tools/knowledge.py backend/init_data/skills/wegent-knowledge/SKILL.md backend/tests/mcp_server/test_tools.py -git commit -m "feat(backend): expose knowledge document reads to MCP" -``` - -## Task 4: Refactor REST `/detail` Endpoints To Use Orchestrator - -**Files:** -- Modify: `backend/app/api/endpoints/knowledge.py` -- Create: `backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py` - -- [ ] **Step 1: Write the failing endpoint tests** - -Create `backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py` with: - -```python -from unittest.mock import AsyncMock, patch - - -def test_standalone_detail_uses_orchestrator(test_client, test_token): - payload = { - "document_id": 9, - "content": "abcd", - "content_length": 10, - "truncated": True, - "summary": {"summary": "hello"}, - } - - with patch( - "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", - new_callable=AsyncMock, - return_value=payload, - ) as mock_detail: - response = test_client.get( - "/api/knowledge-documents/9/detail", - headers={"Authorization": f"Bearer {test_token}"}, - ) - - assert response.status_code == 200 - assert response.json() == payload - mock_detail.assert_awaited_once() - - -def test_kb_scoped_detail_uses_orchestrator(test_client, test_token): - payload = { - "document_id": 9, - "content": "abcd", - "content_length": 10, - "truncated": False, - "summary": {"summary": "hello"}, - } - - with patch( - "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", - new_callable=AsyncMock, - return_value=payload, - ) as mock_detail: - response = test_client.get( - "/api/knowledge-bases/77/documents/9/detail", - headers={"Authorization": f"Bearer {test_token}"}, - ) - - assert response.status_code == 200 - assert response.json() == payload - mock_detail.assert_awaited_once() - - -def test_standalone_detail_maps_not_found_error_to_404(test_client, test_token): - with patch( - "app.api.endpoints.knowledge.knowledge_orchestrator.get_document_detail", - new_callable=AsyncMock, - side_effect=ValueError("Document not found"), - ): - response = test_client.get( - "/api/knowledge-documents/404/detail", - headers={"Authorization": f"Bearer {test_token}"}, - ) - - assert response.status_code == 404 - assert response.json()["detail"] == "Document not found" -``` - -- [ ] **Step 2: Run the endpoint tests to verify they fail** - -Run: - -```bash -uv run --project backend --group dev pytest tests/api/endpoints/test_knowledge_document_detail_endpoints.py -v -``` - -Expected: - -```text -FAIL ... because the current endpoints still execute inline read logic and do not call knowledge_orchestrator.get_document_detail -``` - -- [ ] **Step 3: Refactor the standalone `/detail` handler** - -Convert the standalone route to async and replace the inline body in `backend/app/api/endpoints/knowledge.py` with an orchestrator call: - -```python -@document_router.get("/{document_id}/detail") -@trace_async("get_document_detail_standalone", "knowledge.api") -async def get_document_detail_standalone(...): - try: - return await knowledge_orchestrator.get_document_detail( - db=db, - user=current_user, - document_id=document_id, - include_content=include_content, - include_summary=include_summary, - ) - except ValueError as e: - error_msg = str(e) - if "not found" in error_msg.lower(): - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=error_msg) - if "access denied" in error_msg.lower(): - raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=error_msg) - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=error_msg) -``` - -- [ ] **Step 4: Refactor the KB-scoped `/detail` handler** - -Replace the inline body in the async KB-scoped route with: - -```python -@summary_router.get("/{kb_id}/documents/{doc_id}/detail", response_model=DocumentDetailResponse) -@trace_async("get_document_detail", "knowledge.api") -async def get_document_detail(...): - try: - return await knowledge_orchestrator.get_document_detail( - db=db, - user=current_user, - document_id=doc_id, - include_content=include_content, - include_summary=include_summary, - ) - except ValueError as e: - error_msg = str(e) - if "not found" in error_msg.lower(): - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=error_msg) - if "access denied" in error_msg.lower(): - raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=error_msg) - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=error_msg) -``` - -- [ ] **Step 5: Run the endpoint tests to verify they pass** - -Run: - -```bash -uv run --project backend --group dev pytest tests/api/endpoints/test_knowledge_document_detail_endpoints.py -v -``` - -Expected: - -```text -PASSED ... test_standalone_detail_uses_orchestrator -PASSED ... test_kb_scoped_detail_uses_orchestrator -PASSED ... test_standalone_detail_maps_not_found_error_to_404 -``` - -- [ ] **Step 6: Run the complete targeted regression suite** - -Run: - -```bash -uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py tests/mcp_server/test_tools.py tests/api/endpoints/test_knowledge_document_detail_endpoints.py tests/services/rag/test_document_read_service.py -v -``` - -Expected: - -```text -all selected tests PASS -``` - -- [ ] **Step 7: Commit the REST refactor** - -```bash -git add backend/app/api/endpoints/knowledge.py backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py -git commit -m "refactor(backend): reuse orchestrator in knowledge detail endpoints" -``` - -## Task 5: Final Verification And Cleanup - -**Files:** -- Modify: `backend/app/services/knowledge/orchestrator.py` -- Modify: `backend/app/mcp_server/tools/knowledge.py` -- Modify: `backend/app/api/endpoints/knowledge.py` -- Modify: `backend/app/schemas/knowledge.py` -- Modify: `backend/init_data/skills/wegent-knowledge/SKILL.md` -- Modify: `backend/tests/services/knowledge/test_orchestrator.py` -- Modify: `backend/tests/mcp_server/test_tools.py` -- Modify: `backend/tests/api/endpoints/test_knowledge_document_detail_endpoints.py` - -- [ ] **Step 1: Run formatting-safe compilation checks** - -Run: - -```bash -uv run --project backend --group dev python -m py_compile app/services/knowledge/orchestrator.py app/mcp_server/tools/knowledge.py app/api/endpoints/knowledge.py app/schemas/knowledge.py -``` - -Expected: - -```text -no output -``` - -- [ ] **Step 2: Re-run the backend verification commands** - -Run: - -```bash -uv run --project backend --group dev pytest tests/services/knowledge/test_orchestrator.py tests/mcp_server/test_tools.py tests/api/endpoints/test_knowledge_document_detail_endpoints.py tests/services/rag/test_document_read_service.py -v -``` - -Expected: - -```text -all selected tests PASS -``` - -- [ ] **Step 3: Review the skill documentation text in the repo** - -Confirm `backend/init_data/skills/wegent-knowledge/SKILL.md` now communicates: - -```markdown -- read_document_content is available -- default limit is 100000 -- continue reading by advancing offset -- use has_more to detect remaining content -``` - -- [ ] **Step 4: Capture final status without rewriting history** - -```bash -git status --short -git log --oneline -n 5 -``` - -Expected: - -```text -working tree clean except for any intentional uncommitted notes -recent commits show one commit per completed task -```