refactor(knowledge_runtime): switch RAG protocol from value mode to reference mode#1030
Merged
qdaxb merged 20 commits intowecode-ai:mainfrom Apr 28, 2026
Merged
refactor(knowledge_runtime): switch RAG protocol from value mode to reference mode#1030qdaxb merged 20 commits intowecode-ai:mainfrom
qdaxb merged 20 commits intowecode-ai:mainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… resolution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…avior Conditionally set vector_weight/keyword_weight to None when retrieval_mode is not "hybrid", matching Backend RagRuntimeResolver behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refactor IndexExecutor, QueryExecutor, and AdminExecutor to resolve runtime configs (retriever, embedding model, splitter) from the database via ConfigResolver instead of receiving them directly in request payloads (value mode -> reference mode). Key changes: - IndexExecutor: uses ConfigResolver.resolve_index_config() to get retriever_config, embedding_model_config, index_owner_user_id, and splitter_config from DB - QueryExecutor: iterates over knowledge_base_ids instead of knowledge_base_configs, uses ConfigResolver.resolve_query_config() for each KB - AdminExecutor: uses ConfigResolver.resolve_retriever_config() and _get_knowledge_base() to get retriever config and index owner user_id - All executors: manage DB session lifecycle with proper cleanup (get_session -> next -> try/finally close) - Tests: updated to mock get_session and ConfigResolver methods on executor instances, added tests for config resolution error handling and DB session cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add SQLAlchemy session management, KnowledgeDocumentReadOnly model, and database initialization in main.py lifespan. Config now supports DATABASE_URL environment variable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
RemoteRagGateway now only passes knowledge_base_id + user_id instead of full configs. ConnectionTestRuntimeSpec changed to use knowledge_base_id + user_id. Retriever test-connection endpoint simplified to test directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…or DB session Replace manual get_session()/try/finally/close pattern with FastAPI's Depends(get_db) injection. Executors now receive db in constructor, session lifecycle managed by framework. Add resolve_admin_config() to ConfigResolver to avoid accessing private _get_knowledge_base. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…perations Replace resolve_retriever_config + _get_knowledge_base combination with single resolve_admin_config call. Remove resolve_retriever_config public method since it's no longer used outside ConfigResolver. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move process_custom_headers_placeholders and supporting functions from Backend's model_resolver.py and KR's config_resolver.py to a shared module. Both Backend and KR now use the same implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t-connection Revert ConnectionTestRuntimeSpec back to retriever_config (not modifying runtime_specs.py). Restore test_connection_local to use spec.retriever_config directly (no DB query needed). Remove test_connection from RemoteRagGateway since retrievers endpoint tests storage directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move RuntimeRetrieverConfig, RuntimeEmbeddingModelConfig, and RuntimeRetrievalConfig from knowledge_runtime_protocol.py to a dedicated runtime_config.py module. Re-export from both the protocol module and __init__.py for backward compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e mode RemotePurgeKnowledgeIndexRequest and RemoteDropKnowledgeIndexRequest no longer have index_owner_user_id and retriever_config fields. Switch from build_internal_* to build_public_* runtime spec builders that resolve configs from DB. Add DATABASE_URL to docker-compose for KR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Key changes based on comparison of zy_kb_runtime and wegent/refactor/kr-config-reference-mode branches: - Switch KR DB layer to shared.db.sync_session (reduce duplication) - Use KnowledgeDocument inheriting shared Base (id + attachment_id + splitter_config) - Rename AdminConfig to AdminResolvedConfig (FETCH_HEAD pattern) - Remove KR test-connection endpoint (Backend retrievers test directly) - Remove RemoteTestConnectionRequest protocol model - Restore Backend /internal/rag Bearer Token auth (verify_internal_service_token) - Keep HEAD choices: ConfigResolutionError, required user_id, single query config, group retriever fallback, remove retrieval_policy/index_families Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughRefactors runtime configuration from request-embedded "value mode" to database-backed "reference mode" via a new ConfigResolver. Adds internal service-token auth for RAG routes, removes remote test-connection RPCs in favor of direct storage backend testing, and updates executors/endpoints/tests to resolve configs from the DB. Changes
Sequence DiagramsequenceDiagram
participant Client as rgba(52,152,219,0.5) Client
participant API as rgba(46,204,113,0.5) Knowledge Runtime Endpoint
participant Exec as rgba(155,89,182,0.5) Executor
participant Resolver as rgba(241,196,15,0.5) ConfigResolver
participant DB as rgba(231,76,60,0.5) Database
participant Storage as rgba(52,73,94,0.5) Storage Backend
Client->>API: HTTP request (user_id, knowledge_base_id)
API->>Exec: instantiate Executor(db_session)
Exec->>Resolver: resolve_*_config(db, knowledge_base_id, user_id)
Resolver->>DB: load KB, kinds, user, documents
DB-->>Resolver: KB/kind/user/document records
Resolver->>Resolver: decrypt creds, process placeholders
Resolver-->>Exec: RuntimeRetrieverConfig, RuntimeEmbeddingModelConfig, RetrievalConfig
Exec->>Storage: create_storage_backend_from_config(resolved_retriever_config)
Storage-->>Exec: storage backend instance
Exec->>Storage: perform operation (index/query/purge)
Storage-->>Exec: results
Exec-->>API: response payload
API-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker-compose.yml (1)
287-294:⚠️ Potential issue | 🟠 MajorDefault
DATABASE_URLdoes not match themysqlservice defaults; rag profile will fail to connect on a fresh setup.The default
mysql+pymysql://root:password@mysql:3306/wegentmismatches themysqlservice in this same compose file, where the defaults areMYSQL_ROOT_PASSWORD=123456(notpassword), and the application database/user aretask_manager/task_user/task_password(nowegentDB is created). Thebackendservice correctly uses${MYSQL_USER:-task_user}:${MYSQL_PASSWORD:-task_password}@mysql:3306/${MYSQL_DATABASE:-task_manager}. With the defaults shipped here,knowledge_runtime(rag profile) will fail to start onceinit_db(settings.database_url)runs.Additionally,
depends_ononly listsbackend; since this service now requires DB connectivity at startup, it should also wait formysql: service_healthy(matching whatbackenddoes).🛠 Proposed fix to align defaults with mysql service and add health gate
# Database connection for config resolution (reference-mode) - - DATABASE_URL=${DATABASE_URL:-mysql+pymysql://root:password@mysql:3306/wegent} + - DATABASE_URL=${DATABASE_URL:-mysql+pymysql://${MYSQL_USER:-task_user}:${MYSQL_PASSWORD:-task_password}@mysql:3306/${MYSQL_DATABASE:-task_manager}} # OpenTelemetry Configuration (uncomment to enable) # - OTEL_ENABLED=true # - OTEL_SERVICE_NAME=wegent-knowledge-runtime # - OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4317 depends_on: - - backend + mysql: + condition: service_healthy + backend: + condition: service_started🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 287 - 294, The default DATABASE_URL value and startup ordering are wrong: update the DATABASE_URL default to match the compose mysql defaults used by the backend (use MYSQL_USER/MYSQL_PASSWORD/MYSQL_DATABASE defaults — e.g. mysql+pymysql://task_user:task_password@mysql:3306/task_manager or interpolate with ${MYSQL_USER:-task_user}:${MYSQL_PASSWORD:-task_password}@mysql:3306/${MYSQL_DATABASE:-task_manager}) so init_db(settings.database_url) can connect, and add mysql to the depends_on for this service with the same healthcheck gating used by backend (depends_on: - backend and - mysql with condition: service_healthy) so the service waits for the database to be healthy before startup.knowledge_runtime/knowledge_runtime/services/admin_executor.py (1)
35-43:⚠️ Potential issue | 🟡 MinorStale docstring entry:
test_connection.The class docstring still lists
test_connection: Test storage backend connectioneven though this PR removes thetest_connectionmethod fromAdminExecutor. Update the operation list to match.📝 Proposed fix
Operations: - delete_document_index: Delete a specific document's index - purge_knowledge_index: Delete all chunks for a knowledge base - drop_knowledge_index: Physically drop the index/collection - list_chunks: List all chunks in a knowledge base - - test_connection: Test storage backend connection """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/services/admin_executor.py` around lines 35 - 43, The class docstring for AdminExecutor lists an outdated operation "test_connection"; update the docstring inside the AdminExecutor class to remove the `test_connection: Test storage backend connection` bullet so the operations list only contains the current methods (delete_document_index, purge_knowledge_index, drop_knowledge_index, list_chunks), ensuring the docstring text matches the actual implemented methods in AdminExecutor.
🧹 Nitpick comments (9)
shared/models/runtime_config.py (1)
39-46: Consider validating weight presence vs.retrieval_mode.
vector_weight/keyword_weightare bounded to[0,1]individually, but nothing enforces that they are provided (and ideally sum to ~1.0) whenretrieval_mode == "hybrid", or rejected/ignored otherwise. Given this is the strict reference-mode contract (extra="forbid"), amodel_validator(mode="after")would catch misconfigured KB specs at the boundary instead of silently producing wrong retrieval results downstream.♻️ Suggested validator
-from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator @@ class RuntimeRetrievalConfig(_RuntimeConfigModel): """Normalized retrieval config for a single knowledge base target.""" top_k: int = Field(default=20, gt=0) score_threshold: float = Field(default=0.7, ge=0.0, le=1.0) retrieval_mode: RetrievalMode = "vector" vector_weight: float | None = Field(default=None, ge=0.0, le=1.0) keyword_weight: float | None = Field(default=None, ge=0.0, le=1.0) + + `@model_validator`(mode="after") + def _check_hybrid_weights(self) -> "RuntimeRetrievalConfig": + if self.retrieval_mode == "hybrid": + if self.vector_weight is None or self.keyword_weight is None: + raise ValueError( + "hybrid retrieval_mode requires both vector_weight and keyword_weight" + ) + return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/models/runtime_config.py` around lines 39 - 46, RuntimeRetrievalConfig currently allows vector_weight and keyword_weight independently but does not enforce presence/relationship vs. retrieval_mode; add a pydantic model_validator(mode="after") on RuntimeRetrievalConfig to: when retrieval_mode == "hybrid" require both vector_weight and keyword_weight are not None and their sum is approximately 1.0 (or within a small epsilon), and when retrieval_mode != "hybrid" require both weights are None (or raise) so extra="forbid" surfaces bad specs; raise ValueError with clear messages on violations to block invalid configs.knowledge_runtime/knowledge_runtime/models/knowledge_document.py (1)
26-26: Use a callable (default=dict) instead of a mutable literal{}.
Column(JSON, ..., default={})binds the same{}instance as the column-level Python default; if anywhere in the codebase that resolved value is mutated before flush (e.g.doc.splitter_config["foo"] = ...on a freshly created instance) the mutation will leak across rows. The idiomatic and safe form is to pass a callable, which SQLAlchemy invokes per insert to produce a fresh dict.🛡️ Proposed fix
- splitter_config = Column(JSON, nullable=False, default={}) + splitter_config = Column(JSON, nullable=False, default=dict)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/models/knowledge_document.py` at line 26, The Column definition for splitter_config uses a mutable literal default={} which can be shared across instances; change the Column(JSON, nullable=False, default={}) to use a callable default (e.g., default=dict) so SQLAlchemy creates a fresh dict per row; update the splitter_config Column declaration in the KnowledgeDocument model to use default=dict to avoid shared-mutable-state bugs.backend/app/api/endpoints/adapter/retrievers.py (1)
257-276: Minor: preferlogger.exceptionand consider sanitizing the surfaced error string.Two small things in the new error path:
logger.error(f"Retriever connection test failed: {str(e)}")discards the traceback. Usinglogger.exception(...)here would preserve the stack trace — important for diagnosing backend driver failures (qdrant/elasticsearch client errors often wrap the underlying exception).- Returning
f"Connection failed: {str(e)}"to the client can echo back library-internal details (and sometimes the URL/credential context embedded in the exception by some clients). Since this endpoint accepts user-supplied credentials, consider surfacing a generic message to the client and logging the full exception detail server-side only.♻️ Suggested change
except Exception as e: - logger.error(f"Retriever connection test failed: {str(e)}") - return {"success": False, "message": f"Connection failed: {str(e)}"} + logger.exception("Retriever connection test failed") + return {"success": False, "message": "Connection failed"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/endpoints/adapter/retrievers.py` around lines 257 - 276, The error handler in the retriever connection test currently logs without a traceback and returns the full exception string to the client; change the except Exception block to call logger.exception("Retriever connection test failed") to preserve the stack trace and replace the client-facing message with a sanitized generic string (e.g. "Connection failed") while logging the full exception server-side; keep the ValueError branch unchanged and ensure this affects the code path around create_storage_backend_from_config and the awaited storage_backend.test_connection call.knowledge_runtime/tests/test_admin_executor.py (1)
62-71: Optional: extract the repeatedresolve_admin_configmock into a fixture/helper.The same
admin_executor._config_resolver.resolve_admin_config = MagicMock(return_value=AdminResolvedConfig(index_owner_user_id=7, retriever_config=mock_retriever_config))block is duplicated across all five test methods. A small helper or autouse fixture parameterized bymock_retriever_configwould remove ~25 lines of duplication and make future changes to the resolver contract a single-point edit.♻️ Example helper
+@pytest.fixture +def patch_resolve_admin_config(admin_executor, mock_retriever_config): + admin_executor._config_resolver.resolve_admin_config = MagicMock( + return_value=AdminResolvedConfig( + index_owner_user_id=7, + retriever_config=mock_retriever_config, + ) + ) + return admin_executor._config_resolver.resolve_admin_configThen each test can simply request
patch_resolve_admin_configinstead of repeating the inline assignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_admin_executor.py` around lines 62 - 71, Duplicate test setup: extract the repeated mock assignment of admin_executor._config_resolver.resolve_admin_config into a reusable pytest fixture/helper (e.g., patch_resolve_admin_config) that returns or applies MagicMock(return_value=AdminResolvedConfig(index_owner_user_id=7, retriever_config=mock_retriever_config)); update each test to accept this fixture (or call the helper) instead of repeating the inline assignment; keep the existing patch of create_storage_backend_from_runtime_config and reference the same mock_retriever_config in the fixture so changes to the resolver contract are centralized.knowledge_runtime/knowledge_runtime/main.py (1)
36-43: Consider fail-fast when DB-backed flows are mandatory.With
database_urlempty, the service starts successfully but every endpoint that depends onConfigResolver(index/query/admin) will fail at request time with no early signal. If DB-backed config resolution is now the only supported mode in production, raising at startup (or at least making this conditional on aSTANDALONE_MODE-like flag) would surface misconfiguration immediately rather than after the first failed request. The current warning-only behavior is fine for dev but is easy to miss in container logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/main.py` around lines 36 - 43, The startup currently only warns when settings.database_url is missing but several endpoints rely on DB-backed ConfigResolver (init_db); change startup to fail-fast in production by raising an error when settings.database_url is empty unless an explicit opt-out flag is set (e.g., STANDALONE_MODE or settings.standalone_mode). Locate the block that checks settings.database_url and either call init_db(settings.database_url) as today or, if missing and not in standalone mode, raise a RuntimeError (or exit with non-zero) with a clear message indicating DB-backed config resolution is required; keep the current warning behavior only when standalone mode is enabled or when a dev flag is present so dev runs remain unaffected.knowledge_runtime/knowledge_runtime/services/index_executor.py (1)
36-112: LGTM!The DB-backed resolution flow is wired cleanly:
ConfigResolver.resolve_index_configis invoked first, and downstream calls correctly consumeconfig.retriever_config,config.embedding_model_config,config.index_owner_user_id, andconfig.splitter_configinstead of request-supplied fields. Conversion of log statements to parameterized logging is a nice ancillary improvement.Minor optional note:
executeis now ~70 lines, slightly over the preferred 50-line guideline. Splitting the "resolve config + fetch content" prelude from the "build pipeline + index" portion into small private helpers could improve readability, but is non-blocking.As per coding guidelines: "Maximum function length of 50 lines preferred for all functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/services/index_executor.py` around lines 36 - 112, The execute method exceeds the preferred 50-line guideline; refactor it by extracting the prelude (config resolution and content fetching) into a private helper (e.g., _resolve_config_and_fetch that calls ConfigResolver.resolve_index_config and ContentFetcher.fetch and returns config, binary_data, source_file, file_extension) and the pipeline build + indexing into another helper (e.g., _build_and_index that creates storage via create_storage_backend_from_runtime_config, embedding model via create_embedding_model_from_runtime_config, constructs DocumentService and calls DocumentService.index_document_from_binary), then have execute call these two helpers and return the result so execute remains concise while preserving all existing behavior and logging.knowledge_runtime/tests/test_config_resolver.py (1)
668-671: Mock target is the wrong module — these patches are no-ops.
config_resolver.pydoesfrom shared.utils.placeholder import process_custom_headers_placeholdersat module load, binding a local reference insideknowledge_runtime.services.config_resolver. Patchingshared.utils.placeholder.process_custom_headers_placeholdersdoes not rebind that local reference, so production code keeps calling the real function. In these particular tests,custom_headershappens to be empty (or the assertion doesn't depend on the substitution), so the tests still pass — but the patches are misleading and won't help if a future test relies on them.If you intend to mock the helper used by
_build_resolved_embedding_model_config, patch the imported name where it's used:♻️ Suggested change
- patch( - "shared.utils.placeholder.process_custom_headers_placeholders", - side_effect=lambda h, u: h, - ), + patch( + "knowledge_runtime.services.config_resolver." + "process_custom_headers_placeholders", + side_effect=lambda h, u: h, + ),Also applies to: 711-714, 746-749
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver.py` around lines 668 - 671, The test patches are targeting the helper in its original module, which is a no-op because config_resolver.py imports process_custom_headers_placeholders at module load; update the patches to target the name where it is imported and used (the bound symbol in the config resolver module) — i.e., replace patches of "shared.utils.placeholder.process_custom_headers_placeholders" with "knowledge_runtime.services.config_resolver.process_custom_headers_placeholders" so that calls inside _build_resolved_embedding_model_config (and any other functions in that module) are properly mocked; apply this change to the three patch sites flagged in the tests.knowledge_runtime/knowledge_runtime/services/config_resolver.py (1)
433-441: Redundant blanketexcept Exception(ruff BLE001).
shared.utils.crypto.decrypt_api_keyalready returns the original value when the input is not encrypted and otherwise propagates only crypto-specific failures fromdecrypt_sensitive_data. Catching everyExceptionhere both hides real bugs (e.g., aTypeErrorfrom passing a non-string intovalue.startswithetc.) and is redundant for the documented "backward-compatibility for plaintext" path. Either narrow theexceptto the specific decryption errors raised bydecrypt_sensitive_data, or rely ondecrypt_api_keyand drop the try/except.♻️ Proposed change
`@staticmethod` def _decrypt_optional_value(value: Any) -> Any: """Decrypt an optional encrypted value. Returns original if decryption fails.""" if not value: return value try: return decrypt_api_key(value) - except Exception: + except Exception: # noqa: BLE001 # best-effort: keep ciphertext on decrypt failure + logger.warning("Failed to decrypt sensitive value; using original.") return value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/knowledge_runtime/services/config_resolver.py` around lines 433 - 441, The helper _decrypt_optional_value currently masks all exceptions with a blanket except Exception which hides real bugs; remove the try/except and simply return decrypt_api_key(value) (keeping the early "if not value: return value" check) so decryption errors from decrypt_sensitive_data propagate correctly; alternatively, if you must swallow only crypto failures, import and catch the specific exception thrown by decrypt_sensitive_data (e.g., DecryptionError) instead of Exception, referencing decrypt_api_key and decrypt_sensitive_data in your change.knowledge_runtime/knowledge_runtime/services/query_executor.py (1)
52-60: Optional: parallelize per-KB queries for multi-KB requests.
execute()resolves config and runs each KB query strictly sequentially. With a multi-KB request, latency scales linearly withlen(knowledge_base_ids), even though each KB query is independent and the resolution / underlying retrieval are I/O-bound. Consider running them concurrently withasyncio.gather(still bounded by storage backend connection limits) to reduce tail latency for queries spanning several knowledge bases.⚡ Sketch
- for knowledge_base_id in request.knowledge_base_ids: - records = await self._query_knowledge_base( - request=request, - knowledge_base_id=knowledge_base_id, - ) - all_records.extend(records) + results = await asyncio.gather( + *( + self._query_knowledge_base( + request=request, + knowledge_base_id=kb_id, + ) + for kb_id in request.knowledge_base_ids + ) + ) + for records in results: + all_records.extend(records)🤖 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 52 - 60, execute() currently iterates request.knowledge_base_ids and awaits _query_knowledge_base sequentially, causing linear latency; change it to run per-KB queries concurrently using asyncio.gather on tasks created from calling self._query_knowledge_base(request=request, knowledge_base_id=kb_id) for each kb_id, then flatten/extend the results into all_records; optionally wrap coroutine creation with a bounded semaphore or an async pool to limit concurrent connections to the storage backend if needed.
🤖 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/knowledge_runtime/services/config_resolver.py`:
- Around line 355-379: The current fallback query hard-codes Kind.namespace ==
"default", which can leak a public retriever into a tenant namespace; update the
fallback in the retriever lookup to preserve the requested namespace (use
Kind.namespace == namespace instead of "default") so lookups remain scoped to
the requested tenant for the Kind with Kind.name == name and Kind.kind ==
"Retriever"; also ensure this change is applied where the retriever lookup logic
(the Kind query using variables name and namespace) occurs and add no-op
behavior otherwise (leave user_id==0 fallback as-is or remove if not needed by
design).
In `@knowledge_runtime/tests/test_config_resolver.py`:
- Around line 119-129: The sentinel pattern in _make_document is broken because
the default argument splitter_config=object() creates a new object at definition
time and the check splitter_config is object() creates a different object at
call time, so the identity test always fails and the default never applies; fix
by introducing a module-level sentinel (e.g., _SENTINEL = object()) or use None
as the default and then in _make_document check if splitter_config is _SENTINEL
(or is None) and only then set doc.splitter_config = {"chunk_size": 512}; update
the function signature and the conditional that assigns doc.splitter_config
accordingly so omitted calls receive the proper default.
---
Outside diff comments:
In `@docker-compose.yml`:
- Around line 287-294: The default DATABASE_URL value and startup ordering are
wrong: update the DATABASE_URL default to match the compose mysql defaults used
by the backend (use MYSQL_USER/MYSQL_PASSWORD/MYSQL_DATABASE defaults — e.g.
mysql+pymysql://task_user:task_password@mysql:3306/task_manager or interpolate
with
${MYSQL_USER:-task_user}:${MYSQL_PASSWORD:-task_password}@mysql:3306/${MYSQL_DATABASE:-task_manager})
so init_db(settings.database_url) can connect, and add mysql to the depends_on
for this service with the same healthcheck gating used by backend (depends_on: -
backend and - mysql with condition: service_healthy) so the service waits for
the database to be healthy before startup.
In `@knowledge_runtime/knowledge_runtime/services/admin_executor.py`:
- Around line 35-43: The class docstring for AdminExecutor lists an outdated
operation "test_connection"; update the docstring inside the AdminExecutor class
to remove the `test_connection: Test storage backend connection` bullet so the
operations list only contains the current methods (delete_document_index,
purge_knowledge_index, drop_knowledge_index, list_chunks), ensuring the
docstring text matches the actual implemented methods in AdminExecutor.
---
Nitpick comments:
In `@backend/app/api/endpoints/adapter/retrievers.py`:
- Around line 257-276: The error handler in the retriever connection test
currently logs without a traceback and returns the full exception string to the
client; change the except Exception block to call logger.exception("Retriever
connection test failed") to preserve the stack trace and replace the
client-facing message with a sanitized generic string (e.g. "Connection failed")
while logging the full exception server-side; keep the ValueError branch
unchanged and ensure this affects the code path around
create_storage_backend_from_config and the awaited
storage_backend.test_connection call.
In `@knowledge_runtime/knowledge_runtime/main.py`:
- Around line 36-43: The startup currently only warns when settings.database_url
is missing but several endpoints rely on DB-backed ConfigResolver (init_db);
change startup to fail-fast in production by raising an error when
settings.database_url is empty unless an explicit opt-out flag is set (e.g.,
STANDALONE_MODE or settings.standalone_mode). Locate the block that checks
settings.database_url and either call init_db(settings.database_url) as today
or, if missing and not in standalone mode, raise a RuntimeError (or exit with
non-zero) with a clear message indicating DB-backed config resolution is
required; keep the current warning behavior only when standalone mode is enabled
or when a dev flag is present so dev runs remain unaffected.
In `@knowledge_runtime/knowledge_runtime/models/knowledge_document.py`:
- Line 26: The Column definition for splitter_config uses a mutable literal
default={} which can be shared across instances; change the Column(JSON,
nullable=False, default={}) to use a callable default (e.g., default=dict) so
SQLAlchemy creates a fresh dict per row; update the splitter_config Column
declaration in the KnowledgeDocument model to use default=dict to avoid
shared-mutable-state bugs.
In `@knowledge_runtime/knowledge_runtime/services/config_resolver.py`:
- Around line 433-441: The helper _decrypt_optional_value currently masks all
exceptions with a blanket except Exception which hides real bugs; remove the
try/except and simply return decrypt_api_key(value) (keeping the early "if not
value: return value" check) so decryption errors from decrypt_sensitive_data
propagate correctly; alternatively, if you must swallow only crypto failures,
import and catch the specific exception thrown by decrypt_sensitive_data (e.g.,
DecryptionError) instead of Exception, referencing decrypt_api_key and
decrypt_sensitive_data in your change.
In `@knowledge_runtime/knowledge_runtime/services/index_executor.py`:
- Around line 36-112: The execute method exceeds the preferred 50-line
guideline; refactor it by extracting the prelude (config resolution and content
fetching) into a private helper (e.g., _resolve_config_and_fetch that calls
ConfigResolver.resolve_index_config and ContentFetcher.fetch and returns config,
binary_data, source_file, file_extension) and the pipeline build + indexing into
another helper (e.g., _build_and_index that creates storage via
create_storage_backend_from_runtime_config, embedding model via
create_embedding_model_from_runtime_config, constructs DocumentService and calls
DocumentService.index_document_from_binary), then have execute call these two
helpers and return the result so execute remains concise while preserving all
existing behavior and logging.
In `@knowledge_runtime/knowledge_runtime/services/query_executor.py`:
- Around line 52-60: execute() currently iterates request.knowledge_base_ids and
awaits _query_knowledge_base sequentially, causing linear latency; change it to
run per-KB queries concurrently using asyncio.gather on tasks created from
calling self._query_knowledge_base(request=request, knowledge_base_id=kb_id) for
each kb_id, then flatten/extend the results into all_records; optionally wrap
coroutine creation with a bounded semaphore or an async pool to limit concurrent
connections to the storage backend if needed.
In `@knowledge_runtime/tests/test_admin_executor.py`:
- Around line 62-71: Duplicate test setup: extract the repeated mock assignment
of admin_executor._config_resolver.resolve_admin_config into a reusable pytest
fixture/helper (e.g., patch_resolve_admin_config) that returns or applies
MagicMock(return_value=AdminResolvedConfig(index_owner_user_id=7,
retriever_config=mock_retriever_config)); update each test to accept this
fixture (or call the helper) instead of repeating the inline assignment; keep
the existing patch of create_storage_backend_from_runtime_config and reference
the same mock_retriever_config in the fixture so changes to the resolver
contract are centralized.
In `@knowledge_runtime/tests/test_config_resolver.py`:
- Around line 668-671: The test patches are targeting the helper in its original
module, which is a no-op because config_resolver.py imports
process_custom_headers_placeholders at module load; update the patches to target
the name where it is imported and used (the bound symbol in the config resolver
module) — i.e., replace patches of
"shared.utils.placeholder.process_custom_headers_placeholders" with
"knowledge_runtime.services.config_resolver.process_custom_headers_placeholders"
so that calls inside _build_resolved_embedding_model_config (and any other
functions in that module) are properly mocked; apply this change to the three
patch sites flagged in the tests.
In `@shared/models/runtime_config.py`:
- Around line 39-46: RuntimeRetrievalConfig currently allows vector_weight and
keyword_weight independently but does not enforce presence/relationship vs.
retrieval_mode; add a pydantic model_validator(mode="after") on
RuntimeRetrievalConfig to: when retrieval_mode == "hybrid" require both
vector_weight and keyword_weight are not None and their sum is approximately 1.0
(or within a small epsilon), and when retrieval_mode != "hybrid" require both
weights are None (or raise) so extra="forbid" surfaces bad specs; raise
ValueError with clear messages on violations to block invalid configs.
🪄 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: 4c40c1de-e68a-430e-9166-e5a85687496e
⛔ Files ignored due to path filters (2)
knowledge_runtime/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
backend/app/api/endpoints/adapter/retrievers.pybackend/app/api/endpoints/internal/rag.pybackend/app/services/auth/__init__.pybackend/app/services/auth/internal_service_token.pybackend/app/services/rag/embedding/factory.pybackend/app/services/rag/remote_gateway.pybackend/app/services/rag/runtime_resolver.pybackend/tests/api/endpoints/test_retrievers_api.pybackend/tests/services/rag/test_local_gateway.pybackend/tests/services/rag/test_remote_gateway.pydocker-compose.ymlknowledge_runtime/knowledge_runtime/api/endpoints/admin.pyknowledge_runtime/knowledge_runtime/api/endpoints/index.pyknowledge_runtime/knowledge_runtime/api/endpoints/query.pyknowledge_runtime/knowledge_runtime/config.pyknowledge_runtime/knowledge_runtime/main.pyknowledge_runtime/knowledge_runtime/models/__init__.pyknowledge_runtime/knowledge_runtime/models/knowledge_document.pyknowledge_runtime/knowledge_runtime/services/admin_executor.pyknowledge_runtime/knowledge_runtime/services/config_resolver.pyknowledge_runtime/knowledge_runtime/services/index_executor.pyknowledge_runtime/knowledge_runtime/services/query_executor.pyknowledge_runtime/pyproject.tomlknowledge_runtime/tests/test_admin_executor.pyknowledge_runtime/tests/test_config_resolver.pyknowledge_runtime/tests/test_index_executor.pyknowledge_runtime/tests/test_query_executor.pyshared/models/__init__.pyshared/models/knowledge_runtime_protocol.pyshared/models/runtime_config.pyshared/tests/test_knowledge_runtime_protocol.pyshared/utils/placeholder.py
…ument The default argument `splitter_config=object()` creates a new object at definition time, but `splitter_config is object()` creates a different object at call time, so the identity check always failed. Replace with a module-level `_SENTINEL = object()` so the default applies correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
knowledge_runtime/tests/test_config_resolver.py (3)
122-131: Type hint inconsistent with sentinel default.
splitter_config: dict | None = _SENTINELis incorrect:_SENTINELisobject(), which is neitherdictnorNone. Type checkers will flag this. UseAnyto match the broader type:♻️ Suggested change
-from unittest.mock import MagicMock, patch +from typing import Any +from unittest.mock import MagicMock, patch @@ -def _make_document( - document_id: int = 100, splitter_config: dict | None = _SENTINEL -) -> MagicMock: +def _make_document( + document_id: int = 100, splitter_config: Any = _SENTINEL +) -> MagicMock:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver.py` around lines 122 - 131, The type hint on _make_document's parameter is too narrow: `splitter_config: dict | None = _SENTINEL` is inconsistent because _SENTINEL is an object, not dict or None; update the annotation to a broader type (e.g., typing.Any or Any) so the default sentinel is valid, adjust the import to include Any if needed, and keep the function logic using _SENTINEL to detect the default branch (refer to function _make_document and the sentinel _SENTINEL).
1-1104: File exceeds the 1000-line guideline.This file is 1104 lines. Consider splitting along the existing section banners (e.g.,
test_resolve_index_config.py,test_parse_kb_retrieval_config.py,test_build_resolved_*.py,test_helpers.py) and moving the shared_make_*factories into aconftest.pyto keep each module focused.As per coding guidelines: "if a file exceeds 1000 lines, split it into multiple sub-modules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver.py` around lines 1 - 1104, This test module is over 1000 lines; split it into focused files and centralize shared factories: create separate test files (e.g., tests for resolve_index_config, parse_kb_retrieval_config, build_resolved_* and helper functions) and move the shared factory helpers (_make_kb_kind, _make_retriever_kind, _make_model_kind, _make_document, _make_user, _SENTINEL) plus common fixtures (resolver, mock_db) into conftest.py; update imports in each new test file to use the fixtures and factories and keep each test file under 1000 lines while preserving existing test class and function names (e.g., TestResolveIndexConfig, TestParseKbRetrievalConfig, TestBuildResolvedRetrieverConfig, TestBuildResolvedEmbeddingModelConfig, TestProcessCustomHeadersPlaceholders) so tests run unchanged.
821-866: Tests don't actually validate the user-priority / fallback behavior.These tests only assert
result is not Noneafter wiringmock_db.query...first.return_valueto a fake. They don't verify which filter clauses were applied, the ordering byuser_id(so user-specific records win overuser_id=0), or that the public fallback kicks in only when the first lookup misses. Thetest_group_namespace_fallback_to_publiccase at least exercisesside_effect=[None, public_retriever]but doesn't assert that two queries happened or that the second one useduser_id=0.Consider asserting on the calls made against
mock_db(e.g.,mock_db.query.return_value.filter.call_args_list) or capturing thefilterarguments to confirm the resolver appliesuser_idpriority and falls back touser_id=0for group namespaces. As per coding guidelines, "Kind resource is uniquely identified by THREE fields:namespace,name, anduser_id; always filter by all three fields" — these tests should pin that contract.Also applies to: 877-909
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver.py` around lines 821 - 866, Update the three failing tests to assert the actual DB filter calls and ordering instead of only checking result non-null: in test_default_namespace_queries_with_user_priority, assert that mock_db.query.return_value.filter.return_value.order_by was called and inspect mock_db.query.return_value.filter.call_args_list to verify filters included name="test-retriever", namespace="default" and that order_by was invoked to prefer user_id=42 over user_id=0; in test_group_namespace_queries_without_user_id, assert the single filter call includes namespace="team-ns", name="test-retriever" and user_id=42 (or absence if intended) to match the three-field identification rule; in test_group_namespace_fallback_to_public, assert two separate query/filter sequences occurred (e.g., len(filter.call_args_list) == 2) and that the first used user_id=42 while the second used user_id=0 (public fallback) by inspecting the call args for user_id values. Ensure you reference resolver._get_retriever_kind and mock_db.query.return_value.filter.call_args_list when adding these assertions.
🤖 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/tests/test_config_resolver.py`:
- Around line 670-673: The test patches currently target
"shared.utils.placeholder.process_custom_headers_placeholders" but
config_resolver.py imports the function directly, so the patch must target the
usage site; update all three patches to mock
"knowledge_runtime.services.config_resolver.process_custom_headers_placeholders"
(the same symbol imported into that module) and keep the side_effect=lambda h,
u: h to preserve passthrough behavior; ensure you change the three occurrences
that correspond to the tests referencing config_resolver's call site.
---
Nitpick comments:
In `@knowledge_runtime/tests/test_config_resolver.py`:
- Around line 122-131: The type hint on _make_document's parameter is too
narrow: `splitter_config: dict | None = _SENTINEL` is inconsistent because
_SENTINEL is an object, not dict or None; update the annotation to a broader
type (e.g., typing.Any or Any) so the default sentinel is valid, adjust the
import to include Any if needed, and keep the function logic using _SENTINEL to
detect the default branch (refer to function _make_document and the sentinel
_SENTINEL).
- Around line 1-1104: This test module is over 1000 lines; split it into focused
files and centralize shared factories: create separate test files (e.g., tests
for resolve_index_config, parse_kb_retrieval_config, build_resolved_* and helper
functions) and move the shared factory helpers (_make_kb_kind,
_make_retriever_kind, _make_model_kind, _make_document, _make_user, _SENTINEL)
plus common fixtures (resolver, mock_db) into conftest.py; update imports in
each new test file to use the fixtures and factories and keep each test file
under 1000 lines while preserving existing test class and function names (e.g.,
TestResolveIndexConfig, TestParseKbRetrievalConfig,
TestBuildResolvedRetrieverConfig, TestBuildResolvedEmbeddingModelConfig,
TestProcessCustomHeadersPlaceholders) so tests run unchanged.
- Around line 821-866: Update the three failing tests to assert the actual DB
filter calls and ordering instead of only checking result non-null: in
test_default_namespace_queries_with_user_priority, assert that
mock_db.query.return_value.filter.return_value.order_by was called and inspect
mock_db.query.return_value.filter.call_args_list to verify filters included
name="test-retriever", namespace="default" and that order_by was invoked to
prefer user_id=42 over user_id=0; in
test_group_namespace_queries_without_user_id, assert the single filter call
includes namespace="team-ns", name="test-retriever" and user_id=42 (or absence
if intended) to match the three-field identification rule; in
test_group_namespace_fallback_to_public, assert two separate query/filter
sequences occurred (e.g., len(filter.call_args_list) == 2) and that the first
used user_id=42 while the second used user_id=0 (public fallback) by inspecting
the call args for user_id values. Ensure you reference
resolver._get_retriever_kind and
mock_db.query.return_value.filter.call_args_list when adding these assertions.
🪄 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: 6c03ddc0-753b-40f1-8afe-f954219ce2fe
📒 Files selected for processing (1)
knowledge_runtime/tests/test_config_resolver.py
- Fix broken sentinel pattern in _make_document (module-level _SENTINEL instead of per-call object()) - Update internal rag test payloads: replace old fields (index_owner_user_id, retriever_config) with reference-mode fields (knowledge_base_id, user_id) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/api/endpoints/internal/test_rag_retrieve_endpoint.py (1)
117-175:⚠️ Potential issue | 🟠 MajorThe endpoint ignores
user_idfrom the request; the test assertion should verify it is forwarded to the builder, and the endpoint implementation should pass it.
RemoteListChunksRequestrequiresuser_id(shared/models/knowledge_runtime_protocol.py:126), and the test correctly includes it in the payload (line 120). However:
Endpoint implementation (backend/app/api/endpoints/internal/rag.py:655–661) does not forward
user_idwhen callingbuild_internal_list_chunks_runtime_spec, unlike the parallelpurgeanddropendpoints which correctly passuser_id=request.user_idto their respective builders.Test assertion (lines 168–174) does not verify
user_idis forwarded, masking the omission.Update the endpoint to forward
user_id(matching the purge/drop pattern), and adduser_id=9to the assertion to pin the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/api/endpoints/internal/test_rag_retrieve_endpoint.py` around lines 117 - 175, The endpoint is not forwarding user_id to the runtime builder; update the handler in rag.py to call build_internal_list_chunks_runtime_spec(..., user_id=request.user_id) (same pattern as purge/drop handlers) so the RemoteListChunksRequest contract is satisfied, and update the test test_internal_all_chunks_routes_protocol_request_through_local_gateway to assert mock_build_spec.assert_called_once_with(..., knowledge_base_id=7, max_chunks=1000, query="list_index_chunks", metadata_condition=payload["metadata_condition"], user_id=9) so the test verifies the user_id is forwarded when LocalRagGateway.list_chunks is awaited with the returned runtime_spec.
🧹 Nitpick comments (3)
knowledge_runtime/tests/test_config_resolver.py (3)
119-131: Type hint onsplitter_configis inaccurate.
_SENTINEL = object()does not satisfy the declareddict | Noneannotation, which will trip type checkers (mypy/pyright) and is misleading for callers. UseAnyto reflect the sentinel-based contract:♻️ Proposed fix
+from typing import Any + _SENTINEL = object() def _make_document( - document_id: int = 100, splitter_config: dict | None = _SENTINEL + document_id: int = 100, splitter_config: Any = _SENTINEL ) -> MagicMock:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver.py` around lines 119 - 131, The type hint for splitter_config in _make_document is incorrect because _SENTINEL is an object and doesn't match dict | None; update the signature to use typing.Any (import Any) so the parameter can accept the sentinel, dict, or None without type errors, e.g. change "splitter_config: dict | None = _SENTINEL" to "splitter_config: Any = _SENTINEL" and keep the sentinel logic inside the function; reference symbols: _SENTINEL and _make_document.
160-167: Dead mock-db plumbing intest_success_with_document_id.
_get_knowledge_base,_get_user_name,_get_splitter_config, and the two builder methods are all patched directly onresolver, so the chainedmock_db.query.return_value...first.side_effect = [kb, user, doc](and theorder_by.first.return_value = retrieversetup just above) are never consulted. This is misleading for readers and risks masking regressions if the patches are later removed.♻️ Suggested cleanup
- mock_db.query.return_value.filter.return_value.filter.return_value.order_by.return_value.first.return_value = ( - retriever - ) - mock_db.query.return_value.filter.return_value.first.side_effect = [ - kb, - user, - doc, - ] - with ( patch.object(resolver, "_get_knowledge_base", return_value=kb),
retriever,model,doc, anduserlocals can also be dropped if no longer referenced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver.py` around lines 160 - 167, The test_success_with_document_id contains dead mock-db plumbing: remove the unused chained mocks on mock_db.query (the order_by.first.return_value and the .first.side_effect = [kb, user, doc]) because _get_knowledge_base, _get_user_name, _get_splitter_config and the two builder methods are patched directly on resolver and those DB mocks are never used; update the test to rely only on the patched resolver methods (resolver._get_knowledge_base, resolver._get_user_name, resolver._get_splitter_config, and the two builder methods) and delete unused local variables retriever, model, doc, and user if they are no longer referenced to avoid misleading setup and maskable regressions.
821-866:_get_*_kindtests don’t actually verify namespace/user_id priority semantics.These tests are named after important resolution rules (user-priority in
default, no user filter in group namespaces, fallback touser_id=0) but only assert that the result is non-None. With the chainedMagicMock, any query path returns a truthy mock, so an implementation regression (e.g., dropping theuser_idfilter indefault, or skipping theuser_id=0fallback) would not fail these tests.Consider strengthening at least one assertion per test by inspecting the recorded
filter/order_bycalls, e.g.:# verify user_id appears in default-namespace filter chain filter_calls = mock_db.query.return_value.filter.call_args_list assert any("user_id" in str(c) for c in filter_calls)or by spying on the underlying SQLAlchemy column comparisons. Otherwise the “priority” / “fallback” phrasing in the test names is aspirational rather than verified.
Per coding guidelines: "Kind resource is uniquely identified by THREE fields:
namespace,name, anduser_id; always filter by all three fields" — these tests are the natural place to lock that contract in.Also applies to: 877-909
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver.py` around lines 821 - 866, Tests for _get_retriever_kind only assert non-None and don't verify the required filters/priority semantics; update each test (e.g., test_default_namespace_queries_with_user_priority, test_group_namespace_queries_without_user_id, test_group_namespace_fallback_to_public) to assert the query filter chain includes the expected comparisons for namespace, name and user_id by inspecting mock_db.query.return_value.filter.call_args_list (or order_by.call_args_list for user-priority cases) and for the fallback test assert that the first call returned None and a subsequent call included user_id=0 in its filter calls; ensure you reference _get_retriever_kind when adding these assertions so the tests fail if the implementation drops required filters or fallback behavior.
🤖 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/tests/test_config_resolver.py`:
- Around line 1-1104: The test module is too large (1104 lines); split it into
smaller files along class boundaries and extract shared fixtures into
conftest.py: create test_config_resolver_resolve.py containing
TestResolveIndexConfig and TestResolveQueryConfig,
test_config_resolver_builders.py containing TestBuildResolvedRetrieverConfig and
TestBuildResolvedEmbeddingModelConfig, test_config_resolver_helpers.py
containing TestGetKnowledgeBase, TestParseKbRetrievalConfig,
TestGetSplitterConfig, TestGetUserName, TestDecryptOptionalValue,
TestGetRetrieverKind, TestGetModelKind, and test_config_resolver_errors.py
containing TestConfigResolutionError and TestProcessCustomHeadersPlaceholders;
move shared fixtures/factories (resolver, mock_db, _make_kb_kind,
_make_retriever_kind, _make_model_kind, _make_document, _make_user) into a new
conftest.py so each new test module imports them automatically and update
imports/patch targets (e.g., references to ConfigResolver,
ConfigResolutionError, IndexConfig, QueryConfig, RuntimeRetrieverConfig,
RuntimeEmbeddingModelConfig, process_custom_headers_placeholders) accordingly to
keep tests passing.
---
Outside diff comments:
In `@backend/tests/api/endpoints/internal/test_rag_retrieve_endpoint.py`:
- Around line 117-175: The endpoint is not forwarding user_id to the runtime
builder; update the handler in rag.py to call
build_internal_list_chunks_runtime_spec(..., user_id=request.user_id) (same
pattern as purge/drop handlers) so the RemoteListChunksRequest contract is
satisfied, and update the test
test_internal_all_chunks_routes_protocol_request_through_local_gateway to assert
mock_build_spec.assert_called_once_with(..., knowledge_base_id=7,
max_chunks=1000, query="list_index_chunks",
metadata_condition=payload["metadata_condition"], user_id=9) so the test
verifies the user_id is forwarded when LocalRagGateway.list_chunks is awaited
with the returned runtime_spec.
---
Nitpick comments:
In `@knowledge_runtime/tests/test_config_resolver.py`:
- Around line 119-131: The type hint for splitter_config in _make_document is
incorrect because _SENTINEL is an object and doesn't match dict | None; update
the signature to use typing.Any (import Any) so the parameter can accept the
sentinel, dict, or None without type errors, e.g. change "splitter_config: dict
| None = _SENTINEL" to "splitter_config: Any = _SENTINEL" and keep the sentinel
logic inside the function; reference symbols: _SENTINEL and _make_document.
- Around line 160-167: The test_success_with_document_id contains dead mock-db
plumbing: remove the unused chained mocks on mock_db.query (the
order_by.first.return_value and the .first.side_effect = [kb, user, doc])
because _get_knowledge_base, _get_user_name, _get_splitter_config and the two
builder methods are patched directly on resolver and those DB mocks are never
used; update the test to rely only on the patched resolver methods
(resolver._get_knowledge_base, resolver._get_user_name,
resolver._get_splitter_config, and the two builder methods) and delete unused
local variables retriever, model, doc, and user if they are no longer referenced
to avoid misleading setup and maskable regressions.
- Around line 821-866: Tests for _get_retriever_kind only assert non-None and
don't verify the required filters/priority semantics; update each test (e.g.,
test_default_namespace_queries_with_user_priority,
test_group_namespace_queries_without_user_id,
test_group_namespace_fallback_to_public) to assert the query filter chain
includes the expected comparisons for namespace, name and user_id by inspecting
mock_db.query.return_value.filter.call_args_list (or order_by.call_args_list for
user-priority cases) and for the fallback test assert that the first call
returned None and a subsequent call included user_id=0 in its filter calls;
ensure you reference _get_retriever_kind when adding these assertions so the
tests fail if the implementation drops required filters or fallback behavior.
🪄 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: 06f019d4-0100-40d5-bbe6-f88a333af953
📒 Files selected for processing (2)
backend/tests/api/endpoints/internal/test_rag_retrieve_endpoint.pyknowledge_runtime/tests/test_config_resolver.py
…er files Split the 1104-line monolithic test file into 4 focused files along class boundaries and extract shared fixtures/factories into conftest.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
knowledge_runtime/tests/test_config_resolver_helpers.py (2)
242-325: Tests verify return value but not query semantics.
test_default_namespace_queries_with_user_priorityandtest_group_namespace_queries_without_user_idonly assertresult is not None, which passes as long as the configured mock returns anything. They don't verify the actual SQLAlchemy behavior under test (user_id filter,order_by(Kind.user_id.desc()), namespace branching). Consider asserting on the calls, e.g., capturingmock_db.query.return_value.filter.call_args_listand verifying the filter expressions, or at minimum that the user-priority branch usesorder_bywhile the group branch doesn't. Otherwise a regression that swaps the two branches would still let the tests pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver_helpers.py` around lines 242 - 325, Tests currently only assert non-None results; update the two pairs of tests for _get_retriever_kind and _get_model_kind to also assert the expected query behavior: inspect mock_db.query.return_value.filter.call_args_list to ensure the user-priority branch used order_by (verify that mock_db.query.return_value.filter.return_value.order_by was called at least once for "default" namespace tests) and that the group-namespace branch did not call order_by (or only called filter, not order_by) for "team-ns" tests; additionally assert the filter call arguments contain the expected user_id or namespace expressions to validate the correct branch was taken.
229-236: Test asserts mock identity, not realdecrypt_api_keyplaintext pass-through.
test_plain_text_api_keypatchesdecrypt_api_keyto return"sk-plain-key", so the assertion only verifies that_decrypt_optional_valuereturns whatever the patched decrypt returns — i.e., it's effectively a duplicate oftest_successful_decrypt. To meaningfully cover the documented behavior ("plaintextsk-keys pass through"), call the realdecrypt_api_key(no patch) on ask-plaintext, or patch it to mimic real plaintext-detection behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver_helpers.py` around lines 229 - 236, The test test_plain_text_api_key is incorrectly patching knowledge_runtime.services.config_resolver.decrypt_api_key so it only verifies the mock rather than the real plaintext-pass-through behavior; update the test to exercise ConfigResolver._decrypt_optional_value with an actual "sk-..." plaintext API key (do not patch decrypt_api_key) or adjust the patch to mimic real plaintext detection (e.g., have the mock raise or return the input for "sk-" values) so that the assertion verifies that ConfigResolver._decrypt_optional_value returns the plaintext sk- key without delegating to a mocked decrypt function.knowledge_runtime/tests/test_config_resolver_resolve.py (1)
40-50: Dead mock-DB setup — all paths are patched at the resolver level.Lines 40-50 wire up
mock_db.query.return_value.filter…chains and aMagicMockuser, but the test then patches_get_knowledge_base,_get_user_name,_build_resolved_retriever_config,_build_resolved_embedding_model_config, and_get_splitter_configdirectly on the resolver, so none of thosemock_dbquery chains are exercised. TheuserMagicMock and themock_db.querysetup can be removed without changing what the test verifies.♻️ Suggested cleanup
kb = _make_kb_kind(knowledge_base_id=1, user_id=42) - retriever = _make_retriever_kind() - model = _make_model_kind() - user = MagicMock() - user.id = 42 - user.user_name = "testuser" - - mock_db.query.return_value.filter.return_value.filter.return_value.order_by.return_value.first.return_value = ( - retriever - ) - mock_db.query.return_value.filter.return_value.first.side_effect = [ - kb, - user, - ] with ( patch.object(resolver, "_get_knowledge_base", return_value=kb),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge_runtime/tests/test_config_resolver_resolve.py` around lines 40 - 50, Remove the dead DB and user mocks that are not used because the resolver methods are patched: delete the MagicMock user assignment (user = MagicMock(); user.id = 42; user.user_name = "testuser") and the mock_db.query... filter... order_by...first chain setup; keep the existing patches on resolver methods (_get_knowledge_base, _get_user_name, _build_resolved_retriever_config, _build_resolved_embedding_model_config, _get_splitter_config) and any assertions that depend on those patched methods so the test logic remains unchanged.knowledge_runtime/tests/conftest.py (1)
59-157: Underscore-prefixed helpers are imported across test modules.
_make_kb_kind,_make_retriever_kind,_make_model_kind,_make_document, and_make_userare imported by sibling test files (e.g.,test_config_resolver_helpers.py,test_config_resolver_builders.py,test_config_resolver_resolve.py). The leading underscore conventionally signals module-private helpers, which conflicts with cross-module use. Consider either dropping the underscore prefix or extracting the factories into a dedicatedtests/_factories.py(or similar) so the public/private intent is clear andconftest.pystays focused on fixtures. Importing fromconftestis also somewhat fragile under pytest collection.🤖 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 59 - 157, These helper factory functions (_make_kb_kind, _make_retriever_kind, _make_model_kind, _make_document, _make_user) are defined with a leading underscore yet are imported by sibling tests; either remove the underscore prefix to make them public (rename to make_kb_kind, make_retriever_kind, make_model_kind, make_document, make_user and update all imports/usages), or extract them into a new dedicated test factory module (e.g., tests/_factories.py) and update imports in test_config_resolver_helpers.py, test_config_resolver_builders.py, and test_config_resolver_resolve.py to import from that module; also leave conftest.py focused on pytest fixtures only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@knowledge_runtime/tests/conftest.py`:
- Around line 59-157: These helper factory functions (_make_kb_kind,
_make_retriever_kind, _make_model_kind, _make_document, _make_user) are defined
with a leading underscore yet are imported by sibling tests; either remove the
underscore prefix to make them public (rename to make_kb_kind,
make_retriever_kind, make_model_kind, make_document, make_user and update all
imports/usages), or extract them into a new dedicated test factory module (e.g.,
tests/_factories.py) and update imports in test_config_resolver_helpers.py,
test_config_resolver_builders.py, and test_config_resolver_resolve.py to import
from that module; also leave conftest.py focused on pytest fixtures only.
In `@knowledge_runtime/tests/test_config_resolver_helpers.py`:
- Around line 242-325: Tests currently only assert non-None results; update the
two pairs of tests for _get_retriever_kind and _get_model_kind to also assert
the expected query behavior: inspect
mock_db.query.return_value.filter.call_args_list to ensure the user-priority
branch used order_by (verify that
mock_db.query.return_value.filter.return_value.order_by was called at least once
for "default" namespace tests) and that the group-namespace branch did not call
order_by (or only called filter, not order_by) for "team-ns" tests; additionally
assert the filter call arguments contain the expected user_id or namespace
expressions to validate the correct branch was taken.
- Around line 229-236: The test test_plain_text_api_key is incorrectly patching
knowledge_runtime.services.config_resolver.decrypt_api_key so it only verifies
the mock rather than the real plaintext-pass-through behavior; update the test
to exercise ConfigResolver._decrypt_optional_value with an actual "sk-..."
plaintext API key (do not patch decrypt_api_key) or adjust the patch to mimic
real plaintext detection (e.g., have the mock raise or return the input for
"sk-" values) so that the assertion verifies that
ConfigResolver._decrypt_optional_value returns the plaintext sk- key without
delegating to a mocked decrypt function.
In `@knowledge_runtime/tests/test_config_resolver_resolve.py`:
- Around line 40-50: Remove the dead DB and user mocks that are not used because
the resolver methods are patched: delete the MagicMock user assignment (user =
MagicMock(); user.id = 42; user.user_name = "testuser") and the mock_db.query...
filter... order_by...first chain setup; keep the existing patches on resolver
methods (_get_knowledge_base, _get_user_name, _build_resolved_retriever_config,
_build_resolved_embedding_model_config, _get_splitter_config) and any assertions
that depend on those patched methods so the test logic remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fe91780-7db4-4419-9c11-20e485f129c9
📒 Files selected for processing (5)
knowledge_runtime/tests/conftest.pyknowledge_runtime/tests/test_config_resolver_builders.pyknowledge_runtime/tests/test_config_resolver_errors.pyknowledge_runtime/tests/test_config_resolver_helpers.pyknowledge_runtime/tests/test_config_resolver_resolve.py
sunnights
changed the title
# Conflicts: # backend/app/services/rag/embedding/factory.py
qdaxb
approved these changes
Apr 28, 2026
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
2 participants
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
New Features
Bug Fixes
Refactor
Tests