CCOR-13508 - fix(agents): prefer the importable function in a tool closure

src/conductor/ai/agents/runtime/_worker_entries.py

@@ -67,35 +67,69 @@ def _walk_qualname(module_obj, qualname: str):
 # .coroutine; our Guardrail / ToolDef → .func.
 _CONTAINER_ATTRS = ("func", "coroutine")
 
+def _is_importable_function(fn: Callable) -> bool:
+    """Whether *fn* can be referenced by module + qualified name.
+
+    Mirrors the condition :meth:`FunctionRef.of` enforces (it raises a
+    per-case ``SpawnSafetyError`` rather than returning a bool, so the two
+    can't simply share a body). Factored out so the closure walk can *prefer*
+    a candidate that will actually survive the trip to a spawn child.
+    """
+    qualname = getattr(fn, "__qualname__", None)
+    return bool(
+        getattr(fn, "__module__", None)
+        and qualname
+        and "<locals>" not in qualname
+        and "<lambda>" not in qualname
+    )
+
+
 def _extract_from_closure(func: Callable) -> Optional[Callable]:
     """Extract the original user function from a closure's cell variables.
 
     - Shared by :func:`_find_embedded_function` below.
     - In turn shared by :mod:`conductor.ai.agents.frameworks.serializer`'s
       discovery and :class:`FunctionRef`'s parent-verification /
       child-reconstruction — one implementation, can't drift apart.
+    - Preference-ordered, not first-match: one closure can hold several plain
+      functions, with the framework's own nested helpers sitting alongside the
+      user's function. An importable candidate always wins over a nested one,
+      because importability is exactly what the caller needs
+      (``FunctionRef.of``) and is a property of the candidate itself rather
+      than a guess from its parameter names. openai-agents 0.22.3 is why:
+      it added a nested ``_prepare_arguments(input, tool_name)`` beside
+      ``the_func`` in the same closure, and ``input`` is not ``ctx``, so
+      first-match started returning the framework's wrapper.
+    - The nested-candidate fallback is kept for closures that genuinely hold
+      no importable function: callers get the same value (and the same
+      actionable ``SpawnSafetyError`` from ``FunctionRef.of``) as before.
     """
     closure = getattr(func, "__closure__", None)
     if not closure:
         return None
 
+    fallback = None
     for cell in closure:
         try:
             val = cell.cell_contents
         except ValueError:
             continue
-        if inspect.isfunction(val):
-            # Skip internal wrappers that take (ctx, input) or (context, ...)
-            try:
-                sig = inspect.signature(val)
-                param_names = list(sig.parameters.keys())
-                # Internal wrappers typically start with ctx/context as first param
-                if param_names and param_names[0] in ("ctx", "context"):
-                    continue
-                return val
-            except (ValueError, TypeError):
+        if not inspect.isfunction(val):
+            continue
+        # Skip internal wrappers that take (ctx, input) or (context, ...)
+        try:
+            sig = inspect.signature(val)
+            param_names = list(sig.parameters.keys())
+            # Internal wrappers typically start with ctx/context as first param
+            if param_names and param_names[0] in ("ctx", "context"):
                 continue
-    return None
+        except (ValueError, TypeError):
+            continue
+        if _is_importable_function(val):
+            return val
+        if fallback is None:
+            fallback = val
+    return fallback
 
 
 # How many attribute-nesting levels _find_embedded_function will descend.
@@ -118,6 +152,12 @@ def _find_embedded_function(obj: Any, max_depth: int = _DEEP_EXTRACT_MAX_DEPTH)
       shape-based walk degrades to "not found" (the pre-existing, actionable
       ``SpawnSafetyError``) instead of breaking outright if openai-agents
       restructures its internals.
+    - "Degrades to not found" only holds while the walk can't mistake a
+      framework helper for the user's function. openai-agents 0.22.3 showed
+      the failure mode — a new nested helper in the same closure was returned
+      *instead of* the user's function, which is worse than None (the
+      serializer would have registered a worker around the wrapper). See
+      :func:`_extract_from_closure` for the preference rule that fixes it.
     """
     if max_depth <= 0:
         return None

tests/unit/ai/test_worker_entries.py

@@ -225,6 +225,49 @@ def test_cross_process_spawn_roundtrip(self):
         assert p.exitcode == 0
 
 
+class TestClosureCandidatePreference:
+    """_extract_from_closure prefers an importable candidate over a nested one.
+
+    Always-runs counterpart to TestFunctionRefDeepExtract above, which is
+    importorskip'd on the openai-agents extra and pins whatever internals the
+    installed version happens to have. openai-agents 0.22.3 broke the old
+    first-match walk by adding a nested ``_prepare_arguments(input, tool_name)``
+    beside the user's function in one closure — `input` is not `ctx`, so the
+    wrapper was returned instead. These pin the rule, not a library's shape.
+    """
+
+    def test_prefers_importable_over_nested_helper(self):
+        target = helpers.plain_sample  # local binding -> a real closure cell
+
+        def _prepare_arguments(input: str, tool_name: str) -> dict:
+            return {"input": input, "tool": tool_name}
+
+        def impl():
+            return _prepare_arguments, target
+
+        # co_freevars is alphabetical, so the nested helper is seen first —
+        # the same adverse ordering openai-agents 0.22.3 produces.
+        assert impl.__code__.co_freevars == ("_prepare_arguments", "target")
+        found = we._extract_from_closure(impl)
+        assert found is helpers.plain_sample
+        assert FunctionRef.of(found).resolve() is helpers.plain_sample
+
+    def test_falls_back_to_nested_when_nothing_importable(self):
+        # Closures holding no importable function keep the old behaviour: the
+        # nested candidate is still returned, so callers get the same
+        # actionable SpawnSafetyError from FunctionRef.of as before.
+        def nested(city: str) -> str:
+            return city
+
+        def impl():
+            return nested
+
+        found = we._extract_from_closure(impl)
+        assert found is nested
+        with pytest.raises(SpawnSafetyError, match="defined inside a function"):
+            FunctionRef.of(found)
+
+
 # ── Guardrail spawn transport ─────────────────────────────────────────────