Repository navigation
feat: 新增独立 knowledge_runtime 服务支持 RAG 远程调用 - #1001
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a standalone FastAPI Knowledge Runtime service for RAG operations, new env vars and Docker/start scripts, backend gateway and auth token adjustments, a content fetcher with retries, executors (index/query/admin), structured logging, tests, and architecture documentation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Backend
participant KnowledgeRuntime as Knowledge Runtime
participant Storage
participant Embeddings
rect rgba(100,150,200,0.5)
note over Client,Embeddings: RAG Query (remote mode)
Client->>Backend: POST /rag/query
Backend->>Backend: choose runtime via RAG_RUNTIME_MODE
Backend->>KnowledgeRuntime: POST /internal/rag/query
KnowledgeRuntime->>KnowledgeRuntime: QueryExecutor.execute()
loop per KB config
KnowledgeRuntime->>Storage: create storage backend
KnowledgeRuntime->>Embeddings: create embedding model
KnowledgeRuntime->>Storage: execute knowledge_engine query
Storage-->>KnowledgeRuntime: return records
end
KnowledgeRuntime->>KnowledgeRuntime: aggregate, rank, trim
KnowledgeRuntime-->>Backend: RemoteQueryResponse
Backend-->>Client: response
end
rect rgba(200,100,150,0.5)
note over Client,Storage: Indexing (remote mode)
Client->>Backend: POST /rag/index
Backend->>KnowledgeRuntime: POST /internal/rag/index
KnowledgeRuntime->>KnowledgeRuntime: IndexExecutor.execute()
alt PresignedUrl
KnowledgeRuntime->>KnowledgeRuntime: HTTP GET presigned URL
else BackendAttachment
KnowledgeRuntime->>Backend: GET attachment (Authorization: Bearer <token>)
Backend-->>KnowledgeRuntime: stream content
end
KnowledgeRuntime->>Storage: index chunks via DocumentService
Storage-->>KnowledgeRuntime: index result
KnowledgeRuntime-->>Backend: RemoteIndexResponse
Backend-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
knowledge_runtime/knowledge_runtime/services/query_executor.py-159-170 (1)
159-170:⚠️ Potential issue | 🟡 MinorAvoid zero-token estimates for non-empty content and extract the heuristic constant.
len(text) // 4returns0for 1–3 character records, which can underreport usage. Also move the heuristic value to a named constant. As per coding guidelines,**/*.py: Extract magic numbers to constants in Python code.♻️ Proposed fix
logger = logging.getLogger(__name__) +CHARS_PER_TOKEN_ESTIMATE = 4 @@ - return len(text) // 4 + if not text: + return 0 + return max( + 1, + (len(text) + CHARS_PER_TOKEN_ESTIMATE - 1) + // CHARS_PER_TOKEN_ESTIMATE, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/services/query_executor.py` around lines 159 - 170, Extract the magic number by adding a module-level constant (e.g., CHARS_PER_TOKEN = 4) and update the _estimate_tokens method to use it and never return 0 for non-empty input (compute tokens = len(text) // CHARS_PER_TOKEN and return max(1, tokens) for non-empty text, returning 0 only for empty input); reference the _estimate_tokens function and the new CHARS_PER_TOKEN constant when making the change.knowledge_runtime/knowledge_runtime/services/query_executor.py-36-170 (1)
36-170:⚠️ Potential issue | 🟡 MinorAdd OpenTelemetry tracing decorators to QueryExecutor methods.
This service performs remote RAG execution and should be traceable. Per coding guidelines (
**/*.py), add@trace_asyncdecorator toexecute()and_query_knowledge_base(), and@trace_syncdecorator to_extract_document_id()and_estimate_tokens(). Import fromshared.telemetry.decorators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/services/query_executor.py` around lines 36 - 170, Add OpenTelemetry tracing decorators to the QueryExecutor methods: import trace_async and trace_sync from shared.telemetry.decorators, then annotate async methods execute and _query_knowledge_base with `@trace_async` and annotate synchronous helpers _extract_document_id and _estimate_tokens with `@trace_sync`; ensure the import is added at the top of the module and that the decorator names exactly match trace_async and trace_sync so the decorators apply to KnowledgeRuntimeQueryExecutor.execute, _query_knowledge_base, _extract_document_id, and _estimate_tokens.docs/plans/2026-04-19-knowledge-runtime-architecture.md-141-150 (1)
141-150:⚠️ Potential issue | 🟡 MinorRemove the stale internal-token authentication section.
The PR objective says remote Knowledge Runtime communication removes internal service token auth, but this section still requires
Authorization: Bearer <INTERNAL_SERVICE_TOKEN>. Please align the architecture doc with the implemented unauthenticated internal service contract, or explicitly document the actual replacement control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-04-19-knowledge-runtime-architecture.md` around lines 141 - 150, Update the "认证机制" section that currently requires "Authorization: Bearer <INTERNAL_SERVICE_TOKEN>" to reflect the implemented contract: remove the stale INTERNAL_SERVICE_TOKEN requirement and either state that Knowledge Runtime internal communication is unauthenticated or document the actual replacement control mechanism used (e.g., mTLS, IP allowlist, signed requests) so the doc matches implementation; locate the "认证机制" header and the Authorization example and replace or remove that snippet and accompanying sentence accordingly.docs/plans/2026-04-19-knowledge-runtime-architecture.md-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced blocks.
These are diagrams/flows, so
textis enough and will satisfy markdownlint.📝 Proposed fix
-``` +```text ... -``` +``` -``` +```text ... -``` +``` -``` +```text ... -``` +```Also applies to: 210-210, 249-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-04-19-knowledge-runtime-architecture.md` at line 34, Several fenced code blocks use plain ``` without a language, so add the language identifier "text" to each triple-backtick fence (replace ``` with ```text) for the three diagram/flow blocks shown and the other occurrences noted (the additional fenced blocks at the other two locations). Locate the triple-backtick fences (``` ... ```) and change their opening fences to ```text while leaving the block contents unchanged.docs/plans/2026-04-19-knowledge-runtime-architecture.md-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorPlace this Chinese doc under
docs/zh/and add the English counterpart.This new Chinese documentation currently lives under
docs/plans/, which bypasses the repo’s zh/en documentation structure. As per coding guidelines,docs/**/*.md: “Write Chinese documentation first in docs/zh/, then create English versions in docs/en/”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-04-19-knowledge-runtime-architecture.md` around lines 1 - 5, This new Chinese doc currently in docs/plans/ should be moved into the repo's docs/zh/ structure and paired with an English counterpart in docs/en/; rename or relocate the file under docs/zh/ (preserve the YAML frontmatter and the heading "Knowledge Runtime 架构设计"), then create an English file in docs/en/ with the same frontmatter and an English title like "Knowledge Runtime Architecture" and translated content, ensuring both files follow the docs/**/*.md convention and keep internal links and sidebar_position consistent.start.sh-1519-1521 (1)
1519-1521:⚠️ Potential issue | 🟡 MinorValidate
knowledge_enginebefore skipping Knowledge Runtime dependency sync.
sync_python_depsonly probesimport shared; forknowledge_runtime, that can incorrectly skipuv syncwhen the venv exists butknowledge_engineis missing or stale.🛠️ Proposed fix
-sync_python_deps() { +sync_python_deps() { local dir=$1 local name=$2 + shift 2 + local required_modules=("$@") @@ - # Check if shared module is installed - if ! .venv/bin/python -c "import shared" 2>/dev/null; then - need_sync=true - fi + for module in "${required_modules[@]:-shared}"; do + if ! .venv/bin/python -c "import ${module}" 2>/dev/null; then + need_sync=true + break + fi + doneThen call it with the Knowledge Runtime-specific modules:
- sync_python_deps "knowledge_runtime" "Knowledge Runtime" + sync_python_deps "knowledge_runtime" "Knowledge Runtime" shared knowledge_engine🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@start.sh` around lines 1519 - 1521, The current call to sync_python_deps when start_knowledge_runtime is true only probes `import shared` and can skip syncing if the venv exists even though `knowledge_engine` is missing or stale; update the call site that checks `start_knowledge_runtime` to invoke sync_python_deps with the Knowledge Runtime-specific module list (include `knowledge_engine` in the probe list along with `shared`) so the function verifies both `shared` and `knowledge_engine` before deciding to skip the `uv sync`.knowledge_runtime/knowledge_runtime/api/endpoints/index.py-19-35 (1)
19-35:⚠️ Potential issue | 🟡 MinorAdd tracing to the new endpoint.
This public async endpoint currently skips the required OpenTelemetry decorator.
🔭 Proposed fix
from fastapi import APIRouter from knowledge_runtime.services.index_executor import IndexExecutor from shared.models import RemoteIndexRequest +from shared.telemetry.decorators import trace_async @@ `@router.post`("/index") +@trace_async( + span_name="knowledge_runtime.index_document", + tracer_name="knowledge_runtime.api", +) async def index_document(request: RemoteIndexRequest) -> dict[str, Any]:As per coding guidelines,
**/*.py: “Use OpenTelemetry tracing decorators:@trace_async,@trace_syncfor entire functions, add_span_event, set_span_attribute for current span”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/api/endpoints/index.py` around lines 19 - 35, The public async endpoint index_document is missing OpenTelemetry tracing; wrap it with the `@trace_async` decorator (import it from your tracing helper) and add span events/attributes around the work: before calling IndexExecutor().execute add_span_event indicating "index_request_received" and set_span_attribute values such as "doc_ref" and "chunk_count" (or any request fields available), then call executor.execute(request) and add a final add_span_event like "index_completed" and set_span_attribute for result status; ensure imports for trace_async, add_span_event, and set_span_attribute are added and that the IndexExecutor usage remains unchanged.knowledge_runtime/knowledge_runtime/api/endpoints/query.py-17-33 (1)
17-33:⚠️ Potential issue | 🟡 MinorAdd tracing to the new query endpoint.
This public async endpoint should be traced consistently with the rest of RAG orchestration.
🔭 Proposed fix
from fastapi import APIRouter from knowledge_runtime.services.query_executor import QueryExecutor from shared.models import RemoteQueryRequest, RemoteQueryResponse +from shared.telemetry.decorators import trace_async @@ `@router.post`("/query") +@trace_async( + span_name="knowledge_runtime.query_documents", + tracer_name="knowledge_runtime.api", +) async def query_documents(request: RemoteQueryRequest) -> RemoteQueryResponse:As per coding guidelines,
**/*.py: “Use OpenTelemetry tracing decorators:@trace_async,@trace_syncfor entire functions, add_span_event, set_span_attribute for current span”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/api/endpoints/query.py` around lines 17 - 33, The query_documents endpoint is missing OpenTelemetry tracing; add the `@trace_async` decorator to the async function query_documents and create span events/attributes inside the function (use add_span_event and set_span_attribute on the current span) to record key details such as the incoming request.query, number of KB configs in request.kb_configs, and any high-level steps (e.g., "create_backends_start", "execute_query_start"); ensure the decorator import names match the project's tracing utilities and leave QueryExecutor().execute(request) invocation intact so the trace wraps the entire operation.knowledge_runtime/pyproject.toml-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorUse
<3.14if Python 3.13.x is supported.
<=3.13rejects patch releases like 3.13.1 and 3.13.9, accepting only 3.13.0 exactly. Use an exclusive upper bound for the next minor version to properly express support for all Python 3.13.x releases.Proposed fix
-requires-python = ">=3.10,<=3.13" +requires-python = ">=3.10,<3.14"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/pyproject.toml` at line 9, Update the Python version specifier in pyproject.toml to use an exclusive upper bound for the next minor release: change the requires-python value from ">=3.10,<=3.13" to ">=3.10,<3.14" so all Python 3.13.x patch releases are accepted; locate and edit the requires-python entry in the pyproject.toml file to make this replacement.knowledge_runtime/tests/test_logging.py-66-206 (1)
66-206:⚠️ Potential issue | 🟡 MinorIsolate and restore global logging state in these tests.
These tests mutate process-wide loggers and call
handlers.clear()without closing/restoring handlers, which can leak file handles and make later tests order-dependent. Use an autouse fixture to snapshot and restore logging state.Test isolation helper
+from collections.abc import Iterator @@ from knowledge_runtime.core.logging import RequestIdFilter, setup_logging +@pytest.fixture(autouse=True) +def isolated_logging_state() -> Iterator[None]: + """Restore global logging state after each test.""" + root_logger = logging.getLogger() + access_logger = logging.getLogger("uvicorn.access") + + root_handlers = list(root_logger.handlers) + root_level = root_logger.level + access_handlers = list(access_logger.handlers) + access_level = access_logger.level + access_propagate = access_logger.propagate + + yield + + for logger in (root_logger, access_logger): + for handler in logger.handlers[:]: + logger.removeHandler(handler) + handler.close() + + root_logger.handlers[:] = root_handlers + root_logger.setLevel(root_level) + access_logger.handlers[:] = access_handlers + access_logger.setLevel(access_level) + access_logger.propagate = access_propagate + + class TestRequestIdFilter:Based on learnings, Keep tests independent and isolated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_logging.py` around lines 66 - 206, Add an autouse pytest fixture (e.g., isolate_logging or restore_logging) that snapshots global logging state before each test and restores it after: capture root_logger.handlers (and any handlers on loggers used in tests like "uvicorn", "uvicorn.access", "fastapi", "httpx", "httpcore"), their levels and propagate flags, and the open file handlers; in teardown, close any newly-created handlers, reattach the original handlers to the same loggers, and restore original levels/propagate flags so tests like test_setup_logging_console_mode, test_setup_logging_file_mode, test_setup_logging_respects_log_level, test_setup_logging_creates_log_directory, test_setup_logging_suppresses_third_party_logs, test_setup_logging_access_log_separate_file and test_setup_logging_access_log_console_mode run isolated; implement this fixture in the test module (or conftest.py) with `@pytest.fixture`(autouse=True) and ensure you call handler.close() for file handlers before removing them.knowledge_runtime/tests/test_content_fetcher.py-147-156 (1)
147-156:⚠️ Potential issue | 🟡 MinorAssert the retry attempts, not just the final wrapped error.
The comment says this path retries 3 times, but the test would pass even if retries were removed. Capture the
AsyncMockand assert the expected await count.Strengthen the retry assertion
with patch("httpx.AsyncClient") as mock_client: - mock_client.return_value.__aenter__.return_value.get = AsyncMock( - side_effect=httpx.ConnectError("Connection failed") - ) + get_mock = AsyncMock( + side_effect=httpx.ConnectError("Connection failed") + ) + mock_client.return_value.__aenter__.return_value.get = get_mock # The retry decorator will retry 3 times, then raise ContentFetchError with pytest.raises(ContentFetchError) as exc_info: await content_fetcher.fetch(content_ref) assert exc_info.value.retryable + assert get_mock.await_count == 3Based on learnings, Test edge cases and error conditions in tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_content_fetcher.py` around lines 147 - 156, The test only asserts the final wrapped ContentFetchError but doesn't verify the number of retry attempts; capture the mocked get AsyncMock from the patched httpx.AsyncClient (e.g., assign mock_client.return_value.__aenter__.return_value.get to a variable like mock_get), set its side_effect to httpx.ConnectError, run await content_fetcher.fetch(content_ref) inside the pytest.raises block, and then assert the mock_get was awaited the expected number of times (e.g., mock_get.await_count or mock_get.call_count equals 3) to ensure the retry decorator actually retried three times before raising ContentFetchError.knowledge_runtime/knowledge_runtime/core/logging.py-115-151 (1)
115-151:⚠️ Potential issue | 🟡 MinorClose handlers before clearing them.
handlers.clear()detaches handlers without closing file descriptors. Repeated logging setup can leak open files and keep deleted temp log files alive in tests.Close existing handlers safely
+def _close_handlers(logger: logging.Logger) -> None: + """Close and remove all handlers from a logger.""" + for handler in logger.handlers[:]: + logger.removeHandler(handler) + handler.close() + + def setup_logging(log_file_enabled: bool, log_dir: str, log_level: str) -> None: @@ root_logger = logging.getLogger() root_logger.setLevel(level) - root_logger.handlers.clear() + _close_handlers(root_logger) @@ access_handler = _create_file_handler( log_dir, "access.log", logging.DEBUG, log_format, datefmt ) access_logger = logging.getLogger("uvicorn.access") - access_logger.handlers.clear() + _close_handlers(access_logger) access_logger.addHandler(access_handler) access_logger.propagate = False # Don't propagate to root logger @@ # Access logs to console as well access_logger = logging.getLogger("uvicorn.access") - access_logger.handlers.clear() + _close_handlers(access_logger) access_logger.propagate = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/core/logging.py` around lines 115 - 151, Existing logger handlers are being cleared without closing them, which can leak file descriptors; before calling handlers.clear() on root_logger and on the "uvicorn.access" logger, iterate their .handlers and call .close() for each handler, then clear the list; apply this in both the file-logging branch and the console branch (references: root_logger, access_logger, RequestIdFilter, and _create_file_handler) so all previous handlers are properly closed before being removed or replaced.knowledge_runtime/knowledge_runtime/services/content_fetcher.py-213-224 (1)
213-224:⚠️ Potential issue | 🟡 MinorParse
Content-Dispositionrobustly.The current split leaves trailing parameters in the filename, e.g.
filename="a.pdf"; size=1, and misses RFC 5987filename*=values. Use a header parser before deriving the extension.🐛 Proposed fix
+from email.message import Message +from urllib.parse import unquote @@ # Try Content-Disposition header first content_disposition = response.headers.get("Content-Disposition", "") - if "filename=" in content_disposition: - # Parse filename from Content-Disposition - parts = content_disposition.split("filename=") - if len(parts) > 1: - filename = parts[1].strip('"').strip("'") - if "." in filename: - extension = "." + filename.rsplit(".", 1)[-1] - else: - extension = "" - return filename, extension + if content_disposition: + message = Message() + message["Content-Disposition"] = content_disposition + params = dict(message.get_params(header="Content-Disposition")[1:]) + filename = params.get("filename") + filename_star = params.get("filename*") + if filename_star: + _, _, encoded_filename = filename_star.partition("''") + filename = unquote(encoded_filename or filename_star) + if filename: + _, extension = self._split_filename_extension(filename) + return filename, extensionAdditional helper:
def _split_filename_extension(self, filename: str) -> tuple[str, str]: """Split a filename into filename and extension.""" if "." not in filename: return filename, "" return filename, "." + filename.rsplit(".", 1)[-1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/services/content_fetcher.py` around lines 213 - 224, The Content-Disposition parsing is brittle: update the code that reads response.headers.get("Content-Disposition") to robustly handle RFC5987 filename* and strip trailing parameters instead of naïve splitting; first look for a filename* param and decode per RFC5987 (parse the "charset'lang'encoded" form and urllib.parse.unquote the value), fall back to parsing the header with cgi.parse_header (or email.message.Message().get_param) to extract the filename param safely, then call a helper _split_filename_extension(filename: str) to derive and return the (filename, extension) tuple; ensure you return filename and extension only after decoding and trimming surrounding quotes.
🧹 Nitpick comments (2)
knowledge_runtime/tests/conftest.py (1)
10-31: Minor: moveMagicMockimport to module top-level.Both fixtures re-import
MagicMockinside the function body. Lifting the import to the module top makes the fixtures cleaner and avoids repeated imports.♻️ Proposed refactor
import pytest +from unittest.mock import MagicMock @@ `@pytest.fixture` def mock_storage_backend(): """Create a mock storage backend for testing.""" - from unittest.mock import MagicMock - backend = MagicMock() @@ `@pytest.fixture` def mock_embed_model(): """Create a mock embedding model for testing.""" - from unittest.mock import MagicMock - model = MagicMock() return modelAs per coding guidelines: "Follow PEP 8 standard with Black formatter ... and isort for Python code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/conftest.py` around lines 10 - 31, Move the repeated local imports of MagicMock out of the fixtures by adding a single top-level import "from unittest.mock import MagicMock" at the module level and remove the inline imports inside the mock_storage_backend and mock_embed_model fixture functions so both fixtures simply reference MagicMock; ensure no other references rely on the local import and run formatter/isort to satisfy project style.knowledge_runtime/knowledge_runtime/api/router.py (1)
15-21: Consider unifying the/internal/ragprefix handling.The health router is included without a prefix while its handler hard-codes the full path
/internal/rag/health(seeendpoints/health.py), whereas the other routers rely onprefix="/internal/rag". Functionally equivalent, but moving health to the same prefix pattern would make the routing configuration uniform and easier to refactor (e.g., if the base path changes). The "auth required" vs "no auth" comment is currently aspirational since no auth dependency is wired in here.♻️ Optional refactor
-# Health check endpoint (no auth required) -router.include_router(health.router, tags=["health"]) - -# RAG operation endpoints (auth required) -router.include_router(index.router, prefix="/internal/rag", tags=["index"]) +router.include_router(health.router, prefix="/internal/rag", tags=["health"]) +router.include_router(index.router, prefix="/internal/rag", tags=["index"])(and change
@router.get("/internal/rag/health")to@router.get("/health")inhealth.py)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/api/router.py` around lines 15 - 21, Unify the prefix handling by including health.router with the same prefix as the other RAG routers (use router.include_router(health.router, prefix="/internal/rag", tags=["health"])) and update the route declaration in endpoints/health.py from `@router.get`("/internal/rag/health") to `@router.get`("/health"); this keeps the health endpoint under the shared base path while preserving its tag and makes future base-path changes simpler and consistent with index.router and query.router usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/rag/gateway_factory.py`:
- Around line 32-33: Add an OpenTelemetry sync trace decorator and a docstring
to the public factory function get_list_chunks_gateway: import trace_sync from
shared.telemetry.decorators, annotate get_list_chunks_gateway with
`@trace_sync`("rag.get_list_chunks_gateway"), and add a short docstring stating
that it returns the appropriate RagGateway (local or remote) based on the
'list_chunks' runtime mode; keep the existing return of
_build_gateway(settings.get_rag_runtime_mode("list_chunks")).
In `@backend/app/services/rag/retrieval_service.py`:
- Around line 767-769: The call to get_list_chunks_gateway().list_chunks(spec,
db=db) can throw transient Knowledge Runtime errors when routed to
RemoteRagGateway; wrap this call in the same fallback policy used for remote RAG
operations (or catch exceptions in retrieve_with_routing) so failures fall back
to local RAG behavior. Specifically, in retrieval_service.py around the call to
get_list_chunks_gateway() / list_chunks, implement the remote retry/fallback
wrapper used elsewhere for RemoteRagGateway calls (or catch the raised exception
there and call rag_retrieval with the same spec/db) so a transient remote
failure triggers the local rag_retrieval path instead of bubbling up.
In `@docker/knowledge_runtime/Dockerfile`:
- Around line 31-42: Create and switch to a non-root user for runtime: add
commands to create an unprivileged user/group (e.g., appuser), chown the
writable directory /app/logs to that user, and add a USER directive before the
final CMD so uvicorn (knowledge_runtime.main:app) runs as that user; ensure any
previously created runtime folders are owned by the new user and retained in the
image build so the HEALTHCHECK and EXPOSE behavior remains unchanged.
In `@knowledge_runtime/knowledge_runtime/api/endpoints/admin.py`:
- Line 23: The admin APIRouter (router = APIRouter()) currently has no auth
guard while its handlers perform destructive actions; add an internal
authentication/authorization dependency to the router (e.g., provide
dependencies=[Depends(internal_auth)] when constructing APIRouter or apply a
router.include_router(..., dependencies=[Depends(internal_auth)]) or a
middleware) and implement/internalize an internal_auth dependency function that
validates a shared secret/token and raises HTTPException(403) on failure; ensure
this dependency is referenced for the admin router that exposes
delete/purge/drop-index handlers and update the README to document the required
network-level protection or secret expected by internal_auth.
In `@knowledge_runtime/knowledge_runtime/config.py`:
- Around line 15-26: The SettingsConfigDict currently lacks validation_alias
mappings so BaseSettings will pick up HOST/PORT instead of the documented
KNOWLEDGE_RUNTIME_* names and it also defaults host to 0.0.0.0; update
model_config (the SettingsConfigDict) to include validation_alias entries
mapping "host" => ["KNOWLEDGE_RUNTIME_HOST"], "port" =>
["KNOWLEDGE_RUNTIME_PORT"], and "backend_internal_url" =>
["KNOWLEDGE_RUNTIME_BACKEND_INTERNAL_URL"] (or similar names used in
.env.example/docker-compose.yml), and change the default value of the host field
from "0.0.0.0" to "127.0.0.1" so the service binds locally by default unless
explicitly overridden for container deployments.
In `@knowledge_runtime/knowledge_runtime/core/logging.py`:
- Around line 74-81: The TimedRotatingFileHandler is created with backupCount=0
which retains all hourly rotated logs forever; update the handler creation in
logging.py (the TimedRotatingFileHandler instantiation for variable
handler/log_file) to set a reasonable bounded retention (e.g., backupCount to a
positive integer or read from config/env like LOG_BACKUP_COUNT) and ensure the
value is validated (non-negative integer) before passing it to
TimedRotatingFileHandler so file logging enabled by default won't fill disk.
In `@knowledge_runtime/knowledge_runtime/main.py`:
- Around line 56-69: The handler currently sets retryable only for
ConnectionError/TimeoutError and returns str(exc) in the response; change it to
preserve retryability when the exception is a ContentFetchError (or has a
retryable attribute) by checking isinstance(exc, ContentFetchError) or
getattr(exc, "retryable", None) and using that value for retryable, replace the
returned message with a generic safe text like "internal server error" (or a
contextual non-sensitive message) instead of str(exc), and log the actual
exception (and traceback) server-side for diagnostics; update the error
construction in RemoteRagError creation to use the preserved retryable and
include only non-sensitive details such as exception_type (type(exc).__name__)
while keeping full exception text out of the JSONResponse.
In `@knowledge_runtime/knowledge_runtime/services/admin_executor.py`:
- Around line 39-129: The delete_document_index, purge_knowledge_index and
drop_knowledge_index methods perform destructive operations using
caller-supplied requests with no service-to-service authorization; add a
service-auth guard that validates an internal signed token or
middleware/dependency before executing storage_backend calls. Implement and
invoke a reusable validator (e.g., validate_internal_service_token or
require_internal_auth_dependency) at the start of each executor method
(delete_document_index, purge_knowledge_index, drop_knowledge_index) to verify
the caller identity/scope and reject unauthorized requests, and only proceed to
create_storage_backend_from_runtime_config and asyncio.to_thread calls if the
validation succeeds.
- Around line 196-201: The endpoint currently calls
storage_backend.test_connection() synchronously inside an async handler (in
admin_executor.py), which blocks the event loop; change the call to run off the
event loop by executing storage_backend.test_connection() in a worker thread
(e.g., via asyncio.to_thread(...) or loop.run_in_executor(...)) and await the
result, then return the same dict using the awaited success value; ensure you
reference storage_backend.test_connection() and the async handler in
admin_executor.py when making the change.
- Around line 39-207: These async executor entry points are missing
OpenTelemetry tracing; add the decorator import and annotate each async method
with `@trace_async`(span_name="<method_name>",
tracer_name="knowledge_runtime.services.admin") where <method_name> is a
readable span name (e.g., "delete_document_index", "purge_knowledge_index",
"drop_knowledge_index", "list_chunks", "test_connection"); import trace_async
from shared.telemetry.decorators and place the decorator immediately above the
async def for delete_document_index, purge_knowledge_index,
drop_knowledge_index, list_chunks, and test_connection so each method is traced.
In `@knowledge_runtime/knowledge_runtime/services/content_fetcher.py`:
- Around line 99-126: The presigned URL is being logged and included in error
details potentially with credentials; before any logging or attaching to
ContentFetchError use a sanitized URL (strip query and fragment) and a truncated
safe preview (e.g., safe_url = url.split('?',1)[0].split('#',1)[0] then take
safe_url[:100]) and use that in logger.debug and in error details instead of
url[:100] or exception strings; update the fetch method that calls
_extract_filename_from_url, the httpx exception handlers (HTTPStatusError and
TransportError), and any other return/raise sites to reference the sanitized
safe_url and avoid including raw exception text that can contain the full URL.
- Around line 56-61: The retry decorator on fetch() currently only matches
httpx.TransportError so it never triggers because _fetch_from_presigned_url()
and _fetch_from_backend_stream() catch TransportError and re-raise
ContentFetchError(retryable=True); change the decorator predicate to inspect the
exception's retryable attribute instead (e.g., use retry_if_exception(lambda e:
getattr(e, "retryable", False))) so retries occur when
ContentFetchError.retryable is True while keeping reraise=True and the same
stop/wait settings; reference functions: fetch, _fetch_from_presigned_url,
_fetch_from_backend_stream and exception class ContentFetchError.
In `@knowledge_runtime/knowledge_runtime/services/index_executor.py`:
- Around line 75-78: The current logger.info call in index_executor.py logs raw
source_file and request.index_owner_user_id which may contain PII; update the
logger.info usage in the function that performs indexing (look for the
logger.info call referencing source_file and request.index_owner_user_id) to
avoid raw filenames and user IDs — log only non-sensitive identifiers like
request.knowledge_base_id and either omit or redact/hash the filename and user
id (e.g., replace source_file with a basename or a fixed token like "<redacted>"
or a short hash, and replace request.index_owner_user_id with a truncated/hash
value or "<redacted_user>"), and ensure the message still provides context but
never writes full PII to the logs.
In `@knowledge_runtime/knowledge_runtime/services/query_executor.py`:
- Around line 64-67: The logger currently includes raw user content via
request.query in the logger.info call inside the query executor; remove the raw
snippet and instead log non-sensitive identifiers—e.g., log the query length
(len(request.query)), a request ID if available (request.id or
request.request_id), or a non-reversible hash of the query (compute
sha256(request.query) and log its hex) together with total_results and returned
counts; update the logger.info call referenced (the logger.info call that
currently formats request.query[:50]) to use one of these non-sensitive values
and keep total_results=len(all_records) and returned=len(limited_records) in the
message.
In `@knowledge_runtime/tests/test_admin_executor.py`:
- Line 7: Tests use AsyncMock for callables passed into asyncio.to_thread which
expects a synchronous callable; AsyncMock returns a coroutine when called and
breaks assertions. In knowledge_runtime/tests/test_admin_executor.py replace
AsyncMock instances used for storage/backend methods invoked via
asyncio.to_thread with MagicMock (for example the mocks set up around the calls
referenced at lines ~57, 90, 122, 149, 191, 256), set their return_value to the
expected dict/response, and keep AsyncMock only for actual async functions;
ensure any assertions access the mock.return_value directly (not awaited).
---
Minor comments:
In `@docs/plans/2026-04-19-knowledge-runtime-architecture.md`:
- Around line 141-150: Update the "认证机制" section that currently requires
"Authorization: Bearer <INTERNAL_SERVICE_TOKEN>" to reflect the implemented
contract: remove the stale INTERNAL_SERVICE_TOKEN requirement and either state
that Knowledge Runtime internal communication is unauthenticated or document the
actual replacement control mechanism used (e.g., mTLS, IP allowlist, signed
requests) so the doc matches implementation; locate the "认证机制" header and the
Authorization example and replace or remove that snippet and accompanying
sentence accordingly.
- Line 34: Several fenced code blocks use plain ``` without a language, so add
the language identifier "text" to each triple-backtick fence (replace ``` with
```text) for the three diagram/flow blocks shown and the other occurrences noted
(the additional fenced blocks at the other two locations). Locate the
triple-backtick fences (``` ... ```) and change their opening fences to ```text
while leaving the block contents unchanged.
- Around line 1-5: This new Chinese doc currently in docs/plans/ should be moved
into the repo's docs/zh/ structure and paired with an English counterpart in
docs/en/; rename or relocate the file under docs/zh/ (preserve the YAML
frontmatter and the heading "Knowledge Runtime 架构设计"), then create an English
file in docs/en/ with the same frontmatter and an English title like "Knowledge
Runtime Architecture" and translated content, ensuring both files follow the
docs/**/*.md convention and keep internal links and sidebar_position consistent.
In `@knowledge_runtime/knowledge_runtime/api/endpoints/index.py`:
- Around line 19-35: The public async endpoint index_document is missing
OpenTelemetry tracing; wrap it with the `@trace_async` decorator (import it from
your tracing helper) and add span events/attributes around the work: before
calling IndexExecutor().execute add_span_event indicating
"index_request_received" and set_span_attribute values such as "doc_ref" and
"chunk_count" (or any request fields available), then call
executor.execute(request) and add a final add_span_event like "index_completed"
and set_span_attribute for result status; ensure imports for trace_async,
add_span_event, and set_span_attribute are added and that the IndexExecutor
usage remains unchanged.
In `@knowledge_runtime/knowledge_runtime/api/endpoints/query.py`:
- Around line 17-33: The query_documents endpoint is missing OpenTelemetry
tracing; add the `@trace_async` decorator to the async function query_documents
and create span events/attributes inside the function (use add_span_event and
set_span_attribute on the current span) to record key details such as the
incoming request.query, number of KB configs in request.kb_configs, and any
high-level steps (e.g., "create_backends_start", "execute_query_start"); ensure
the decorator import names match the project's tracing utilities and leave
QueryExecutor().execute(request) invocation intact so the trace wraps the entire
operation.
In `@knowledge_runtime/knowledge_runtime/core/logging.py`:
- Around line 115-151: Existing logger handlers are being cleared without
closing them, which can leak file descriptors; before calling handlers.clear()
on root_logger and on the "uvicorn.access" logger, iterate their .handlers and
call .close() for each handler, then clear the list; apply this in both the
file-logging branch and the console branch (references: root_logger,
access_logger, RequestIdFilter, and _create_file_handler) so all previous
handlers are properly closed before being removed or replaced.
In `@knowledge_runtime/knowledge_runtime/services/content_fetcher.py`:
- Around line 213-224: The Content-Disposition parsing is brittle: update the
code that reads response.headers.get("Content-Disposition") to robustly handle
RFC5987 filename* and strip trailing parameters instead of naïve splitting;
first look for a filename* param and decode per RFC5987 (parse the
"charset'lang'encoded" form and urllib.parse.unquote the value), fall back to
parsing the header with cgi.parse_header (or email.message.Message().get_param)
to extract the filename param safely, then call a helper
_split_filename_extension(filename: str) to derive and return the (filename,
extension) tuple; ensure you return filename and extension only after decoding
and trimming surrounding quotes.
In `@knowledge_runtime/knowledge_runtime/services/query_executor.py`:
- Around line 159-170: Extract the magic number by adding a module-level
constant (e.g., CHARS_PER_TOKEN = 4) and update the _estimate_tokens method to
use it and never return 0 for non-empty input (compute tokens = len(text) //
CHARS_PER_TOKEN and return max(1, tokens) for non-empty text, returning 0 only
for empty input); reference the _estimate_tokens function and the new
CHARS_PER_TOKEN constant when making the change.
- Around line 36-170: Add OpenTelemetry tracing decorators to the QueryExecutor
methods: import trace_async and trace_sync from shared.telemetry.decorators,
then annotate async methods execute and _query_knowledge_base with `@trace_async`
and annotate synchronous helpers _extract_document_id and _estimate_tokens with
`@trace_sync`; ensure the import is added at the top of the module and that the
decorator names exactly match trace_async and trace_sync so the decorators apply
to KnowledgeRuntimeQueryExecutor.execute, _query_knowledge_base,
_extract_document_id, and _estimate_tokens.
In `@knowledge_runtime/pyproject.toml`:
- Line 9: Update the Python version specifier in pyproject.toml to use an
exclusive upper bound for the next minor release: change the requires-python
value from ">=3.10,<=3.13" to ">=3.10,<3.14" so all Python 3.13.x patch releases
are accepted; locate and edit the requires-python entry in the pyproject.toml
file to make this replacement.
In `@knowledge_runtime/tests/test_content_fetcher.py`:
- Around line 147-156: The test only asserts the final wrapped ContentFetchError
but doesn't verify the number of retry attempts; capture the mocked get
AsyncMock from the patched httpx.AsyncClient (e.g., assign
mock_client.return_value.__aenter__.return_value.get to a variable like
mock_get), set its side_effect to httpx.ConnectError, run await
content_fetcher.fetch(content_ref) inside the pytest.raises block, and then
assert the mock_get was awaited the expected number of times (e.g.,
mock_get.await_count or mock_get.call_count equals 3) to ensure the retry
decorator actually retried three times before raising ContentFetchError.
In `@knowledge_runtime/tests/test_logging.py`:
- Around line 66-206: Add an autouse pytest fixture (e.g., isolate_logging or
restore_logging) that snapshots global logging state before each test and
restores it after: capture root_logger.handlers (and any handlers on loggers
used in tests like "uvicorn", "uvicorn.access", "fastapi", "httpx", "httpcore"),
their levels and propagate flags, and the open file handlers; in teardown, close
any newly-created handlers, reattach the original handlers to the same loggers,
and restore original levels/propagate flags so tests like
test_setup_logging_console_mode, test_setup_logging_file_mode,
test_setup_logging_respects_log_level, test_setup_logging_creates_log_directory,
test_setup_logging_suppresses_third_party_logs,
test_setup_logging_access_log_separate_file and
test_setup_logging_access_log_console_mode run isolated; implement this fixture
in the test module (or conftest.py) with `@pytest.fixture`(autouse=True) and
ensure you call handler.close() for file handlers before removing them.
In `@start.sh`:
- Around line 1519-1521: The current call to sync_python_deps when
start_knowledge_runtime is true only probes `import shared` and can skip syncing
if the venv exists even though `knowledge_engine` is missing or stale; update
the call site that checks `start_knowledge_runtime` to invoke sync_python_deps
with the Knowledge Runtime-specific module list (include `knowledge_engine` in
the probe list along with `shared`) so the function verifies both `shared` and
`knowledge_engine` before deciding to skip the `uv sync`.
---
Nitpick comments:
In `@knowledge_runtime/knowledge_runtime/api/router.py`:
- Around line 15-21: Unify the prefix handling by including health.router with
the same prefix as the other RAG routers (use
router.include_router(health.router, prefix="/internal/rag", tags=["health"]))
and update the route declaration in endpoints/health.py from
`@router.get`("/internal/rag/health") to `@router.get`("/health"); this keeps the
health endpoint under the shared base path while preserving its tag and makes
future base-path changes simpler and consistent with index.router and
query.router usage.
In `@knowledge_runtime/tests/conftest.py`:
- Around line 10-31: Move the repeated local imports of MagicMock out of the
fixtures by adding a single top-level import "from unittest.mock import
MagicMock" at the module level and remove the inline imports inside the
mock_storage_backend and mock_embed_model fixture functions so both fixtures
simply reference MagicMock; ensure no other references rely on the local import
and run formatter/isort to satisfy project style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 621b324d-01d4-4ce3-90c8-56af2f1fed06
⛔ Files ignored due to path filters (1)
knowledge_runtime/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.env.exampleAGENTS.mdbackend/app/core/config.pybackend/app/services/rag/gateway_factory.pybackend/app/services/rag/remote_gateway.pybackend/app/services/rag/retrieval_service.pydocker-compose.build.ymldocker-compose.ymldocker/knowledge_runtime/Dockerfiledocs/plans/2026-04-19-knowledge-runtime-architecture.mdknowledge_runtime/knowledge_runtime/__init__.pyknowledge_runtime/knowledge_runtime/api/__init__.pyknowledge_runtime/knowledge_runtime/api/endpoints/__init__.pyknowledge_runtime/knowledge_runtime/api/endpoints/admin.pyknowledge_runtime/knowledge_runtime/api/endpoints/health.pyknowledge_runtime/knowledge_runtime/api/endpoints/index.pyknowledge_runtime/knowledge_runtime/api/endpoints/query.pyknowledge_runtime/knowledge_runtime/api/router.pyknowledge_runtime/knowledge_runtime/config.pyknowledge_runtime/knowledge_runtime/core/__init__.pyknowledge_runtime/knowledge_runtime/core/logging.pyknowledge_runtime/knowledge_runtime/main.pyknowledge_runtime/knowledge_runtime/middleware/__init__.pyknowledge_runtime/knowledge_runtime/services/__init__.pyknowledge_runtime/knowledge_runtime/services/admin_executor.pyknowledge_runtime/knowledge_runtime/services/content_fetcher.pyknowledge_runtime/knowledge_runtime/services/index_executor.pyknowledge_runtime/knowledge_runtime/services/query_executor.pyknowledge_runtime/pyproject.tomlknowledge_runtime/tests/__init__.pyknowledge_runtime/tests/conftest.pyknowledge_runtime/tests/test_admin_executor.pyknowledge_runtime/tests/test_content_fetcher.pyknowledge_runtime/tests/test_index_executor.pyknowledge_runtime/tests/test_logging.pyknowledge_runtime/tests/test_query_executor.pyshared/models/__init__.pystart.sh
💤 Files with no reviewable changes (2)
- backend/app/core/config.py
- backend/app/services/rag/remote_gateway.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/services/rag/test_retrieval_service.py`:
- Around line 110-134: The test is too loose by using ANY for the db argument;
instead create a concrete MagicMock (e.g., db_session = MagicMock()) and pass it
into RetrievalService().get_all_chunks_from_knowledge_base, then assert the
exact same object was forwarded: replace
mock_build_spec.assert_called_once_with(..., db=ANY,...) with
mock_build_spec.assert_called_once_with(..., db=db_session, ...), and replace
mock_gateway.list_chunks.assert_awaited_once_with(spec, db=ANY) with
mock_gateway.list_chunks.assert_awaited_once_with(spec, db=db_session); keep the
same call to mock_get_gateway.assert_called_once() and ensure the function under
test is RetrievalService().get_all_chunks_from_knowledge_base.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2e95d73-0bce-4e84-99a8-c697b5a6eacc
📒 Files selected for processing (2)
backend/tests/services/rag/test_remote_gateway.pybackend/tests/services/rag/test_retrieval_service.py
💤 Files with no reviewable changes (1)
- backend/tests/services/rag/test_remote_gateway.py
kissghosts
reviewed
Apr 20, 2026
kissghosts
reviewed
Apr 20, 2026
- 添加认证中间件 verify_internal_token，支持常量时间比较 - 为 RAG 操作端点添加认证依赖（health 除外） - Backend RemoteRagGateway 添加 Authorization header 支持 - 添加 KNOWLEDGE_RUNTIME_TOKEN 配置项 - Docker Compose 添加环境变量配置（默认注释） - 添加 knowledge_runtime 和 Backend 认证测试
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
knowledge_runtime/knowledge_runtime/config.py (1)
22-23:⚠️ Potential issue | 🟠 MajorDefault host
0.0.0.0and genericHOST/PORTenv var names persist.Two concerns here (previously raised, not yet addressed):
- Binding to
0.0.0.0by default exposes an internal-only RAG service on all interfaces. Prefer127.0.0.1and require explicit override (e.g., via the container/compose config) for deployments that need external binding.- The field names
hostandportmap to env varsHOSTandPORT— generic names that will collide with arbitrary shell/CI environments. Addvalidation_alias=AliasChoices("KNOWLEDGE_RUNTIME_HOST", "HOST")(and same for port) to match theKNOWLEDGE_RUNTIME_*scheme used bydocker-compose.yml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/config.py` around lines 22 - 23, Change the default binding and env-var aliases for the host/port fields in knowledge_runtime/config.py: set the default host value from "0.0.0.0" to "127.0.0.1" and add validation_alias=AliasChoices("KNOWLEDGE_RUNTIME_HOST","HOST") to the host field, and add validation_alias=AliasChoices("KNOWLEDGE_RUNTIME_PORT","PORT") to the port field so they map to KNOWLEDGE_RUNTIME_* env vars (with legacy HOST/PORT fallback); update the host and port field definitions (host, port) accordingly.
🧹 Nitpick comments (1)
knowledge_runtime/start.sh (1)
244-248: Redundant exit-code check underset -e.
set -eat line 10 already aborts on non-zero exit fromuv sync, so the$?check never runs. Either dropset -earound this block or remove the redundant guard (shellcheck SC2181).🔧 Suggested fix
-uv sync -if [ $? -ne 0 ]; then - echo -e "${RED}Error: Failed to install dependencies${NC}" - exit 1 -fi +if ! uv sync; then + echo -e "${RED}Error: Failed to install dependencies${NC}" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/start.sh` around lines 244 - 248, The guard checking "$?" after running the "uv sync" command is redundant because "set -e" (declared earlier at line 10) will already abort on any non-zero exit; remove the if-block that tests "$?" and its echo/exit or alternatively temporarily disable "set -e" around the "uv sync" invocation (e.g., set +e; uv sync; rc=$?; set -e) and then keep the existing error handling — pick one approach and apply it consistently in the script around the "uv sync" call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/services/rag/test_remote_gateway.py`:
- Around line 731-772: The test test_gateway_no_auth_header_when_token_empty is
not self-contained because RemoteRagGateway.__init__ falls back to
settings.INTERNAL_SERVICE_TOKEN when auth_token is falsy; update the test to
explicitly set settings.INTERNAL_SERVICE_TOKEN to an empty string (and restore
it if needed) before constructing RemoteRagGateway so the behavior is
deterministic: locate the test function
test_gateway_no_auth_header_when_token_empty and add a controlled assignment to
settings.INTERNAL_SERVICE_TOKEN (or use pytest monkeypatch to set it to "")
prior to instantiating RemoteRagGateway to guarantee the gateway uses an empty
token.
In `@knowledge_runtime/start.sh`:
- Around line 277-280: The PROJECT_ROOT assignment currently uses the caller's
CWD (PROJECT_ROOT=$(cd .. && pwd)) which breaks when the script is executed from
outside knowledge_runtime; change it to compute the script directory (e.g.,
derive SCRIPT_DIR from the script's location using ${BASH_SOURCE[0]} and
dirname) and then set PROJECT_ROOT relative to that SCRIPT_DIR (SCRIPT_DIR/..),
then export PYTHONPATH using that PROJECT_ROOT; also consider adding a cd
"$SCRIPT_DIR" near the top so subsequent checks (the pyproject.toml check and
the .venv/bin/python launch) run relative to the script location.
- Around line 49-52: The --python flag sets PYTHON_PATH/PYTHON_EXEC but is never
passed to uv, so the virtualenv is created with the system default; update the
script to propagate the user-provided PYTHON_PATH into the uv commands (e.g.,
add --python "$PYTHON_PATH" to the uv venv and uv sync invocations and any uv
run/launch command that creates or syncs the .venv) so the venv is created with
the requested interpreter and the server uses that venv; alternatively, remove
the --python flag and related validation if you prefer not to support overrides.
---
Duplicate comments:
In `@knowledge_runtime/knowledge_runtime/config.py`:
- Around line 22-23: Change the default binding and env-var aliases for the
host/port fields in knowledge_runtime/config.py: set the default host value from
"0.0.0.0" to "127.0.0.1" and add
validation_alias=AliasChoices("KNOWLEDGE_RUNTIME_HOST","HOST") to the host
field, and add validation_alias=AliasChoices("KNOWLEDGE_RUNTIME_PORT","PORT") to
the port field so they map to KNOWLEDGE_RUNTIME_* env vars (with legacy
HOST/PORT fallback); update the host and port field definitions (host, port)
accordingly.
---
Nitpick comments:
In `@knowledge_runtime/start.sh`:
- Around line 244-248: The guard checking "$?" after running the "uv sync"
command is redundant because "set -e" (declared earlier at line 10) will already
abort on any non-zero exit; remove the if-block that tests "$?" and its
echo/exit or alternatively temporarily disable "set -e" around the "uv sync"
invocation (e.g., set +e; uv sync; rc=$?; set -e) and then keep the existing
error handling — pick one approach and apply it consistently in the script
around the "uv sync" call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8487427d-636e-48ad-abe4-4f60a997a353
📒 Files selected for processing (11)
backend/.env.examplebackend/app/core/config.pybackend/app/services/rag/remote_gateway.pybackend/tests/services/rag/test_remote_gateway.pydocker-compose.ymlknowledge_runtime/.env.exampleknowledge_runtime/knowledge_runtime/api/router.pyknowledge_runtime/knowledge_runtime/config.pyknowledge_runtime/knowledge_runtime/middleware/auth.pyknowledge_runtime/start.shknowledge_runtime/tests/test_auth.py
✅ Files skipped from review due to trivial changes (2)
- backend/.env.example
- backend/app/core/config.py
🚧 Files skipped from review as they are similar to previous changes (3)
- knowledge_runtime/knowledge_runtime/api/router.py
- backend/app/services/rag/remote_gateway.py
- docker-compose.yml
Comment on lines
+49
to
+52
| --python) | ||
| PYTHON_PATH="$2" | ||
| shift 2 | ||
| ;; |
Contributor
There was a problem hiding this comment.
--python flag is parsed and validated but never affects the venv.
PYTHON_EXEC is used only for version validation and an echo. uv venv and uv sync (lines 240, 244) are invoked without --python "$PYTHON_EXEC", so the venv is created with whatever Python uv auto-detects — not the user's override. The server is then launched from .venv/bin/python (line 290). Effectively, --python is a no-op.
Either pass it through, or drop the flag to avoid misleading users:
🔧 Suggested fix
-if [ ! -d ".venv" ]; then
- echo "Creating virtual environment..."
- uv venv
-fi
+if [ ! -d ".venv" ]; then
+ echo "Creating virtual environment..."
+ uv venv --python "$PYTHON_EXEC"
+fi
-uv sync
+uv sync --python "$PYTHON_EXEC"Also applies to: 154-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@knowledge_runtime/start.sh` around lines 49 - 52, The --python flag sets
PYTHON_PATH/PYTHON_EXEC but is never passed to uv, so the virtualenv is created
with the system default; update the script to propagate the user-provided
PYTHON_PATH into the uv commands (e.g., add --python "$PYTHON_PATH" to the uv
venv and uv sync invocations and any uv run/launch command that creates or syncs
the .venv) so the venv is created with the requested interpreter and the server
uses that venv; alternatively, remove the --python flag and related validation
if you prefer not to support overrides.
- 将 KNOWLEDGE_RUNTIME_TOKEN 重命名为 INTERNAL_SERVICE_TOKEN - Backend/knowledge_runtime/chat_shell 统一使用同一 token 进行服务间认证 - 更新 docker-compose.yml 和 .env.example 中的配置示例 - 更新相关测试用例
sunnights
force-pushed
the
zy_knowledge_runtime
branch
from
April 21, 2026 01:47
e926bd6 to
58e573f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
backend/tests/services/rag/test_remote_gateway.py (1)
731-772:⚠️ Potential issue | 🟡 MinorMake the empty-token test deterministic.
auth_token=""can still fall back tosettings.INTERNAL_SERVICE_TOKEN, so this test depends on ambient settings state unless it pins that setting.Suggested fix
-async def test_gateway_no_auth_header_when_token_empty(mocker) -> None: +async def test_gateway_no_auth_header_when_token_empty(mocker, monkeypatch) -> None: """Test that gateway does not add Authorization header when token is empty.""" + monkeypatch.setattr(settings, "INTERNAL_SERVICE_TOKEN", "") post_mock = mocker.patch( @@ - args, kwargs = post_mock.await_args + _, kwargs = post_mock.await_args🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/services/rag/test_remote_gateway.py` around lines 731 - 772, The test test_gateway_no_auth_header_when_token_empty is non-deterministic because RemoteRagGateway(auth_token="") may pick up settings.INTERNAL_SERVICE_TOKEN; make the test deterministic by explicitly overriding or monkeypatching the INTERNAL_SERVICE_TOKEN to an empty value before creating the gateway (or pass a non-empty sentinel) so that RemoteRagGateway(auth_token="") cannot fall back to settings.INTERNAL_SERVICE_TOKEN; update the test to set the setting (via monkeypatch or the test settings fixture) and then assert headers are {} after calling gateway.query(spec).knowledge_runtime/knowledge_runtime/config.py (1)
21-23:⚠️ Potential issue | 🟠 MajorUse loopback as the safe default host.
This is an internal service and auth is disabled when the token is unset, so direct runs should not bind to all interfaces by default. Container deployments can still override
HOST=0.0.0.0.Suggested fix
# Server configuration - host: str = "0.0.0.0" + host: str = "127.0.0.1" port: int = 8200🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/config.py` around lines 21 - 23, Change the default server bind address from "0.0.0.0" to the loopback address to avoid exposing an internal service by default: update the host default (the host variable in knowledge_runtime.config) to "127.0.0.1" (or "localhost") while leaving port and any environment-based overrides intact so containerized deployments can still set HOST=0.0.0.0.knowledge_runtime/start.sh (2)
10-10:⚠️ Potential issue | 🟠 MajorResolve paths from the script location, not the caller’s CWD.
Running
./knowledge_runtime/start.shfrom the repo root currently checks the wrongpyproject.tomllocation and setsPROJECT_ROOTto the parent of the repo.Suggested fix
set -e + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(dirname "$SCRIPT_DIR")" +cd "$SCRIPT_DIR" @@ # Set PYTHONPATH to include project root -PROJECT_ROOT=$(cd .. && pwd) -export PYTHONPATH="${PYTHONPATH}:${PROJECT_ROOT}" +export PYTHONPATH="${PYTHONPATH:+${PYTHONPATH}:}${PROJECT_ROOT}" echo -e "${GREEN}✓ PYTHONPATH set to include: $PROJECT_ROOT${NC}"Also applies to: 232-235, 256-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/start.sh` at line 10, The script currently computes PROJECT_ROOT from the caller CWD which causes wrong paths; change the path resolution to be relative to the script file itself by computing the script directory (use ${BASH_SOURCE[0]} or $0) and deriving PROJECT_ROOT from that (e.g., script_dir -> parent -> project root) and then use those variables wherever the script reads pyproject.toml or other files (update uses around PROJECT_ROOT and any blocks referenced at lines handling environment setup, PROJECT_ROOT assignment, and the sections around 232-235 and 256-280). Ensure all path joins use the computed script_dir/project_root so the script behaves the same regardless of the current working directory.
49-52:⚠️ Potential issue | 🟡 MinorHonor
--pythonthrough uv and launch withuv run.
--pythonis currently only validated;uv venv,uv sync, and the final.venv/bin/pythonlaunch can still use a different interpreter. Also, this module should execute Python through uv. Based on learnings, All Python modules useuvfor dependency management - always useuv runto execute Python commands.Suggested fix
if [ ! -d ".venv" ]; then echo "Creating virtual environment..." - uv venv + uv venv --python "$PYTHON_EXEC" fi # Sync dependencies -uv sync +uv sync --python "$PYTHON_EXEC" @@ # Start uvicorn -.venv/bin/python -m uvicorn knowledge_runtime.main:app --host "$HOST" --port "$PORT" --reload +uv run --python "$PYTHON_EXEC" python -m uvicorn knowledge_runtime.main:app --host "$HOST" --port "$PORT" --reloadAlso applies to: 237-244, 289-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/start.sh` around lines 49 - 52, The --python flag currently only sets PYTHON_PATH but that value isn't forwarded to uv commands or used to launch Python; update the script so every uv invocation that creates or uses the venv (uv venv, uv sync) and the final execution use uv with the --python "$PYTHON_PATH" option when PYTHON_PATH is set, and replace direct calls to ".venv/bin/python" with "uv run --python \"$PYTHON_PATH\" -- <your-python-command>" (omit the --python argument when PYTHON_PATH is empty) so all Python actions honor the provided interpreter; refer to the PYTHON_PATH variable and the commands "uv venv", "uv sync", and the final launch (currently using .venv/bin/python) to locate where to apply these changes.
🧹 Nitpick comments (1)
knowledge_runtime/.env.example (1)
13-16: Keep dotenv keys in linter order.
dotenv-linterexpectsLOG_DIRbeforeLOG_FILE_ENABLED.Suggested cleanup
# Logging configuration -LOG_FILE_ENABLED=true LOG_DIR=./logs +LOG_FILE_ENABLED=true LOG_LEVEL=INFO🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/.env.example` around lines 13 - 16, The .env.example keys are out of order for dotenv-linter: move the LOG_DIR entry so it appears before LOG_FILE_ENABLED (i.e., reorder the LOG_* block to LOG_DIR then LOG_FILE_ENABLED) in the knowledge_runtime/.env.example file; update any nearby comment grouping if needed and save so the variables LOG_DIR and LOG_FILE_ENABLED follow the linter-expected order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/services/rag/test_remote_gateway.py`:
- Around line 726-728: The tests unpacking post_mock.await_args currently bind
an unused variable as args; change those unpackings to use a throwaway variable
(e.g., "_, kwargs = post_mock.await_args") so Ruff won't flag the unused name.
Update every occurrence in the test file where you have "args, kwargs =
post_mock.await_args" (including the instance around the Authorization assertion
and the other occurrence mentioned) to "_, kwargs = post_mock.await_args".
In `@docker-compose.yml`:
- Around line 116-120: The docker-compose should wire the INTERNAL_SERVICE_TOKEN
into both services and stop publishing the knowledge_runtime port by default:
add the same INTERNAL_SERVICE_TOKEN environment entry to the backend (or rag)
service and to the knowledge_runtime service (use the existing
INTERNAL_SERVICE_TOKEN variable), ensure KNOWLEDGE_RUNTIME_URL remains the
internal address (http://knowledge_runtime:8200) so backend calls go over the
compose network, and remove or move the ports mapping exposing 8200 (the
host-facing "8200:8200" mapping) from the knowledge_runtime service so the
runtime is not published by default (or gate it behind a profile).
In `@knowledge_runtime/knowledge_runtime/middleware/auth.py`:
- Around line 17-23: Ruff B008 is raised because Depends(...) is used as a
default value inside verify_internal_token; instead create a module-level
dependency object and use that in the function signature. Define a top-level
name (e.g., security_dependency = Depends(security)) next to the existing
security = HTTPBearer(...) and then change verify_internal_token to accept
credentials: Optional[HTTPAuthorizationCredentials] = security_dependency so the
Depends call is not inside the function default.
In `@knowledge_runtime/start.sh`:
- Around line 29-32: Default host should be loopback to avoid exposing
unauthenticated internal endpoints; change DEFAULT_HOST from "0.0.0.0" to
"127.0.0.1" in the start.sh defaults and any other identical default blocks
(e.g., the block around lines 61-64) and ensure any uses of DEFAULT_HOST or
variables that seed bind addresses still reference that variable so the script
binds only to loopback by default.
---
Duplicate comments:
In `@backend/tests/services/rag/test_remote_gateway.py`:
- Around line 731-772: The test test_gateway_no_auth_header_when_token_empty is
non-deterministic because RemoteRagGateway(auth_token="") may pick up
settings.INTERNAL_SERVICE_TOKEN; make the test deterministic by explicitly
overriding or monkeypatching the INTERNAL_SERVICE_TOKEN to an empty value before
creating the gateway (or pass a non-empty sentinel) so that
RemoteRagGateway(auth_token="") cannot fall back to
settings.INTERNAL_SERVICE_TOKEN; update the test to set the setting (via
monkeypatch or the test settings fixture) and then assert headers are {} after
calling gateway.query(spec).
In `@knowledge_runtime/knowledge_runtime/config.py`:
- Around line 21-23: Change the default server bind address from "0.0.0.0" to
the loopback address to avoid exposing an internal service by default: update
the host default (the host variable in knowledge_runtime.config) to "127.0.0.1"
(or "localhost") while leaving port and any environment-based overrides intact
so containerized deployments can still set HOST=0.0.0.0.
In `@knowledge_runtime/start.sh`:
- Line 10: The script currently computes PROJECT_ROOT from the caller CWD which
causes wrong paths; change the path resolution to be relative to the script file
itself by computing the script directory (use ${BASH_SOURCE[0]} or $0) and
deriving PROJECT_ROOT from that (e.g., script_dir -> parent -> project root) and
then use those variables wherever the script reads pyproject.toml or other files
(update uses around PROJECT_ROOT and any blocks referenced at lines handling
environment setup, PROJECT_ROOT assignment, and the sections around 232-235 and
256-280). Ensure all path joins use the computed script_dir/project_root so the
script behaves the same regardless of the current working directory.
- Around line 49-52: The --python flag currently only sets PYTHON_PATH but that
value isn't forwarded to uv commands or used to launch Python; update the script
so every uv invocation that creates or uses the venv (uv venv, uv sync) and the
final execution use uv with the --python "$PYTHON_PATH" option when PYTHON_PATH
is set, and replace direct calls to ".venv/bin/python" with "uv run --python
\"$PYTHON_PATH\" -- <your-python-command>" (omit the --python argument when
PYTHON_PATH is empty) so all Python actions honor the provided interpreter;
refer to the PYTHON_PATH variable and the commands "uv venv", "uv sync", and the
final launch (currently using .venv/bin/python) to locate where to apply these
changes.
---
Nitpick comments:
In `@knowledge_runtime/.env.example`:
- Around line 13-16: The .env.example keys are out of order for dotenv-linter:
move the LOG_DIR entry so it appears before LOG_FILE_ENABLED (i.e., reorder the
LOG_* block to LOG_DIR then LOG_FILE_ENABLED) in the
knowledge_runtime/.env.example file; update any nearby comment grouping if
needed and save so the variables LOG_DIR and LOG_FILE_ENABLED follow the
linter-expected order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7834840-d078-4c14-a524-d53c387fb318
📒 Files selected for processing (10)
backend/.env.examplebackend/app/core/config.pybackend/app/services/rag/remote_gateway.pybackend/tests/services/rag/test_remote_gateway.pydocker-compose.ymlknowledge_runtime/.env.exampleknowledge_runtime/knowledge_runtime/config.pyknowledge_runtime/knowledge_runtime/middleware/auth.pyknowledge_runtime/start.shknowledge_runtime/tests/test_auth.py
✅ Files skipped from review due to trivial changes (1)
- backend/app/core/config.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/.env.example
- backend/app/services/rag/remote_gateway.py
- knowledge_runtime/tests/test_auth.py
Comment on lines
+726
to
+728
| args, kwargs = post_mock.await_args | ||
| assert "headers" in kwargs | ||
| assert kwargs["headers"]["Authorization"] == "Bearer test-auth-token-123" |
Contributor
There was a problem hiding this comment.
Drop unused args unpacking in the new auth tests.
Ruff flags these unpacked variables; use _ when only kwargs is asserted.
Suggested fix
- args, kwargs = post_mock.await_args
+ _, kwargs = post_mock.await_args
assert "headers" in kwargs
@@
- args, kwargs = post_mock.await_args
+ _, kwargs = post_mock.await_args
assert kwargs["headers"]["Authorization"] == "Bearer settings-token-456"Also applies to: 818-819
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 726-726: Unpacked variable args is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/services/rag/test_remote_gateway.py` around lines 726 - 728,
The tests unpacking post_mock.await_args currently bind an unused variable as
args; change those unpackings to use a throwaway variable (e.g., "_, kwargs =
post_mock.await_args") so Ruff won't flag the unused name. Update every
occurrence in the test file where you have "args, kwargs = post_mock.await_args"
(including the instance around the Authorization assertion and the other
occurrence mentioned) to "_, kwargs = post_mock.await_args".
Comment on lines
+17
to
+23
| # HTTPBearer security scheme for OpenAPI documentation | ||
| security = HTTPBearer(auto_error=False) | ||
|
|
||
|
|
||
| def verify_internal_token( | ||
| credentials: Optional[HTTPAuthorizationCredentials] = Depends(security), | ||
| ) -> None: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the auth.py file
fd "auth.py" --type f | grep -v __pycache__Repository: wecode-ai/Wegent
Length of output: 372
🏁 Script executed:
# Look at the actual content around lines 17-23
cat -n knowledge_runtime/knowledge_runtime/middleware/auth.py | head -35Repository: wecode-ai/Wegent
Length of output: 1355
🏁 Script executed:
# Check if there's a Ruff configuration in the project
find . -maxdepth 3 -type f \( -name "ruff.toml" -o -name "pyproject.toml" -o -name ".ruff.toml" \) | head -5Repository: wecode-ai/Wegent
Length of output: 206
🏁 Script executed:
# Check FastAPI version and dependencies
find . -maxdepth 2 -type f \( -name "pyproject.toml" -o -name "requirements.txt" -o -name "uv.lock" \) | xargs grep -l "fastapi" 2>/dev/null | head -3Repository: wecode-ai/Wegent
Length of output: 130
🏁 Script executed:
# Check the Ruff configuration in knowledge_runtime project
cat knowledge_runtime/pyproject.toml 2>/dev/null || echo "No pyproject.toml in knowledge_runtime root"Repository: wecode-ai/Wegent
Length of output: 1450
🏁 Script executed:
# Check backend pyproject for FastAPI version and Ruff config
cat backend/pyproject.tomlRepository: wecode-ai/Wegent
Length of output: 5828
🏁 Script executed:
# Verify if Ruff B008 is actually enabled and if it would flag this pattern
grep -A 20 "\[tool.ruff" backend/pyproject.tomlRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
# Check if Ruff is mentioned anywhere in the project
rg "ruff" --type toml -iRepository: wecode-ai/Wegent
Length of output: 113
🏁 Script executed:
# Check if there's a ruff.toml or .ruff.toml file anywhere
find . -name "*ruff*" -type f 2>/dev/nullRepository: wecode-ai/Wegent
Length of output: 71
🏁 Script executed:
# Look at the actual function implementation to see if there are any other issues
cat -n knowledge_runtime/knowledge_runtime/middleware/auth.py | head -50Repository: wecode-ai/Wegent
Length of output: 1955
🏁 Script executed:
# Check FastAPI documentation by searching for Depends usage patterns in other files
rg "Depends\(" knowledge_runtime --type py -B 2 -A 2 | head -40Repository: wecode-ai/Wegent
Length of output: 1912
🏁 Script executed:
# Check the ruff.toml configuration to see if B008 is enabled
cat ruff.tomlRepository: wecode-ai/Wegent
Length of output: 1174
🏁 Script executed:
# Check if there are any other usages of Depends in function defaults with similar pattern
rg "def \w+\([^)]*= Depends\(" --type pyRepository: wecode-ai/Wegent
Length of output: 1056
🏁 Script executed:
# Check how Depends is used as dependencies in route definitions
rg "dependencies=\[Depends\(" knowledge_runtime --type py -A 1Repository: wecode-ai/Wegent
Length of output: 711
🏁 Script executed:
# Let's verify the actual behavior: does Depends() need to be called fresh each time?
# Check FastAPI documentation understanding
cat knowledge_runtime/tests/test_auth.pyRepository: wecode-ai/Wegent
Length of output: 2873
🏁 Script executed:
# Check if there are any pyproject.toml or ruff configs that ignore B008
rg "B008" . --type tomlRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
# Let's verify what B008 actually is and how it's typically handled in FastAPI
web_search "B008 flake8-bugbear FastAPI Depends function default"Repository: wecode-ai/Wegent
Length of output: 110
🏁 Script executed:
# Check if there are any noqa comments or ignores for B008 in the codebase
rg "noqa.*B008|B008.*noqa" . --type pyRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
# Let's also check how the function is actually used in the router
cat -n knowledge_runtime/knowledge_runtime/api/router.py | grep -A 2 -B 2 "verify_internal_token"Repository: wecode-ai/Wegent
Length of output: 713
🏁 Script executed:
# Let's search for information about B008 in FastAPI context
rg "B008" . --type py --no-headingRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
# Check if the knowledge_runtime project actually runs Ruff and if there are any linting issues reported
find knowledge_runtime -name "*.py" -path "*/middleware/auth.py" -exec head -1 {} \;Repository: wecode-ai/Wegent
Length of output: 102
🏁 Script executed:
# Let's verify the exact nature of B008 and FastAPI patterns with a practical check
cat << 'EOF' > /tmp/check_b008.py
# B008 is: "Do not perform function calls in argument defaults"
# FastAPI Depends is a special case - it's designed to be used in defaults
# This WOULD trigger B008:
# def func(x: int = some_function()):
# FastAPI pattern that triggers B008:
# def func(x: Type = Depends(dependency)):
# The suggested workaround:
# dep = Depends(dependency)
# def func(x: Type = dep):
print("B008 is about function calls in defaults.")
print("FastAPI Depends() is technically a function call, so it triggers B008.")
print("The suggested fix (module-level assignment) is a valid workaround.")
EOF
cat /tmp/check_b008.pyRepository: wecode-ai/Wegent
Length of output: 638
🏁 Script executed:
# Let's check if other FastAPI projects in the repo follow this pattern or have workarounds
rg "Depends\(" backend/app/api --type py -B 1 -A 1 | head -50Repository: wecode-ai/Wegent
Length of output: 3094
🏁 Script executed:
# Check if the knowledge_runtime project structure suggests it uses uv
ls -la knowledge_runtime/Repository: wecode-ai/Wegent
Length of output: 559
🏁 Script executed:
# Let's verify if B008 is actually being triggered by running ruff on this specific file
# First, check if ruff is available and try to run it
command -v ruff && ruff check knowledge_runtime/knowledge_runtime/middleware/auth.py --select B008 || echo "Ruff not available or no B008 issues found"Repository: wecode-ai/Wegent
Length of output: 794
🏁 Script executed:
# Let's check if there's a uv.lock to understand dependency management
ls -la knowledge_runtime/uv.lock 2>/dev/null && echo "Found uv.lock" || echo "No uv.lock in knowledge_runtime"Repository: wecode-ai/Wegent
Length of output: 150
🏁 Script executed:
# Verify FastAPI Depends behavior with module-level assignment in tests
cat knowledge_runtime/tests/test_auth.py | grep -A 20 "def test_app"Repository: wecode-ai/Wegent
Length of output: 639
Move the Depends call out of the function default.
Ruff B008 is flagged on line 22. A module-level dependency object preserves FastAPI behavior while satisfying the linter.
Suggested fix
# HTTPBearer security scheme for OpenAPI documentation
security = HTTPBearer(auto_error=False)
+internal_bearer_dependency = Depends(security)
def verify_internal_token(
- credentials: Optional[HTTPAuthorizationCredentials] = Depends(security),
+ credentials: Optional[HTTPAuthorizationCredentials] = internal_bearer_dependency,
) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # HTTPBearer security scheme for OpenAPI documentation | |
| security = HTTPBearer(auto_error=False) | |
| def verify_internal_token( | |
| credentials: Optional[HTTPAuthorizationCredentials] = Depends(security), | |
| ) -> None: | |
| # HTTPBearer security scheme for OpenAPI documentation | |
| security = HTTPBearer(auto_error=False) | |
| internal_bearer_dependency = Depends(security) | |
| def verify_internal_token( | |
| credentials: Optional[HTTPAuthorizationCredentials] = internal_bearer_dependency, | |
| ) -> None: |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 22-22: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@knowledge_runtime/knowledge_runtime/middleware/auth.py` around lines 17 - 23,
Ruff B008 is raised because Depends(...) is used as a default value inside
verify_internal_token; instead create a module-level dependency object and use
that in the function signature. Define a top-level name (e.g.,
security_dependency = Depends(security)) next to the existing security =
HTTPBearer(...) and then change verify_internal_token to accept credentials:
Optional[HTTPAuthorizationCredentials] = security_dependency so the Depends call
is not inside the function default.
- 修复 test_admin_executor 中 AsyncMock 错误使用，改用 MagicMock - 修复 start.sh 从任意目录执行时 PROJECT_ROOT 解析错误 - 配置项支持 KNOWLEDGE_RUNTIME_* 和简单名称两种环境变量 - 修复 content_fetcher 重试条件判断逻辑 - 为 get_list_chunks_gateway 添加 tracing 支持
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
knowledge_runtime/start.sh (2)
283-286: Leading-colon artifact whenPYTHONPATHis unset.
"${PYTHONPATH}:${PROJECT_ROOT}"expands to":${PROJECT_ROOT}"whenPYTHONPATHis not set in the environment, which Python interprets as an empty path entry (i.e., the current working directory). Use the conditional-expansion form to avoid this.🔧 Suggested fix
-export PYTHONPATH="${PYTHONPATH}:${PROJECT_ROOT}" +export PYTHONPATH="${PYTHONPATH:+${PYTHONPATH}:}${PROJECT_ROOT}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/start.sh` around lines 283 - 286, The PYTHONPATH assignment currently uses "${PYTHONPATH}:${PROJECT_ROOT}" which produces a leading colon when PYTHONPATH is unset; update the export to use shell conditional expansion so an empty PYTHONPATH does not insert an empty path entry (e.g., use the ${PYTHONPATH:+...} form) when setting PYTHONPATH with PROJECT_ROOT (referencing the existing PROJECT_ROOT and PYTHONPATH variables set in the script near SCRIPT_DIR).
250-254: Unreachable$?check underset -e.Line 10 sets
set -e, so ifuv syncfails the script exits immediately and lines 251-254 never execute. Either dropset -earound this block or use the idiomaticif ! uv sync; then ...pattern (also resolves Shellcheck SC2181).🔧 Suggested fix
-# Sync dependencies -uv sync -if [ $? -ne 0 ]; then - echo -e "${RED}Error: Failed to install dependencies${NC}" - exit 1 -fi +# Sync dependencies +if ! uv sync; then + echo -e "${RED}Error: Failed to install dependencies${NC}" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/start.sh` around lines 250 - 254, The current `set -e` makes the `if [ $? -ne 0 ]` check after the `uv sync` call unreachable; replace the pattern with the idiomatic check by running the command directly in the conditional (e.g., `if ! uv sync; then ...`) or temporarily disable `set -e` around this block (`set +e` before `uv sync` and `set -e` after) and then use the exit-code check, and remove the standalone `if [ $? -ne 0 ]` form so the error handling for `uv sync` (the echo and exit) executes correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@knowledge_runtime/start.sh`:
- Around line 262-281: The script currently only updates BACKEND_INTERNAL_URL in
.env when creating the file, so the --backend-url (BACKEND_URL) flag is ignored
if .env already exists; move the sed update block (the macOS/Linux conditional
that replaces BACKEND_INTERNAL_URL using sed) out of the "if [ ! -f '.env' ]"
branch and run it unconditionally after the existence check, but only when
BACKEND_URL is non-empty; use the same OSTYPE check and sed commands as in the
diff and ensure you still print a notice (or warning) when you override an
existing .env value so users know their flag was applied.
---
Nitpick comments:
In `@knowledge_runtime/start.sh`:
- Around line 283-286: The PYTHONPATH assignment currently uses
"${PYTHONPATH}:${PROJECT_ROOT}" which produces a leading colon when PYTHONPATH
is unset; update the export to use shell conditional expansion so an empty
PYTHONPATH does not insert an empty path entry (e.g., use the ${PYTHONPATH:+...}
form) when setting PYTHONPATH with PROJECT_ROOT (referencing the existing
PROJECT_ROOT and PYTHONPATH variables set in the script near SCRIPT_DIR).
- Around line 250-254: The current `set -e` makes the `if [ $? -ne 0 ]` check
after the `uv sync` call unreachable; replace the pattern with the idiomatic
check by running the command directly in the conditional (e.g., `if ! uv sync;
then ...`) or temporarily disable `set -e` around this block (`set +e` before
`uv sync` and `set -e` after) and then use the exit-code check, and remove the
standalone `if [ $? -ne 0 ]` form so the error handling for `uv sync` (the echo
and exit) executes correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f43d675-5b53-4dfc-8bd1-385edd0844c9
📒 Files selected for processing (5)
backend/app/services/rag/gateway_factory.pyknowledge_runtime/knowledge_runtime/config.pyknowledge_runtime/knowledge_runtime/services/content_fetcher.pyknowledge_runtime/start.shknowledge_runtime/tests/test_admin_executor.py
✅ Files skipped from review due to trivial changes (1)
- knowledge_runtime/tests/test_admin_executor.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/services/rag/gateway_factory.py
- knowledge_runtime/knowledge_runtime/services/content_fetcher.py
Comment on lines
+262
to
+281
| if [ ! -f ".env" ]; then | ||
| if [ ! -f ".env.example" ]; then | ||
| echo -e "${RED}Error: .env.example not found${NC}" | ||
| exit 1 | ||
| fi | ||
| cp .env.example .env | ||
| echo -e "${GREEN}✓ Created .env from .env.example${NC}" | ||
|
|
||
| # Update backend URL in .env | ||
| if [[ "$OSTYPE" == "darwin"* ]]; then | ||
| # macOS | ||
| sed -i '' "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env | ||
| else | ||
| # Linux | ||
| sed -i "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env | ||
| fi | ||
| echo -e "${YELLOW}Note: Please review and update .env file with your actual configuration${NC}" | ||
| else | ||
| echo -e "${GREEN}✓ .env file already exists${NC}" | ||
| fi |
Contributor
There was a problem hiding this comment.
--backend-url is silently ignored when .env already exists.
The sed update to BACKEND_INTERNAL_URL only runs inside the if [ ! -f ".env" ] branch. On the second and subsequent runs (or any time the user has a pre-existing .env), passing --backend-url http://... does nothing — the server will read the stale value from .env, contradicting the configuration banner printed at lines 126-128. This is surprising for a flag advertised in --help.
Either apply the override unconditionally, or warn the user when the flag is provided but .env already exists.
🔧 Suggested fix
# Configure environment variables
+ENV_EXISTED=true
if [ ! -f ".env" ]; then
+ ENV_EXISTED=false
if [ ! -f ".env.example" ]; then
echo -e "${RED}Error: .env.example not found${NC}"
exit 1
fi
cp .env.example .env
echo -e "${GREEN}✓ Created .env from .env.example${NC}"
-
- # Update backend URL in .env
- if [[ "$OSTYPE" == "darwin"* ]]; then
- # macOS
- sed -i '' "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env
- else
- # Linux
- sed -i "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env
- fi
echo -e "${YELLOW}Note: Please review and update .env file with your actual configuration${NC}"
else
echo -e "${GREEN}✓ .env file already exists${NC}"
fi
+
+# Apply --backend-url override (or default) so the CLI flag is always honored
+if [[ "$OSTYPE" == "darwin"* ]]; then
+ sed -i '' "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env
+else
+ sed -i "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ ! -f ".env" ]; then | |
| if [ ! -f ".env.example" ]; then | |
| echo -e "${RED}Error: .env.example not found${NC}" | |
| exit 1 | |
| fi | |
| cp .env.example .env | |
| echo -e "${GREEN}✓ Created .env from .env.example${NC}" | |
| # Update backend URL in .env | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| # macOS | |
| sed -i '' "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env | |
| else | |
| # Linux | |
| sed -i "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env | |
| fi | |
| echo -e "${YELLOW}Note: Please review and update .env file with your actual configuration${NC}" | |
| else | |
| echo -e "${GREEN}✓ .env file already exists${NC}" | |
| fi | |
| if [ ! -f ".env" ]; then | |
| ENV_EXISTED=false | |
| if [ ! -f ".env.example" ]; then | |
| echo -e "${RED}Error: .env.example not found${NC}" | |
| exit 1 | |
| fi | |
| cp .env.example .env | |
| echo -e "${GREEN}✓ Created .env from .env.example${NC}" | |
| echo -e "${YELLOW}Note: Please review and update .env file with your actual configuration${NC}" | |
| else | |
| ENV_EXISTED=true | |
| echo -e "${GREEN}✓ .env file already exists${NC}" | |
| fi | |
| # Apply --backend-url override (or default) so the CLI flag is always honored | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| sed -i '' "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env | |
| else | |
| sed -i "s|BACKEND_INTERNAL_URL=.*|BACKEND_INTERNAL_URL=$BACKEND_URL|g" .env | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@knowledge_runtime/start.sh` around lines 262 - 281, The script currently only
updates BACKEND_INTERNAL_URL in .env when creating the file, so the
--backend-url (BACKEND_URL) flag is ignored if .env already exists; move the sed
update block (the macOS/Linux conditional that replaces BACKEND_INTERNAL_URL
using sed) out of the "if [ ! -f '.env' ]" branch and run it unconditionally
after the existence check, but only when BACKEND_URL is non-empty; use the same
OSTYPE check and sed commands as in the diff and ensure you still print a notice
(or warning) when you override an existing .env value so users know their flag
was applied.
- 为 admin_executor 的 5 个方法添加 OpenTelemetry 追踪装饰器 - 异常处理器保留 retryable 属性并使用通用错误消息避免信息泄露 - 修复测试：确保 token 为空的测试确定性，使用具体 db_session 替代 ANY
cocowh
pushed a commit
to cocowh/Wegent
that referenced
this pull request
May 11, 2026
feat: 新增独立 knowledge_runtime 服务支持 RAG 远程调用 (wecode-ai#1001) See merge request weibo_rd/common/wecode/wegent!1994
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit