fix(tools): translate PluginNotFoundError to ToolProviderNotFoundError - #41837
Open
Taranum01 wants to merge 9 commits into
Open
fix(tools): translate PluginNotFoundError to ToolProviderNotFoundError#41837Taranum01 wants to merge 9 commits into
Taranum01 wants to merge 9 commits into
Conversation
…enius#41594) `sqlalchemy.engine` (and its sub-loggers like `sqlalchemy.pool`) have `propagate = False` set in ext_logging.init_app to avoid duplicate logs, so the root logger's LOG_TZ-aware formatter never gets a chance to format SQLAlchemy records. When SQLALCHEMY_ECHO is enabled, engine log timestamps are emitted in the server's local timezone while the surrounding application logs are already converted to LOG_TZ, producing inconsistent timestamps within a single log stream. This change adds `apply_timezone_to_sqlalchemy_loggers()` in ext_logging.py and calls it from ext_database.init_app() right after the SQLAlchemy engine is eagerly created. The helper walks the SQLAlchemy logger hierarchy and applies the same LOG_TZ converter to the formatters of any handlers SQLAlchemy has attached, so engine log timestamps agree with the rest of the application logs. The fix preserves the existing non-propagating behavior (no duplicate logs) and is a no-op when LOG_OUTPUT_FORMAT is not "text" or when LOG_TZ is unset. Regression test covers: - JSON output format: no-op - LOG_TZ unset: no-op - LOG_TZ set: converter patched on sqlalchemy.engine, .pool, and bare SQLAlchemy loggers - Handler with no formatter: left alone, no exception - Idempotency across repeated calls
…logging The langgenius#41594 / langgenius#41629 fix (langgenius#41629) called apply_timezone_to_sqlalchemy_loggers() from ext_database.init_app(), which adds a new import edge extensions.ext_database -> extensions.ext_logging. Combined with the pre-existing libs.oauth_bearer -> extensions.ext_database -> extensions.ext_logging -> core.logging.* chain, importlinter's backend-layers contract now reports libs transitively reaching core. Add the new edge to the ignore_imports list, alongside the existing extensions exceptions (libs.external_api -> extensions.ext_logging, etc.). Siblings inside the extensions package are free to depend on each other; this exception is purely to mark the new edge and keep the migration baseline honest.
…lper (langgenius#41594 / langgenius#41629 follow-up) - Add `-> None` to every test method. - Add a small `_formatter_converter(formatter)` helper that asserts the formatter is not None and casts the converter to a typed `Callable[[float], time.struct_time]`, then route every `formatter.converter` access through it. - All 6 tests still pass; pyrefly reports 0 diagnostics on the file.
…anggenius#41649) The PUT/DELETE/auth endpoints for MCP providers accept a ``provider_id`` in the request body and pass it straight to SQLAlchemy, which (via the ``StringUUID`` column type) sends it to Postgres as a UUID. When a non-UUID value (e.g. ``"fast-mcp"``) is sent — typically when a stale server identifier ends up in the request body — Postgres raises ``psycopg2.errors.InvalidTextRepresentation: invalid input syntax for type uuid`` and the user sees a generic 500. The API layer should fail earlier with a clean 400 instead. This change adds a ``_validate_uuid`` helper and a ``field_validator`` on each of the three payloads that take ``provider_id`` (``MCPProviderUpdatePayload``, ``MCPProviderDeletePayload``, ``MCPAuthPayload``). The helper accepts the existing UUID string form (``xxxxxxxx-xxxx-...``) and rejects anything else with a Pydantic ``ValidationError`` that the existing ``@model_validate`` decorator turns into a 400. Tests (7 new, in ``api/tests/unit_tests/controllers/console/workspace/test_mcp_provider_id_uuid.py``): - ``MCPProviderUpdatePayload`` accepts a valid UUID, rejects ``"fast-mcp"`` and ``""`` with a ValidationError on the ``provider_id`` field. - ``MCPProviderDeletePayload`` and ``MCPAuthPayload`` accept a valid UUID and reject a non-UUID string. All 32 pre-existing tests in ``test_tool_providers.py`` still pass. ruff check / ruff format --check clean, pyrefly 0 diagnostics, importlinter 25 kept / 0 broken.
…tests (langgenius#41649 follow-up) mypy + pyrefly flag every test method in the new test_mcp_provider_id_uuid.py as missing a return annotation. Add `-> None` to all seven test methods. Pure test annotation, no behavior change.
…langgenius#41714) `_get_uuids` overrode the base ``VectorBase._get_uuids`` with ``uuid5(URL_NAMESPACE, page_content)`` (UUID v5) so the same content inserted twice would dedupe to a single Weaviate object. That was self-consistent, but the cleanup path (``batch_clean_document_task`` → ``index_processor.clean`` → ``vector.delete_by_ids``) was passing the segment's ``index_node_id`` from the database — a random UUID v4. ``uuid5(content)`` never equals ``index_node_id``, so ``delete_by_id`` silently no-op'd on every Weaviate vector store and ``Cleaned documents ...`` was logged as success while the objects remained as searchable orphans. Use the same source the rest of the VDB stack uses (``VectorBase._get_uuids``): the ``doc_id`` stored in each document's metadata. Insert writes under that id, the cleanup path already passes the same id, and ``delete_by_ids`` now actually targets the object that was inserted. The deduplication property previously claimed by the UUID5 path is already provided by the upstream ``_filter_duplicate_texts`` step on the same ``doc_id``, so removing the UUID5 hash does not change semantics for any caller that already provides a stable ``doc_id``. The pre-fix UUID5 path is removed; the ``_get_uuids`` override now matches the parent's parameter name (``texts``) so pyrefly stops flagging the override as inconsistent. Tests (2 new in ``providers/vdb/vdb-weaviate/tests/unit_tests/test_weaviate_vector.py``): - ``test_get_uuids_uses_doc_id_to_match_cleanup_path`` — pins the contract: any document with a ``doc_id`` must round-trip through ``_get_uuids`` as that exact ``doc_id``, so insert and delete agree on the Weaviate object id. - ``test_get_uuids_skips_documents_without_doc_id`` — documents without a ``doc_id`` are skipped, so ``add_texts`` falls back to a fresh uuid4 for them (inherited from the base class). All 41 existing tests in the suite still pass.
langgenius#41822) TiDB's ``FTS_MATCH_WORD`` signature is ``FTS_MATCH_WORD(query, column)``, but the pre-fix code generated ``FTS_MATCH_WORD(text, :query)`` (column first, query second). TiDB rejected the bound-parameter form with ``This version of TiDB doesn't yet support 'match against a non-constant string'``, silently breaking Full-Text Search and Hybrid Search for every TiDB Vector user with ``TIDB_VECTOR_ENABLE_FULLTEXT_SEARCH=true``. The two occurrences of the function call in ``search_by_full_text`` — the WHERE clause predicate and the score projection — both need the query as the first argument and the column as the second. Tests: - The existing ``test_search_by_full_text_queries_tidb_fts_and_scores`` assertion is updated to the new argument order. - A new ``test_search_by_full_text_uses_query_first_argument_order`` pins the contract: both occurrences in the generated SQL must use ``FTS_MATCH_WORD(:query, text)`` (and never ``FTS_MATCH_WORD(text, :query)``), so the swap can't regress silently. All 29 existing tests still pass. ``ruff check`` / ``ruff format --check`` clean, ``importlinter`` 25 kept / 0 broken.
langgenius#41805) `ToolManager.get_builtin_provider` falls back to `PluginToolManager.fetch_tool_provider` when the requested identifier isn't in the builtin cache. The plugin daemon raises `PluginNotFoundError` (and the lower-level `PluginDaemonNotFoundError`) when the provider is unknown to it, and neither was caught at the boundary. The error bubbled up to the console API generic exception handler as a 500. Translate both to `ToolProviderNotFoundError` at the `get_plugin_provider` boundary so the API returns a controlled 4xx (response matches the existing "missing provider" path that already returns `ToolProviderNotFoundError`). This is defense in depth behind the frontend fixes in langgenius#40686 and langgenius#41088 — the Agent V2 MCP-server-id path referenced in the report is no longer exercised, but any other caller hitting the builtin credential endpoints with a non-builtin identifier now gets a clean 4xx instead of a 500. Tests (1 new, parameterized over both daemon error types, in `tests/unit_tests/core/tools/test_tool_manager.py`): - `test_get_plugin_provider_translates_plugin_not_found_to_domain_error[daemon_error0]` — `PluginNotFoundError` - `test_get_plugin_provider_translates_plugin_not_found_to_domain_error[daemon_error1]` — `PluginDaemonNotFoundError` Both confirm the exception is wrapped to `ToolProviderNotFoundError` matching the "plugin provider <name> not found" message. The existing `test_get_plugin_provider_raises_when_provider_missing` test unchanged (covers the `None`-from-fetch path). All 46 existing tests in the file still pass. `ruff check` / `ruff format --check` clean, `pyrefly` 0 errors, `importlinter` 25 kept / 0 broken.
Contributor
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-09-04 19:55:49.140604209 +0000
+++ /tmp/pyrefly_pr.txt 2026-09-04 19:55:34.860576038 +0000
@@ -6617,29 +6617,29 @@
ERROR Argument `list[FromClause]` is not assignable to parameter `tables` with type `Sequence[Table] | None` in function `sqlalchemy.sql.schema.MetaData.create_all` [bad-argument-type]
--> tests/unit_tests/core/tools/test_tool_file_manager.py:30:56
ERROR Argument `Literal['openapi']` is not assignable to parameter `schema_type_str` with type `ApiProviderSchemaType | SQLCoreOperations[ApiProviderSchemaType]` in function `models.tools.ApiToolProvider.__init__` [bad-argument-type]
- --> tests/unit_tests/core/tools/test_tool_manager.py:86:25
+ --> tests/unit_tests/core/tools/test_tool_manager.py:87:25
ERROR `SimpleNamespace` is not assignable to attribute `entity` with type `ToolProviderEntityWithPlugin` [bad-assignment]
- --> tests/unit_tests/core/tools/test_tool_manager.py:448:32
+ --> tests/unit_tests/core/tools/test_tool_manager.py:482:32
ERROR `SimpleNamespace` is not assignable to attribute `entity` with type `ToolProviderEntityWithPlugin` [bad-assignment]
- --> tests/unit_tests/core/tools/test_tool_manager.py:468:32
+ --> tests/unit_tests/core/tools/test_tool_manager.py:502:32
ERROR Argument `SimpleNamespace` is not assignable to parameter `agent_tool` with type `AgentToolEntity` in function `core.tools.tool_manager.ToolManager.get_agent_tool_runtime` [bad-argument-type]
- --> tests/unit_tests/core/tools/test_tool_manager.py:620:32
+ --> tests/unit_tests/core/tools/test_tool_manager.py:654:32
ERROR Argument `SimpleNamespace` is not assignable to parameter `workflow_tool` with type `WorkflowToolRuntimeSpec` in function `core.tools.tool_manager.ToolManager.get_workflow_tool_runtime` [bad-argument-type]
- --> tests/unit_tests/core/tools/test_tool_manager.py:667:35
+ --> tests/unit_tests/core/tools/test_tool_manager.py:701:35
ERROR Argument `SimpleNamespace` is not assignable to parameter `agent_tool` with type `AgentToolEntity` in function `core.tools.tool_manager.ToolManager.get_agent_tool_runtime` [bad-argument-type]
- --> tests/unit_tests/core/tools/test_tool_manager.py:703:36
+ --> tests/unit_tests/core/tools/test_tool_manager.py:737:36
ERROR Cannot set item in `dict[str, I18nObject | None]` [unsupported-operation]
- --> tests/unit_tests/core/tools/test_tool_manager.py:809:55
+ --> tests/unit_tests/core/tools/test_tool_manager.py:843:55
ERROR `SimpleNamespace` is not assignable to attribute `entity` with type `ToolProviderEntityWithPlugin` [bad-assignment]
- --> tests/unit_tests/core/tools/test_tool_manager.py:899:32
+ --> tests/unit_tests/core/tools/test_tool_manager.py:933:32
ERROR Argument `Literal['']` is not assignable to parameter `typ` with type `Literal['api', 'builtin', 'mcp', 'workflow'] | None` in function `core.tools.tool_manager.ToolManager.list_providers_from_api` [bad-argument-type]
- --> tests/unit_tests/core/tools/test_tool_manager.py:944:96
+ --> tests/unit_tests/core/tools/test_tool_manager.py:978:96
ERROR `SimpleNamespace` is not assignable to attribute `entity` with type `ToolProviderEntityWithPlugin` [bad-assignment]
- --> tests/unit_tests/core/tools/test_tool_manager.py:1095:30
+ --> tests/unit_tests/core/tools/test_tool_manager.py:1129:30
ERROR `SimpleNamespace` is not assignable to attribute `entity` with type `ToolProviderEntityWithPlugin` [bad-assignment]
- --> tests/unit_tests/core/tools/test_tool_manager.py:1119:30
+ --> tests/unit_tests/core/tools/test_tool_manager.py:1153:30
ERROR Cannot index into `str` [bad-index]
- --> tests/unit_tests/core/tools/test_tool_manager.py:1124:20
+ --> tests/unit_tests/core/tools/test_tool_manager.py:1158:20
ERROR Class member `_DummyTool._invoke` overrides parent class `Tool` in an inconsistent manner [bad-override]
--> tests/unit_tests/core/tools/test_tool_provider_controller.py:30:9
ERROR Class member `_DummyTool.tool_provider_type` overrides a member in a parent class but is missing an `@override` decorator [missing-override-decorator]
|
Contributor
Pyrefly Type Coverage
|
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
1 participant
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.
Fixes #41805
What problem does this PR solve?
ToolManager.get_builtin_providerfalls back toPluginToolManager.fetch_tool_providerwhen the requested identifier isn't in the builtin cache. The plugin daemon raisesPluginNotFoundError(and the lower-levelPluginDaemonNotFoundError) when the provider is unknown to it, and neither was caught at the boundary. The error bubbled up to the console API generic exception handler as a 500.What is changed and how it works?
Translate both to
ToolProviderNotFoundErrorat theget_plugin_providerboundary so the API returns a controlled 4xx (response matches the existing "missing provider" path that already returnsToolProviderNotFoundError). This is defense in depth behind the frontend fixes in #40686 and #41088 — the Agent V2 MCP-server-id path referenced in the report is no longer exercised, but any other caller hitting the builtin credential endpoints with a non-builtin identifier now gets a clean 4xx instead of a 500.How it was tested?
1 new test (parameterized over both daemon error types):
test_get_plugin_provider_translates_plugin_not_found_to_domain_error[daemon_error0]—PluginNotFoundErrortest_get_plugin_provider_translates_plugin_not_found_to_domain_error[daemon_error1]—PluginDaemonNotFoundErrorBoth confirm the exception is wrapped to
ToolProviderNotFoundErrormatching the "plugin provider not found" message. The existingtest_get_plugin_provider_raises_when_provider_missingtest unchanged (covers theNone-from-fetch path).