mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 21:26:39 +00:00
d01538a54c
* fix(reflection): reject malformed variable paths before diagnosing them resolve_variable splits on the last colon and assumed both halves were usable. Two realistic typos fell through into misleading diagnoses: - "json:" reached getattr(module, "") and reported "Module json does not define a attribute/class" -- a blank name in the middle of the sentence. - "json:loads:extra" left the extra colon inside the module part, so the failed import was reported as Missing dependency and the user was told to run uv add json:loads, an instruction that cannot work. Both are path mistakes, so they now get the same format guidance as a path with no colon at all. Every provider, extension and model class path in the repo flows through here, so the message is what an operator sees first. Tests cover the two typos and assert the missing-dependency hint is not offered for them. * fix(reflection): reject relative module paths too, and pin the empty-module half Review follow-ups on the malformed-path check: - A leading-dot path (.foo:bar) is a relative import, and import_module raises TypeError rather than ImportError for it. Every loader built on this function catches ImportError, so the call crashed instead of reporting the path. These paths are absolute by construction, so reject the shape. - ":attr" was already covered by the new condition but had no test, which left the empty-module half unpinned. Added one. Both were mutation-checked: dropping each condition turns exactly its own test red. * fix(reflection): reject whitespace and trailing dots in variable paths too Review follow-up on the shapes that stay inside the ImportError contract but land in the missing-dependency branch: - "json.:attr" advertises a distribution named `json.` - "json :loads" and " json:attr" advertise `uv add json ` / `uv add json`, where the space that IS the typo is invisible in the rendered hint - "json:attr\n" reaches getattr with a name the caller never wrote A dotted module path has no trailing dot and neither half carries whitespace, so these get the same format guidance as the other four shapes. Whitespace is checked inside both halves rather than at the ends, since "json :loads" hides the typo in the middle. Each new condition half is mutation-checked: removing the trailing-dot test fails only the trailing-dot case, and removing both whitespace tests fails exactly the two whitespace cases. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
144 lines
5.5 KiB
Python
144 lines
5.5 KiB
Python
"""Tests for reflection resolvers."""
|
|
|
|
import pytest
|
|
|
|
from deerflow.reflection import resolvers
|
|
from deerflow.reflection.resolvers import resolve_variable
|
|
|
|
|
|
def test_resolve_variable_reports_install_hint_for_missing_google_provider(monkeypatch: pytest.MonkeyPatch):
|
|
"""Missing google provider should return actionable install guidance."""
|
|
|
|
def fake_import_module(module_path: str):
|
|
raise ModuleNotFoundError(f"No module named '{module_path}'", name=module_path)
|
|
|
|
monkeypatch.setattr(resolvers, "import_module", fake_import_module)
|
|
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable("langchain_google_genai:ChatGoogleGenerativeAI")
|
|
|
|
message = str(exc_info.value)
|
|
assert "Could not import module langchain_google_genai" in message
|
|
assert "uv add langchain-google-genai" in message
|
|
|
|
|
|
def test_resolve_variable_reports_install_hint_for_missing_google_transitive_dependency(
|
|
monkeypatch: pytest.MonkeyPatch,
|
|
) -> None:
|
|
"""Missing transitive dependency should still return actionable install guidance."""
|
|
|
|
def fake_import_module(module_path: str):
|
|
# Simulate provider module existing but a transitive dependency (e.g. `google`) missing.
|
|
raise ModuleNotFoundError("No module named 'google'", name="google")
|
|
|
|
monkeypatch.setattr(resolvers, "import_module", fake_import_module)
|
|
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable("langchain_google_genai:ChatGoogleGenerativeAI")
|
|
|
|
message = str(exc_info.value)
|
|
# Even when a transitive dependency is missing, the hint should still point to the provider package.
|
|
assert "uv add langchain-google-genai" in message
|
|
|
|
|
|
def test_resolve_variable_invalid_path_format():
|
|
"""Invalid variable path should fail with format guidance."""
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable("invalid.variable.path")
|
|
|
|
assert "doesn't look like a variable path" in str(exc_info.value)
|
|
|
|
|
|
def test_resolve_variable_rejects_an_empty_variable_name():
|
|
"""A trailing colon is a malformed path, not a missing attribute.
|
|
|
|
``"json:"`` used to reach ``getattr(module, "")`` and report
|
|
``Module json does not define a attribute/class`` -- a blank name in the
|
|
middle of the sentence, which reads as a broken message rather than as the
|
|
path mistake it is.
|
|
"""
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable("json:")
|
|
|
|
message = str(exc_info.value)
|
|
assert "doesn't look like a variable path" in message
|
|
|
|
|
|
def test_resolve_variable_rejects_extra_colons_instead_of_blaming_a_dependency():
|
|
"""``"json:loads:extra"`` used to be diagnosed as a missing dependency.
|
|
|
|
``rsplit(":", 1)`` split into module ``"json:loads"`` and reported
|
|
``Missing dependency 'json:loads'`` with an ``uv add json:loads`` hint --
|
|
advice that cannot work, because the colon is a typo rather than part of a
|
|
distribution name.
|
|
"""
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable("json:loads:extra")
|
|
|
|
message = str(exc_info.value)
|
|
assert "doesn't look like a variable path" in message
|
|
assert "uv add" not in message
|
|
|
|
|
|
def test_resolve_variable_rejects_a_relative_module_path():
|
|
"""A leading dot would escape the ImportError contract entirely.
|
|
|
|
``import_module(".foo")`` raises ``TypeError``, not ``ImportError``, so a
|
|
provider/extension/middleware loader that catches ``ImportError`` crashed
|
|
instead of reporting a bad path. These paths are absolute by construction.
|
|
"""
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable(".foo:bar")
|
|
|
|
message = str(exc_info.value)
|
|
assert "doesn't look like a variable path" in message
|
|
|
|
|
|
def test_resolve_variable_rejects_an_empty_module_half():
|
|
"""``":attr"`` used to reach ``import_module("")`` and surface ValueError."""
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable(":attr")
|
|
|
|
message = str(exc_info.value)
|
|
assert "doesn't look like a variable path" in message
|
|
|
|
|
|
def test_resolve_variable_rejects_a_trailing_dot_in_the_module_path():
|
|
"""``"json.:attr"`` used to advertise a distribution named ``json.``.
|
|
|
|
It stays inside the ImportError contract, so the failure mode is the
|
|
advice: the missing-dependency branch proposes ``uv add json.``, which
|
|
cannot work because a dotted module path never ends in a dot.
|
|
"""
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable("json.:attr")
|
|
|
|
message = str(exc_info.value)
|
|
assert "doesn't look like a variable path" in message
|
|
assert "uv add" not in message
|
|
|
|
|
|
def test_resolve_variable_rejects_stray_whitespace_in_the_module_path():
|
|
"""A space inside the module half is invisible in the rendered hint.
|
|
|
|
``"json :loads"`` (a pasted sentence fragment) reached the
|
|
missing-dependency branch and suggested ``uv add json `` -- the trailing
|
|
space is the typo and is not visible in the message.
|
|
"""
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable("json :loads")
|
|
|
|
message = str(exc_info.value)
|
|
assert "doesn't look like a variable path" in message
|
|
assert "uv add" not in message
|
|
|
|
|
|
def test_resolve_variable_rejects_surrounding_whitespace_in_the_path():
|
|
"""Leading/trailing space around the whole path is the same mistake."""
|
|
for path in (" json:attr", "json:attr ", "json:attr\n"):
|
|
with pytest.raises(ImportError) as exc_info:
|
|
resolve_variable(path)
|
|
message = str(exc_info.value)
|
|
assert "doesn't look like a variable path" in message, path
|
|
assert "uv add" not in message, path
|