From d01538a54c9d54556f75988585cf2daca276cc5e Mon Sep 17 00:00:00 2001 From: yetuge <182123929+yetuge@users.noreply.github.com> Date: Thu, 22 Oct 2026 08:05:17 +0800 Subject: [PATCH] fix(reflection): reject malformed variable paths before diagnosing them (#7016) * 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 --- .../harness/deerflow/reflection/resolvers.py | 30 ++++++ backend/tests/test_reflection_resolvers.py | 94 +++++++++++++++++++ 2 files changed, 124 insertions(+) diff --git a/backend/packages/harness/deerflow/reflection/resolvers.py b/backend/packages/harness/deerflow/reflection/resolvers.py index b5b33967aa..947ffc5638 100644 --- a/backend/packages/harness/deerflow/reflection/resolvers.py +++ b/backend/packages/harness/deerflow/reflection/resolvers.py @@ -44,6 +44,36 @@ def resolve_variable[T]( module_path, variable_name = variable_path.rsplit(":", 1) except ValueError as err: raise ImportError(f"{variable_path} doesn't look like a variable path. Example: parent_package_name.sub_package_name.module_name:variable_name") from err + # A path carries exactly one colon. Without this check the split above + # misfiles the two realistic typos: a trailing colon yields an empty + # attribute name ("Module json does not define a attribute/class"), and an + # extra colon leaves it inside the module part, where the failed import is + # reported as a missing dependency with an unusable `uv add json:loads` + # hint. Both are path mistakes, so they get the path-format message. + # + # A leading dot is the third shape: it is a relative import, and + # import_module raises TypeError ("the 'package' argument is required") + # rather than ImportError, so callers that catch ImportError -- every + # provider/extension/middleware loader -- would see an unhandled crash. + # These paths are absolute by construction, so reject it here too. + # + # Stray whitespace and a trailing dot are the fourth: they stay inside the + # ImportError contract but land in the missing-dependency branch, which + # then advertises a distribution named `json.` or `json ` -- text that + # cannot work, and where the space is invisible in the rendered hint. A + # dotted module path has neither, so the same format guidance applies. + # Whitespace is checked inside both halves (not just at the ends), because + # `"json :loads"` and `"json:attr\n"` each hide the typo on one side. + if ( + not module_path + or not variable_name + or ":" in module_path + or module_path.startswith(".") + or module_path.endswith(".") + or any(character.isspace() for character in module_path) + or any(character.isspace() for character in variable_name) + ): + raise ImportError(f"{variable_path} doesn't look like a variable path. Example: parent_package_name.sub_package_name.module_name:variable_name") try: module = import_module(module_path) diff --git a/backend/tests/test_reflection_resolvers.py b/backend/tests/test_reflection_resolvers.py index 8ce0ea6a77..8fb473d070 100644 --- a/backend/tests/test_reflection_resolvers.py +++ b/backend/tests/test_reflection_resolvers.py @@ -47,3 +47,97 @@ def test_resolve_variable_invalid_path_format(): 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