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