diff --git a/CHANGELOG.md b/CHANGELOG.md index c1d3d6470..c2ca74469 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,11 @@ - **What stays unresolved, by name.** A module the scope does not contain (the scope spelled from the repository root, `svc.app.tools` with scope `svc/app`, is read inside it), a relative import above the scope, more than one matching module location, a name bound twice or only inside an `if`/`try`, a wildcard import, an import cycle, a class or other value, a parameter or other local assignment of the scope that uses the name, a name that scope binds more than once, a module attribute the same module reassigns (`tools.lookup = ...`, `setattr`), an attribute named like a step of the chain on an imported module that code running first reassigns (every module on the chain, the agent's own file included, every enclosing package's `__init__.py`, and every in-scope module those import; the defining module handing its function on does not count), and such a module that cannot be read (a link, or a missing relative module not imported under `except ImportError`), an SDK function without `@function_tool`, a symbolic link, or more than 64 modules read. The gap names the reason (`Not resolved because …`) and is scoped to the agent that lists the tool, so another agent's change in the same file is still established. One agent binding two different functions under one name is named, not resolved. The ADK unresolved-tool warning keeps its wording. No schema or contract change. - **Identity.** One agent binding two different functions under one name binds neither, in both readers and whatever their order, and names both definitions. `import a.b` then `a.b.f` reads the submodule, as the import system does. A reference is read where it is used: a builder's own import is followed like a module-level one, `nonlocal` follows the outer function, a nested `def` that is the only one of its name is that definition, a module-level agent binds what the module binds at top level (its `def`, its import, its wrapper assignment) and never a same-named `def` or wrapper nested in a function, a module-level list's names are read at module level whatever the building function binds, and a factory's own toolset or wrapper variable is read like a module-level one. An SDK list variable is read only when the scope that binds it binds it once, to a literal list, and every use of it in the file only reads it — iterated, indexed, compared, tested, handed to a read-only builtin, standard-library reader or logger's method (each proven by its binding), to an agent's (or a copy's) own `tools=`, or to a function whose every use of that parameter is such a read. Spreading it (`[*TOOLS, x]`, `f(*TOOLS)`) or testing it (`TOOLS or []` in a condition) is a read. A method call on it, `+=`, a second name (including through `x or y`), a tuple, a return, `*args`, `globals()`, `sys.modules` or importing the module by `__name__` makes it dynamic. For `scan`, a definition that an import reaches and another configured source also reads (spelling the module's path the same way) is one catalog tool, and the binding reaches it through the exact definition the reader resolved; a `{tool: …}` selector for it is not ambiguous, and the dropped copy's guard evidence goes with it. A source an inventory completes keeps its own observation, so when that source imports a definition another source also reads, the catalog holds both and a selector for it is ambiguous. When what the module binds is not established (a name rebound, or bound only inside an `if`) and the ADK reader falls back to a same-named `def` or wrapper, that binding is named but never established: in a comparison its row is `not_established` on whichever side it is present, added, removed or changed, and so is the row of every tool the module's bindings of that name could give the agent instead, each followed to its definition (a function from a module outside the scope by its imported name); every one of the agent's rows is when one of those cannot be followed or a wildcard import could bind the name; for `scan` it stays the medium-confidence shadowed definition it was. `x = FunctionTool(func=x)` right after `def x` wraps that `def`, and is not a guess. Code that runs before the name is used but is not read — a relative import above the scope, an absolute import of a module the repository holds outside the scope (read from the compared commit's tree, or the checkout for `scan`, at the root, under `src/`, or under any directory between the root and the scope, modules the interpreter preloads, a standard-library name at a root that is a regular package, and the scope's own package aside; a linked or submodule entry counts, and a namespace directory only when it holds the named submodule, so SDK apps under `agents/` still import the SDK), a relative module no file provides (a generated `*_pb2` or `_version` aside), or a package `__getattr__` that is not the lazy-submodule idiom (or that the package could subvert through `sys.modules`, `globals()`, `__name__` or a patched `importlib`) — keeps the tool named. The `__init__.py` of every package above the scope, and what each imports (every package on the way to it and each submodule named), are read too; there a reassignment counts unless it sets one attribute of another module file outside the scope, a module of the scope they import is read like the chain's own, a change to `__path__` is a caveat (`pkgutil.extend_path` aside), their files are read in batches, and what they import in turn, or import by name at run time, is not followed. `sys.modules` and `globals()` are read by allow-list: a store whose key names a module on the chain, a package above one or the framework's own modules is a named stop, a module rebinding its own name through them or its own module object (however spelled, including `__dict__` and `vars()` stores) is a reassignment, reads (a comparison, iteration, a spread, `pkgutil.iter_modules(__path__)`, `get_type_hints(globalns=globals())`) are nothing, and any other use (`mods = sys.modules`, `|=`, a computed key or `setattr`, the module object anywhere but an attribute, a plain alias or a reader, `__dict__.update`, a frame's `f_globals`, `builtins.globals` under another name) or a change to `__path__` is a caveat: its row is `not_established` with the reason, including through a `FunctionTool` wrapper, and for `scan` the ADK module stays at medium. Attest's lazy loaders (`importlib.import_module(f".{name}", __name__)`, `if name == "x": from . import x`) stay established; `from . import other as x` is not the idiom. When a definition another source reads under its own name is the one the reader resolved, the binding reaches it through its exact locator, never a same-named local definition. The module-binding walk and the SDK list reader are linear in the tree, and a chain of thousands of attributes no longer crashes the run. +- Read a Google ADK tool a local factory builds, and a tools list built in the agent's function. (#865) + - **The problem.** visulate/visulate-for-oracle#526 bound `save_memory_tool = create_save_memory_tool()` and `read_memory_tool = create_read_memory_tool()` in its root agent's builder; each factory returns `FunctionTool` around a nested async function, and the reader stopped at the local assignment, so the two new memory tools were unresolved like the agent's nine others. MuhammadVT/smart-assignment#46 built `tools = [...]` in the agent's function and appended a triage tool under `if triage_enabled:`; the reader read the whole list as a dynamic tools expression, losing its unconditional tools. + - **Factories.** A factory call — `tool = create_tool()` bound once and unconditionally in the agent's function or at a module's top level, or `tools=[create_tool()]` — is followed, as syntax and never run, to the factory's one unconditional `return`: `FunctionTool(inner)` / `LongRunningFunctionTool(inner)`, a plain function, a name bound once to one of those, or another factory's call, up to four factories deep. The tool is the function wrapped — one nested in the factory and defined before the `return`, or one the factory's module binds — with its own signature and location; `import_path` records the factory as a step, and the factory's code and the values each of the agent's calls gives its parameters (a literal, or a name or module attribute bound once to one; defaults filled in), which its closure holds, are part of its implementation digest, so `make_sql_tool(readonly=False)` in place of `readonly=True` is a changed tool and the same call spelled otherwise is not; a value this read cannot name is a limit on that tool naming the argument, and a `not_established` row only when the calling module changed. + - **What stays named, with why.** A factory that returns from more than one place or only under a condition, calls itself on the way, is decorated, a generator or `async`; a wrapped function that is decorated, a parameter, defined more than once in its file (a tool is known by its name there), or changed or handed on in the factory (`inner.__name__ = ...`, `setattr`, a helper), a module function included; a tool changed or handed on where it is bound (`tool.name = ...`, `rename(tool)`); and a factory the repository holds outside the read scope, named as such. Any other call — a third-party package, a class, a name bound twice or through a wildcard — keeps the answer it had. On visulate#526 with `--scope ai-agent`, the memory tools are read, and the remote-delegate factory, which renames the function it returns per call, leaves the nine delegates named with that reason — so the two memory tools are `not_established` candidate additions, not a complete eleven-tool surface; they were not rows before. + - **A tools list.** `tools = [a, b]` (or a tuple) bound once in the agent's function, with `tools.append(c)`, `.extend([...])`, `.insert(i, c)` or `tools += [...]` statements, is read member by member up to the statement that builds the agent, which copies the list, so an addition after it — or after an early `return` of another agent — is not that agent's. An addition before it under a condition or in a loop is named on the agent, which is then not complete, and is never read as bound. Any other use of the list — handed to a call, aliased, returned, changed another way, read by a nested function, a starred member — and a module-level list keep the dynamic tools expression. On smart-assignment#46 the unconditional tools are read and the triage tool is named as conditional. ### Changes - Compare exact repository script bytes for supported, selected Claude Code and Codex hook executable references. A script-only edit produces an attributable hook row without asserting a permission expansion; its `why` names the direction as not established (#820), so the Claude Code Stop hook lists it apart from widenings rather than staying silent; `check` routes the dependency for review from either compared revision. The documented shell spellings resolve (`"$CLAUDE_PROJECT_DIR"/path`, `"${CLAUDE_PROJECT_DIR}"/path`, the variable and path quoted together, or unquoted with a plain path, and `CLAUDE_PLUGIN_ROOT` alike in a plugin's hooks). A script this entry cannot read withholds only its own bytes: an unchanged shared limit, or a script absent from both sides, is an `unchanged_limits` entry, and any other makes the comparison `partial` with every grant still compared. A script whose working-tree bytes differ only by a checkout's line-ending conversion is `unchanged_not_proven`, not a row. A selected hook whose script is not resolved (an interpreter wrapper, a relative path, a conditional expansion, a compound command or a malformed group) is named as a `script_not_resolved` coverage item while the change could touch it, never a row. A pre-#702 baseline is incomparable only for a hook that now binds a script. `check` and a provided diff (`check --diff`) leave out a selected script the change does not touch, so an untouched missing, ignored or linked script changes no `check` decision; a touched one is routed for review, and compared from the diff where the diff also touches its declaration. Verification binds ignored inputs and missing-path observations, and refuses current authority for unsafe reads. Mode-only changes and recursive dependencies are not compared. (#702) diff --git a/docs/application-comparison.md b/docs/application-comparison.md index 673f0120f..d2de0d992 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -223,6 +223,46 @@ module's SHA-256. `import_path` is evidence, not compared meaning — moving an import is not a change. An OpenAI Agents SDK definition must carry the SDK's `@function_tool`. +A Google ADK tool built by a factory is the function the factory wraps. A +factory call — `tool = create_tool()` bound once and unconditionally in the +agent's function or at a module's top level, or `tools=[create_tool()]` — is +read, never run, to the factory's one unconditional `return`: +`FunctionTool(inner)` / `LongRunningFunctionTool(inner)`, a plain function, a +name bound once to one of those, or another factory's call, up to four +factories deep. The function is one nested in the factory and defined before +the `return`, or one the factory's module binds; `import_path` records the +factory as a step, and the factory's code and the values each of this +agent's calls gives its parameters — what the tool's closure holds — are +part of its implementation digest: a literal, or a name or module attribute +bound once to one (a list, dict or set — at any depth, or through another +name — only when nothing else in its scope uses it, never a module's), a +function by its own code (not the helpers it calls, as for any tool), or a +parameter of the factory the call is made in, with the factory's defaults for +the rest. So +`make_sql_tool(readonly=False)` in place of `readonly=True` is a changed +tool, while spelling the same call otherwise (an alias, a keyword, the +default written out), a docstring, or another agent's call is not. A value +this read cannot name — a builder's parameter, a computed value — is named +as a limit on that agent's tool, with the arguments and where they are +given; a row appears, `not_established`, only when the calling module or the +factory changed — a value set from another module leaves only the limit. A +module constant the factory's own body reads is not part of it, as for any +function. A factory that returns from more than one place or +under a condition, calls itself, is decorated, a generator or `async`, a +wrapped function that is decorated, a parameter, defined more than once in its +file (a tool is known by its name there), or changed or handed on in the +factory (`inner.__name__ = ...` renames the tool, for a module function, +`impl.search` or a function-local import too), a tool changed or handed on +where it is bound (reading `tool.name`, listing it in a tools list, or +wrapping it in `FunctionTool(func=...)` is not), and a factory the repository +holds outside the scope stay named with why; any other call — a third-party +package, a class, a name bound twice — is what it was. A tools list built in +the agent's function — `tools = [a, b]` with `tools.append(c)`, +`.extend([...])`, `.insert(i, c)` or `tools += [...]` — is read member by +member up to the statement that builds the agent, which copies it; an addition +under a condition or in a loop before it is named on the agent, never read as +bound, and any other use of the list keeps it a dynamic tools expression. + The boundary is narrow. Only regular `.py` files inside the selected scope are read, parsed with `ast` and never imported or run. Symbolic links are not followed, and a module name must match a file's exact spelling. An absolute diff --git a/docs/manifest-v0.1.md b/docs/manifest-v0.1.md index e2d03d884..bd07e505a 100644 --- a/docs/manifest-v0.1.md +++ b/docs/manifest-v0.1.md @@ -144,6 +144,8 @@ Supported static ADK signals: - Python `Agent` / `LlmAgent` definitions with literal `tools=[...]`. - Plain function tools referenced in an agent tools list. - `FunctionTool(func=...)` and `LongRunningFunctionTool(func=...)` wrappers. +- A tool a local factory returns (`tool = create_tool()`), read to the factory's one unconditional `return FunctionTool(inner)`. +- A tools list built in the agent's function with `append` / `extend` / `insert` / `+=`; an addition under a condition is named, not bound. - `OpenAPIToolset` when a local spec path can be resolved from a literal path or `Path("...").read_text()`. - `McpToolset` metadata, including static `tool_filter` and explicit `inventory_path` / `tool_inventory_path` hints. - Agent Config YAML `tools`, `sub_agents`, callbacks, plugins, and local config references. diff --git a/src/agents_shipgate/cli/application_diff.py b/src/agents_shipgate/cli/application_diff.py index aa292e59c..e5d09c58f 100644 --- a/src/agents_shipgate/cli/application_diff.py +++ b/src/agents_shipgate/cli/application_diff.py @@ -160,7 +160,7 @@ def _source_path(root: Path, ref: str) -> str: return ref -def _definition(root: Path, tool: Any) -> dict[str, Any]: +def _definition(root: Path, tool: Any, agent: str | None = None) -> dict[str, Any]: """Read a unique function's AST; line movement/comments are not changes.""" path = _source_path(root, tool.source_ref or "") symbol = tool.annotations.get("python_symbol") @@ -203,17 +203,32 @@ def _definition(root: Path, tool: Any) -> dict[str, Any]: node.body = body # Include defaults and decorators: approval decorators and changed default # bounds are review-relevant even when the executable body is unchanged. - return { + code = ast.dump( + node, + include_attributes=False, + **({"show_empty": True} if sys.version_info >= (3, 13) else {}), + ) + # A tool a factory makes also holds what the factory and this agent's + # calls to it put in its closure (#865 review). + calls = (getattr(tool, "extraction", None) or {}).get("factory_calls") + made_by = [str(item) for item in calls.get(agent, [])] if isinstance(calls, dict) and agent is not None else [] + # ``unknown::``: the call as written is compared, + # the value it names is not — an open question, not a change. + unnamed = [item.split(":", 2)[2] for item in made_by if item.startswith("unknown:")] + if made_by: + code += "|factory:" + ",".join(item.split(":", 2)[1] if item.startswith("unknown:") else item for item in made_by) + result = { "source": path, "line": node.lineno, - "implementation_sha256": _digest( - ast.dump( - node, - include_attributes=False, - **({"show_empty": True} if sys.version_info >= (3, 13) else {}), - ) - ), + "implementation_sha256": _digest(code), } + if unnamed: + result["unestablished"] = ( + f"A factory call that makes {tool.name} gives it a value this read cannot name " + f"({'; '.join(sorted(set(unnamed)))}), which its closure holds; whether that value " + "changed is not established." + ) + return result #: Bytes one repository-directory listing may take. @@ -803,7 +818,15 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) if key in ambiguous_agents: continue tool = tool_by_id[edge.tool_id] - definition = _definition(root, tool) + definition = _definition(root, tool, key[1]) + if definition.get("unestablished"): + result.gap( + definition["unestablished"], + source=key[0], + agent=key[1], + tool=tool.name, + affects="implementation", + ) if definition["implementation_sha256"] is None: result.gap( f"Implementation location unresolved: {tool.name} ({tool.source_ref}).", @@ -1000,6 +1023,13 @@ def compare( for side, value in (("base", before), ("head", after)): if "definition" in value and value["definition"]["implementation_sha256"] is None: uncertainty[side] = ["The bound callable's implementation could not be read."] + if before_meaning != _meaning(after): + # A factory value this read cannot name moved with the code + # around it: a candidate change, not an established one. + for side, value in (("base", before), ("head", after)): + reason = value.get("definition", {}).get("unestablished") + if reason: + uncertainty.setdefault(side, []).append(reason) for side, observed in (("base", base), ("head", head)): reasons = observed.tool_gaps(key) if reasons: @@ -1195,7 +1225,9 @@ def _published_binding(binding: dict[str, Any] | None, scope: str) -> dict[str, if "target_source" in result: result["target_source"] = _location(scope, result["target_source"]) if "definition" in result: - result["definition"] = dict(result["definition"]) + # Why the implementation is unknown is published as the row's + # uncertainty, not as a field of the definition. + result["definition"] = {key: value for key, value in result["definition"].items() if key != "unestablished"} result["definition"]["source"] = _location(scope, result["definition"]["source"]) if "import_path" in result: result["import_path"] = [ diff --git a/src/agents_shipgate/inputs/google_adk.py b/src/agents_shipgate/inputs/google_adk.py index 5da0f2017..6781685ef 100644 --- a/src/agents_shipgate/inputs/google_adk.py +++ b/src/agents_shipgate/inputs/google_adk.py @@ -1,7 +1,11 @@ from __future__ import annotations import ast +import copy as copy_module import dataclasses +import hashlib +import json +import sys from collections.abc import Callable from dataclasses import dataclass, field from pathlib import Path @@ -38,6 +42,7 @@ from agents_shipgate.inputs.openapi import load_openapi_tools from agents_shipgate.inputs.protocol import LoadedAdapterResult from agents_shipgate.inputs.python_imports import ( + FACTORY_RETURN, LOCAL_BINDING, MODULE_NOT_FOUND, NOT_BOUND, @@ -1011,6 +1016,12 @@ def __init__( # ``(aliases, name bindings)`` per defining module, for the annotation # and shadowing checks that module's own spelling decides. self.module_names: dict[str, tuple[dict[str, str], dict[str, list[ast.AST]]]] = {} + # Scope indexes of the modules a tool factory is read in (#865). + self.module_scopes: dict[str, ScopeIndex] = {} + # ``(module, name) -> line`` where a module-level factory tool is changed. + self.module_changes: dict[tuple[str, str], int | None] = {} + # ``module -> name -> lines`` of the functions a tool can be made from. + self.module_function_lines: dict[str, dict[str, list[int]]] = {} # Tool names produced by one toolset construction, keyed by the AST # call node. A toolset assigned to a variable and shared between # agents is loaded once, not once per agent. @@ -1083,6 +1094,30 @@ def extract(self) -> list[LoadedToolSource]: handoffs_at = len(self.artifacts.sub_agents) self._record_agent_callbacks_plugins_subagents(call, agent_name) handoffs = self.artifacts.sub_agents[handoffs_at:] + flowed = self._local_tool_list(tools_expr, call) if isinstance(tools_expr, ast.Name) else None + if flowed is not None: + # ``tools = [...]`` built in the agent's function (#865): its + # members read as a literal list would be. + elements, conditional = flowed + self.artifacts.agents[-1]["tool_count"] = len(elements) + len(conditional) + binding = self._binding_for(agent_name, call) + loaded_sources.extend( + self._read_construction( + call, agent_name, binding, ast.List(elts=elements, ctx=ast.Load()), tools, handoffs + ) + ) + for line in conditional: + message = ( + f"Google ADK agent {agent_name!r} adds a tool to its tools list only under a " + f"condition or in a loop at {self.source_ref}:{line}, which is not established." + ) + self._surface_warning(message, SURFACE_GAP_DYNAMIC_TOOLS) + if message not in binding.issues: + binding.issues.append(message) + if conditional: + # Not the same agent as another construction of its name. + self.agent_sites.setdefault(agent_name, {})[id(call)] = (call.lineno, call) + continue if not isinstance(tools_expr, (ast.List, ast.Tuple)): if tools_expr is not None: self._surface_warning( @@ -1233,6 +1268,101 @@ def _record_agent_subclasses(self) -> None: SURFACE_GAP_AGENT_SUBCLASS, ) + def _local_tool_list(self, name: ast.Name, agent: ast.Call) -> tuple[list[ast.expr], list[int]] | None: + """The members of ``tools = [a, b]`` bound once in the agent's function, + with ``tools.append(c)`` / ``.extend([...])`` / ``.insert(i, c)`` / + ``tools += [...]`` statements, and the lines of those made under a + condition or in a loop (#865). + + None — the dynamic tools expression it was — for any other use of the + list in that function: handed to a call, returned, aliased, changed + another way, read by a nested function, or a starred member. + + The agent copies the list when it is built, so only additions before + the statement that builds it count; one in the same compound + statement is named (#865 review). + """ + + # ``tools += [...]`` binds the name too; read below as an addition. + found = [ + item + for item in self.scopes.enclosing_bindings(name, name.id) + if not isinstance(self.scopes.parents.get(item), ast.AugAssign) + ] + if len(found) != 1 or not isinstance(found[0], ast.Name): + return None + statement = self.scopes.statement_of(found[0]) + value = getattr(statement, "value", None) + if not ( + statement is not None + and ( + (isinstance(statement, ast.Assign) and statement.targets == [found[0]]) + or (isinstance(statement, ast.AnnAssign) and statement.target is found[0]) + ) + and isinstance(value, ast.List | ast.Tuple) + and _unconditional(statement, self.scopes) + ): + return None + function = self.scopes.parents[statement] + body = list(getattr(function, "body", [])) + built_at = _top_statement(agent, function, self.scopes.parents) + if built_at not in body: + return None + elements: list[ast.expr] = list(value.elts) + conditional: list[int] = [] + for node in ast.walk(function): + if not isinstance(node, ast.Name) or node.id != name.id or node is found[0]: + continue + if self._enclosing_scope(node) is not function: + return None + parent = self.scopes.parents.get(node) + if isinstance(parent, ast.keyword) and parent.arg == "tools": + call = self.scopes.parents.get(parent) + if isinstance(call, ast.Call) and self._is_agent_call(call): + continue + return None + added: list[ast.expr] | None = None + change: ast.stmt | None = None + if isinstance(parent, ast.AugAssign) and parent.target is node and isinstance(parent.op, ast.Add): + if isinstance(parent.value, ast.List | ast.Tuple): + added, change = list(parent.value.elts), parent + elif isinstance(parent, ast.Attribute) and parent.value is node and isinstance(node.ctx, ast.Load): + call = self.scopes.parents.get(parent) + holder = self.scopes.parents.get(call) if call is not None else None + if isinstance(call, ast.Call) and call.func is parent and isinstance(holder, ast.Expr) and not call.keywords: + if parent.attr == "append" and len(call.args) == 1: + added, change = [call.args[0]], holder + elif parent.attr == "insert" and len(call.args) == 2: + added, change = [call.args[1]], holder + elif ( + parent.attr == "extend" + and len(call.args) == 1 + and isinstance(call.args[0], ast.List | ast.Tuple) + ): + added, change = list(call.args[0].elts), holder + if added is None or change is None: + return None + top = _top_statement(change, function, self.scopes.parents) + if top not in body or body.index(top) > body.index(built_at): + # After the agent is built: not its list any more. + continue + if top is change and body.index(top) < body.index(built_at): + elements += added + else: + conditional.append(change.lineno) + if any(isinstance(item, ast.Starred) for item in elements): + return None + return elements, conditional + + def _enclosing_scope(self, node: ast.AST) -> ast.AST | None: + current = self.scopes.parents.get(node) + while current is not None and not isinstance( + current, ast.FunctionDef | ast.AsyncFunctionDef | ast.Lambda | ast.ClassDef | ast.Module + | ast.ListComp | ast.SetComp | ast.DictComp | ast.GeneratorExp + ): + current = self.scopes.parents.get(current) + return current + def _surface_warning(self, message: str, reason: str) -> None: """Report a construct that leaves part of this module's surface unknown.""" @@ -1781,6 +1911,24 @@ def _extract_call_expr( return [] if call_name in OPENAPI_TOOLSET_NAMES | MCP_TOOLSET_NAMES: return self._extract_toolset_call(expr, agent_name, binding) + spelling = reference_spelling(expr.func) + made = ( + self._factory_call(expr, self.module, f"{spelling}()") + if spelling is not None and self.resolver is not None and self.module is not None + else None + ) + if made is not None: + # ``tools=[create_tool()]``: what the factory returns (#865). + resolution, long_running = made + if resolution.resolved: + self._bind_resolved(resolution, tools, agent_name, binding, long_running) + return [] + warning = ( + f"Google ADK agent {agent_name!r} has a tool expression that could not be statically resolved." + ) + self._surface_warning(warning, SURFACE_GAP_UNRESOLVED_EXPRESSION) + self._record_unresolved_reference(warning, agent_name, f"{spelling}()", resolution) + return [] self._surface_warning( f"Google ADK agent {agent_name!r} has a tool expression that could not be statically resolved.", SURFACE_GAP_UNRESOLVED_EXPRESSION, @@ -1829,6 +1977,9 @@ def _local_meaning( recorded = self.wrappers.get(name, {}).get("call") or self.toolset_assignments.get(name) if value is not None and value is recorded: return "flat" + made = self._local_factory(local, statement, spelling) + if made is not None: + return made return ( Resolution( reference=spelling, @@ -1838,6 +1989,69 @@ def _local_meaning( False, ) + def _wrapped_function(self, node: ast.Name) -> bool: + """``FunctionTool(func=fn, require_confirmation=True)`` with ``fn`` a + factory's function: the wrapper reads it.""" + + parent = self.scopes.parents.get(node) + call = self.scopes.parents.get(parent) if isinstance(parent, ast.keyword) else parent + return ( + isinstance(call, ast.Call) + and _qualified_name(call.func, self.aliases) in FUNCTION_TOOL_NAMES | LONG_RUNNING_TOOL_NAMES + and _call_func_expr(call) is node + ) + + def _local_factory( + self, local: ast.Name, statement: ast.stmt | None, spelling: str + ) -> tuple[Resolution, bool] | None: + """``tool = create_tool()`` in the agent's own function, bound once and + unconditionally: the tool the factory returns (#865). None when the + call is not to application code, or the binding is conditional.""" + + value = getattr(statement, "value", None) + if ( + self.resolver is None + or self.module is None + or statement is None + or not isinstance(value, ast.Call) + or not ( + (isinstance(statement, ast.Assign) and statement.targets == [local]) + or (isinstance(statement, ast.AnnAssign) and statement.target is local) + ) + or not _unconditional(statement, self.scopes) + ): + return None + made = self._factory_call(value, self.module, spelling) + if made is None: + return None + plain = not any(step.get("returns_tool") for step in made[0].steps) + function = self.scopes.parents[statement] + changed = _changed_at( + function, + local.id, + self.scopes.parents, + bound=local, + allowed=lambda node: _in_tools_argument(node, self.scopes.parents, self._is_agent_call) + or _in_local_list(node, self.scopes.parents, self._is_agent_call) + # Wrapping a function it returns; wrapping a tool again is not a tool. + or (plain and self._wrapped_function(node)), + ) + if changed is not None: + return ( + Resolution( + reference=spelling, + reason=FACTORY_RETURN, + detail=( + f"{spelling!r}, the tool {reference_spelling(value.func)!r} returns at " + f"{self.source_ref}:{statement.lineno}, is changed or handed on at " + f"{self.source_ref}:{changed}" + ), + steps=made[0].steps, + ), + False, + ) + return made + def _module_level_flat(self, name: str) -> str | None: """Whether the flat maps describe ``name``'s module-level binding. @@ -2092,7 +2306,53 @@ def _resolve_reference(self, spelling: str) -> tuple[Resolution | None, bool]: if self.resolver is None or self.module is None: return None, False - return self._through_wrapper(self.resolver.resolve(self.module, spelling)) + resolution, long_running = self._through_wrapper(self.resolver.resolve(self.module, spelling)) + value, home = resolution.value, resolution.module + if resolution.resolved or not isinstance(value, ast.Call) or home is None: + return resolution, long_running + aliases, _ = self._names_of(home) + if _qualified_name(value.func, aliases) in ( + FUNCTION_TOOL_NAMES | LONG_RUNNING_TOOL_NAMES | OPENAPI_TOOLSET_NAMES | MCP_TOOLSET_NAMES + ): + return resolution, long_running + # ``tool = create_tool()`` at a module's top level: what the factory + # returns (#865), unless the module changes the tool afterwards. + made = self._factory_call(value, home, spelling) + if made is None: + return resolution, long_running + inner, inner_long_running = made + scopes = self._scopes_for(home) + statement = scopes.statement_of(value) + holders = statement.targets if isinstance(statement, ast.Assign) else [getattr(statement, "target", None)] + holder = holders[0] if len(holders) == 1 and isinstance(holders[0], ast.Name) else None + changed: int | None = None + if holder is not None: + cache_key = (home.ref, holder.id) + if cache_key not in self.module_changes: + self.module_changes[cache_key] = _changed_at( + home.tree, holder.id, scopes.parents, bound=holder, strict=False + ) + changed = self.module_changes[cache_key] + if holder is None or changed is not None: + return ( + dataclasses.replace( + resolution, + reason=FACTORY_RETURN, + detail=( + f"{spelling!r}, the tool {reference_spelling(value.func)!r} returns at " + f"{home.ref}:{value.lineno}, is changed at {home.ref}:{changed}" + ), + ), + False, + ) + return ( + dataclasses.replace( + inner, + steps=(*resolution.steps, *inner.steps), + caveats=tuple(dict.fromkeys((*resolution.caveats, *inner.caveats))), + ), + inner_long_running, + ) def _through_wrapper(self, resolution: Resolution) -> tuple[Resolution, bool]: """Continue into ``name = FunctionTool(func)`` in the module that built it. @@ -2138,6 +2398,482 @@ def _through_wrapper(self, resolution: Resolution) -> tuple[Resolution, bool]: call_name in LONG_RUNNING_TOOL_NAMES, ) + def _scopes_for(self, module: PythonModule) -> ScopeIndex: + if module is self.module: + return self.scopes + scopes = self.module_scopes.get(module.ref) + if scopes is None: + scopes = self.module_scopes[module.ref] = ScopeIndex(module.tree) + return scopes + + def _factory_call( + self, + call: ast.Call, + module: PythonModule, + reference: str, + *, + depth: int = 0, + seen: frozenset[tuple[str, int]] = frozenset(), + outer: ast.FunctionDef | ast.AsyncFunctionDef | None = None, + ) -> tuple[Resolution, bool] | None: + """The tool ``factory(...)`` returns, read from the factory's body (#865). + + Followed when the call names a function this read can resolve: its one + unconditional ``return`` is ``FunctionTool(inner)`` (or + ``LongRunningFunctionTool``), a plain function, a name bound once to + one of those, or another factory's call, up to + :data:`MAX_FACTORY_DEPTH` deep. Nothing is executed. None: the call is + not to application code (a builtin, a third-party package), so the + caller keeps the answer it had. + """ + + assert self.resolver is not None + spelling = reference_spelling(call.func) + if spelling is None: + return None + scopes = self._scopes_for(module) + name = spelling.split(".", 1)[0] + found = scopes.enclosing_bindings(call.func, name) + callee: Resolution + import_node: tuple[ast.Import | ast.ImportFrom, ast.alias] | None = None + if len(found) > 1: + callee = Resolution( + reference=spelling, + reason=LOCAL_BINDING, + detail=local_binding_detail(module.ref, name, found[0], rebound=True), + ) + elif found: + local = found[0] + statement = scopes.statement_of(local) + if isinstance(local, ast.alias) and isinstance(statement, ast.Import | ast.ImportFrom): + import_node = (statement, local) + callee = self.resolver.resolve_local_import(module, statement, local, spelling) + elif ( + isinstance(local, ast.FunctionDef | ast.AsyncFunctionDef) + and spelling == name + and _unconditional(local, scopes) + ): + callee = Resolution( + reference=spelling, + module=module, + definition=local, + steps=(_step(module, local, "definition"),), + ) + else: + callee = Resolution( + reference=spelling, + reason=LOCAL_BINDING, + detail=local_binding_detail(module.ref, name, local), + ) + else: + callee = self.resolver.resolve(module, spelling) + bound = module.bindings.get(name, []) + if len(bound) == 1 and isinstance(bound[0].node, ast.alias): + statement = bound[0].statement + if isinstance(statement, ast.Import | ast.ImportFrom): + import_node = (statement, bound[0].node) + where = f"{module.ref}:{call.lineno}" + if not callee.resolved: + # Only a function this read reaches is a factory it follows. A + # third-party package, a class, a name bound twice or through a + # wildcard keep the answer they had (#865 review) — except + # application code outside the scope, named with the scope that + # would read it. + if callee.reason != MODULE_NOT_FOUND or import_node is None: + return None + statement, alias = import_node + if isinstance(statement, ast.ImportFrom) and not statement.level: + dotted, names = statement.module or "", [alias.name] + elif isinstance(statement, ast.Import): + dotted, names = alias.name, [] + else: + return None + if not dotted or not self.resolver.repository_holds(dotted, names): + return None + held = "; the repository holds it outside the read scope, which a scope including it would read" + return ( + dataclasses.replace( + callee, + reference=reference, + detail=f"{reference!r} is the tool {spelling!r} returns at {where}, and {callee.detail}{held}", + ), + False, + ) + factory, home = callee.definition, callee.module + assert factory is not None and home is not None + if module is self.module and not found: + self._require_proven_name(name) + key = (home.ref, factory.lineno) + # The factory's body and this call's own arguments are part of what + # the tool does: its closure (#865 review). + # The values the call gives the factory's parameters are what the + # closure holds; a value this read cannot name leaves it unknown. + values, unnamed = self._call_values(call, factory, module, home, outer) + # Unnamed: the call as written, and which value is not named — the + # implementation is then an open question, not a changed tool. + # An unnamed value may be set anywhere in the calling module: its + # bytes stand in for it, so an untouched module is no change. + digest = hashlib.sha256( + (_code_dump(factory) + (values if values is not None else _dump(call) + module.sha256)).encode() + ).hexdigest() + steps = ( + *callee.steps, + { + **_step(home, factory, "factory"), + "factory_ast": digest if unnamed is None else f"unknown:{digest}:{unnamed}", + "returns_tool": _returns_tool(factory, self._names_of(home)[0]), + }, + ) + + def stop(detail: str) -> tuple[Resolution, bool]: + return ( + Resolution( + reference=reference, + reason=FACTORY_RETURN, + detail=f"{reference!r} is the tool factory {factory.name!r} ({home.ref}:{factory.lineno}) returns, and {detail}", + steps=steps, + caveats=callee.caveats, + ), + False, + ) + + if key in seen: + return stop("it calls itself on the way, which is not followed") + if depth >= MAX_FACTORY_DEPTH: + return stop(f"it is reached through more than {MAX_FACTORY_DEPTH} factories, which is not followed") + if factory.decorator_list: + return stop("it is decorated, which may change what it returns") + if isinstance(factory, ast.AsyncFunctionDef): + return stop("it is a coroutine function, whose call returns a coroutine, not a tool") + returns, yields = _own_returns(factory) + if yields: + return stop("it is a generator") + if len(returns) != 1: + return stop(f"it returns from {len(returns)} places" if returns else "it never returns") + returned = returns[0] + if returned not in factory.body: + return stop(f"it returns only under a condition, at {home.ref}:{returned.lineno}") + if returned.value is None: + return stop("it returns nothing") + followed = self._factory_returned( + returned.value, factory, home, returned, reference, depth, seen | {key}, stop + ) + resolution, long_running = followed + return ( + dataclasses.replace( + resolution, + steps=(*steps, *resolution.steps), + caveats=tuple(dict.fromkeys((*callee.caveats, *resolution.caveats))), + ), + long_running, + ) + + def _function_lines(self, module: PythonModule) -> dict[str, list[int]]: + """``name -> lines`` of the functions a tool can be minted from in a + module — not a class's methods — read once per module (#865 review).""" + + lines = self.module_function_lines.get(module.ref) + if lines is None: + lines = {} + stack: list[ast.AST] = [module.tree] + while stack: + node = stack.pop() + for child in ast.iter_child_nodes(node): + if isinstance(child, ast.ClassDef): + continue + if isinstance(child, ast.FunctionDef | ast.AsyncFunctionDef): + lines.setdefault(child.name, []).append(child.lineno) + stack.append(child) + for found in lines.values(): + found.sort() + self.module_function_lines[module.ref] = lines + return lines + + def _call_values( + self, + call: ast.Call, + factory: ast.FunctionDef | ast.AsyncFunctionDef, + module: PythonModule, + home: PythonModule, + outer: ast.FunctionDef | ast.AsyncFunctionDef | None = None, + ) -> tuple[str | None, str | None]: + """What each of the factory's parameters holds at this call, as data, + with the factory's defaults for the rest — so ``make(True)``, + ``make(readonly=True)`` and ``mk(readonly=True)`` are one call — and, + when a value is not data this read can name, which one: ``(None, + "readonly=ro at agent.py:12")`` (#865 review).""" + + arguments = factory.args + positional = [*arguments.posonlyargs, *arguments.args] + where = f"{module.ref}:{call.lineno}" + if any(isinstance(item, ast.Starred) for item in call.args) or any(item.arg is None for item in call.keywords): + return None, f"'*' or '**' arguments at {where}" + if len(call.args) > len(positional): + return None, f"extra positional arguments at {where}" + given: dict[str, ast.expr] = {item.arg: arg for item, arg in zip(positional, call.args, strict=False)} + for keyword in call.keywords: + assert keyword.arg is not None + given[keyword.arg] = keyword.value + names = [item.arg for item in [*positional, *arguments.kwonlyargs]] + if set(given) - set(names): + return None, f"arguments the factory does not name at {where}" + defaults: dict[str, ast.expr] = dict( + zip([item.arg for item in positional[len(positional) - len(arguments.defaults) :]], arguments.defaults, strict=False) + ) + for item, default in zip(arguments.kwonlyargs, arguments.kw_defaults, strict=False): + if default is not None: + defaults[item.arg] = default + held: dict[str, str] = {} + unnamed: list[str] = [] + for name in names: + if name in given: + data = self._data(given[name], module, call, outer=outer) + expression = given[name] + elif name in defaults: + data = self._data(defaults[name], home, defaults[name]) + expression = defaults[name] + else: + return None, f"no value for {name!r} at {where}" + if data is None: + unnamed.append(f"{name}={ast.unparse(expression)}") + else: + held[name] = data + if unnamed: + return None, f"{', '.join(unnamed)} at {where}" + return json.dumps(sorted(held.items())), None + + def _data( + self, + node: ast.expr, + module: PythonModule, + at: ast.AST, + depth: int = 0, + *, + outer: ast.FunctionDef | ast.AsyncFunctionDef | None = None, + ) -> str | None: + """``node`` as data: a literal; a name or module attribute bound once, + unconditionally, to one — a list, dict or set only when nothing else + in its scope uses it, and never a module's, which another module can + change; a function, by its code; or a parameter of the factory this + call is made in, which that factory's own call supplies. Else None.""" + + if depth > 4: + return None + if _written_out(node): + if isinstance(node, ast.Set): + return "{" + ",".join(sorted(_dump(item) for item in node.elts)) + "}" + return _dump(node) + scopes = self._scopes_for(module) + if isinstance(node, ast.Name): + found = scopes.enclosing_bindings(at, node.id) + if found: + if len(found) != 1: + return None + if isinstance(found[0], ast.arg) and outer is not None and scopes.parents.get( + scopes.parents.get(found[0]) + ) is outer: + # ``def make_named(readonly): return make_sql_tool(readonly=readonly)``. + return f"" + if not isinstance(found[0], ast.Name): + return None + statement = scopes.statement_of(found[0]) + if not ( + isinstance(statement, ast.Assign | ast.AnnAssign) + and statement.value is not None + and (statement.targets == [found[0]] if isinstance(statement, ast.Assign) else statement.target is found[0]) + and _unconditional(statement, scopes) + ): + return None + if (isinstance(statement.value, ast.Name) or _holds_mutable(statement.value)) and _changed_at( + scopes.parents[statement], node.id, scopes.parents, bound=found[0], allowed=lambda use: use is node + ) is not None: + # ``policy = {...}; policy["readonly"] = False``, also + # through ``policy = base`` or ``({...},)``. + return None + return self._data(statement.value, module, statement, depth + 1) + spelling = reference_spelling(node) + if spelling is None or self.resolver is None: + return None + resolution = self.resolver.resolve(module, spelling) + if resolution.caveats or resolution.module is None: + return None + if resolution.definition is not None: + # ``make(upper)``: a function, by its code. + return "" + value = resolution.value + if value is None or _holds_mutable(value): + # A module's mutable value another module can change. + return None + return self._data(value, resolution.module, value, depth + 1) + + def _factory_returned( + self, + value: ast.expr, + factory: ast.FunctionDef | ast.AsyncFunctionDef, + home: PythonModule, + returned: ast.Return, + reference: str, + depth: int, + seen: frozenset[tuple[str, int]], + stop: Callable[[str], tuple[Resolution, bool]], + *, + through: str | None = None, + ) -> tuple[Resolution, bool]: + """What a factory's ``return`` value is, as a resolution with only the + steps past the factory itself.""" + + scopes = self._scopes_for(home) + aliases, _ = self._names_of(home) + if isinstance(value, ast.Call): + call_name = _qualified_name(value.func, aliases) + if call_name in FUNCTION_TOOL_NAMES | LONG_RUNNING_TOOL_NAMES: + func_expr = _call_func_expr(value) + if func_expr is None: + return stop(f"its {call_name} names no function") + root = value.func + while isinstance(root, ast.Attribute): + root = root.value + if isinstance(root, ast.Name): + # As for any wrapper: ADK's constructor only while the name + # is still the import it resolves through. + bound = self._names_of(home)[1].get(root.id, []) + local = scopes.enclosing_bindings(value, root.id) + adk_import = bool(local) and all( + isinstance(item, ast.alias) + and isinstance(statement := scopes.statement_of(item), ast.ImportFrom) + and (statement.module or "").startswith("google.adk") + for item in local + ) + if not adk_import and (local or len(bound) != 1 or not isinstance(bound[0], ast.alias)): + self._note_surface_gap(SURFACE_GAP_SHADOWED_FRAMEWORK_SYMBOL) + resolution = self._factory_function( + func_expr, factory, home, returned, reference, stop, wrapper=value + ) + return resolution, resolution.resolved and call_name in LONG_RUNNING_TOOL_NAMES + followed = self._factory_call(value, home, reference, depth=depth + 1, seen=seen, outer=factory) + if followed is None: + return stop( + f"it returns what {reference_spelling(value.func) or 'a call'!r} returns at " + f"{home.ref}:{value.lineno}, which is not application code this read follows" + ) + return followed + if isinstance(value, ast.Name) and through is None: + found = scopes.enclosing_bindings(value, value.id) + if len(found) == 1 and isinstance(found[0], ast.Name): + statement = scopes.statement_of(found[0]) + bound_to = ( + statement.value + if isinstance(statement, ast.Assign) + and len(statement.targets) == 1 + and statement.targets[0] is found[0] + else statement.value + if isinstance(statement, ast.AnnAssign) and statement.target is found[0] + else None + ) + if bound_to is not None and statement in factory.body and statement.lineno < returned.lineno: + # ``tool = FunctionTool(inner)`` then ``return tool``. + changed = _changed_at( + factory, value.id, scopes.parents, bound=found[0], returned=returned + ) + if changed is not None: + return stop(f"it changes the {value.id!r} it returns at {home.ref}:{changed}") + return self._factory_returned( + bound_to, factory, home, returned, reference, depth, seen, stop, through=value.id + ) + if isinstance(value, ast.Name | ast.Attribute): + # A plain callable: ADK wraps it in a ``FunctionTool`` itself. + return self._factory_function(value, factory, home, returned, reference, stop), False + return stop(f"it returns an expression at {home.ref}:{returned.lineno} this read does not follow") + + def _factory_function( + self, + expr: ast.expr, + factory: ast.FunctionDef | ast.AsyncFunctionDef, + home: PythonModule, + returned: ast.Return, + reference: str, + stop: Callable[[str], tuple[Resolution, bool]], + *, + wrapper: ast.Call | None = None, + ) -> Resolution: + """The function a factory wraps or returns: a function nested in it, + defined once, before its ``return`` and never changed or handed on + (``inner.__name__ = ...`` renames the tool), or one its module binds.""" + + assert self.resolver is not None + spelling = reference_spelling(expr) + if spelling is None: + return stop(f"the function it wraps at {home.ref}:{returned.lineno} is not a named function")[0] + scopes = self._scopes_for(home) + name = spelling.split(".", 1)[0] + found = scopes.enclosing_bindings(expr, name) + if len(found) > 1: + return stop(local_binding_detail(home.ref, name, found[0], rebound=True))[0] + if found: + inner = found[0] + statement = scopes.statement_of(inner) + if isinstance(inner, ast.alias) and isinstance(statement, ast.Import | ast.ImportFrom): + changed = ( + _changed_at(factory, name, scopes.parents, bound=inner, returned=returned, wrapper=wrapper) + if spelling == name + else _attribute_changed(factory, spelling, scopes.parents, returned=returned, wrapper=wrapper) + ) + if changed is not None: + return stop(f"{spelling!r} is changed or handed on at {home.ref}:{changed}, which may rename it")[0] + resolution = self.resolver.resolve_local_import(home, statement, inner, spelling) + return dataclasses.replace(resolution, reference=reference) + if not ( + isinstance(inner, ast.FunctionDef | ast.AsyncFunctionDef) + and spelling == name + and inner in factory.body + and inner.lineno < returned.lineno + ): + return stop(local_binding_detail(home.ref, name, inner))[0] + if inner.decorator_list: + return stop(f"{name!r} ({home.ref}:{inner.lineno}) is decorated, which may replace it")[0] + same = self._function_lines(home).get(name, []) + if len(same) > 1: + # A tool is known by its name in its file (#865 review). + return stop( + f"{home.ref} defines {name!r} more than once (lines " + f"{', '.join(str(line) for line in same)}), so which one is the tool is not established" + )[0] + changed = _changed_at( + factory, name, scopes.parents, bound=inner, returned=returned, wrapper=wrapper + ) + if changed is not None: + return stop( + f"{name!r} is changed or handed on at {home.ref}:{changed}, which may rename it" + )[0] + return Resolution( + reference=reference, + module=home, + definition=inner, + steps=(_step(home, inner, "definition"),), + ) + # ``search.__name__ = "lookup"`` in the factory renames it too, as + # does ``impl.search.__name__ = ...``. + changed = ( + _changed_at(factory, name, scopes.parents, bound=factory, returned=returned, wrapper=wrapper) + if spelling == name + else _attribute_changed(factory, spelling, scopes.parents, returned=returned, wrapper=wrapper) + ) + if changed is not None: + return stop(f"{spelling!r} is changed or handed on at {home.ref}:{changed}, which may rename it")[0] + resolution = self.resolver.resolve(home, spelling) + if resolution.resolved: + return dataclasses.replace(resolution, reference=reference) + if resolution.reason in (None, NOT_BOUND): + return stop(f"{spelling!r} is not bound in {home.ref}")[0] + return dataclasses.replace( + resolution, + reference=reference, + detail=( + f"{reference!r} is the tool factory {factory.name!r} ({home.ref}:" + f"{factory.lineno}) returns, and {resolution.detail}" + ), + ) + def _imported_root(self, expr: ast.Attribute) -> bool: """Whether a dotted reference starts at a name this module imports.""" @@ -2170,18 +2906,19 @@ def _record_unresolved_reference( ) -> None: if resolution is None or resolution.reason in (None, NOT_BOUND): return - self.artifacts.unresolved_references.append( - { - "agent_name": agent_name, - "reference": spelling, - "warning": warning, - "reason": resolution.reason, - "detail": resolution.detail, - "source_id": self.source_id, - "source_ref": self.source_ref, - "import_resolution": resolution.evidence(), - } - ) + record = { + "agent_name": agent_name, + "reference": spelling, + "warning": warning, + "reason": resolution.reason, + "detail": resolution.detail, + "source_id": self.source_id, + "source_ref": self.source_ref, + "import_resolution": resolution.evidence(), + } + # A factory called N times inline is one record (#865 review). + if record not in self.artifacts.unresolved_references: + self.artifacts.unresolved_references.append(record) def _bind_wrapped_reference( self, @@ -2229,13 +2966,16 @@ def _bind_resolved( node, module = resolution.definition, resolution.module assert node is not None and module is not None - # The spelling this module used has to hold up like a local name. - self._require_proven_name(resolution.reference.split(".", 1)[0]) + # A factory's function: its call was proven where it was read (#865). + made = any(step.get("binding") == "factory" for step in resolution.steps) + if not made: + # The spelling this module used has to hold up like a local name. + self._require_proven_name(resolution.reference.split(".", 1)[0]) if any(step.get("module_getattr") for step in resolution.steps): # A package ``__getattr__`` could have answered before the # submodule did; the definition is named, not proven. self._note_surface_gap(SURFACE_GAP_SHADOWED_DEFINITION) - if module is self.module: + if module is self.module and not (made and node not in module.tree.body): # ``alias = local_function``: the chain came back to this module. self._bind_function_tool(node, tools, agent_name, binding, long_running) tool = self.canonical_function_tools.get(node.name) @@ -2258,9 +2998,10 @@ def _bind_resolved( name, name_bindings, aliases ), ) - # Minted from an import: another source reading that module - # observes the same definition; the catalog keeps one (#879). - tool.extraction["imported_definition"] = True + if module is not self.module: + # Minted from an import: another source reading that module + # observes the same definition; the catalog keeps one (#879). + tool.extraction["imported_definition"] = True self.imported_function_tools[key] = tool tools.append(tool) else: @@ -2282,6 +3023,20 @@ def _bind_resolved( recorded = tool.extraction.setdefault("import_resolutions", []) if evidence not in recorded: recorded.append(evidence) + # Every factory call that makes this tool is part of its + # implementation: the factory's body and the call's arguments, which + # its closure holds (#865 review). + # Per agent: another agent's call to the same factory is its own. + # ``unknown``: a call passes a value this read cannot name. + made = [str(step["factory_ast"]) for step in resolution.steps if "factory_ast" in step] + if made: + calls = tool.extraction.setdefault("factory_calls", {}) + calls[agent_name] = sorted({*calls.get(agent_name, []), *made}) + if binding.recording is not None: + # This construction's closure is part of its site: two + # constructions whose factory values differ are not twins + # (#865 review). + binding.recording.append((f"{tool.name}@factory", ",".join(sorted(made)))) def _names_of( self, module: PythonModule @@ -3464,6 +4219,299 @@ def _parameters( #: at. A binding reached through anything else — an ``if``/``try``/``for`` body, #: a ``with`` block — is conditional or order-dependent, which is exactly what #: this check exists to refuse. +#: How many factory calls deep a tool factory's return is followed (#865). +MAX_FACTORY_DEPTH = 4 + + +def _step(module: PythonModule, node: ast.FunctionDef | ast.AsyncFunctionDef, binding: str) -> dict[str, Any]: + """One hop of a resolution's evidence, in the resolver's shape.""" + + return { + "path": module.ref, + "line": node.lineno, + "name": node.name, + "sha256": module.sha256, + "binding": binding, + } + + +def _unconditional(node: ast.stmt, scopes: ScopeIndex) -> bool: + """Whether a statement runs whenever its function (or module) body does.""" + + parent = scopes.parents.get(node) + return isinstance(parent, ast.Module | ast.FunctionDef | ast.AsyncFunctionDef) and node in parent.body + + +def _own_returns(function: ast.FunctionDef | ast.AsyncFunctionDef) -> tuple[list[ast.Return], bool]: + """The ``return`` statements of ``function`` itself, and whether it yields.""" + + returns: list[ast.Return] = [] + yields = False + stack: list[ast.AST] = list(function.body) + while stack: + node = stack.pop() + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef | ast.Lambda): + continue + if isinstance(node, ast.Return): + returns.append(node) + elif isinstance(node, ast.Yield | ast.YieldFrom): + yields = True + stack.extend(ast.iter_child_nodes(node)) + return sorted(returns, key=lambda item: (item.lineno, item.col_offset)), yields + + +def _changed_at( + function: ast.AST, + name: str, + parents: dict[ast.AST, ast.AST], + *, + bound: ast.AST, + returned: ast.Return | None = None, + wrapper: ast.Call | None = None, + strict: bool = True, + allowed: Callable[[ast.Name], bool] | None = None, +) -> int | None: + """The line where ``name`` — a factory's function or the tool it returns — + may be changed, or None. + + Any store or deletion through it (``inner.__name__ = ...``, + ``inner.__dict__[...]``), a rebinding, and ``setattr``/``delattr``/ + ``update_wrapper`` on it always count. ``strict``: so does any other use — + handing it to a call, a container or another name — except being the + ``return`` value or the function ``wrapper`` wraps (#865). + """ + + for node in ast.walk(function): + if not isinstance(node, ast.Name) or node.id != name or node is bound: + continue + line = getattr(node, "lineno", 0) + if not isinstance(node.ctx, ast.Load): + return line + parent = parents.get(node) + if isinstance(parent, ast.Attribute | ast.Subscript) and parent.value is node: + outer = parent + while isinstance(parents.get(outer), ast.Attribute | ast.Subscript): + outer = parents[outer] + if not isinstance(getattr(outer, "ctx", None), ast.Load): + return line + if isinstance(parent, ast.Call) and node in parent.args: + callee = reference_spelling(parent.func) or "" + if callee.rsplit(".", 1)[-1] in {"setattr", "delattr", "update_wrapper"}: + return line + if not strict or (isinstance(parent, ast.Call) and parent.func is node): + # Calling it changes nothing about it. + continue + if isinstance(parent, ast.Attribute) and parent.value is node: + # ``tool.name`` read — logged, formatted — leaves it as it is; a + # method call on it may not. + outer = parent + while isinstance(parents.get(outer), ast.Attribute | ast.Subscript) and getattr(parents[outer], "value", None) is outer: + outer = parents[outer] + holder = parents.get(outer) + if isinstance(holder, ast.Call) and holder.func is outer: + # A method call on it may change it. + return line + handed = ( + isinstance(holder, ast.keyword) + or (isinstance(holder, ast.Call) and outer in holder.args) + # ``f = tool.func``, ``[tool.func]``: another name for a part of it. + or isinstance(holder, ast.Assign | ast.AnnAssign | ast.NamedExpr | ast.List | ast.Tuple | ast.Set | ast.Dict) + ) + if handed and not ( + isinstance(outer, ast.Attribute) and outer.value is node and outer.attr in _DESCRIPTIVE_ATTRIBUTES + ): + # ``rename_fn(tool.func)``: a part of it handed on (#865 review). + return line + continue + if isinstance(parent, ast.Return) and parent is returned: + continue + if wrapper is not None and parent is wrapper: + continue + if isinstance(parent, ast.keyword) and parents.get(parent) is wrapper and wrapper is not None: + continue + if allowed is not None and allowed(node): + continue + return line + return None + + +def _code_dump(function: ast.FunctionDef | ast.AsyncFunctionDef) -> str: + """A function's code, without docstrings (description, not behaviour).""" + + copy = copy_module.deepcopy(function) + for node in ast.walk(copy): + body = getattr(node, "body", None) + if ( + isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef) + and body + and isinstance(body[0], ast.Expr) + and isinstance(body[0].value, ast.Constant) + and isinstance(body[0].value.value, str) + ): + node.body = body[1:] or [ast.Pass()] + return _dump(copy) + + +#: A tool's attributes that are text: handing one on changes nothing. +_DESCRIPTIVE_ATTRIBUTES = frozenset({"name", "description", "__name__", "__doc__", "__qualname__"}) + + +def _returns_tool(function: ast.FunctionDef | ast.AsyncFunctionDef, aliases: dict[str, str]) -> bool: + """Whether a factory's own ``return`` is (or may be) a tool object rather + than a plain function: a wrapper call, anything but a name, or a name its + body binds to a call (#865 review).""" + + returns, _ = _own_returns(function) + for node in returns: + value = node.value + if value is None: + continue + if isinstance(value, ast.Call) and _qualified_name(value.func, aliases) in FUNCTION_TOOL_NAMES | LONG_RUNNING_TOOL_NAMES: + return True + if not isinstance(value, ast.Name): + return True + # ``tool = FunctionTool(inner)`` / ``tool: FunctionTool = ...``: its own + # bindings only, not a nested function's. + stack: list[ast.AST] = list(function.body) + while stack: + item = stack.pop() + if isinstance(item, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef | ast.Lambda): + continue + bound = ( + item.targets + if isinstance(item, ast.Assign) + else [item.target] + if isinstance(item, ast.AnnAssign | ast.NamedExpr) + else [] + ) + if any(isinstance(target, ast.Name) and target.id == value.id for target in bound) and isinstance( + getattr(item, "value", None), ast.Call + ): + return True + stack.extend(ast.iter_child_nodes(item)) + return False + + +def _dump(node: ast.AST) -> str: + """``ast.dump`` that reads the same on every supported Python.""" + + return ast.dump(node, include_attributes=False, **({"show_empty": True} if sys.version_info >= (3, 13) else {})) + + +def _holds_mutable(node: ast.AST) -> bool: + """A list, dict or set anywhere in a literal: ``({"readonly": True},)``.""" + + return any(isinstance(item, ast.List | ast.Dict | ast.Set) for item in ast.walk(node)) + + +def _written_out(node: ast.AST) -> bool: + """A value written out in full: a constant, or a container of them.""" + + if isinstance(node, ast.Constant): + return True + if isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.USub | ast.UAdd | ast.Not): + return _written_out(node.operand) + if isinstance(node, ast.List | ast.Tuple | ast.Set): + return all(_written_out(item) for item in node.elts) + if isinstance(node, ast.Dict): + return all(key is not None and _written_out(key) for key in node.keys) and all(_written_out(item) for item in node.values) + return False + + +def _attribute_changed( + function: ast.AST, + spelling: str, + parents: dict[ast.AST, ast.AST], + *, + returned: ast.Return | None, + wrapper: ast.Call | None, +) -> int | None: + """``_changed_at`` for a module attribute (``impl.search``): a store + through it, or handing it to a call other than the wrapper (#865 review).""" + + for node in ast.walk(function): + if not isinstance(node, ast.Attribute) or reference_spelling(node) != spelling: + continue + parent = parents.get(node) + if (wrapper is not None and (parent is wrapper or (isinstance(parent, ast.keyword) and parents.get(parent) is wrapper))) or ( + isinstance(parent, ast.Return) and parent is returned + ): + continue + outer: ast.AST = node + while isinstance(parents.get(outer), ast.Attribute | ast.Subscript) and getattr(parents[outer], "value", None) is outer: + outer = parents[outer] + if not isinstance(getattr(outer, "ctx", None), ast.Load): + return node.lineno + holder = parents.get(outer) + if isinstance(holder, ast.keyword) or (isinstance(holder, ast.Call) and outer in holder.args): + return node.lineno + if isinstance(holder, ast.Assign | ast.AnnAssign | ast.NamedExpr | ast.List | ast.Tuple | ast.Set | ast.Dict): + # ``f = impl.search``: another name for it (#865 review). + return node.lineno + return None + + +def _in_local_list( + node: ast.AST, parents: dict[ast.AST, ast.AST], is_agent_call: Callable[[ast.Call], bool] +) -> bool: + """A member of ``tools = [a, b]`` whose every use is an agent's ``tools=``: + the list the tools-list reader follows, and nothing else (#865 review).""" + + holder = parents.get(node) + statement = parents.get(holder) if isinstance(holder, ast.List | ast.Tuple) else None + if not isinstance(statement, ast.Assign | ast.AnnAssign) or statement.value is not holder: + return False + target = statement.targets[0] if isinstance(statement, ast.Assign) and len(statement.targets) == 1 else getattr(statement, "target", None) + function = parents.get(statement) + while function is not None and not isinstance(function, ast.FunctionDef | ast.AsyncFunctionDef | ast.Module): + function = parents.get(function) + if not isinstance(target, ast.Name) or function is None: + return False + for use in ast.walk(function): + if not isinstance(use, ast.Name) or use.id != target.id or use is target: + continue + holder = parents.get(use) + call = parents.get(holder) + if isinstance(holder, ast.keyword) and holder.arg == "tools" and isinstance(call, ast.Call) and is_agent_call(call): + continue + # What the tools-list reader follows: additions to the list. + if isinstance(holder, ast.AugAssign) and holder.target is use: + continue + if ( + isinstance(holder, ast.Attribute) + and holder.attr in {"append", "extend", "insert"} + and isinstance(call, ast.Call) + and call.func is holder + and isinstance(parents.get(call), ast.Expr) + ): + continue + return False + return True + + +def _in_tools_argument( + node: ast.AST, parents: dict[ast.AST, ast.AST], is_agent_call: Callable[[ast.Call], bool] +) -> bool: + """Whether ``node`` is a member of an agent call's ``tools=`` list.""" + + parent = parents.get(node) + if isinstance(parent, ast.List | ast.Tuple): + parent = parents.get(parent) + if not isinstance(parent, ast.keyword) or parent.arg != "tools": + return False + call = parents.get(parent) + return isinstance(call, ast.Call) and is_agent_call(call) + + +def _top_statement(node: ast.AST, function: ast.AST, parents: dict[ast.AST, ast.AST]) -> ast.AST | None: + """The statement directly in ``function``'s body that holds ``node``.""" + + current: ast.AST | None = node + while current is not None and parents.get(current) is not function: + current = parents.get(current) + return current + + _TOP_LEVEL_BINDING_STATEMENTS = ( ast.Assign, ast.AnnAssign, diff --git a/src/agents_shipgate/inputs/python_imports.py b/src/agents_shipgate/inputs/python_imports.py index 8a652253c..f09340414 100644 --- a/src/agents_shipgate/inputs/python_imports.py +++ b/src/agents_shipgate/inputs/python_imports.py @@ -62,6 +62,8 @@ UNREADABLE_MODULE = "unreadable_module" RESOLUTION_LIMIT = "resolution_limit" LOCAL_BINDING = "local_binding" +#: A tool factory's return that is not one function wrapped as a tool (#865). +FACTORY_RETURN = "factory_return" #: Distinct modules one resolver will parse, and lookups one reference may #: take. A repository-local tool is normally one or two hops away; the bounds @@ -683,6 +685,13 @@ def _unread_import( "which could reassign it" ) + def repository_holds(self, dotted: str, names: list[str]) -> bool: + """Whether the repository holds, anywhere, the module an absolute import + names: application code outside the read scope, not a third-party + package (#865).""" + + return self._repository_provides(dotted.split("."), names) + def _repository_provides(self, parts: list[str], names: list[str]) -> bool: """Whether the repository holds the module an absolute import names. @@ -2750,6 +2759,7 @@ def local_binding_detail(ref: str, name: str, node: ast.AST, *, rebound: bool = __all__ = [ "AMBIGUOUS_MODULE", "CONDITIONAL_BINDING", + "FACTORY_RETURN", "IMPORT_CYCLE", "ImportResolver", "LINKED_MODULE", diff --git a/tests/test_adk_tool_factories.py b/tests/test_adk_tool_factories.py new file mode 100644 index 000000000..4611834d5 --- /dev/null +++ b/tests/test_adk_tool_factories.py @@ -0,0 +1,926 @@ +"""A Google ADK tool built by a factory is the function the factory wraps (#865). + +``save_memory_tool = create_save_memory_tool()`` in visulate/visulate-for-oracle#526 +was an unresolved tool: the reader stopped at the local assignment. The factory's +one unconditional ``return FunctionTool(save_memory_record)`` names the tool, so +the reader follows it — read, never run — and names every factory it cannot +follow, with why. +""" + +from __future__ import annotations + +import pytest +from test_imported_tool_bindings import _adk, _commit, _compare, _edges, _git, _write + +AGENT = '''from google.adk.agents import LlmAgent +from .remote_tool import create_remote_delegate_tool +from common.tools import create_save_memory_tool, create_read_memory_tool + + +def create_root_agent() -> LlmAgent: + nl2sql_tool = create_remote_delegate_tool("nl2sql_agent", "http://localhost:10001") + save_memory_tool = create_save_memory_tool() + read_memory_tool = create_read_memory_tool() + return LlmAgent(name="visulate_root_agent", tools=[TOOLS]) +''' +REMOTE = '''from google.adk.tools.function_tool import FunctionTool + + +def create_remote_delegate_tool(agent_name: str, endpoint_url: str) -> FunctionTool: + async def _delegate(message: str) -> str: + return message + + _delegate.__name__ = f"delegate_to_{agent_name}" + return FunctionTool(_delegate) +''' +MEMORY = '''from google.adk.tools.function_tool import FunctionTool + + +def create_save_memory_tool() -> FunctionTool: + async def save_memory_record(content: str, filename: str = "") -> str: + return content + + return FunctionTool(save_memory_record) + + +def create_read_memory_tool() -> FunctionTool: + async def read_memory_record(filename: str) -> str: + return filename + + return FunctionTool(read_memory_record) +''' + + +def _visulate(tools: str = "nl2sql_tool, read_memory_tool, save_memory_tool") -> dict[str, str | None]: + return { + "ai-agent/root_agent/__init__.py": "", + "ai-agent/root_agent/agent.py": AGENT.replace("TOOLS", tools), + "ai-agent/root_agent/remote_tool.py": REMOTE, + "ai-agent/common/__init__.py": "", + "ai-agent/common/tools.py": MEMORY, + } + + +def _why(artifacts) -> dict[str, tuple[str, str]]: + return {item["reference"]: (item["reason"], item["detail"]) for item in artifacts.unresolved_references} + + +def test_the_visulate_memory_factories_resolve_to_the_functions_they_wrap(tmp_path): + (tmp_path / ".git").mkdir() + _write(tmp_path, _visulate()) + loaded, artifacts = _adk(tmp_path / "ai-agent", "root_agent/agent.py") + + assert _edges(loaded, artifacts) == [ + ("visulate_root_agent", "read_memory_record", "common/tools.py:12"), + ("visulate_root_agent", "save_memory_record", "common/tools.py:5"), + ] + tools = {tool.name: tool for source in loaded for tool in source.tools} + assert tools["save_memory_record"].function_signature == "save_memory_record(content, filename) -> str" + (resolution,) = tools["save_memory_record"].extraction["import_resolutions"] + # Every module read on the way, and what each hop is. + assert [(step["path"], step["binding"]) for step in resolution["steps"]] == [ + ("root_agent/agent.py", "import"), + ("common/tools.py", "definition"), + ("common/tools.py", "factory"), + ("common/tools.py", "definition"), + ] + assert resolution["definition"] == "common/tools.py:5" + assert {item["path"] for item in resolution["inputs"]} == {"root_agent/agent.py", "common/tools.py"} + # The remote delegate renames the function it returns: named, with why. + reason, detail = _why(artifacts)["nl2sql_tool"] + assert reason == "factory_return" + assert "'_delegate' is changed or handed on at root_agent/remote_tool.py:8" in detail + assert "Google ADK agent 'visulate_root_agent' references unresolved tool 'nl2sql_tool'." in artifacts.warnings + + +def test_a_factory_outside_the_read_scope_is_named_with_the_scope_that_reads_it(tmp_path): + (tmp_path / ".git").mkdir() + _write(tmp_path, _visulate()) + loaded, artifacts = _adk(tmp_path / "ai-agent" / "root_agent") + + assert _edges(loaded, artifacts) == [] + reason, detail = _why(artifacts)["save_memory_tool"] + assert reason == "module_not_found" + assert detail.startswith("'save_memory_tool' is the tool 'create_save_memory_tool' returns at agent.py:8") + assert "the repository holds it outside the read scope" in detail + + +FACTORY = "from google.adk.tools import FunctionTool, LongRunningFunctionTool\n\n\n" +LOOKUP = "def lookup(query: str) -> str:\n return query\n" + + +@pytest.mark.parametrize( + ("factory", "location"), + [ + ("def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n", "tools.py:5"), + ("def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(func=inner)\n", "tools.py:5"), + ( + "def make():\n def inner(query: str) -> str:\n return query\n\n tool = FunctionTool(inner)\n return tool\n", + "tools.py:5", + ), + ("def make():\n def inner(query: str) -> str:\n return query\n\n return inner\n", "tools.py:5"), + (LOOKUP + "\n\ndef make():\n return FunctionTool(lookup)\n", "tools.py:4"), + ( + "def _make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n\n\n" + "def make():\n return _make()\n", + "tools.py:5", + ), + ], + ids=["wrapped", "wrapped-by-keyword", "wrapped-then-named", "plain-function", "module-function", "factory-of-factory"], +) +def test_a_factory_s_one_return_is_its_tool(tmp_path, factory, location): + agent = "from google.adk.agents import Agent\nfrom tools import make\n\nTOOL\n\nroot = Agent(name='root', tools=[ITEM])\n" + for tool, item in (("tool = make()", "tool"), ("", "make()"), ("def build():\n return make()\n\n\ntool = build()", "tool")): + root = tmp_path / item.replace("()", "_call") / tool[:4].strip() + _write(root, {"tools.py": FACTORY + factory, "agent.py": agent.replace("TOOL", tool).replace("ITEM", item)}) + loaded, artifacts = _adk(root) + assert [location for _, _, location in _edges(loaded, artifacts)] == [location], (tool, artifacts.warnings) + assert artifacts.warnings == [] + + +def test_a_long_running_factory_tool_is_long_running(tmp_path): + factory = "def make():\n def inner(query: str) -> str:\n return query\n\n return LongRunningFunctionTool(inner)\n" + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make()])\n"}) + loaded, _ = _adk(tmp_path) + (tool,) = [tool for source in loaded for tool in source.tools] + assert tool.annotations.get("long_running") is True + + +@pytest.mark.parametrize( + ("factory", "why"), + [ + ( + "def make(fast):\n def inner(query: str) -> str:\n return query\n\n def other(query: str) -> str:\n return query\n\n" + " if fast:\n return FunctionTool(inner)\n return FunctionTool(other)\n", + "it returns from 2 places", + ), + ( + "def make(fast):\n def inner(query: str) -> str:\n return query\n\n if fast:\n return FunctionTool(inner)\n", + "it returns only under a condition", + ), + ("def make():\n return make()\n", "it calls itself on the way"), + ( + "import functools\n\n\ndef make():\n @functools.lru_cache\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n", + "'inner' (tools.py:9) is decorated", + ), + ( + "def make(name):\n def inner(query: str) -> str:\n return query\n\n setattr(inner, '__name__', name)\n return FunctionTool(inner)\n", + "'inner' is changed or handed on at tools.py:8", + ), + ( + "def rename(function):\n function.__name__ = 'other'\n\n\ndef make():\n def inner(query: str) -> str:\n return query\n\n" + " rename(inner)\n return FunctionTool(inner)\n", + "'inner' is changed or handed on at tools.py:12", + ), + ( + "def make():\n def inner(query: str) -> str:\n return query\n\n tool = FunctionTool(inner)\n tool.name = 'other'\n return tool\n", + "it changes the 'tool' it returns at tools.py:9", + ), + ( + "def make(inner):\n return FunctionTool(inner)\n", + "'inner' is bound by a parameter", + ), + ("def make():\n yield FunctionTool(print)\n", "it is a generator"), + ("def make():\n return [FunctionTool(print)]\n", "returns an expression"), + ], + ids=[ + "two-returns", + "conditional-return", + "recursive", + "decorated-function", + "renamed-by-setattr", + "handed-to-a-helper", + "tool-renamed", + "parameter", + "generator", + "list", + ], +) +def test_a_factory_this_read_cannot_follow_is_named_with_why(tmp_path, factory, why): + agent = "from google.adk.agents import Agent\nfrom tools import make\n\n\ndef build():\n tool = make(True)\n return Agent(name='root', tools=[tool])\n" + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": agent}) + loaded, artifacts = _adk(tmp_path) + + assert _edges(loaded, artifacts) == [] + assert "Google ADK agent 'root' references unresolved tool 'tool'." in artifacts.warnings + reason, detail = _why(artifacts)["tool"] + assert reason == "factory_return", detail + assert why in detail + + +@pytest.mark.parametrize( + ("agent", "reason"), + [ + ( + # Bound only under a condition: which tool, if any, is not known. + "def build(flag):\n if flag:\n tool = make()\n return Agent(name='root', tools=[tool])\n", + "local_binding", + ), + ( + # The agent's module changes the tool it was handed. + "def build():\n tool = make()\n tool.name = 'other'\n return Agent(name='root', tools=[tool])\n", + "factory_return", + ), + ], + ids=["conditional-binding", "changed-at-the-call-site"], +) +def test_the_call_site_of_a_factory_is_read_too(tmp_path, agent, reason): + factory = "def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n" + _write( + tmp_path, + {"tools.py": FACTORY + factory, "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\n\n" + agent}, + ) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + assert _why(artifacts)["tool"][0] == reason + + +def test_a_third_party_factory_keeps_the_answer_it_had(tmp_path): + _write( + tmp_path, + { + "agent.py": "from google.adk.agents import Agent\nfrom vendor.tools import make_search\n\n\n" + "def build():\n tool = make_search()\n return Agent(name='root', tools=[tool, make_search()])\n" + }, + ) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + # The local binding stays the named stop; the inline call the expression it was. + assert _why(artifacts)["tool"][0] == "local_binding" + assert "Google ADK agent 'root' has a tool expression that could not be statically resolved." in artifacts.warnings + assert "make_search()" not in _why(artifacts) + + +def test_application_diff_shows_the_memory_tools_a_factory_adds(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _visulate("read_memory_tool")) + head = _commit(tmp_path, _visulate("read_memory_tool, save_memory_tool")) + result = _compare(tmp_path, base, head, "--scope", "ai-agent") + + assert result["comparison_status"] == "compared" + assert [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] == [ + ("visulate_root_agent", "save_memory_record", "added"), + ] + after = result["rows"][0]["after"] + assert after["definition"]["source"] == "ai-agent/common/tools.py" + assert after["definition"]["line"] == 5 + + +def test_resolving_two_tools_does_not_complete_the_visulate_surface(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _visulate("nl2sql_tool")) + head = _commit(tmp_path, _visulate()) + result = _compare(tmp_path, base, head, "--scope", "ai-agent") + + # The base's delegate is not read, so it may have been either memory tool: + # the two are candidate additions, and the delegate stays named. + assert result["comparison_status"] == "partial" + assert [(row["tool"], row["change"], row["candidate_change"]) for row in result["rows"]] == [ + ("read_memory_record", "not_established", "added"), + ("save_memory_record", "not_established", "added"), + ] + assert any( + "nl2sql_tool" in gap["reason"] for gap in result["head"]["coverage_gaps"] + ), result["head"]["coverage_gaps"] + + +def test_a_factory_s_function_changing_is_an_implementation_change(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _visulate()) + head = _commit(tmp_path, {"ai-agent/common/tools.py": MEMORY.replace("return content", "return content.upper()")}) + result = _compare(tmp_path, base, head, "--scope", "ai-agent") + rows = [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] + assert ("visulate_root_agent", "save_memory_record", "changed") in rows + + +# -- a tools list built in the agent's function --------------------------------- + +LIST_TOOLS = ( + "from google.adk.tools import FunctionTool\n\n\n" + "def intake(name: str) -> str:\n return name\n\n\n" + "def route(zone: str) -> str:\n return zone\n\n\n" + "def triage(case: str) -> str:\n return case\n" +) + + +def _list_agent(body: str) -> dict[str, str]: + return { + "tools.py": LIST_TOOLS, + "agent.py": "from google.adk.agents import LlmAgent\nfrom google.adk.tools import FunctionTool\n" + "from tools import intake, route, triage\n\n\ndef build(enabled):\n" + + "".join(f" {line}\n" for line in body.splitlines()) + + " return LlmAgent(name='assign', tools=tools)\n", + } + + +@pytest.mark.parametrize( + ("body", "bound"), + [ + ("tools = [FunctionTool(intake)]\ntools.append(FunctionTool(route))", ["intake", "route"]), + ("tools = [intake]\ntools.extend([route, triage])", ["intake", "route", "triage"]), + ("tools = [intake]\ntools.insert(0, route)", ["intake", "route"]), + ("tools = [intake]\ntools += [route]", ["intake", "route"]), + ("tools: list = (intake, route)", ["intake", "route"]), + ], + ids=["append", "extend", "insert", "augmented", "annotated-tuple"], +) +def test_a_tools_list_built_in_the_agent_s_function_is_read(tmp_path, body, bound): + _write(tmp_path, _list_agent(body)) + loaded, artifacts = _adk(tmp_path) + assert [tool for _, tool, _ in _edges(loaded, artifacts)] == bound + assert artifacts.warnings == [] + + +def test_a_tool_added_under_a_condition_is_named_beside_the_list(tmp_path): + # MuhammadVT/smart-assignment#46: the triage tool joins only when enabled. + _write(tmp_path, _list_agent("tools = [FunctionTool(intake), route]\nif enabled:\n tools.append(FunctionTool(triage))")) + loaded, artifacts = _adk(tmp_path) + + assert [tool for _, tool, _ in _edges(loaded, artifacts)] == ["intake", "route"] + message = ( + "Google ADK agent 'assign' adds a tool to its tools list only under a condition or in a loop " + "at agent.py:9, which is not established." + ) + assert message in artifacts.warnings + (observation,) = [item for source in loaded for item in source.binding_observations] + assert observation.tools_complete is False + assert observation.issues == [message] + + +@pytest.mark.parametrize( + "body", + [ + "tools = [intake]\nregister(tools)", + "tools = [intake]\ntools.remove(intake)", + "tools = [intake]\nother = tools\nother.append(route)", + "tools = [intake]\nhelper = lambda: tools.append(route)", + "tools = [intake]\ntools.extend(more_tools())", + "tools = [*base_tools(), intake]", + "if enabled:\n tools = [intake]\nelse:\n tools = [route]", + ], + ids=["handed-to-a-call", "removed", "aliased", "nested-function", "extended-by-a-call", "starred", "rebound"], +) +def test_any_other_use_of_the_list_keeps_it_dynamic(tmp_path, body): + _write(tmp_path, _list_agent(body)) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + assert "Google ADK agent 'assign' uses a dynamic tools expression." in artifacts.warnings + + +def test_a_module_level_tools_list_stays_dynamic(tmp_path): + # Another module may change a module's list: not read here. + _write( + tmp_path, + { + "tools.py": LIST_TOOLS, + "agent.py": "from google.adk.agents import LlmAgent\nfrom tools import intake\n\n" + "TOOLS = [intake]\nroot = LlmAgent(name='assign', tools=TOOLS)\n", + }, + ) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + assert "Google ADK agent 'assign' uses a dynamic tools expression." in artifacts.warnings + + +def test_application_diff_does_not_add_a_conditional_tool(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _list_agent("tools = [FunctionTool(intake), route]")) + head = _commit( + tmp_path, _list_agent("tools = [FunctionTool(intake), route]\nif enabled:\n tools.append(FunctionTool(triage))") + ) + result = _compare(tmp_path, base, head) + + assert result["comparison_status"] == "partial" + assert [row["tool"] for row in result["rows"] if row["change"] == "added"] == [] + assert any("only under a condition" in gap["reason"] for gap in result["head"]["coverage_gaps"]) + + +# -- #865 review, round 1 ------------------------------------------------------- + + +SQL = ( + "from google.adk.tools import FunctionTool\n\n\n" + "def make_sql_tool(readonly):\n target = 'TARGET'\n\n" + " def run_sql(query: str) -> str:\n if readonly and query.lower().startswith('drop'):\n" + " raise ValueError(target)\n return query\n\n return FunctionTool(run_sql)\n" +) + + +@pytest.mark.parametrize( + ("base", "head"), + [ + ({"READONLY": "True", "TARGET": "staging"}, {"READONLY": "False", "TARGET": "staging"}), + ({"READONLY": "True", "TARGET": "staging"}, {"READONLY": "True", "TARGET": "production"}), + ], + ids=["call-argument", "closure-value"], +) +def test_what_a_factory_closes_over_is_part_of_the_tool(tmp_path, base, head): + def files(values): + return { + "tools.py": SQL.replace("TARGET", values["TARGET"]), + "agent.py": "from google.adk.agents import Agent\nfrom tools import make_sql_tool\n\n" + f"root = Agent(name='root', tools=[make_sql_tool(readonly={values['READONLY']})])\n", + } + + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, files(base)) + after = _commit(tmp_path, files(head)) + result = _compare(tmp_path, before, after) + assert [(row["tool"], row["change"]) for row in result["rows"]] == [("run_sql", "changed")] + + +def test_a_factory_docstring_is_not_a_change(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, _visulate()) + after = _commit( + tmp_path, + {"ai-agent/common/tools.py": MEMORY.replace("return content", '"""Save a record."""\n return content')}, + ) + result = _compare(tmp_path, before, after, "--scope", "ai-agent") + assert [row for row in result["rows"] if row["tool"] == "save_memory_record"] == [] + + +@pytest.mark.parametrize( + ("body", "bound"), + [ + ( + "tools = [intake]\nhelper = LlmAgent(name='helper', tools=tools)\ntools.append(route)\n" + "return LlmAgent(name='assign', tools=tools, sub_agents=[helper])", + {"helper": ["intake"], "assign": ["intake", "route"]}, + ), + ( + "tools = [intake]\nif enabled:\n return LlmAgent(name='fast', tools=tools)\ntools.append(route)\n" + "return LlmAgent(name='assign', tools=tools)", + {"fast": ["intake"], "assign": ["intake", "route"]}, + ), + ("tools = [intake]\nreturn LlmAgent(name='assign', tools=tools)\ntools.append(route)", {"assign": ["intake"]}), + ], + ids=["built-before-the-append", "early-return", "after-return"], +) +def test_an_addition_after_the_agent_is_built_is_not_its_tool(tmp_path, body, bound): + files = _list_agent(body) + # The body builds its own agents: drop the trailing one ``_list_agent`` adds. + head, _, tail = files["agent.py"].rpartition(" return LlmAgent(name='assign', tools=tools)\n") + files["agent.py"] = head + tail + _write(tmp_path, files) + loaded, artifacts = _adk(tmp_path) + edges: dict[str, list[str]] = {} + for agent, tool, _ in _edges(loaded, artifacts): + edges.setdefault(agent, []).append(tool) + assert edges == bound + assert not [warning for warning in artifacts.warnings if "only under a condition" in warning] + + +@pytest.mark.parametrize( + "factory", + [ + LOOKUP + "\n\ndef make():\n lookup.__name__ = 'search'\n return FunctionTool(lookup)\n", + LOOKUP + "\n\ndef make():\n setattr(lookup, '__name__', 'search')\n return FunctionTool(lookup)\n", + ], + ids=["attribute", "setattr"], +) +def test_a_module_function_renamed_in_the_factory_is_named(tmp_path, factory): + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make()])\n"}) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + assert "'lookup' is changed or handed on at tools.py" in _why(artifacts)["make()"][1] + + +def test_two_factories_nesting_one_name_are_named_not_a_crash(tmp_path): + factories = ( + "def make_a():\n def run(query: str) -> str:\n return query\n\n return FunctionTool(run)\n\n\n" + "def make_b():\n def run(order: str) -> str:\n return order\n\n return FunctionTool(run)\n" + ) + _git(tmp_path, "init", "-q", "-b", "main") + agent = ( + "from google.adk.agents import Agent\nfrom tools import make_a, make_b\n\n" + "a = Agent(name='a', tools=[make_a()])\nb = Agent(name='b', tools=[make_b()])\n" + ) + before = _commit(tmp_path, {"tools.py": FACTORY + LOOKUP, "agent.py": agent.replace("make_a()", "").replace("make_b()", "")}) + after = _commit(tmp_path, {"tools.py": FACTORY + factories, "agent.py": agent}) + result = _compare(tmp_path, before, after) + assert result["comparison_status"] == "partial" + assert any("defines 'run' more than once" in gap["reason"] for gap in result["head"]["coverage_gaps"]) + + +def test_an_async_factory_is_named(tmp_path): + factory = "async def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n" + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make()])\n"}) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + assert "coroutine" in _why(artifacts)["make()"][1] + + +def test_a_shadowed_function_tool_in_the_factory_is_not_proven(tmp_path): + factory = ( + "from wrappers import Wrapped\n\nFunctionTool = Wrapped\n\n\n" + "def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make()])\n"}) + loaded, _ = _adk(tmp_path) + (tool,) = [tool for source in loaded for tool in source.tools] + assert tool.extraction_confidence != "high" + + +@pytest.mark.parametrize( + "imports", + [ + "try:\n from vendor.tools import SearchTool\nexcept ImportError:\n from vendor.legacy import SearchTool\n", + "from vendor.tools import *\n", + "class SearchTool:\n pass\n", + ], + ids=["try-except-import", "wildcard", "class"], +) +def test_a_call_that_is_not_an_application_factory_keeps_its_answer(tmp_path, imports): + _write(tmp_path, {"agent.py": "from google.adk.agents import Agent\n" + imports + "\nroot = Agent(name='root', tools=[SearchTool()])\n"}) + loaded, artifacts = _adk(tmp_path) + assert "Google ADK agent 'root' has a tool expression that could not be statically resolved." in artifacts.warnings + assert artifacts.unresolved_references == [] + + +@pytest.mark.parametrize( + "use", + ["rename(tool)", "setattr(tool.func, '__name__', 'other')"], + ids=["handed-to-a-helper", "function-renamed"], +) +def test_a_factory_tool_changed_at_the_call_site_is_named(tmp_path, use): + factory = "def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n" + agent = ( + "from google.adk.agents import Agent\nfrom tools import make, rename\n\n\n" + f"def build():\n tool = make()\n {use}\n return Agent(name='root', tools=[tool])\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory + "\n\ndef rename(tool):\n tool.name = 'other'\n", "agent.py": agent}) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + assert _why(artifacts)["tool"][0] == "factory_return" + + +def test_one_record_per_unresolved_factory_call(tmp_path): + factory = "def make():\n return make()\n" + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make(), make(), make()])\n"}) + _, artifacts = _adk(tmp_path) + assert len(artifacts.unresolved_references) == 1 + + +# -- #865 review, round 2 ------------------------------------------------------- + +SQL_FACTORY = ( + "from google.adk.tools import FunctionTool\n\n\n" + "def make_sql_tool(readonly=True):\n def run_sql(query: str) -> str:\n" + " if readonly and query.lower().startswith('drop'):\n raise ValueError(query)\n" + " return query\n\n return FunctionTool(run_sql)\n" +) + + +def _sql_rows(tmp_path, base: str, head: str, extra: dict | None = None): + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, {"tools.py": SQL_FACTORY, **(extra or {}), "agent.py": base}) + after = _commit(tmp_path, {"agent.py": head}) + result = _compare(tmp_path, before, after) + return result, [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] + + +AGENT_HEAD = "from google.adk.agents import Agent\nfrom tools import make_sql_tool\n" + + +def test_a_local_value_the_factory_is_given_is_part_of_the_tool(tmp_path): + body = AGENT_HEAD + "\n\ndef build():\n ro = RO\n return Agent(name='root', tools=[make_sql_tool(readonly=ro)])\n" + _, rows = _sql_rows(tmp_path, body.replace("RO", "True"), body.replace("RO", "False")) + assert rows == [("root", "run_sql", "changed")] + + +def test_an_imported_constant_the_factory_is_given_is_part_of_the_tool(tmp_path): + agent = AGENT_HEAD + "from config import READONLY\n\nroot = Agent(name='root', tools=[make_sql_tool(readonly=READONLY)])\n" + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, {"tools.py": SQL_FACTORY, "config.py": "READONLY = True\n", "agent.py": agent}) + after = _commit(tmp_path, {"config.py": "READONLY = False\n"}) + result = _compare(tmp_path, before, after) + assert [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] == [("root", "run_sql", "changed")] + + +def test_a_value_this_read_cannot_name_leaves_the_implementation_unknown(tmp_path): + body = AGENT_HEAD + "\n\ndef build(ro):\n return Agent(name='root', tools=[make_sql_tool(readonly=ro)])\n\n\nroot = build(RO)\n" + result, rows = _sql_rows(tmp_path, body.replace("RO", "True"), body.replace("RO", "False")) + assert rows == [("root", "run_sql", "not_established")] + assert result["comparison_status"] == "partial" + (row,) = result["rows"] + assert any( + "a value this read cannot name (readonly=ro at agent.py:" in reason for reason in row["uncertainty"]["head"] + ), row["uncertainty"] + + +@pytest.mark.parametrize( + "head", + [ + "from google.adk.agents import Agent\nfrom tools import make_sql_tool as mk\n\nroot = Agent(name='root', tools=[mk(readonly=True)])\n", + AGENT_HEAD + "\nroot = Agent(name='root', tools=[make_sql_tool(True)])\n", + AGENT_HEAD + "\nroot = Agent(name='root', tools=[make_sql_tool()])\n", + AGENT_HEAD + "\nroot = Agent(name='root', tools=[make_sql_tool(readonly=True)])\nother = Agent(name='other', tools=[make_sql_tool(readonly=False)])\n", + ], + ids=["alias", "positional", "default", "another-agent-calls-it"], +) +def test_the_same_call_spelled_otherwise_is_not_a_change(tmp_path, head): + base = AGENT_HEAD + "\nroot = Agent(name='root', tools=[make_sql_tool(readonly=True)])\n" + _, rows = _sql_rows(tmp_path, base, head) + assert [row for row in rows if row[0] == "root"] == [] + + +@pytest.mark.parametrize( + "factory", + [ + "import impl\n\n\ndef make():\n impl.search.__name__ = 'lookup'\n return FunctionTool(impl.search)\n", + "import impl\n\n\ndef make():\n setattr(impl.search, '__name__', 'lookup')\n return impl.search\n", + "def make():\n from impl import search\n\n search.__name__ = 'lookup'\n return FunctionTool(search)\n", + ], + ids=["module-attribute", "setattr-on-module-attribute", "local-import"], +) +def test_a_function_renamed_in_the_factory_however_spelled_is_named(tmp_path, factory): + _write( + tmp_path, + { + "impl.py": "def search(query: str) -> str:\n return query\n", + "tools.py": FACTORY + factory, + "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make()])\n", + }, + ) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + assert "is changed or handed on" in _why(artifacts)["make()"][1] + + +@pytest.mark.parametrize( + "body", + [ + "tool = make()\n tools = [tool]\n return Agent(name='root', tools=tools)", + "tool = make()\n logging.info('bound %s', tool.name)\n return Agent(name='root', tools=[tool])", + "tool = make()\n return Agent(name='root', instruction=f'Use {tool.name}', tools=[tool])", + "fn = make_plain()\n return Agent(name='root', tools=[FunctionTool(func=fn, require_confirmation=True)])", + ], + ids=["local-list", "logged", "instruction", "wrapped-for-approval"], +) +def test_reading_a_factory_tool_where_it_is_bound_is_not_a_change(tmp_path, body): + factory = ( + "def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n\n\n" + "def make_plain():\n def plain(query: str) -> str:\n return query\n\n return plain\n" + ) + agent = ( + "import logging\n\nfrom google.adk.agents import Agent\nfrom google.adk.tools import FunctionTool\n" + f"from tools import make, make_plain\n\n\ndef build():\n {body}\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": agent}) + loaded, artifacts = _adk(tmp_path) + expected = ["plain"] if "make_plain" in body else ["inner"] + assert [tool for _, tool, _ in _edges(loaded, artifacts)] == expected, artifacts.unresolved_references + + +def test_a_class_method_of_the_same_name_is_not_a_second_tool(tmp_path): + factory = ( + "class Runner:\n def run(self):\n return None\n\n\n" + "def make():\n def run(query: str) -> str:\n return query\n\n return FunctionTool(run)\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make()])\n"}) + loaded, artifacts = _adk(tmp_path) + assert [tool for _, tool, _ in _edges(loaded, artifacts)] == ["run"] + + +def test_a_function_local_adk_import_in_the_factory_is_proven(tmp_path): + factory = ( + "def make():\n from google.adk.tools import FunctionTool\n\n" + " def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n" + ) + _write(tmp_path, {"tools.py": factory, "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make()])\n"}) + loaded, _ = _adk(tmp_path) + (tool,) = [tool for source in loaded for tool in source.tools] + assert tool.extraction_confidence == "high" + + +# -- #865 review, round 3 ------------------------------------------------------- + + +@pytest.mark.parametrize( + ("base_prefix", "head_prefix", "argument"), + [ + ("policy = {'readonly': True}\n ", "policy = {'readonly': True}\n policy['readonly'] = False\n ", "policy"), + ("allowed = ['select']\n ", "allowed = ['select']\n allowed.append('delete')\n ", "allowed"), + ], + ids=["dict-item-changed", "list-appended"], +) +def test_a_mutable_value_changed_after_it_is_bound_is_not_data(tmp_path, base_prefix, head_prefix, argument): + body = AGENT_HEAD + "\n\ndef build():\n PREFIXreturn Agent(name='root', tools=[make_sql_tool(readonly=ARG)])\n" + result, rows = _sql_rows( + tmp_path, + body.replace("PREFIX", base_prefix).replace("ARG", argument), + body.replace("PREFIX", head_prefix).replace("ARG", argument), + ) + assert rows == [("root", "run_sql", "not_established")], rows + + +def test_a_module_level_mutable_constant_is_not_data(tmp_path): + agent = AGENT_HEAD + "from config import POLICY\n\nroot = Agent(name='root', tools=[make_sql_tool(readonly=POLICY)])\n" + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, {"tools.py": SQL_FACTORY, "config.py": "POLICY = {'readonly': True}\n", "agent.py": agent}) + after = _commit(tmp_path, {"config.py": "POLICY = {'readonly': True}\nPOLICY['readonly'] = False\n"}) + result = _compare(tmp_path, before, after) + assert result["comparison_status"] == "partial" + assert any("cannot name" in gap["reason"] for gap in result["head"]["coverage_gaps"]) + + +def test_an_unnamed_value_in_an_untouched_module_is_a_gap_not_a_row(tmp_path): + body = AGENT_HEAD + "\n\ndef build(ro):\n return Agent(name='root', tools=[make_sql_tool(readonly=ro)])\n\n\nroot = build(True)\n" + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, {"tools.py": SQL_FACTORY, "agent.py": body, "README.md": "a\n"}) + after = _commit(tmp_path, {"README.md": "b\n"}) + result = _compare(tmp_path, before, after) + assert result["rows"] == [] + assert result["comparison_status"] == "partial" + assert any("readonly=ro at agent.py:" in gap["reason"] for gap in result["head"]["coverage_gaps"]) + + +@pytest.mark.parametrize( + ("factory", "call"), + [ + ( + "def upper(text: str) -> str:\n return text.upper()\n\n\ndef make(transform):\n" + " def run(query: str) -> str:\n return transform(query)\n\n return FunctionTool(run)\n", + "make(upper)", + ), + ( + SQL_FACTORY.replace("from google.adk.tools import FunctionTool\n\n\n", "") + + "\n\ndef make_named(readonly):\n return make_sql_tool(readonly=readonly)\n", + "make_named(True)", + ), + ], + ids=["function-argument", "forwarding-factory"], +) +def test_a_value_the_read_can_follow_is_data(tmp_path, factory, call): + names = call.replace("(", ", ").replace(")", "").replace(", True", "") + agent = f"from google.adk.agents import Agent\nfrom tools import {names}\n\nroot = Agent(name='root', tools=[{call}])\n" + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, {"tools.py": FACTORY + factory, "agent.py": agent, "README.md": "a\n"}) + after = _commit(tmp_path, {"README.md": "b\n"}) + result = _compare(tmp_path, before, after) + assert result["rows"] == [] and result["comparison_status"] == "compared", result["head"]["coverage_gaps"] + + +@pytest.mark.parametrize( + "use", + [ + "rename_fn(tool.func)", + "bucket = [tool]\n for each in bucket:\n each.name = 'renamed'", + "rename_all([tool])", + ], + ids=["part-handed-on", "local-list-looped", "list-handed-on"], +) +def test_a_factory_tool_handed_on_at_the_call_site_is_named(tmp_path, use): + factory = "def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n" + helpers = "\n\ndef rename_fn(fn):\n fn.__name__ = 'renamed'\n\n\ndef rename_all(tools):\n for tool in tools:\n tool.name = 'renamed'\n" + agent = ( + "from google.adk.agents import Agent\nfrom tools import make, rename_fn, rename_all\n\n\n" + f"def build():\n tool = make()\n {use}\n return Agent(name='root', tools=[tool])\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory + helpers, "agent.py": agent}) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + + +def test_another_name_for_a_module_function_in_the_factory_is_named(tmp_path): + factory = "import impl\n\n\ndef make():\n f = impl.search\n f.__name__ = 'lookup'\n return FunctionTool(impl.search)\n" + _write( + tmp_path, + { + "impl.py": "def search(query: str) -> str:\n return query\n", + "tools.py": FACTORY + factory, + "agent.py": "from google.adk.agents import Agent\nfrom tools import make\n\nroot = Agent(name='root', tools=[make()])\n", + }, + ) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + + +def test_wrapping_a_factory_tool_again_is_not_a_tool(tmp_path): + factory = "def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n" + agent = ( + "from google.adk.agents import Agent\nfrom google.adk.tools import FunctionTool\nfrom tools import make\n\n\n" + "def build():\n tool = make()\n return Agent(name='root', tools=[FunctionTool(func=tool)])\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": agent}) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + + +# -- #865 review, round 4 ------------------------------------------------------- + + +@pytest.mark.parametrize( + ("base_prefix", "head_prefix", "argument"), + [ + ( + "base = {'readonly': True}\n policy = base\n ", + "base = {'readonly': True}\n policy = base\n policy['readonly'] = False\n ", + "policy", + ), + ( + "policy = ({'readonly': True},)\n ", + "policy = ({'readonly': True},)\n policy[0]['readonly'] = False\n ", + "policy", + ), + ], + ids=["through-an-alias", "inside-a-tuple"], +) +def test_a_mutable_value_reached_another_way_is_not_data(tmp_path, base_prefix, head_prefix, argument): + body = AGENT_HEAD + "\n\ndef build():\n PREFIXreturn Agent(name='root', tools=[make_sql_tool(readonly=ARG)])\n" + _, rows = _sql_rows( + tmp_path, + body.replace("PREFIX", base_prefix).replace("ARG", argument), + body.replace("PREFIX", head_prefix).replace("ARG", argument), + ) + assert rows == [("root", "run_sql", "not_established")], rows + + +def test_a_module_tuple_holding_a_dict_is_not_data(tmp_path): + agent = AGENT_HEAD + "from config import POLICY\n\nroot = Agent(name='root', tools=[make_sql_tool(readonly=POLICY)])\n" + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, {"tools.py": SQL_FACTORY, "config.py": "POLICY = ({'readonly': True},)\n", "agent.py": agent}) + after = _commit(tmp_path, {"config.py": "POLICY = ({'readonly': True},)\nPOLICY[0]['readonly'] = False\n"}) + result = _compare(tmp_path, before, after) + assert result["comparison_status"] == "partial" + assert any("cannot name" in gap["reason"] for gap in result["head"]["coverage_gaps"]) + + +@pytest.mark.parametrize( + "use", + [ + "f = tool.func\n f.__name__ = 'renamed'", + "rename_all([tool.func])", + "if True:\n bucket = [tool]\n for each in bucket:\n each.name = 'renamed'", + ], + ids=["part-aliased", "part-in-a-container", "list-in-a-block"], +) +def test_a_factory_tool_handed_on_another_way_is_named(tmp_path, use): + factory = "def make():\n def inner(query: str) -> str:\n return query\n\n return FunctionTool(inner)\n" + helpers = "\n\ndef rename_all(items):\n for item in items:\n item.__name__ = 'renamed'\n" + agent = ( + "from google.adk.agents import Agent\nfrom tools import make, rename_all\n\n\n" + f"def build():\n tool = make()\n {use}\n return Agent(name='root', tools=[tool])\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory + helpers, "agent.py": agent}) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [] + + +@pytest.mark.parametrize( + ("factory", "wrapped"), + [ + ("def make():\n def run(query: str) -> str:\n return query.upper()\n\n return run\n", True), + ( + "def make():\n def run(query: str) -> str:\n return query\n\n" + " tool: FunctionTool = FunctionTool(run)\n return tool\n", + False, + ), + ], + ids=["plain-function-with-an-expression-inside", "annotated-tool"], +) +def test_only_a_plain_function_a_factory_returns_is_wrapped(tmp_path, factory, wrapped): + agent = ( + "from google.adk.agents import Agent\nfrom google.adk.tools import FunctionTool\nfrom tools import make\n\n\n" + "def build():\n fn = make()\n return Agent(name='root', tools=[FunctionTool(func=fn, require_confirmation=True)])\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": agent}) + loaded, artifacts = _adk(tmp_path) + assert [tool for _, tool, _ in _edges(loaded, artifacts)] == (["run"] if wrapped else []) + + +@pytest.mark.parametrize( + "append", + ["tools.append(route)", "if enabled:\n tools.append(route)"], + ids=["unconditional", "conditional"], +) +def test_a_factory_tool_in_a_list_that_grows_is_read(tmp_path, append): + factory = ( + "def make():\n def run(query: str) -> str:\n return query\n\n return FunctionTool(run)\n\n\n" + "def route(zone: str) -> str:\n return zone\n" + ) + agent = ( + "from google.adk.agents import Agent\nfrom tools import make, route\n\n\n" + f"def build(enabled):\n tool = make()\n tools = [tool]\n {append}\n return Agent(name='root', tools=tools)\n" + ) + _write(tmp_path, {"tools.py": FACTORY + factory, "agent.py": agent}) + loaded, artifacts = _adk(tmp_path) + assert "run" in [tool for _, tool, _ in _edges(loaded, artifacts)] + + +# -- #865 review, round 5 ------------------------------------------------------- + + +def test_two_constructions_whose_factory_values_differ_are_not_one_agent(tmp_path): + body = ( + AGENT_HEAD + "from tools import lookup\n\nroot_agent = Agent(name='root', tools=[make_sql_tool(readonly=RO_A), lookup])\n\n\n" + "def build():\n return Agent(name='root', tools=[make_sql_tool(readonly=RO_B), lookup])\n" + ) + tools = SQL_FACTORY + "\n\ndef lookup(query: str) -> str:\n return query\n" + _git(tmp_path, "init", "-q", "-b", "main") + before = _commit(tmp_path, {"tools.py": tools, "agent.py": body.replace("RO_A", "True").replace("RO_B", "False")}) + after = _commit(tmp_path, {"agent.py": body.replace("RO_A", "False").replace("RO_B", "True")}) + result = _compare(tmp_path, before, after) + assert result["comparison_status"] == "partial" + assert any("more than once" in gap["reason"] for gap in result["head"]["coverage_gaps"]), result["head"]["coverage_gaps"] diff --git a/tests/test_application_diff_review.py b/tests/test_application_diff_review.py index a1b0e2134..914c6ae9d 100644 --- a/tests/test_application_diff_review.py +++ b/tests/test_application_diff_review.py @@ -69,8 +69,8 @@ def test_unknown_implementation_does_not_hide_an_observed_addition(repo, monkeyp definition = module._definition - def missing(root, tool): - value = definition(root, tool) + def missing(root, tool, *args): + value = definition(root, tool, *args) if tool.name == "lookup": value["implementation_sha256"] = None return value