From 5d7b8221110b76a688fc9fc3d2a19623f1c4a0d6 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Fri, 25 Sep 2026 20:03:11 -0700 Subject: [PATCH 1/7] fix(diff --application): an agent the reader cannot see never reads as compared (#876) On kkmiecik-coder/CRM#5 the quoting agent, built by `return Agent(...)` inside a function, gained a tool. The only agent the SDK reader observed was a test double, so the comparison said `compared` with no rows. - Every OpenAI Agents SDK construction is observed or named. - Observed: `return Agent(...)`, `self.agent = Agent(...)`, inline in a list, positional names, and `Agent[Ctx](...)`. A function-local agent variable is identified by its literal name. - Named limits, on the agent or the file: - one identity built at two sites that bind different tools; - `**` or extra positional arguments; - a capability-passing copy (`clone`, `dataclasses.replace`, `copy.replace`) of a value not proven to be a non-agent; - an instance of a same-module `Agent` subclass. - A capability change after construction (assign, extend, slice, `setattr`, or a handle `t = x.tools`) is read in its own scope. It is a limit on the agent it reaches, a limit on the file when unknown, and nothing when proven not to be an agent. - The census in `diff` names, on each module that is not read as an SDK source: a copy, a capability change, and the use of an SDK `Agent` subclass defined elsewhere. - Test files are not the application. They are listed in `excluded_tests`, never read as sources, and never refuse a comparison. A duplicate tool in application code limits only its own file. - A Google ADK agent name constructed twice in one file is a named limit. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 + docs/application-comparison.md | 92 +- src/agents_shipgate/cli/application_diff.py | 164 +++- src/agents_shipgate/inputs/google_adk.py | 17 + .../inputs/openai_sdk_static.py | 875 ++++++++++++++++- tests/test_application_diff_identity.py | 23 +- tests/test_application_diff_unobserved.py | 927 ++++++++++++++++++ tests/test_imported_tool_review.py | 3 +- 8 files changed, 2074 insertions(+), 34 deletions(-) create mode 100644 tests/test_application_diff_unobserved.py diff --git a/CHANGELOG.md b/CHANGELOG.md index bac9c6e7..9f4bbb4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,13 @@ - `diff --application` no longer reads another library's `Agent` or `function_tool` as the OpenAI Agents SDK's, and no longer reports an agent it stopped observing as removed. On speechmatics/speechmatics-academy#142, a LiveKit voice agent (`from livekit.agents import Agent, function_tool`) moved its tools into an `Agent` subclass that passes them through `super().__init__`. The comparison printed `compared` with four `REMOVED agent → …` rows although the same four tools were still bound. It now prints `not_established`: no supported application agent on either side. (#871 follow-up; related #580, #864, #865) - **Framework identity.** Discovery and the SDK reader count `function_tool` and `Agent` as the SDK's only when the name is imported from the absolute `agents`/`openai_agents` package, or is not imported at all (the existing reading, unless a wildcard import from another module could supply it). The name is resolved in the scope that uses it, as Python resolves it, so an import in a sibling function never decides it, and a parameter or local assignment of that name is not the SDK's. A name imported from anywhere else — `livekit.agents`, a relative `.agents` package — is another library's. A LiveKit file is no longer an SDK candidate, and a file using both reads only the SDK's symbols. A relative import is no longer any framework's import signal. `scan` of a declared SDK source reads the same way. - **Unobserved is not removed.** One side may observe an agent while the other side's same file still assigns or imports its name (`from factory import agent`), or passes it as `name=`, through a construction the reader does not support: an `Agent` subclass, a factory, `Agent[Context](...)` or `.clone()`. That side then records a coverage gap for the agent. The result is `partial`, and its rows are `not_established` with `candidate_change` `removed` or `added`. An agent referenced only in another agent's `handoffs` is not an observed construction. An agent whose name is gone from the file is still an established removal, and rows for other agents are unaffected. No schema changes. +- `diff --application` no longer reports `compared` with no changes while an agent it could not see gained a tool. On kkmiecik-coder/CRM#5, the quoting agent `Wycena`, built by `return Agent(name="Wycena", tools=NARZEDZIA_WYCENY)` inside a function, gained `wyslij_obraz`; the only agent read was a test double, so the result said `compared` and printed nothing. (#876) + - **Every SDK construction is observed or named.** The OpenAI Agents SDK reader read only `name = Agent(...)`. It now also reads `return Agent(...)`, `self.agent = Agent(...)`, agents inline in a list, `Agent("name")` and `Agent[Context](...)`, identified by the literal `name`. An agent assigned to a plain name keeps that name as its identity, so renaming its `name=` or moving it between scopes changes nothing; only a variable name assigned in more than one function or class body (two builders' local `agent`) gives way to each agent's literal `name`, so those are two agents, and a handoff to such a variable reaches its own. What it cannot read is a named limit on that agent, so other agents' rows stand: one identity constructed at two sites that bind different tools (identical constructions are one agent), including a Google ADK agent name; `**` or extra positional arguments; a copy (`clone`, `dataclasses.replace`, `copy.replace`) passing its own `tools`/`handoffs`/`mcp_servers`, or only `**` on a value known to be an agent; and an agent built from a same-module `Agent` subclass, including one made by `type("X", (Agent,), {})`. A construction without a literal name is a limit on its file. + - **Changes after construction.** A change to an agent's tools, handoffs or MCP servers — assigning, extending or slicing `x.tools`, a list method on it, `setattr`/`delattr` by name, or a handle `t = x.tools` that is itself changed later (a handle only read, like `len(request.tools)`, is nothing) — is read in the scope where it happens: on an agent the reader constructed (directly, through `self.agent`, or through a module function that returns it) it is a limit on that agent, and a change in place is one on every agent built from the same list object; on a value proven not to be an agent (`Settings()`, a literal, `type(...)()`, the instance's own `self.tools`) it is nothing; on anything else it is a limit on the file. In a module that is not read as an SDK source — a Google ADK source included, whose reader does not follow a change after construction either — a copy is a named limit on it, and so is a change to any object's tools once the module imports anything from the scope, or is itself a Google ADK source — through an alias, a loop, a parameter or a call's result, as in an SDK file — unless the object is plainly not an agent (a literal, or an instance of a class the scope defines that is not an `Agent` subclass); a module that imports nothing from the scope has another library's `.tools`. A module that imports an SDK `Agent` subclass from the scope, directly or through a package that re-exports it (`from app.core import *`), is limited too, never one that only spells its name in a string or imports a vendor class of the same name. + - **Rows and verdicts that change.** A construction that used to be unread and silently absent is now either read (rows appear) or named (the result, and a `scan` of the same code, becomes `partial` / `insufficient_evidence` instead of complete). A Google ADK agent name constructed at two sites in one file is `insufficient_evidence` for `scan` too, where it was `review_required`. A copy passing no capabilities of its own, and a subclass nothing instantiates, are not limits. + - **Test files are not the application.** Test files (the discovery convention, relative to the scope: `test`/`tests` directories, `test_*.py`, `*_test.py`, `conftest.py`, `test.py`, `tests.py`) are listed per side in `excluded_tests`, printed in the text output, and not read as agent sources, so a test double cannot establish a scope and a test file's own defects are not application gaps. + - **A duplicate tool definition limits one file.** A file defining one tool name twice refused the whole comparison with exit 2, and the message asked for the test file to be edited. It is now a named limit on that file; its agents are not compared, and every other file's are. Seen on alliance-genome/agr_ai_curation#842 and usestrix/strix#1103. + - Additive on the unreleased `0.1` advisory result (`excluded_tests` per side); no schema or contract bump. - `diff --application` no longer refuses a repository over a link or submodule its application reader never opens, and reads a Google ADK agent imported from the package root. (#871 follow-up, measured for #868) - **The problem.** On 2026-09-25, against open third-party pull requests that edit SDK/ADK tool wiring, 15 of the first 27 runs at the default root scope exited 2 with `Git tree contains unsupported external binding`. The path named was never application source: `CLAUDE.md -> AGENTS.md` (dlt-hub/dlt#4417, jaegertracing/jaeger#9636, omnigent-ai/omnigent#6611, wandb/weave#7948), a linked `.claude/skills/…` or `.agents/skills/…` directory (asterinas#3834, vllm#57322, kagent-dev/kagent#2788), `agent/VERSION -> ../../VERSION` (TencentCloud/CubeSandbox#1508), `.pylintrc` (tensorflow#128063), and a vendored submodule (temporalio/sdk-python#1868). The root scope took the unscoped archive route, which refuses every link and gitlink. `from google.adk import Agent` was not read as an agent, so CubeSandbox#1508's `root_agent` gave `not_established` ("No supported application agents were established"). - **Links.** The root scope now uses the scoped materializer a named scope already used, which recreates each link as a link. A link is never read through. Every link under the scope is censused, a dangling one included, and gapped only where it can hide application source: a `*.py` link whose target is not a Python input the scope already reads (an alias of one is compared at the target's path; reading it too made one agent two ambiguous ones and hid the real file's change, as a named scope did on main); a link to a directory outside the scope that holds Python; and a link resolving to nothing in the repository where the other side reads source at or beneath its path. Replacing `agent.py` with a dangling link, or a source directory with an absolute link, is therefore `not_established`, never a removal. A link Python discovery would not read changes nothing, and a scope that is itself a link is still refused. The host-configuration census, which counts every link that could conceal a *host* path, no longer adds application coverage gaps. diff --git a/docs/application-comparison.md b/docs/application-comparison.md index 4c3410ef..212e3527 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -68,13 +68,95 @@ artifact from the existing host diff JSON and verifier receipt. - `not_established`: neither side established a supported application agent. - Exit 2: refs/materialization/input could not be read. This is not no change. +Every OpenAI Agents SDK `Agent(...)` construction in a file the scope reads is +either an observed agent or a named limit. An agent assigned to a plain name +is identified by that name (`assistant = Agent(...)` is `assistant`), as +before, so renaming its `name=` or moving it into a builder changes nothing. +Only a variable name assigned in more than one function or class body — two +builders' local `agent` — gives way to each agent's literal `name`, and a handoff to such a +variable is named by the agent it holds. Any other construction is identified +by its literal `name`: `return Agent(name="Quote", ...)` inside a factory +function, `self.agent = Agent(name="Held", ...)`, an agent inline in a list, +or `Agent("Positional")`. `Agent[Context](...)` is the same construction. + +What the reader cannot read is a named limit on the agent it concerns, so other +agents' rows in the file stand: + +- one identity constructed at more than one site in a file with different + tools or handoffs (two `return Agent(name="Quote", ...)` branches, or a + literal `name` equal to another agent's variable) — the binding graph would + merge them, so the tools are attributed to neither; constructions that bind + exactly the same tools and handoffs are one agent. The same holds for a + Google ADK agent name; +- a construction with `**` keyword unpacking or positional arguments after its + name, which can carry `tools` or `handoffs`; +- an agent whose `tools`, `handoffs` or `mcp_servers` are changed after + construction: assigned, extended or sliced (`agent.tools.append(...)`, + `agent.tools = [...]`, `agent.tools[:] = ...`), `setattr`/`delattr` by name, + or a handle taken on them (`t = agent.tools`) that is itself changed later — + a handle only read, like `len(request.tools)`, is nothing. The changed value is read in + the scope of the change (a change in place reaches every agent built from the + same list object): an agent the reader constructed — directly, through + `self.agent`, or through a module function that returns it — carries the + limit; a value proven not to be an agent (`Settings()`, a literal, + `type(...)()`, the instance's own `self.tools`) carries nothing; anything + else is a limit on the file; +- a copy that passes its own `tools`, `handoffs` or `mcp_servers` + (`agent.clone(tools=...)`, `dataclasses.replace(agent, tools=...)`, + `copy.replace(...)`), wherever the original came from, unless the original + is proven not to be an agent. A copy passing only `**` is a limit only on a + value known to be an agent. A copy that passes none of them keeps the + original's tools, which the original's rows already compare, and is not a + limit; +- an agent built from a subclass of the SDK's `Agent` defined in the same + module, whose tools arrive through its constructor, including one made with + `type("X", (Agent,), {})`. + +A construction whose `name` is not a literal is a limit on its file. + +A module that is not read as an SDK source is still read for what can change +an agent without it: a capability-passing copy, and — once the module imports +anything from the scope, or is a Google ADK source, whose reader does not +follow a change after construction — a change to any object's tools, handoffs, MCP +servers or sub-agents, reached by an alias, a loop, a parameter or a call's +result as in an SDK file, unless the object is plainly not an agent (a literal, +or an instance of a class the scope defines that is not an `Agent` subclass). +A module that imports nothing from the scope has another library's `.tools`. +In any module, importing an SDK `Agent` subclass from the scope — `from core +import Assistant`, `core.Assistant` after `import core`, or through a package +that re-exports it with `from app.core import *` — is a limit; a subclass +nothing imports, a vendor class of the same name, or a name that only appears +in a string is not. Each is a limit on the module where it appears. The scope's +own package path counts as the scope (`from svc.app.x import …` under +`--scope svc/app`). + +Not read at all: an `Agent` re-exported through a project module, +`functools.partial(Agent, ...)`, or a subclass defined outside the scope. + An agent one side observes is absent from the other only when that side's file no longer names it. If the file still assigns or imports the agent's name -(`from factory import agent`), or passes it as `name=`, through a construction the reader does not support (an `Agent` -subclass passing `tools` through `super().__init__`, a factory, -`Agent[Context](...)`, `.clone()`), that side records a gap for the agent and -its rows are `not_established`, never `removed` or `added`. An agent referenced -only in another agent's `handoffs` is not an observed construction. +(`from factory import agent`, `agent = build()`), or passes it as `name=`, +through a construction the reader does not read (an `Agent` subclass, a +`.clone()`), that side records a gap for the agent and its rows are +`not_established`, never `removed` or `added`. A factory's own `return +Agent(name="assistant", ...)` is read under that literal name in the module +that builds it; it is a construction site of its own, not evidence about the +name the factory's result is assigned to. An agent referenced only in another +agent's `handoffs` is not an observed construction. + +Test files never establish the application. A file under a `test` or `tests` +directory, a `test_*.py` or `*_test.py` module, `conftest.py`, `test.py` or +`tests.py` — matched case-sensitively and relative to the selected scope, the +convention discovery uses — is listed in each side's `excluded_tests`, printed +in the text output, and not read as an agent source: a test double's agent is +not the application's agent, so it cannot make a scope count as established, +and a test file's own defects (a tool defined twice, an unsupported framework, +a parse failure) are not the application's gaps. A product module that only +looks like a test is excluded too, which the printed list makes visible; the +import resolver still follows a tool the application imports from such a file. + +A file that defines one tool name twice is a named limit on that file, and the +agents it constructs are not compared; every other file in the scope still is. Framework identity follows the import, not the spelling. `Agent` and `function_tool` are the OpenAI Agents SDK's only when imported from the absolute diff --git a/src/agents_shipgate/cli/application_diff.py b/src/agents_shipgate/cli/application_diff.py index b0984cec..036c50e1 100644 --- a/src/agents_shipgate/cli/application_diff.py +++ b/src/agents_shipgate/cli/application_diff.py @@ -20,6 +20,7 @@ from agents_shipgate.cli.discovery import detect_workspace from agents_shipgate.cli.discovery.artifacts import _candidate_files, _skip_part +from agents_shipgate.cli.discovery.signals import _is_test_path from agents_shipgate.cli.scan.source_loading import _build_canonical_tools from agents_shipgate.cli.verify.git import ( PromisedObjectsMissingError, @@ -29,14 +30,18 @@ ensure_git_workspace, tree_sha, ) +from agents_shipgate.core.adopter_text import DUPLICATE_TOOL_IN_SOURCE from agents_shipgate.core.agent_bindings import resolve_agent_binding_graph from agents_shipgate.core.artifacts import ArtifactBag from agents_shipgate.core.domain import ANY_TOOL -from agents_shipgate.core.errors import ConfigError +from agents_shipgate.core.errors import ConfigError, InputParseError from agents_shipgate.core.privacy import sanitize_report_payload from agents_shipgate.core.verification_identity import build_engine_requirement from agents_shipgate.inputs.google_adk import load_google_adk_artifacts -from agents_shipgate.inputs.openai_sdk_static import load_openai_sdk_static_tools +from agents_shipgate.inputs.openai_sdk_static import ( + census_module, + load_openai_sdk_static_tools, +) from agents_shipgate.inputs.python_imports import RepositoryLayout, repository_layout from agents_shipgate.schemas.manifest import ToolSourceConfig @@ -73,6 +78,9 @@ class Observations: submodules: dict[str, str] = field(default_factory=dict) #: Links under the scope that resolve to nothing in the tree. unresolved_links: list[str] = field(default_factory=list) + #: Test files under the scope, which never establish the application + #: (#876): a test double's agent is not the application's agent. + excluded_tests: list[str] = field(default_factory=list) def gap( self, @@ -137,6 +145,7 @@ def summary(self) -> dict[str, Any]: "coverage_gaps": sorted( self.coverage_gaps, key=lambda gap: json.dumps(gap, sort_keys=True) ), + "excluded_tests": sorted(self.excluded_tests), } @@ -363,6 +372,17 @@ def observe( if not root.is_relative_to(tree.resolve()): raise ConfigError(f"Application scope escapes its tree: {scope}") python_files = [p for p in _candidate_files(root) if p.suffix == ".py"] + # Test code is not the application (#876). A test double's agent must not + # establish the scope, and a test file's own defects (a tool defined twice, + # an unsupported framework, a parse failure) must not stand in for the + # application's. Tests are listed, not read. + result.excluded_tests = sorted( + p.relative_to(root).as_posix() + for p in python_files + if _is_test_path(p.relative_to(root).as_posix()) + ) + tests = set(result.excluded_tests) + python_files = [p for p in python_files if p.relative_to(root).as_posix() not in tests] for file in python_files: if file.stat().st_size > MAX_PYTHON_BYTES: result.gap(f"Python input exceeds {MAX_PYTHON_BYTES} bytes: {file.relative_to(root)}") @@ -383,19 +403,62 @@ def observe( # candidate list becoming negative evidence. Bound this second parse too. if len(python_files) > max_python_files: result.gap(f"Python input census exceeds {max_python_files} files.") + # A copy of an agent that passes its own tools, or a change to an agent's + # tools, can live in a module no reader reads as an SDK source; and an + # agent subclass defined in one module can be instantiated in another + # (#876 review). + censuses: dict[str, Any] = {} + texts: dict[str, str] = {} + # The SDK reader reads its own sources' copies and changes. + read_as_sdk = { + path + for framework in detected.frameworks + if framework.type == "openai_agents_sdk" + for path in framework.candidate_files + } + read_as_adk = { + path + for framework in detected.frameworks + if framework.type == "google_adk" + for path in framework.candidate_files + } + # Top-level names a module in the scope can be imported by. + local_modules = frozenset( + { + PurePosixPath(file.relative_to(root).as_posix()).parts[0].removesuffix(".py") + for file in python_files + } + # The scope, and every package above it, may be what its modules + # import through (``from svc.app.agents_def import …``). + | {root.name} + | set(PurePosixPath(scope).parts) + ) for file in python_files[:max_python_files]: - if file.relative_to(root).as_posix() in linked: + relative = file.relative_to(root).as_posix() + if relative in linked: continue try: - ast.parse(file.read_bytes()) + raw = file.read_bytes() + parsed = ast.parse(raw) except (SyntaxError, ValueError, RecursionError, OSError): result.gap( - f"Python input could not be parsed: {file.relative_to(root)}", - source=file.relative_to(root).as_posix(), + f"Python input could not be parsed: {relative}", + source=relative, ) + continue + texts[relative] = raw.decode("utf-8", errors="replace") + censuses[relative] = census_module( + parsed, + texts[relative], + local_modules, + read_as_sdk=relative in read_as_sdk, + read_as_adk=relative in read_as_adk, + ) for framework in detected.frameworks: if framework.type not in SUPPORTED and framework.candidate_files: for path in framework.candidate_files: + if path in tests: + continue result.gap( f"Application comparison does not yet support {framework.type}: {path}", source=path, @@ -406,18 +469,81 @@ def observe( for f in detected.frameworks if f.type in SUPPORTED for p in f.candidate_files - if p not in linked + if p not in linked and p not in tests } ) sources = [ ToolSourceConfig(id=f"{kind}:{path}", type=kind, path=path) for kind, path in entries ] + sdk_sources = {path for kind, path in entries if kind == "openai_agents_sdk"} + for path, census in sorted(censuses.items()): + if path in sdk_sources: + # The SDK reader reads these itself, against the agents it saw. + continue + if census.copies: + result.gap( + f"An agent copy at {path}:{census.copies[0]} passes its own tools, handoffs " + "or MCP servers in a module that does not import the OpenAI Agents SDK; it " + "is not read.", + source=path, + ) + changes = [ + line for line, built in census.changes if not _plain_scope_class(built, censuses) + ] + if changes: + result.gap( + f"An object's tools, handoffs, MCP servers or sub-agents are changed at " + f"{path}:{changes[0]}, in a module the comparison does not read for " + "that; agents it reaches are not established.", + source=path, + ) + for defining, census in sorted(censuses.items()): + for name, line in sorted(census.subclasses.items()): + # A package that re-exports the class (``from app.core import *``) + # provides it too, transitively. + providers = {defining} + grown = True + while grown: + grown = False + for path, other in censuses.items(): + if path not in providers and any( + other.reexports(name, provider) for provider in providers + ): + providers.add(path) + grown = True + for path, other in sorted(censuses.items()): + if path != defining and any(other.uses(name, provider) for provider in providers): + result.gap( + f"{path} uses {name}, an OpenAI Agents SDK agent subclass defined at " + f"{defining}:{line}; agents built from it are not read.", + source=path, + ) result.sources = [{"type": s.type, "path": s.path} for s in sources] for source in sources: _observe_source(result, root, source) return result +def _plain_scope_class(built: tuple[str, str] | None, censuses: dict[str, Any]) -> bool: + """Whether a receiver was built from a class the scope defines that is no agent. + + ``p = Payload(); p.tools = specs`` with ``Payload`` a model in + ``app/schemas.py`` changes a request payload, not an agent (#876 review). + """ + if built is None: + return False + tail, name = built + defining = [ + census + for path, census in censuses.items() + if PurePosixPath(path).stem == tail + or (PurePosixPath(path).name == "__init__.py" and PurePosixPath(path).parent.name == tail) + ] + return bool(defining) and all( + name in census.classes and name not in census.subclasses for census in defining + ) + + def _resolved(path: Path) -> Path | None: try: return path.resolve(strict=True) @@ -553,7 +679,19 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) ) attributed.add(warning) result.handoff_only |= handoff_targets - constructed - tools, warnings = _build_canonical_tools(loaded) + try: + tools, warnings = _build_canonical_tools(loaded) + except InputParseError as exc: + if exc.details.get("failure") != DUPLICATE_TOOL_IN_SOURCE: + raise + # One file's duplicate is a limit on that file, not a refusal of every + # other agent in the scope (#876). + result.gap( + f"{source.path} defines the tool {exc.details.get('tool_name')!r} more " + "than once, so the agents it constructs are not compared.", + source=source.path, + ) + return for warning in warnings: result.gap(warning, source=source.path) graph, _ = resolve_agent_binding_graph(None, tools, bag, loaded) @@ -1139,5 +1277,15 @@ def in_scope(path: str, selected: str = selected_scope) -> bool: for side in ("base", "head"): for limit in payload[side]["limits"]: typer.echo(f" {side} limit: {_one_line(limit)}") + # Test files are never the application (#876); say which were left + # out, so a product module that only looks like a test is visible. + excluded = sorted( + set(payload["base"].get("excluded_tests", [])) + | set(payload["head"].get("excluded_tests", [])) + ) + if excluded: + shown = ", ".join(_one_line(path) for path in excluded[:5]) + more = f", and {len(excluded) - 5} more" if len(excluded) > 5 else "" + typer.echo(f"Test files not read as the application ({len(excluded)}): {shown}{more}") typer.echo(payload["limits"][-1]) return 0 diff --git a/src/agents_shipgate/inputs/google_adk.py b/src/agents_shipgate/inputs/google_adk.py index 6ca5c464..1688223e 100644 --- a/src/agents_shipgate/inputs/google_adk.py +++ b/src/agents_shipgate/inputs/google_adk.py @@ -190,6 +190,9 @@ #: ``lookup`` and an imported ``other.lookup``. The model sees one name for two #: callables, so which one runs is not something the source settles (#864). SURFACE_GAP_DUPLICATE_TOOL_NAME = "duplicate_tool_name" +#: One agent name constructed at more than one call site in a module: the +#: binding graph merges them, so no tool can be attributed to either (#876). +SURFACE_GAP_DUPLICATE_AGENT_NAME = "duplicate_agent_name" SURFACE_GAP_UNRESOLVED_SUB_AGENT = "unresolved_sub_agent" #: The module reaches an agent's ``tools`` attribute after construction, or #: builds an agent from unpacked keyword arguments. Reading the ``tools=`` @@ -1018,6 +1021,8 @@ def __init__( # ordinal within one agent's tool list, never a line number. self.inline_slot_counts: dict[str, int] = {} self.agent_bindings: dict[str, _AdkAgentBinding] = {} + #: ``agent name -> {id(call): line}`` for every construction site. + self.agent_sites: dict[str, dict[int, int]] = {} # Reasons this module's tool surface was not proven complete (#393). # Empty at the end of ``extract`` is what earns ``SURFACE_ENUMERATED``. self.surface_gaps: list[str] = [] @@ -1403,6 +1408,18 @@ def _binding_for(self, agent_name: str, call: ast.Call) -> _AdkAgentBinding: source_pointer=f"{self.source_ref}:{call.lineno}", ) self.agent_bindings[agent_name] = binding + sites = self.agent_sites.setdefault(agent_name, {}) + if id(call) not in sites: + sites[id(call)] = call.lineno + if len(sites) == 2: + lines = ", ".join(str(line) for line in sorted(sites.values())) + reason = ( + f"Google ADK agent {agent_name!r} is constructed more than once in " + f"{self.source_ref} (lines {lines}); its tools are not attributed to " + "either construction." + ) + self._surface_warning(reason, SURFACE_GAP_DUPLICATE_AGENT_NAME) + binding.issues.append(reason) return binding def _binding_observations(self) -> list[AgentBindingObservation]: diff --git a/src/agents_shipgate/inputs/openai_sdk_static.py b/src/agents_shipgate/inputs/openai_sdk_static.py index fbd11362..6ec20052 100644 --- a/src/agents_shipgate/inputs/openai_sdk_static.py +++ b/src/agents_shipgate/inputs/openai_sdk_static.py @@ -2,7 +2,8 @@ import ast from collections.abc import Callable -from pathlib import Path +from dataclasses import dataclass +from pathlib import Path, PurePosixPath from typing import Any, ClassVar, Literal from agents_shipgate.core.domain import ( @@ -223,10 +224,41 @@ def _extract_agent_bindings( scopes = ScopeIndex(tree) module = imports.resolver.entry(path, tree, text) import_aliases: dict[str, str] = {} + # The call an assignment binds to a plain name: that name is the + # agent's identity, as it always has been. Any other construction — + # ``return Agent(...)``, ``self.agent = Agent(...)``, an agent inline in + # a list — is identified by its literal ``name`` (#876). A variable name + # assigned in more than one function (two builders' local ``agent``) is + # no identity at all, so those agents take their literal ``name`` too; + # anywhere else a rename or a move between scopes keeps the identity + # it had (#876 review). + assigned: dict[int, str] = {} + variable_scopes: dict[str, set[int]] = {} + function_local: set[int] = set() for node in ast.walk(tree): if isinstance(node, ast.ImportFrom): for alias in node.names: import_aliases[alias.asname or alias.name] = alias.name + if isinstance(node, (ast.Assign, ast.AnnAssign)): + target = _assignment_target(node) + if target and isinstance(node.value, ast.Call): + assigned[id(node.value)] = target + enclosing = _enclosing_scope(scopes, node) + variable_scopes.setdefault(target, set()).add(id(enclosing)) + if enclosing is not None: + function_local.add(id(node.value)) + shared = {name for name, found in variable_scopes.items() if len(found) > 1} + + def identity_of( + call: ast.Call, + assigned: dict[int, str] = assigned, + function_local: set[int] = function_local, + shared: set[str] = shared, + ) -> str | None: + target, literal = assigned.get(id(call)), _literal_agent_name(call) + if literal is not None and target in shared and id(call) in function_local: + return literal + return target or literal tool_lists = _ToolLists( tree, scopes, @@ -238,22 +270,113 @@ def _extract_agent_bindings( else None ), ) + subclasses = _agent_subclasses(tree, sdk_names) + values = _AgentValues(tree, scopes, tool_lists.module_bindings, sdk_names, subclasses) + #: ``list binding -> identities`` of agents constructed with that list. + list_holders: dict[object, set[str]] = {} + copies: list[ast.Call] = [] + + def unread(reason: str, pointer: str, kind: str, path: str = source_ref) -> None: + # A construction the reader saw but cannot establish is a named + # limit on this file, never silence: an unobserved agent must not + # let the comparison read as complete (#876). + warnings.append(reason) + recovery_evidence.append(SourceRecoveryEvidence( + warning=reason, source_id=source.id, source_type="openai_agents_sdk", + source_ref=pointer, path=path, + recovery=CoverageRecovery(kind="unresolved", reason=kind), + )) + + def unread_agent( + call: ast.Call, + reason: str, + kind: str, + file_observations: list[AgentBindingObservation], + source_ref: str = source_ref, + identity_of: Callable[[ast.Call], str | None] = identity_of, + unread: Callable[..., None] = unread, + values: _AgentValues = values, + ) -> None: + # An agent whose capabilities the reader cannot read is still an + # agent: a named limit on *it*, so other agents' rows stand. + pointer = f"{source_ref}:{call.lineno}" + identity = identity_of(call) + if identity is None: + unread(reason, pointer, kind) + return + values.constructed[id(call)] = identity + warnings.append(reason) + file_observations.append( + AgentBindingObservation( + agent=identity, + source_id=source.id, + source=source_ref, + source_pointer=pointer, + tools_complete=False, + handoffs_complete=False, + issues=[reason], + ) + ) + + file_observations: list[AgentBindingObservation] = [] for node in ast.walk(tree): - if not isinstance(node, (ast.Assign, ast.AnnAssign)): + if not isinstance(node, ast.Call): continue - target = _assignment_target(node) - call = node.value if isinstance(node.value, ast.Call) else None - if ( - not target - or call is None - or not sdk_names.denotes( - dotted_name(call.func), call, "Agent", DEFAULT_AGENT_CONSTRUCTORS + pointer = f"{source_ref}:{node.lineno}" + if _copy_shape(node) is not None: + # Whether the receiver is an agent is known only once every + # construction in the file has been read. + copies.append(node) + continue + func = node.func.value if isinstance(node.func, ast.Subscript) else node.func + if isinstance(func, ast.Name) and func.id in subclasses: + unread_agent( + node, + f"OpenAI Agents SDK agent at {pointer} is built from the subclass " + f"{func.id!r} (line {subclasses[func.id]}), whose constructor this " + "reader does not read.", + "sdk_agent_subclass_unread", + file_observations, + ) + continue + if not _denotes_agent(sdk_names, node): + continue + call = node + target = identity_of(call) + if target is None: + unread( + f"OpenAI Agents SDK agent constructed at {pointer} has no literal " + "name, so it cannot be identified; its tools are not read.", + pointer, + "sdk_agent_identity_unresolved", + ) + continue + values.constructed[id(call)] = target + # The list object this agent holds, when ``tools=`` names one: an + # in-place change through any agent sharing it reaches this one too. + shared_tools = _keyword(call, "tools") + if isinstance(shared_tools, ast.Name): + found_list = scopes.enclosing_bindings(call, shared_tools.id) + list_holders.setdefault( + id(found_list[0]) if found_list else ("module", shared_tools.id), set() + ).add(target) + opaque = _opaque_arguments(call) + if opaque is not None: + reason = ( + f"OpenAI Agents SDK agent {target!r} at {pointer} is constructed with " + f"{opaque}, so its tools and handoffs are not read." + ) + warnings.append(reason) + file_observations.append( + AgentBindingObservation( + agent=target, source_id=source.id, source=source_ref, + source_pointer=pointer, tools_complete=False, + handoffs_complete=False, issues=[reason], + ) ) - ): continue tools_expr = _keyword(call, "tools") references = tool_lists.references(tools_expr, call) - pointer = f"{source_ref}:{call.lineno}" issues: list[str] = [] tools_complete = True names: list[str] = [] @@ -372,7 +495,9 @@ def _extract_agent_bindings( ) warnings.append(reason) tool_issues[tool.name] = reason - handoff_names = tool_lists.names(_keyword(call, "handoffs"), call, import_aliases) + handoff_names = tool_lists.names( + _keyword(call, "handoffs"), call, import_aliases, identity_of, sdk_names + ) handoffs_complete = True if handoff_names is None: reason = f"OpenAI Agents SDK agent {target!r} has dynamic handoffs at {pointer}." @@ -380,7 +505,7 @@ def _extract_agent_bindings( issues.append(reason) handoffs_complete = False handoff_names = [] - observations.append( + file_observations.append( AgentBindingObservation( agent=target, source_id=source.id, @@ -395,6 +520,109 @@ def _extract_agent_bindings( issues=issues, ) ) + for node in copies: + clone_of = _capability_rebinding_copy( + node, lambda receiver, node=node, values=values: values.is_agent(receiver, node) + ) + if clone_of is None: + continue + pointer = f"{source_ref}:{node.lineno}" + unread_agent( + node, + f"OpenAI Agents SDK agent copy at {pointer} ({clone_of}) is not read: " + "it passes its own tools, handoffs or MCP servers.", + "sdk_agent_clone_unread", + file_observations, + ) + # A change to an agent's tools, handoffs or MCP servers after it is + # constructed — ``agent.tools.append(x)``, ``self.agent.tools = [...]``, + # ``setattr(agent, "tools", ...)``, a handle ``t = agent.tools`` — is a + # limit on the agent the changed value is, read in the scope of the + # change. A value the reader cannot identify is a limit on the file; + # one it can prove is not an agent is nothing (#876 review). + unattributed = False + limited: set[str] = set() + for receiver, site in _capability_changes(tree, scopes=scopes): + owner = values.owner(receiver, site) + # One limit per agent, and one for the file: the first site names it. + if owner is None or (owner is True and unattributed) or owner in limited: + continue + if isinstance(owner, str): + limited.add(owner) + pointer = f"{source_ref}:{site.lineno}" + if owner is True: + unattributed = True + unread( + f"OpenAI Agents SDK tools, handoffs or MCP servers are changed at " + f"{pointer} on a value this reader cannot identify; the agents " + f"constructed in {source_ref} are not established.", + pointer, + "sdk_capability_change_unattributed", + ) + continue + reason = ( + f"OpenAI Agents SDK agent {owner!r} has its tools, handoffs or MCP " + f"servers changed after construction at {pointer}, which this reader " + "does not follow." + ) + warnings.append(reason) + # Changed in place (not re-assigned): every agent built from the same + # list object holds the change too (#876 review). + reached = {owner} + if not _reassigns(site): + for holders in list_holders.values(): + if owner in holders: + reached |= holders + for observation in file_observations: + if observation.agent in reached: + observation.tools_complete = False + observation.handoffs_complete = False + if reason not in observation.issues: + observation.issues.append(reason) + # One identity constructed twice in a file — ``if premium: return + # Agent(name="Quote", ...)`` / ``else: return Agent(name="Quote", ...)`` + # — is merged by the binding graph into one agent. Its tools cannot be + # attributed to either construction, so the agent is incomplete rather + # than silently the union of both (#876 review). Constructions that + # bind exactly the same tools and handoffs are the same agent either way. + sites: dict[str, list[str]] = {} + shapes: dict[str, set[tuple[object, ...]]] = {} + for observation in file_observations: + sites.setdefault(observation.agent, []).append(observation.source_pointer or source_ref) + shapes.setdefault(observation.agent, set()).add( + ( + tuple(observation.tool_names), + tuple(sorted(observation.tool_locators.items())), + tuple(observation.handoff_names), + observation.tools_complete, + observation.handoffs_complete, + tuple(observation.issues), + ) + ) + for identity, pointers in sites.items(): + if len(pointers) < 2: + continue + if len(shapes[identity]) == 1 and not next(iter(shapes[identity]))[-1]: + # The same agent either way: one observation, not two that the + # comparison would read as an ambiguous identity. + first = next(o for o in file_observations if o.agent == identity) + file_observations = [ + o for o in file_observations if o.agent != identity or o is first + ] + continue + reason = ( + f"OpenAI Agents SDK agent {identity!r} is constructed more than once in " + f"{source_ref} ({', '.join(p.rsplit(':', 1)[-1] for p in pointers)}); " + "its tools are not attributed to either construction." + ) + warnings.append(reason) + for observation in file_observations: + if observation.agent == identity: + observation.tools_complete = False + observation.handoffs_complete = False + if reason not in observation.issues: + observation.issues.append(reason) + observations.extend(file_observations) return ( list(dict.fromkeys(warnings)), observations, @@ -524,6 +752,592 @@ def _literal_tool_list_concatenation(value: ast.AST | None) -> bool: ) +def _denotes_agent(sdk_names: _SdkNames, node: ast.AST) -> bool: + """Whether ``node`` — a call, or a class base — names the SDK's ``Agent``. + + ``Agent[Context](...)`` parameterizes the class before calling it; the + subscript does not change which class is constructed. + """ + func = node.func if isinstance(node, ast.Call) else node + if isinstance(func, ast.Subscript): + func = func.value + return sdk_names.denotes(dotted_name(func), node, "Agent", DEFAULT_AGENT_CONSTRUCTORS) + + +def _literal_agent_name(call: ast.Call) -> str | None: + """The ``name`` an unassigned ``Agent(...)`` is constructed with.""" + + value = _keyword(call, "name") + if value is None and call.args: + value = call.args[0] + return _const_str(value) if isinstance(value, ast.expr) else None + + +#: Keywords that give an agent copy capabilities of its own. +_CAPABILITY_KEYWORDS = frozenset({"tools", "handoffs", "mcp_servers"}) +#: Methods that change a list in place. +_LIST_MUTATORS = frozenset({"append", "extend", "insert", "remove", "pop", "clear"}) + + +#: Spellings of ``replace`` that copy a dataclass with changed fields. +_REPLACE_FUNCTIONS = frozenset({"replace", "dataclasses.replace", "copy.replace"}) + + +def _copy_shape(call: ast.Call) -> tuple[ast.expr, str, bool] | None: + """``(receiver, kind, explicit)`` for ``x.clone(...)`` / ``replace(x, ...)`` passing capabilities. + + ``explicit``: a ``tools=`` / ``handoffs=`` / ``mcp_servers=`` keyword, as + opposed to only ``**`` unpacking, which could carry anything or nothing. + """ + + keywords = {keyword.arg for keyword in call.keywords} + explicit = bool(keywords & _CAPABILITY_KEYWORDS) + if not explicit and None not in keywords: + return None + func = call.func + if isinstance(func, ast.Attribute) and func.attr == "clone": + return func.value, "clone", explicit + name = dotted_name(func) + if name in _REPLACE_FUNCTIONS and call.args: + return call.args[0], "copy.replace" if name == "copy.replace" else "dataclasses.replace", explicit + return None + + +def _capability_rebinding_copy( + call: ast.Call, is_agent: Callable[[ast.expr], bool | None] | None = None +) -> str | None: + """``x.clone(tools=...)`` or ``replace(x, tools=...)``: a copy with its own capabilities. + + A copy that passes none of them keeps the original's tools, which the + original's own rows already compare, so it is not a limit (#876 review). + ``is_agent`` answers for the receiver: a value proven not to be an agent + (``Settings()``) is never a copy, and one that passes only ``**`` is a copy + only when the receiver is proven to be one. + """ + + shape = _copy_shape(call) + if shape is None: + return None + receiver, kind, explicit = shape + verdict = is_agent(receiver) if is_agent is not None else None + if verdict is False or (not explicit and verdict is not True): + return None + return kind + + +def _capability_changes( + tree: ast.Module, + capabilities: frozenset[str] = _CAPABILITY_KEYWORDS, + *, + scopes: ScopeIndex | None = None, +) -> list[tuple[ast.expr, ast.stmt | ast.Call]]: + """``(receiver, site)`` wherever a module changes an object's capability list. + + Assigning, extending or deleting ``x.tools`` (or its items), calling a list + method on it, ``setattr`` / ``delattr`` by name, and a handle on it + (``t = x.tools``, ``t = getattr(x, "tools")``) that is itself changed + later in its scope. A plain read — ``len(request.tools)``, + ``tools = request.tools`` that is only read — changes nothing (#876 review). + """ + + scopes = scopes or ScopeIndex(tree) + + def changed_handle(statement: ast.stmt) -> bool: + target = _assignment_target(statement) if isinstance(statement, ast.Assign | ast.AnnAssign) else None + if target is None: + return True + scope = _enclosing_function(scopes, statement) or tree + defining = statement.targets[0] if isinstance(statement, ast.Assign) else statement.target + # The handle's own assignment aside, every use of it must be a read. + for node in ast.walk(scope): + if not isinstance(node, ast.Name) or node.id != target or node is defining: + continue + if not isinstance(node.ctx, ast.Load) or not _read_only_use( + node, scopes.parents, lambda call, *_: _leaves_arguments_alone(call) + ): + return True + return False + + def reflective(call: ast.Call, names: frozenset[str]) -> bool: + return ( + isinstance(call.func, ast.Name) + and call.func.id in names + and len(call.args) >= 2 + and isinstance(call.args[1], ast.Constant) + and call.args[1].value in capabilities + ) + + changes: list[tuple[ast.expr, ast.stmt | ast.Call]] = [] + for node in ast.walk(tree): + if isinstance(node, ast.Assign | ast.AugAssign | ast.AnnAssign | ast.Delete): + targets = node.targets if isinstance(node, ast.Assign | ast.Delete) else [node.target] + for target in targets: + if isinstance(target, ast.Subscript): + target = target.value + if isinstance(target, ast.Attribute) and target.attr in capabilities: + changes.append((target.value, node)) + value = node.value if isinstance(node, ast.Assign | ast.AnnAssign) else None + if isinstance(value, ast.Attribute) and value.attr in capabilities: + if changed_handle(node): + changes.append((value.value, node)) + elif isinstance(value, ast.Call) and reflective(value, frozenset({"getattr"})): + if changed_handle(node): + changes.append((value.args[0], node)) + elif isinstance(node, ast.Call): + func = node.func + if ( + isinstance(func, ast.Attribute) + and func.attr in _LIST_MUTATORS + and isinstance(func.value, ast.Attribute) + and func.value.attr in capabilities + ): + changes.append((func.value.value, node)) + elif reflective(node, frozenset({"setattr", "delattr"})): + changes.append((node.args[0], node)) + return changes + + +#: Constructors whose result is never an agent. +_NON_AGENT_CONSTRUCTORS = frozenset( + { + "dict", + "list", + "set", + "tuple", + "object", + "SimpleNamespace", + "types.SimpleNamespace", + "defaultdict", + "collections.defaultdict", + "OrderedDict", + "collections.OrderedDict", + } +) +_NON_AGENT_LITERALS = ( + ast.Constant, + ast.List, + ast.Tuple, + ast.Set, + ast.Dict, + ast.JoinedStr, + ast.Lambda, + ast.ListComp, + ast.SetComp, + ast.DictComp, + ast.GeneratorExp, +) + + +class _AgentValues: + """What a name holds where it is used, as far as agents go (#876 review).""" + + def __init__( + self, + tree: ast.Module, + scopes: ScopeIndex, + module_bindings: dict[str, list[Any]], + sdk_names: _SdkNames, + subclasses: dict[str, int], + ) -> None: + self.scopes = scopes + self.module_bindings = module_bindings + self.sdk_names = sdk_names + self.subclasses = subclasses + #: ``id(call) -> identity`` of every agent construction the reader read. + self.constructed: dict[int, str] = {} + self._class_attributes: dict[int, dict[str, list[ast.expr]]] = {} + self.classes = {node.name for node in tree.body if isinstance(node, ast.ClassDef)} + self.functions = { + node.name: node + for node in tree.body + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef) + } + + def value_of(self, name: str, site: ast.AST) -> ast.expr | None: + """The value ``name`` is bound to once, seen from ``site``; else None.""" + + found = self.scopes.enclosing_bindings(site, name) + if found: + if len(found) != 1 or not isinstance(found[0], ast.Name): + return None + statement = self.scopes.statement_of(found[0]) + else: + bindings = self.module_bindings.get(name, []) + if ( + len(bindings) != 1 + or not bindings[0].top_level + or not isinstance(bindings[0].node, ast.Name) + ): + return None + statement = bindings[0].statement + if isinstance(statement, ast.Assign | ast.AnnAssign) and _assignment_target(statement) == name: + return statement.value + return None + + def is_agent(self, expr: ast.expr, site: ast.AST) -> bool | None: + """True: an agent; False: provably not one; None: unknown.""" + + if isinstance(expr, ast.Name): + value = self.value_of(expr.id, site) + if value is None: + return None + expr = value + if isinstance(expr, _NON_AGENT_LITERALS): + return False + if isinstance(expr, ast.Call): + if id(expr) in self.constructed or _denotes_agent(self.sdk_names, expr): + return True + if self.returned_agent(expr) is not None: + return True + if self._not_an_agent(expr): + return False + return None + + def _not_an_agent(self, call: ast.Call) -> bool: + if isinstance(call.func, ast.Call): + # ``type("P", (), {})()``: a class made on the spot, from no base + # that could be an agent. Any other call of a call is unknown. + maker = call.func + return ( + dotted_name(maker.func) == "type" + and len(maker.args) >= 2 + and isinstance(maker.args[1], ast.Tuple) + and not maker.args[1].elts + ) + name = dotted_name(call.func) + if name in _NON_AGENT_CONSTRUCTORS: + return True + return name in self.classes and name not in self.subclasses + + def returned_agent(self, call: ast.Call) -> str | None: + """The one observed agent a module-level function's every ``return`` gives back.""" + + name = dotted_name(call.func) + function = self.functions.get(name) if name else None + if function is None: + return None + identities: set[str] = set() + stack: list[ast.AST] = list(function.body) + while stack: + node = stack.pop() + if isinstance(node, _SCOPE_NODES): + continue + if isinstance(node, ast.Return): + value = node.value + if isinstance(value, ast.Name): + value = self.value_of(value.id, node) + if not isinstance(value, ast.Call) or id(value) not in self.constructed: + return None + identities.add(self.constructed[id(value)]) + stack.extend(ast.iter_child_nodes(node)) + return identities.pop() if len(identities) == 1 else None + + def owner(self, receiver: ast.expr, site: ast.AST) -> str | bool | None: + """The agent a capability change reaches: its identity, None, or True (unknown).""" + + if isinstance(receiver, ast.Name) and receiver.id in {"self", "cls"}: + # The instance's own list: an agent only when the class is an + # agent subclass, whose instances are already a named limit. + return None + if ( + isinstance(receiver, ast.Attribute) + and isinstance(receiver.value, ast.Name) + and receiver.value.id in {"self", "cls"} + ): + assigned = self._self_attribute_values(receiver.attr, site) + identities = {self.constructed.get(id(value)) for value in assigned} + if assigned and None not in identities and len(identities) == 1: + return identities.pop() + return True + if isinstance(receiver, ast.Name): + value = self.value_of(receiver.id, site) + if value is None: + return True + if isinstance(value, ast.Call): + if id(value) in self.constructed: + return self.constructed[id(value)] + returned = self.returned_agent(value) + if returned is not None: + return returned + return None if self.is_agent(value, site) is False else True + return True + + def _self_attribute_values(self, attr: str, site: ast.AST) -> list[ast.expr]: + current = self.scopes.parents.get(site) + while current is not None and not isinstance(current, ast.ClassDef): + current = self.scopes.parents.get(current) + if current is None: + return [] + values = self._class_attributes.get(id(current)) + if values is None: + # One walk per class, not one per change site (#876 review). + values = {} + for node in ast.walk(current): + if isinstance(node, ast.Assign | ast.AnnAssign) and node.value is not None: + targets = node.targets if isinstance(node, ast.Assign) else [node.target] + for target in targets: + if ( + isinstance(target, ast.Attribute) + and isinstance(target.value, ast.Name) + and target.value.id in {"self", "cls"} + ): + values.setdefault(target.attr, []).append(node.value) + self._class_attributes[id(current)] = values + return values.get(attr, []) + + +#: Capability attributes of either supported framework's agents. +_CENSUS_CAPABILITIES = frozenset({"tools", "handoffs", "mcp_servers", "sub_agents"}) + + +@dataclass(frozen=True) +class ModuleCensus: + """What a module no reader reads as an SDK source could do to an agent.""" + + #: Lines of ``x.clone(tools=...)`` / ``replace(x, tools=...)`` copies. + copies: list[int] + #: ``(line, constructor)`` where an object this module can reach from the + #: scope has its capability lists changed. ``constructor`` is the + #: ``(module tail, class)`` a receiver was built from by an import, so the + #: comparison can drop one that is plainly not an agent. + changes: list[tuple[int, tuple[str, str] | None]] + #: SDK ``Agent`` subclasses the module defines, by name, with their line. + subclasses: dict[str, int] + #: Every identifier the module's code spells (names, attributes, imports), + #: so a use of a subclass is found in code, never in a string or comment. + identifiers: frozenset[str] = frozenset() + #: ``(module tail, name)`` for each ``from … import name``; ``*`` for a + #: wildcard. With ``modules``, how a use names another module's class. + imported: frozenset[tuple[str, str]] = frozenset() + #: Tails of the modules imported whole (``import app.core``, ``from app import core``). + modules: frozenset[str] = frozenset() + #: Every class the module defines at top level. + classes: frozenset[str] = frozenset() + + def uses(self, name: str, defining: str) -> bool: + """Whether this module's code uses ``name`` from the module at ``defining``.""" + + tail = _module_tail(defining) + if (tail, name) in self.imported: + return True + return name in self.identifiers and (tail in self.modules or (tail, "*") in self.imported) + + def reexports(self, name: str, defining: str) -> bool: + """Whether ``name`` from ``defining`` is in this module's namespace for others.""" + + tail = _module_tail(defining) + return (tail, name) in self.imported or (tail, "*") in self.imported + + +def _module_tail(path: str) -> str: + posix = PurePosixPath(path) + return posix.parent.name if posix.name == "__init__.py" else posix.stem + + +def census_module( + tree: ast.Module, + text: str, + local_modules: frozenset[str] = frozenset(), + *, + read_as_sdk: bool = False, + read_as_adk: bool = False, +) -> ModuleCensus: + """The capability-changing constructs of one module, for the comparison (#876 review). + + A copy of a value not proven to be something else, a change to the + capability list of an object this module imports from the scope + (``agent.quote_agent.tools.append(...)``), and SDK ``Agent`` subclasses. + A change on a local object or a parameter of a module that never imports + the SDK is left out: it is almost always another library's ``.tools``. A + module read as an SDK source is read for its copies and changes by the + reader itself, so only its subclasses and identifiers are collected here. + + One walk collects what every module needs; the rest runs only where that + walk found something to read. + """ + + names: set[str] = set() + imported: set[tuple[str, str]] = set() + modules: set[str] = set() + imports_scope = False + touches_capabilities = False + for node in ast.walk(tree): + if isinstance(node, ast.Name): + names.add(node.id) + elif isinstance(node, ast.Attribute): + names.add(node.attr) + touches_capabilities = touches_capabilities or node.attr in _CENSUS_CAPABILITIES + elif isinstance(node, ast.ImportFrom): + # Only an import from the scope can name the scope's classes: a + # vendor module of the same stem is not ``app/core.py``. + local = bool(node.level) or (node.module or "").split(".", 1)[0] in local_modules + imports_scope = imports_scope or local + tail = (node.module or "").rsplit(".", 1)[-1] + for alias in node.names: + names.update(part for part in (alias.name, alias.asname) if part) + if local: + if tail: + imported.add((tail, alias.name)) + modules.add(alias.name) + elif isinstance(node, ast.Import): + for alias in node.names: + names.update(part for part in (alias.name.rsplit(".", 1)[-1], alias.asname) if part) + if alias.name.split(".", 1)[0] in local_modules: + imports_scope = True + modules.add(alias.name.rsplit(".", 1)[-1]) + elif ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id in {"setattr", "delattr", "getattr"} + ): + touches_capabilities = True + identifiers_of = { + "identifiers": frozenset(names), + "imported": frozenset(imported), + "modules": frozenset(modules), + "classes": frozenset(node.name for node in tree.body if isinstance(node, ast.ClassDef)), + } + subclasses: dict[str, int] = {} + sdk_names: _SdkNames | None = None + if "Agent" in text: + sdk_names = _SdkNames(tree) + subclasses = _agent_subclasses(tree, sdk_names) + wants_copies = not read_as_sdk and ("clone(" in text or "replace(" in text) + # A module that imports nothing from the scope cannot reach its agents; its + # ``.tools`` are another library's. One that does can reach them through + # any value — an alias, a loop, a parameter, a call's result — so only a + # value proven not to be an agent is left out (#876 review). + # A Google ADK source constructs its own agents, and its reader does not + # follow a change to them after construction, so it is read like one. + wants_changes = ( + not read_as_sdk and touches_capabilities and (imports_scope or read_as_adk) + ) + if not wants_copies and not wants_changes: + return ModuleCensus([], [], subclasses, **identifiers_of) + scopes = ScopeIndex(tree) + bindings = _module_bindings(tree)[0] + copies: list[int] = [] + if wants_copies: + values = _AgentValues(tree, scopes, bindings, sdk_names or _SdkNames(tree), subclasses) + copies = sorted( + node.lineno + for node in ast.walk(tree) + if isinstance(node, ast.Call) + and _capability_rebinding_copy( + node, lambda receiver, node=node: values.is_agent(receiver, node) + ) + is not None + ) + + changes: list[tuple[int, tuple[str, str] | None]] = [] + if wants_changes: + values = _AgentValues(tree, scopes, bindings, sdk_names or _SdkNames(tree), subclasses) + + def constructor(receiver: ast.expr, site: ast.AST) -> tuple[str, str] | None: + value = ( + values.value_of(receiver.id, site) if isinstance(receiver, ast.Name) else None + ) + if not isinstance(value, ast.Call) or not isinstance(value.func, ast.Name): + return None + for item in bindings.get(value.func.id, []): + statement = item.statement + if isinstance(statement, ast.ImportFrom) and isinstance(item.node, ast.alias): + return ((statement.module or "").rsplit(".", 1)[-1], item.node.name) + return None + + changes = sorted( + { + (site.lineno, constructor(receiver, site)) + for receiver, site in _capability_changes(tree, _CENSUS_CAPABILITIES, scopes=scopes) + if values.owner(receiver, site) is not None + }, + key=lambda item: item[0], + ) + return ModuleCensus(copies, changes, subclasses, **identifiers_of) + + +def _reassigns(site: ast.AST) -> bool: + """Whether a capability change gives the agent a new list rather than changing its own.""" + + if isinstance(site, ast.Assign | ast.AnnAssign): + targets = site.targets if isinstance(site, ast.Assign) else [site.target] + return all(isinstance(target, ast.Attribute) for target in targets) + return isinstance(site, ast.Call) and isinstance(site.func, ast.Name) and site.func.id in { + "setattr", + "delattr", + } + + +def _enclosing_scope(scopes: ScopeIndex, node: ast.AST) -> ast.AST | None: + """The function or class body ``node`` is in; None at module level.""" + + current = scopes.parents.get(node) + while current is not None and not isinstance(current, ast.Module): + if isinstance(current, ast.FunctionDef | ast.AsyncFunctionDef | ast.Lambda | ast.ClassDef): + return current + current = scopes.parents.get(current) + return None + + +def _enclosing_function(scopes: ScopeIndex, node: ast.AST) -> ast.AST | None: + current = scopes.parents.get(node) + while current is not None and not isinstance(current, ast.Module | ast.ClassDef): + if isinstance(current, ast.FunctionDef | ast.AsyncFunctionDef | ast.Lambda): + return current + current = scopes.parents.get(current) + return None + + +def _agent_subclasses(tree: ast.Module, sdk_names: _SdkNames) -> dict[str, int]: + """Classes in this module that subclass the SDK's ``Agent``, transitively. + + ``Dyn = type("Dyn", (Agent,), {})`` makes one too (#876 review). + """ + + classes: list[tuple[str, list[ast.expr], int]] = [ + (node.name, list(node.bases), node.lineno) + for node in ast.walk(tree) + if isinstance(node, ast.ClassDef) + ] + for node in ast.walk(tree): + if ( + isinstance(node, ast.Assign) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and isinstance(node.value, ast.Call) + and dotted_name(node.value.func) == "type" + and len(node.value.args) >= 2 + and isinstance(node.value.args[1], ast.Tuple) + ): + classes.append((node.targets[0].id, list(node.value.args[1].elts), node.lineno)) + found: dict[str, int] = {} + changed = True + while changed: + changed = False + for name, bases, line in classes: + if name in found: + continue + for base in bases: + inner = base.value if isinstance(base, ast.Subscript) else base + if _denotes_agent(sdk_names, base) or ( + isinstance(inner, ast.Name) and inner.id in found + ): + found[name] = line + changed = True + break + return found + + +def _opaque_arguments(call: ast.Call) -> str | None: + """Arguments that can carry ``tools`` or ``handoffs`` the reader cannot see.""" + + if any(keyword.arg is None for keyword in call.keywords): + return "keyword unpacking (**)" + if len(call.args) > 1 or any(isinstance(arg, ast.Starred) for arg in call.args): + return "positional arguments after its name" + return None + + def _assignment_target(node: ast.Assign | ast.AnnAssign) -> str | None: targets = node.targets if isinstance(node, ast.Assign) else [node.target] return targets[0].id if len(targets) == 1 and isinstance(targets[0], ast.Name) else None @@ -533,8 +1347,6 @@ def _keyword(call: ast.Call, name: str) -> ast.AST | None: return next((item.value for item in call.keywords if item.arg == name), None) -#: Methods that change a list in place. -_LIST_MUTATORS = frozenset({"append", "extend", "insert", "remove", "pop", "clear"}) #: Calls that read the values they are given and never change them. _READ_ONLY_CALLS = frozenset( { @@ -874,14 +1686,41 @@ def references( return references def names( - self, value: ast.AST | None, node: ast.AST, aliases: dict[str, str] + self, + value: ast.AST | None, + node: ast.AST, + aliases: dict[str, str], + identity_of: Callable[[ast.Call], str | None] | None = None, + sdk_names: _SdkNames | None = None, ) -> list[str] | None: - """Handoff names: plain names only, read through ``from`` import aliases.""" + """Handoff names: plain names only, read through ``from`` import aliases. + + A name a function binds to an agent it constructs is that agent's + identity, which is its literal ``name`` (#876 review). + """ elements = self._elements(value, node) if elements is None or not all(isinstance(item, ast.Name) for item in elements): return None - return [aliases.get(item.id, item.id) for item in elements if isinstance(item, ast.Name)] + names: list[str] = [] + for item in elements: + assert isinstance(item, ast.Name) + found = self.scopes.enclosing_bindings(item, item.id) + statement = self.scopes.statement_of(found[0]) if len(found) == 1 else None + value_node = getattr(statement, "value", None) + if ( + identity_of is not None + and sdk_names is not None + and isinstance(found[0] if found else None, ast.Name) + and isinstance(value_node, ast.Call) + and _denotes_agent(sdk_names, value_node) + ): + identity = identity_of(value_node) + if identity is not None: + names.append(identity) + continue + names.append(aliases.get(item.id, item.id)) + return names _SCOPE_NODES = ( diff --git a/tests/test_application_diff_identity.py b/tests/test_application_diff_identity.py index 20a3d35b..dab3bd5e 100644 --- a/tests/test_application_diff_identity.py +++ b/tests/test_application_diff_identity.py @@ -67,7 +67,6 @@ def __init__(self, tools): # (the name it is assigned to) while hiding its tool list from the reader. UNREAD_CONSTRUCTIONS = { "subclass": SDK_SUBCLASS, - "generic": SDK.replace("Agent(name=", "Agent[dict](name="), "factory": SDK.replace( 'agent = Agent(name="assistant", tools=TOOLS)', 'def build(tools):\n return Agent(name="assistant", tools=tools)\nagent = build(TOOLS)', @@ -178,10 +177,18 @@ def test_import_bound_agent_name_is_not_a_removal(repo, spelling): ) result = run(repo, base, head) assert result["comparison_status"] == "partial" + # The factory's own construction is read since #876, under its literal + # name in the module that builds it. That is a head-side construction + # site, not evidence about `agent`: the absence of `agent` in agent.py + # stays not established. assert [ (r["agent"], r["tool"], r["change"], r["candidate_change"]) for r in result["rows"] - ] == [("agent", "lookup", "not_established", "removed")] + ] == [ + ("agent", "lookup", "not_established", "removed"), + ("assistant", "lookup", "added", None), + ] assert list(result["rows"][0]["uncertainty"]) == ["head"] + assert result["rows"][1]["after"]["agent_source"] == "agent_factory.py" @pytest.mark.parametrize("construction", sorted(UNREAD_CONSTRUCTIONS)) @@ -204,6 +211,18 @@ def test_unread_head_construction_is_not_a_removal(repo, construction): ) +def test_generic_construction_is_read(repo): + # `Agent[Context](...)` constructs the SDK's Agent; it was unread before #876. + source = SDK.replace("Agent(name=", "Agent[dict](name=") + base = commit(repo, {"agent.py": source.replace("TOOLS", "[lookup]")}) + head = commit(repo, {"agent.py": source.replace("TOOLS", "[lookup, execute]")}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared" + assert [(r["agent"], r["tool"], r["change"]) for r in result["rows"]] == [ + ("agent", "execute", "added") + ] + + def test_handoff_reference_is_not_an_observed_construction(repo): # `handoffs=[worker]` makes `worker` a graph agent without reading its own # construction, so it cannot stand in for the worker's tool list. diff --git a/tests/test_application_diff_unobserved.py b/tests/test_application_diff_unobserved.py new file mode 100644 index 00000000..8c3ff12e --- /dev/null +++ b/tests/test_application_diff_unobserved.py @@ -0,0 +1,927 @@ +"""#876: an agent the reader cannot see must never let a comparison read as complete. + +Each case here was a silent miss before the fix: the SDK reader only saw +``name = Agent(...)``, so an agent built any other way produced no observation +and no limit, and a test double's agent was enough to establish the scope. +""" + +import ast + +import pytest +from test_application_diff import commit, run +from test_application_diff import repo as repo + +TOOLS = '''from agents import function_tool +@function_tool +def quote(item: str) -> str: + return item +@function_tool +def send_image(name: str) -> dict: + return {"name": name} +''' + + +def _agents(body: str) -> str: + return TOOLS + "from agents import Agent\n" + body + + +def _pairs(result): + return [(r["agent"], r["tool"], r["change"]) for r in result["rows"]] + + +@pytest.mark.parametrize( + ("body", "agent"), + [ + ( + 'def build():\n return Agent(name="Quote", instructions="q", tools=TOOLS)\n', + "Quote", + ), + ( + 'class Holder:\n def __init__(self):\n' + ' self.agent = Agent(name="Held", instructions="h", tools=TOOLS)\n', + "Held", + ), + ('AGENTS = [Agent(name="Listed", instructions="l", tools=TOOLS)]\n', "Listed"), + ('typed = Agent[dict](name="Typed", instructions="t", tools=TOOLS)\n', "typed"), + ('def build():\n return Agent("Positional", tools=TOOLS)\n', "Positional"), + ], +) +def test_every_agent_construction_is_observed(repo, body, agent): + base = commit(repo, {"agent.py": _agents(body.replace("TOOLS", "[quote]"))}) + head = commit(repo, {"agent.py": _agents(body.replace("TOOLS", "[quote, send_image]"))}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [(agent, "send_image", "added")] + + +@pytest.mark.parametrize( + ("body", "reason"), + [ + ( + 'def build(name):\n return Agent(name=name, instructions="q", tools=[quote])\n', + "has no literal name", + ), + ( + 'class Custom(Agent):\n pass\nhelper = Custom(name="c", tools=[quote])\n', + "built from the subclass 'Custom'", + ), + ( + 'base = Agent(name="base", tools=[quote])\n' + 'copy = base.clone(tools=[quote, send_image])\n', + "agent copy at agent.py:", + ), + ( + "from shared import base_agent\n" + "copy = base_agent.clone(tools=[quote, send_image])\n", + "agent copy at agent.py:", + ), + ], +) +def test_an_agent_the_reader_cannot_identify_is_a_named_limit(repo, body, reason): + base = commit(repo, {"agent.py": _agents(body)}) + head = commit(repo, {"agent.py": _agents(body) + "# touched\n"}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any(reason in limit for limit in result["head"]["limits"]) + assert all(g["source"] == "agent.py" for g in result["head"]["coverage_gaps"]) + + +def test_a_test_double_does_not_establish_the_application(repo): + """The kkmiecik-coder/CRM#5 shape: the real agent is built by a function.""" + + real = 'def build():\n return Agent(name="Wycena", instructions="w", tools=TOOLS)\n' + double = ( + "from agents import Agent, function_tool\n@function_tool\n" + "def fake(q: str) -> str:\n return q\n" + 'agent = Agent(name="double", tools=[fake])\n' + ) + base = commit( + repo, + { + "bots/agents.py": _agents(real.replace("TOOLS", "[quote]")), + "bots/tests/test_turn.py": double, + }, + ) + head = commit(repo, {"bots/agents.py": _agents(real.replace("TOOLS", "[quote, send_image]"))}) + result = run(repo, base, head) + assert _pairs(result) == [("Wycena", "send_image", "added")] + for side in ("base", "head"): + assert [a["name"] for a in result[side]["agents"]] == ["Wycena"] + assert result[side]["excluded_tests"] == ["bots/tests/test_turn.py"] + + +def test_a_test_file_with_a_duplicate_tool_does_not_refuse_the_comparison(repo): + duplicated = ( + "from agents import Agent, function_tool\n" + "@function_tool\ndef _tool(q: str) -> str:\n return q\n" + "@function_tool\ndef _tool(q: str) -> str:\n return q + q\n" + 'agent = Agent(name="x", tools=[_tool])\n' + ) + body = 'assistant = Agent(name="assistant", tools=TOOLS)\n' + base = commit( + repo, + { + "agent.py": _agents(body.replace("TOOLS", "[quote]")), + "tests/unit/test_executor.py": duplicated, + }, + ) + head = commit(repo, {"agent.py": _agents(body.replace("TOOLS", "[quote, send_image]"))}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("assistant", "send_image", "added")] + + +def test_a_duplicate_tool_in_application_code_limits_only_that_file(repo): + duplicated = ( + "from agents import Agent, function_tool\n" + "@function_tool\ndef dup(q: str) -> str:\n return q\n" + "@function_tool\ndef dup(q: str) -> str:\n return q + q\n" + 'other = Agent(name="other", tools=[dup])\n' + ) + body = 'assistant = Agent(name="assistant", tools=TOOLS)\n' + base = commit( + repo, {"agent.py": _agents(body.replace("TOOLS", "[quote]")), "other.py": duplicated} + ) + head = commit(repo, {"agent.py": _agents(body.replace("TOOLS", "[quote, send_image]"))}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert _pairs(result) == [("assistant", "send_image", "added")] + assert any( + g["source"] == "other.py" and "defines the tool 'dup' more than once" in g["reason"] + for g in result["head"]["coverage_gaps"] + ) + + +def test_a_livekit_agent_subclass_is_not_the_sdk(repo): + livekit = ( + "from livekit.agents import Agent\n" + "class Focus(Agent):\n pass\n" + ) + base = commit(repo, {"agent.py": livekit}) + head = commit(repo, {"agent.py": livekit + "# touched\n"}) + result = run(repo, base, head) + assert not any("subclass" in limit for limit in result["head"]["limits"]) + + +# The invariant behind #876, checked over every fixture shape above: a result +# may say ``compared`` only when every ``Agent(...)`` construction in the +# scope's non-test SDK files is either an observed agent or a named limit. +_SHAPES = [ + 'def build():\n return Agent(name="Quote", tools=[quote])\n', + 'class H:\n def __init__(self):\n self.a = Agent(name="Held", tools=[quote])\n', + 'AGENTS = [Agent(name="Listed", tools=[quote])]\n', + 'typed = Agent[dict](name="Typed", tools=[quote])\n', + 'def build(n):\n return Agent(name=n, tools=[quote])\n', + 'assistant = Agent(name="assistant", tools=[quote])\n', +] + + +@pytest.mark.parametrize("body", _SHAPES) +def test_compared_never_leaves_a_construction_site_unaccounted(repo, body): + source = _agents(body) + base = commit(repo, {"agent.py": source}) + head = commit(repo, {"agent.py": source + "# touched\n"}) + result = run(repo, base, head) + sites = [ + node.lineno + for node in ast.walk(ast.parse(source)) + if isinstance(node, ast.Call) + and ( + (isinstance(node.func, ast.Name) and node.func.id == "Agent") + or ( + isinstance(node.func, ast.Subscript) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "Agent" + ) + ) + ] + observed = { + int(a["location"].rsplit(":", 1)[1]) + for a in result["head"]["agents"] + if a["location"] and ":" in a["location"] + } + limited = { + line + for line in sites + for limit in result["head"]["limits"] + if f"agent.py:{line}" in limit + } + unaccounted = [line for line in sites if line not in observed | limited] + if result["comparison_status"] == "compared": + assert unaccounted == [] + else: + assert unaccounted == [] or result["head"]["limits"] + + +# --------------------------------------------------------------------------- +# #876 review: paths that still read `compared` with no rows, or limits that +# downgraded rows they had nothing to do with. + + +def _compare(repo, before: str, after: str, path: str = "agent.py"): + base = commit(repo, {path: _agents(before)}) + head = commit(repo, {path: _agents(after)}) + return run(repo, base, head) + + +def test_one_identity_constructed_twice_is_not_merged(repo): + body = ( + "def build(premium):\n" + " if premium:\n" + ' return Agent(name="Quote", tools=PREMIUM)\n' + ' return Agent(name="Quote", tools=FREE)\n' + ) + result = _compare( + repo, + body.replace("PREMIUM", "[send_image]").replace("FREE", "[quote]"), + body.replace("PREMIUM", "[quote]").replace("FREE", "[send_image]"), + ) + assert result["comparison_status"] == "partial" + assert any("constructed more than once" in limit for limit in result["head"]["limits"]) + + +def test_a_literal_name_equal_to_a_variable_name_is_not_merged(repo): + body = ( + 'assistant = Agent(name="Helper", tools=HELPER)\n' + 'def build():\n return Agent(name="assistant", tools=OTHER)\n' + ) + result = _compare( + repo, + body.replace("HELPER", "[send_image]").replace("OTHER", "[quote]"), + body.replace("HELPER", "[quote]").replace("OTHER", "[quote, send_image]"), + ) + assert result["comparison_status"] == "partial" + assert all(row["change"] == "not_established" for row in result["rows"]) + + +@pytest.mark.parametrize( + "construction", + [ + 'COMMON = {"tools": TOOLS}\ndef build():\n return Agent(name="Quote", **COMMON)\n', + 'COMMON = {"tools": TOOLS}\nquote_agent = Agent(name="Quote", **COMMON)\n', + 'def build():\n return Agent("Quote", "desc", TOOLS)\n', + ], +) +def test_opaque_arguments_are_a_named_limit(repo, construction): + result = _compare( + repo, + construction.replace("TOOLS", "[quote]"), + construction.replace("TOOLS", "[quote, send_image]"), + ) + assert result["comparison_status"] == "partial" + assert any("so its tools and handoffs are not read" in limit for limit in result["head"]["limits"]) + + +@pytest.mark.parametrize( + ("before", "after"), + [ + ( + 'QUOTE = [quote]\nquote_agent = Agent(name="Quote", tools=QUOTE)\n', + 'QUOTE = [quote]\nQUOTE.append(send_image)\nquote_agent = Agent(name="Quote", tools=QUOTE)\n', + ), + ( + 'QUOTE = [quote]\ndef build():\n return Agent(name="Quote", tools=QUOTE)\n', + 'QUOTE = [quote]\nQUOTE += [send_image]\ndef build():\n return Agent(name="Quote", tools=QUOTE)\n', + ), + ( + 'import os\nQUOTE = [quote]\ndef build():\n return Agent(name="Quote", tools=QUOTE)\n', + 'import os\nif os.environ.get("X"):\n QUOTE = [quote, send_image]\nelse:\n QUOTE = [quote]\n' + 'def build():\n return Agent(name="Quote", tools=QUOTE)\n', + ), + ], +) +def test_a_list_changed_or_bound_twice_is_dynamic(repo, before, after): + result = _compare(repo, before, after) + assert result["comparison_status"] == "partial" + assert result["rows"] == [] or all(row["change"] == "not_established" for row in result["rows"]) + + +def test_each_builder_reads_its_own_local_list(repo): + body = ( + 'def build_a():\n tools = [quote]\n return Agent(name="A", tools=tools)\n' + 'def build_b():\n tools = B_TOOLS\n return Agent(name="B", tools=tools)\n' + ) + result = _compare( + repo, + body.replace("B_TOOLS", "[quote]"), + body.replace("B_TOOLS", "[quote, send_image]"), + ) + assert _pairs(result) == [("B", "send_image", "added")] + + +@pytest.mark.parametrize( + "mutation", + [ + "quote_agent.tools.append(send_image)\n", + "quote_agent.tools = [quote, send_image]\n", + ], +) +def test_tools_changed_after_construction_is_a_named_limit(repo, mutation): + body = 'quote_agent = Agent(name="Quote", tools=[quote])\n' + result = _compare(repo, body, body + mutation) + assert result["comparison_status"] == "partial" + assert any("changed after construction" in limit for limit in result["head"]["limits"]) + + +def test_dataclasses_replace_with_tools_is_a_named_limit(repo): + body = 'import dataclasses\nquote_agent = Agent(name="Quote", tools=[quote])\n' + result = _compare( + repo, body, body + "refund = dataclasses.replace(quote_agent, tools=[send_image])\n" + ) + assert result["comparison_status"] == "partial" + assert any("agent copy at agent.py" in limit for limit in result["head"]["limits"]) + + +def test_a_copy_without_its_own_capabilities_is_not_a_limit(repo): + """The SDK docs' `robot_agent = pirate_agent.clone(name=..., instructions=...)`.""" + + body = ( + 'pirate_agent = Agent(name="Pirate", tools=TOOLS)\n' + 'robot_agent = pirate_agent.clone(name="Robot", instructions="beep")\n' + ) + result = _compare( + repo, body.replace("TOOLS", "[quote]"), body.replace("TOOLS", "[quote, send_image]") + ) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("pirate_agent", "send_image", "added")] + + +def test_an_unused_subclass_does_not_downgrade_other_agents(repo): + body = ( + "class LoggingAgent(Agent):\n pass\n" + 'triage = Agent(name="triage", tools=TOOLS)\n' + ) + result = _compare( + repo, body.replace("TOOLS", "[quote]"), body.replace("TOOLS", "[quote, send_image]") + ) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("triage", "send_image", "added")] + + +def test_an_instantiated_subclass_limits_only_its_own_agent(repo): + body = ( + "class LoggingAgent(Agent):\n pass\n" + 'logged = LoggingAgent(name="logged", tools=[quote])\n' + 'triage = Agent(name="triage", tools=TOOLS)\n' + ) + result = _compare( + repo, body.replace("TOOLS", "[quote]"), body.replace("TOOLS", "[quote, send_image]") + ) + assert result["comparison_status"] == "partial" + assert _pairs(result) == [("triage", "send_image", "added")] + assert {g["agent"] for g in result["head"]["coverage_gaps"]} == {"logged"} + + +def test_text_output_names_the_test_files_it_did_not_read(repo): + from typer.testing import CliRunner + + from agents_shipgate.cli.main import app + + body = 'assistant = Agent(name="assistant", tools=TOOLS)\n' + base = commit( + repo, + { + "agent.py": _agents(body.replace("TOOLS", "[quote]")), + "tests/test_agent.py": "def test_x():\n pass\n", + }, + ) + head = commit(repo, {"agent.py": _agents(body.replace("TOOLS", "[quote, send_image]"))}) + result = CliRunner().invoke( + app, ["diff", "--application", "--workspace", str(repo), "--base", base, "--head", head] + ) + assert result.exit_code == 0, result.output + assert "tests/test_agent.py" in result.output + + +def test_a_copy_in_a_module_without_the_sdk_import_is_a_named_limit(repo): + main = 'main_agent = Agent(name="Main", tools=[quote])\n' + base = commit( + repo, + { + "app/__init__.py": "", + "app/main.py": _agents(main), + "app/variants.py": "from app.main import main_agent\n", + }, + ) + head = commit( + repo, + { + "app/variants.py": ( + "from app.main import main_agent\n" + "from app.main import send_image\n" + 'refund_agent = main_agent.clone(name="Refund", tools=[send_image])\n' + ) + }, + ) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any("app/variants.py:3" in limit for limit in result["head"]["limits"]) + + +def test_adk_agent_name_constructed_twice_is_not_merged(repo): + source = ( + "from google.adk.agents import LlmAgent\n\n\n" + "def submit(q: str) -> str:\n return q\n\n\n" + "def submit_orchestrated(q: str) -> str:\n return q\n\n\n" + 'root_agent = LlmAgent(name="reviewer", model="m", tools=[submit])\n\n\n' + "def make_agent():\n" + ' return LlmAgent(name="reviewer", model="m", tools=TOOLS)\n' + ) + base = commit(repo, {"agent.py": source.replace("TOOLS", "[]")}) + head = commit(repo, {"agent.py": source.replace("TOOLS", "[submit_orchestrated]")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any("constructed more than once" in limit for limit in result["head"]["limits"]) + assert all(row["change"] == "not_established" for row in result["rows"]) + + +# --------------------------------------------------------------------------- +# #876 review, round 2: a list or an agent changed from another scope, through +# another name or in another module; copies and changes on values that are not +# agents; and identities that were one only by their variable's spelling. + + +@pytest.mark.parametrize( + "shape", + [ + "QUOTE_TOOLS = [quote]\ndef enable():\n QUOTE_TOOLS.append(send_image)\n", + "QUOTE_TOOLS = [quote]\ndef enable():\n global QUOTE_TOOLS\n" + " QUOTE_TOOLS = [quote, send_image]\n", + "QUOTE_TOOLS = []\ndef register(fn):\n QUOTE_TOOLS.append(fn)\n return fn\n" + "register(quote)\n", + "QUOTE_TOOLS = [quote]\nalias = QUOTE_TOOLS\nalias.append(send_image)\n", + "def add_images(lst):\n lst.append(send_image)\n" + "QUOTE_TOOLS = [quote]\nadd_images(QUOTE_TOOLS)\n", + "QUOTE_TOOLS = [quote]\nQUOTE_TOOLS[0] = send_image\n", + ], + ids=["append-in-a-function", "global-rebinding", "decorator-registry", "alias", "helper", "subscript"], +) +def test_a_module_list_changed_from_anywhere_is_dynamic(repo, shape): + after = shape + 'quote_agent = Agent(name="Quote", tools=QUOTE_TOOLS)\n' + result = _compare(repo, 'quote_agent = Agent(name="Quote", tools=[quote])\n', after) + assert result["comparison_status"] == "partial" + assert any("dynamic tools expression" in limit for limit in result["head"]["limits"]) + + +@pytest.mark.parametrize( + "inner", + [ + " def extend():\n nonlocal tools\n tools = [quote, send_image]\n", + " def extend():\n tools.append(send_image)\n", + ], + ids=["nonlocal", "closure"], +) +def test_a_builder_list_changed_by_a_nested_function_is_dynamic(repo, inner): + after = ( + "def build():\n tools = [quote]\n" + inner + " extend()\n" + ' return Agent(name="Quote", tools=tools)\n' + ) + result = _compare( + repo, 'def build():\n return Agent(name="Quote", tools=[quote])\n', after + ) + assert result["comparison_status"] == "partial" + + +def test_handoffs_changed_in_a_function_are_dynamic(repo): + base = 'sub_a = Agent(name="SubA", tools=[quote])\ntriage = Agent(name="Triage", handoffs=[sub_a])\n' + head = ( + 'sub_a = Agent(name="SubA", tools=[quote])\nsub_b = Agent(name="SubB", tools=[send_image])\n' + "HANDOFFS = [sub_a]\ndef enable():\n HANDOFFS.append(sub_b)\n" + 'triage = Agent(name="Triage", handoffs=HANDOFFS)\n' + ) + result = _compare(repo, base, head) + assert result["comparison_status"] == "partial" + assert any("dynamic handoffs" in limit for limit in result["head"]["limits"]) + + +@pytest.mark.parametrize( + ("before", "after", "agent"), + [ + ( + 'class Svc:\n def __init__(self):\n' + ' self.agent = Agent(name="Held", tools=[quote])\n', + 'class Svc:\n def __init__(self):\n' + ' self.agent = Agent(name="Held", tools=[quote])\n' + " self.agent.tools.append(send_image)\n", + "Held", + ), + ( + 'def build_quote_agent():\n return Agent(name="Quote", tools=[quote])\n' + "quote_agent = build_quote_agent()\n", + 'def build_quote_agent():\n return Agent(name="Quote", tools=[quote])\n' + "quote_agent = build_quote_agent()\nquote_agent.tools.append(send_image)\n", + "Quote", + ), + ( + 'quote_agent = Agent(name="Quote", tools=[quote])\n', + 'quote_agent = Agent(name="Quote", tools=[quote])\nquote_agent.tools[:] = [send_image]\n', + "quote_agent", + ), + ( + 'quote_agent = Agent(name="Quote", tools=[quote])\n', + 'quote_agent = Agent(name="Quote", tools=[quote])\n' + 'setattr(quote_agent, "tools", [send_image])\n', + "quote_agent", + ), + ( + 'quote_agent = Agent(name="Quote", tools=[quote])\n', + 'quote_agent = Agent(name="Quote", tools=[quote])\n' + "handle = quote_agent.tools\nhandle.append(send_image)\n", + "quote_agent", + ), + ], + ids=["self-attribute", "builder-result", "slice-store", "setattr", "handle"], +) +def test_an_agent_changed_after_construction_is_a_limit_on_it(repo, before, after, agent): + result = _compare(repo, before, after) + assert result["comparison_status"] == "partial" + assert any( + f"agent {agent!r} has its tools, handoffs or MCP servers changed" in limit + for limit in result["head"]["limits"] + ) + + +def test_a_change_on_a_value_the_reader_cannot_identify_limits_the_file(repo): + body = 'quote_agent = Agent(name="Quote", tools=[quote])\n' + change = "def extend(agent):\n agent.tools.append(send_image)\nextend(quote_agent)\n" + result = _compare(repo, body, body + change) + assert result["comparison_status"] == "partial" + assert any("value this reader cannot identify" in limit for limit in result["head"]["limits"]) + + +def test_a_change_in_a_module_without_the_sdk_import_is_a_named_limit(repo): + agents = 'quote_agent = Agent(name="Quote", tools=[quote])\n' + base = commit(repo, {"agent.py": _agents(agents), "wiring.py": "import agent\n"}) + head = commit( + repo, + {"wiring.py": "import agent\n\nagent.quote_agent.tools.append(agent.send_image)\n"}, + ) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any("changed at wiring.py:3" in limit for limit in result["head"]["limits"]) + + +def test_copy_replace_with_tools_is_a_named_limit(repo): + body = 'import copy\nquote_agent = Agent(name="Quote", tools=[quote])\n' + result = _compare(repo, body, body + "refund = copy.replace(quote_agent, tools=[send_image])\n") + assert result["comparison_status"] == "partial" + assert any("(copy.replace) is not read" in limit for limit in result["head"]["limits"]) + + +def test_a_subclass_used_from_another_module_is_a_named_limit(repo): + subclass = "from agents import Agent\n\n\nclass BaseAgent(Agent):\n pass\n" + agents = 'main_agent = Agent(name="Main", tools=[quote])\n' + base = commit(repo, {"base_agent.py": subclass, "agent.py": _agents(agents)}) + head = commit( + repo, + { + "agent.py": _agents( + "from base_agent import BaseAgent\n" + agents + + 'def build():\n return BaseAgent(name="Quote", tools=[quote, send_image])\n' + ) + }, + ) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any( + "uses BaseAgent, an OpenAI Agents SDK agent subclass defined at base_agent.py:4" in limit + for limit in result["head"]["limits"] + ) + + +@pytest.mark.parametrize( + "noise", + [ + "from dataclasses import dataclass, replace\n@dataclass\nclass Settings:\n debug: bool = False\n" + "SETTINGS = replace(Settings(), **{'debug': True})\n", + "class Settings:\n def clone(self, **kw):\n return self\ns = Settings().clone(handoffs=3)\n", + 'def make_payload():\n triage = type("P", (), {})()\n triage.tools = ["x"]\n return triage\n', + ], + ids=["config-replace", "non-agent-clone", "same-named-local"], +) +def test_copies_and_changes_on_values_that_are_not_agents_are_not_limits(repo, noise): + before = 'triage = Agent(name="Triage", tools=[quote])\n' + after = 'triage = Agent(name="Triage", tools=[quote, send_image])\n' + noise + result = _compare(repo, before, after) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("triage", "send_image", "added")] + + +def test_two_builders_local_agent_variables_are_two_agents(repo): + body = ( + 'def build_a():\n agent = Agent(name="A", tools=[quote])\n return agent\n' + 'def build_b():\n agent = Agent(name="B", tools=B_TOOLS)\n return agent\n' + ) + result = _compare( + repo, body.replace("B_TOOLS", "[quote]"), body.replace("B_TOOLS", "[quote, send_image]") + ) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("B", "send_image", "added")] + + +def test_a_local_handoff_target_is_named_by_its_identity(repo): + """Two builders' ``sub`` variables are two agents, and each handoff reaches its own.""" + + body = ( + 'def build_a():\n sub = Agent(name="SubA", tools=[quote])\n' + ' return Agent(name="TriageA", handoffs=[sub])\n' + 'def build_b():\n sub = Agent(name="SubB", tools=B_TOOLS)\n' + ' return Agent(name="TriageB", handoffs=[sub])\n' + ) + result = _compare( + repo, body.replace("B_TOOLS", "[quote]"), body.replace("B_TOOLS", "[quote, send_image]") + ) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("SubB", "send_image", "added")] + + +def test_a_rename_or_move_keeps_a_single_local_agents_identity(repo): + before = 'def build():\n agent = Agent(name="Support Bot", tools=[quote])\n return agent\n' + after = before.replace('"Support Bot"', '"Support Bot v2"') + result = _compare(repo, before, after) + assert result["comparison_status"] == "compared" + assert result["rows"] == [] + + +def test_identical_constructions_of_one_identity_are_one_agent(repo): + body = ( + "import os\nif os.environ.get('MINI'):\n" + ' agent = Agent(name="A", model="mini", tools=TOOLS)\n' + "else:\n" + ' agent = Agent(name="A", model="big", tools=TOOLS)\n' + ) + result = _compare( + repo, body.replace("TOOLS", "[quote]"), body.replace("TOOLS", "[quote, send_image]") + ) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("agent", "send_image", "added")] + + +def test_many_module_lists_are_read_in_linear_time(tmp_path): + import time + + from agents_shipgate.inputs.openai_sdk_static import load_openai_sdk_static_tools + from agents_shipgate.schemas.manifest import ToolSourceConfig + + def fastest(count: int) -> float: + lines = [TOOLS, "from agents import Agent\n"] + for index in range(count): + lines.append(f"t_{index} = [quote]\na_{index} = Agent(name='A_{index}', tools=t_{index})\n") + path = tmp_path / f"big_{count}.py" + path.write_text("".join(lines)) + source = ToolSourceConfig(id="sdk", type="openai_agents_sdk", path=path.name) + best = float("inf") + for _ in range(3): + started = time.perf_counter() + load_openai_sdk_static_tools(source, None, tmp_path) + best = min(best, time.perf_counter() - started) + return best + + small, large = fastest(100), fastest(1000) + # Ten times the lists may cost about ten times as much, never the hundred + # times a rescan of the file per list cost (41 s at 1,500 lists). + assert large < max(small * 40, 1.0) + + +# --------------------------------------------------------------------------- +# #876 review, round 3: reads that change nothing, uses spelled only in text, +# and identities that must survive a rename. + + +@pytest.mark.parametrize( + "helper", + [ + "def describe(request):\n tools = request.tools\n return len(tools)\n", + "def config_tools(x):\n return getattr(x, 'tools', None)\n", + ], + ids=["handle-only-read", "getattr-read"], +) +def test_reading_another_objects_tools_is_not_a_change(repo, helper): + before = 'main_agent = Agent(name="Main", tools=[quote])\n' + helper + after = 'main_agent = Agent(name="Main", tools=[quote, send_image])\n' + helper + result = _compare(repo, before, after) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("main_agent", "send_image", "added")] + + +@pytest.mark.parametrize( + "module", + [ + "def handle(req):\n tools = req.tools\n return tools\n", + "from vendor import Payload\n\np = Payload()\np.tools = ['x']\n", + "def registered(server):\n return getattr(server, 'tools', None)\n", + ], + ids=["request-handle", "library-object-store", "getattr"], +) +def test_another_librarys_tools_in_a_non_sdk_module_are_not_a_limit(repo, module): + agents = 'main_agent = Agent(name="Main", tools=TOOLS)\n' + base = commit(repo, {"agent.py": _agents(agents.replace("TOOLS", "[quote]")), "api.py": module}) + head = commit(repo, {"agent.py": _agents(agents.replace("TOOLS", "[quote, send_image]"))}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("main_agent", "send_image", "added")] + + +@pytest.mark.parametrize( + ("core", "other"), + [ + ( + "from agents import Agent\n\n\nclass Assistant(Agent):\n pass\n", + "PROMPT = 'You are an Assistant.'\n", + ), + ( + "from agents import Agent as SdkAgent\n\n\nclass Agent(SdkAgent):\n pass\n", + "from agents import Agent\n\nhelper = Agent(name='Helper')\n", + ), + ], + ids=["word-in-a-string", "wrapper-named-like-the-sdk-class"], +) +def test_a_subclass_is_used_only_where_code_imports_it(repo, core, other): + agents = 'main_agent = Agent(name="Main", tools=TOOLS)\n' + base = commit( + repo, + {"core.py": core, "other.py": other, "agent.py": _agents(agents.replace("TOOLS", "[quote]"))}, + ) + head = commit(repo, {"agent.py": _agents(agents.replace("TOOLS", "[quote, send_image]"))}) + result = run(repo, base, head) + assert not any("agent subclass" in limit for limit in result["head"]["limits"]) + assert ("main_agent", "send_image", "added") in _pairs(result) + + +def test_an_agent_class_made_with_type_is_a_named_limit(repo): + before = 'main_agent = Agent(name="Main", tools=[quote])\n' + after = before + 'Dyn = type("Dyn", (Agent,), {})\nd = Dyn(name="D", tools=[quote, send_image])\n' + result = _compare(repo, before, after) + assert result["comparison_status"] == "partial" + assert any("subclass 'Dyn'" in limit for limit in result["head"]["limits"]) + + +@pytest.mark.parametrize( + ("before", "after"), + [ + ( + 'def build():\n agent = Agent(name="Support Bot", tools=[quote])\n return agent\n', + 'def build():\n agent = Agent(name="Support Bot v2", tools=[quote])\n return agent\n', + ), + ( + 'NAME = "Quote"\ndef build():\n quote_agent = Agent(name=NAME, tools=[quote])\n return quote_agent\n', + 'def build():\n quote_agent = Agent(name="Quote", tools=[quote])\n return quote_agent\n', + ), + ( + 'quote_agent = Agent(name="Quote", tools=[quote])\n', + 'def build():\n quote_agent = Agent(name="Quote", tools=[quote])\n return quote_agent\n', + ), + ], + ids=["display-rename", "constant-to-literal", "module-to-local"], +) +def test_a_refactor_keeps_a_single_agents_identity(repo, before, after): + result = _compare(repo, before, after) + assert result["rows"] == [] + + +def test_a_change_repeated_on_one_agent_is_one_limit(repo): + body = ( + 'class Svc:\n def __init__(self):\n self.agent = Agent(name="Held", tools=[quote])\n' + + "".join(f" self.agent.tools.append(send_image) # {index}\n" for index in range(50)) + ) + result = _compare(repo, 'held = Agent(name="Held2", tools=[quote])\n', body) + changed = [limit for limit in result["head"]["limits"] if "changed after construction" in limit] + assert len(changed) == 1 + + +# --------------------------------------------------------------------------- +# #876 review, round 4: a module without the SDK import reaching an agent by +# any value, re-exported subclasses, and class bodies as scopes. + +WIRING_AGENTS = ( + TOOLS + "from agents import Agent\n" + 'quote_agent = Agent(name="Quote", tools=[quote])\n' + 'other = Agent(name="Other", tools=[quote])\n' + "def get_agent():\n return quote_agent\n" +) + + +@pytest.mark.parametrize( + "wiring", + [ + "from app.agents_def import get_agent, send_image\nget_agent().tools.append(send_image)\n", + "from app.agents_def import quote_agent, send_image\na = quote_agent\na.tools.append(send_image)\n", + "from app.agents_def import quote_agent, other, send_image\n" + "for a in (quote_agent, other):\n a.tools.append(send_image)\n", + "from app.agents_def import quote_agent, send_image\n" + "def patch(agent):\n agent.tools.append(send_image)\npatch(quote_agent)\n", + ], + ids=["call-result", "local-alias", "loop", "helper-parameter"], +) +def test_a_module_without_the_sdk_import_reaching_an_agent_is_a_limit(repo, wiring): + base = commit(repo, {"app/__init__.py": "", "app/agents_def.py": WIRING_AGENTS, "app/wiring.py": "x = 1\n"}) + head = commit(repo, {"app/wiring.py": wiring}) + result = run(repo, base, head, "--scope", "app") + assert result["comparison_status"] == "partial" + assert any("wiring.py" in limit for limit in result["head"]["limits"]) + + +def test_a_scope_below_its_top_package_still_sees_its_own_imports(repo): + files = { + "svc/__init__.py": "", + "svc/app/__init__.py": "", + "svc/app/agents_def.py": WIRING_AGENTS, + "svc/app/wiring.py": "x = 1\n", + } + base = commit(repo, files) + head = commit( + repo, + { + "svc/app/wiring.py": "from svc.app.agents_def import quote_agent, send_image\n" + "quote_agent.tools.append(send_image)\n" + }, + ) + result = run(repo, base, head, "--scope", "svc/app") + assert result["comparison_status"] == "partial" + + +def test_a_model_the_scope_defines_is_not_an_agent(repo): + schemas = "class Payload:\n tools = None\n" + payload = "from app.schemas import Payload\ndef build(specs):\n p = Payload()\n p.tools = specs\n return p\n" + agents = TOOLS + "from agents import Agent\nmain_agent = Agent(name='Main', tools=TOOLS_LIST)\n" + base = commit( + repo, + { + "app/__init__.py": "", + "app/schemas.py": schemas, + "app/payload.py": payload, + "app/agents_def.py": agents.replace("TOOLS_LIST", "[quote]"), + }, + ) + head = commit(repo, {"app/agents_def.py": agents.replace("TOOLS_LIST", "[quote, send_image]")}) + result = run(repo, base, head, "--scope", "app") + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("main_agent", "send_image", "added")] + + +def test_a_subclass_reexported_through_a_package_star_import_is_a_limit(repo): + base = commit( + repo, + { + "app/__init__.py": "", + "app/core.py": "from agents import Agent\n\n\nclass Assistant(Agent):\n pass\n", + "app/agents/__init__.py": "from app.core import *\n", + "app/main.py": "x = 1\n", + }, + ) + head = commit( + repo, + {"app/main.py": "from app.agents import Assistant\nq = Assistant(name='Q', tools=[])\n"}, + ) + result = run(repo, base, head, "--scope", "app") + assert result["comparison_status"] == "partial" + assert any("main.py uses Assistant" in limit for limit in result["head"]["limits"]) + + +def test_a_same_stem_vendor_class_is_not_the_scopes_subclass(repo): + core = "from agents import Agent\n\n\nclass Assistant(Agent):\n pass\n" + main = ( + TOOLS + "from agents import Agent\nfrom vendorlib.core import Assistant as VendorAssistant\n" + "main_agent = Agent(name='Main', tools=TOOLS_LIST)\n" + ) + base = commit(repo, {"core.py": core, "main.py": main.replace("TOOLS_LIST", "[quote]")}) + head = commit(repo, {"main.py": main.replace("TOOLS_LIST", "[quote, send_image]")}) + result = run(repo, base, head) + assert _pairs(result) == [("main_agent", "send_image", "added")] + assert not any("agent subclass" in limit for limit in result["head"]["limits"]) + + +def test_two_class_bodies_with_the_same_attribute_are_two_agents(repo): + body = ( + 'class Billing:\n agent = Agent(name="Billing", tools=[quote])\n' + 'class Support:\n agent = Agent(name="Support", tools=SUPPORT)\n' + ) + result = _compare( + repo, body.replace("SUPPORT", "[quote]"), body.replace("SUPPORT", "[quote, send_image]") + ) + assert result["comparison_status"] == "compared" + assert _pairs(result) == [("Support", "send_image", "added")] + + +def test_a_list_changed_through_one_agent_limits_every_agent_sharing_it(repo): + body = ( + "SHARED = [quote]\nfirst = Agent(name='first', tools=SHARED)\n" + "second = Agent(name='second', tools=SHARED)\nfirst.tools.append(send_image)\n" + ) + result = _compare(repo, body + "# base\n", body) + agents = {gap["agent"] for gap in result["head"]["coverage_gaps"] if "changed after" in gap["reason"]} + assert agents == {"first", "second"} + + +def test_an_adk_agent_changed_after_construction_is_a_limit(repo): + source = ( + "from google.adk.agents import Agent\n\n\n" + "def lookup(q: str) -> str:\n return q\n\n\n" + "def other(q: str) -> str:\n return q\n\n\n" + "root_agent = Agent(name='app', model='m', tools=[lookup])\n" + ) + base = commit(repo, {"agent.py": source}) + head = commit(repo, {"agent.py": source + "root_agent.tools.append(other)\n"}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any("changed at agent.py" in limit for limit in result["head"]["limits"]) diff --git a/tests/test_imported_tool_review.py b/tests/test_imported_tool_review.py index ed72f69f..fd983fb4 100644 --- a/tests/test_imported_tool_review.py +++ b/tests/test_imported_tool_review.py @@ -842,7 +842,8 @@ def test_sdk_list_passed_to_a_helper_is_dynamic_only_when_it_can_change(repo, he if dynamic: assert result["comparison_status"] == "partial" else: - assert result["comparison_status"] == "compared" + # The list stays established. A copy passing its own tools is a limit on + # the copy (#876), never on the agent whose list it reads. assert ("agent", "lookup", "changed") in _rows(result) From 74aae6e4796e062ad4810135f972dd1d77d19199 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Sun, 27 Sep 2026 10:47:21 -0700 Subject: [PATCH 2/7] fix(#876): pass bindings_at to _leaves_arguments_alone after the #864 squash The #876 change was written on the pre-squash #864 commits, where `_leaves_arguments_alone(call)` took one argument. Main's squash of #864 made it `(call, bindings_at)`, so after the cherry-pick `_capability_changes` raised TypeError and `diff --application` exited 1 on any SDK file taking a handle `t = x.tools` that is later handed to a call. It now builds the file's `bindings_at` once and passes it, as the other callers do. `test_reading_another_objects_tools_is_not_a_change[handle-only-read]` crashed before this and passes after. Co-Authored-By: Claude Opus 5.5 --- src/agents_shipgate/inputs/openai_sdk_static.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/agents_shipgate/inputs/openai_sdk_static.py b/src/agents_shipgate/inputs/openai_sdk_static.py index 6ec20052..da11c803 100644 --- a/src/agents_shipgate/inputs/openai_sdk_static.py +++ b/src/agents_shipgate/inputs/openai_sdk_static.py @@ -841,6 +841,7 @@ def _capability_changes( """ scopes = scopes or ScopeIndex(tree) + bindings_at = _bindings_at(scopes, _module_bindings(tree)[0]) def changed_handle(statement: ast.stmt) -> bool: target = _assignment_target(statement) if isinstance(statement, ast.Assign | ast.AnnAssign) else None @@ -853,7 +854,7 @@ def changed_handle(statement: ast.stmt) -> bool: if not isinstance(node, ast.Name) or node.id != target or node is defining: continue if not isinstance(node.ctx, ast.Load) or not _read_only_use( - node, scopes.parents, lambda call, *_: _leaves_arguments_alone(call) + node, scopes.parents, lambda call, *_: _leaves_arguments_alone(call, bindings_at) ): return True return False From fcf2186b91aa9a4f43917ce55af8ba3721775dfa Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Sun, 27 Sep 2026 10:52:46 -0700 Subject: [PATCH 3/7] fix(#876): name a Google ADK agent subclass as a limit The ADK reader reads each `Agent(...)`/`LlmAgent(...)` call, never a subclass's constructor. With `class Helper(LlmAgent)` beside an established root agent, a tool `helper = Helper(..., tools=[...])` gained left the comparison `compared` with no rows, on main and on the first #876 pass. The reader now records a surface warning for each class whose base resolves, through an import only, to an ADK agent class, so the module is `partial` and its tool surface is no longer enumerated for `scan`. Ported from PR #880's ADK census. Also carries over #880's reproduction of the issue itself (an imported tool list under `--scope app` beside a test double) and its ADK subclass test; both answer `compared` on main. Co-Authored-By: Claude Opus 5.5 --- src/agents_shipgate/inputs/google_adk.py | 37 +++++++++++++++ tests/test_application_diff_unobserved.py | 58 +++++++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/src/agents_shipgate/inputs/google_adk.py b/src/agents_shipgate/inputs/google_adk.py index 1688223e..97dd12d9 100644 --- a/src/agents_shipgate/inputs/google_adk.py +++ b/src/agents_shipgate/inputs/google_adk.py @@ -193,6 +193,9 @@ #: One agent name constructed at more than one call site in a module: the #: binding graph merges them, so no tool can be attributed to either (#876). SURFACE_GAP_DUPLICATE_AGENT_NAME = "duplicate_agent_name" +#: A class deriving from an ADK agent class: instances are built by calling the +#: subclass, which this reader does not follow, so their tools are unread (#876). +SURFACE_GAP_AGENT_SUBCLASS = "agent_subclass_unread" SURFACE_GAP_UNRESOLVED_SUB_AGENT = "unresolved_sub_agent" #: The module reaches an agent's ``tools`` attribute after construction, or #: builds an agent from unpacked keyword arguments. Reading the ``tools=`` @@ -1092,6 +1095,7 @@ def extract(self) -> list[LoadedToolSource]: loaded_sources.extend( self._extract_tool_expr(item, tools, agent_name, binding) ) + self._record_agent_subclasses() self._resolve_extraction_evidence(warnings_before, loaded_sources) return [ LoadedToolSource( @@ -1104,6 +1108,39 @@ def extract(self) -> list[LoadedToolSource]: *loaded_sources, ] + def _record_agent_subclasses(self) -> None: + """Name each class deriving from an ADK agent class (#876; ported from PR #880). + + ``extract`` reads every ``Agent(...)`` call; an instance of a subclass + is not one, so its wiring is a named limit rather than silently absent. + A base is ADK's only while its root name is bound by nothing but an + import, as ``_framework_symbol_is_proven`` requires of a call. + """ + + for node in ast.walk(self.tree): + if not isinstance(node, ast.ClassDef): + continue + for base in node.bases: + expression = base.value if isinstance(base, ast.Subscript) else base + root = expression + while isinstance(root, ast.Attribute): + root = root.value + if ( + isinstance(root, ast.Name) + and _qualified_name(expression, self.aliases) in AGENT_CLASS_NAMES + and all( + isinstance(binding, ast.alias) + for binding in self.name_bindings.get(root.id, []) + ) + ): + self._surface_warning( + f"Google ADK agent class {node.name!r} at {self.source_ref}:" + f"{node.lineno} derives from an agent class; agents built from it " + "are not read.", + SURFACE_GAP_AGENT_SUBCLASS, + ) + break + def _surface_warning(self, message: str, reason: str) -> None: """Report a construct that leaves part of this module's surface unknown.""" diff --git a/tests/test_application_diff_unobserved.py b/tests/test_application_diff_unobserved.py index 8c3ff12e..d099a60f 100644 --- a/tests/test_application_diff_unobserved.py +++ b/tests/test_application_diff_unobserved.py @@ -925,3 +925,61 @@ def test_an_adk_agent_changed_after_construction_is_a_limit(repo): result = run(repo, base, head) assert result["comparison_status"] == "partial" assert any("changed at agent.py" in limit for limit in result["head"]["limits"]) + + +# Ported from PR #880 (tests/test_application_diff_constructions.py). + + +def test_issue_reproduction_with_an_imported_list_and_a_scope(repo): + """The issue's own minimal reproduction: the builder imports its list.""" + + tools = TOOLS + "QUOTE_TOOLS = LIST\n" + builder = ( + "from agents import Agent\nfrom app.tools import QUOTE_TOOLS, quote\n" + "def build_quote_agent():\n" + ' return Agent(name="Quote", instructions="q", tools=QUOTE_TOOLS)\n' + ) + double = ( + "from agents import Agent, function_tool\n@function_tool\n" + "def fake_lookup(q: str) -> str:\n return q\n" + 'agent = Agent(name="TestAgent", instructions="t", tools=[fake_lookup])\n' + ) + base = commit( + repo, + { + "app/tools.py": tools.replace("LIST", "[quote]"), + "app/agents_def.py": builder, + "app/tests/test_turn.py": double, + }, + ) + head = commit(repo, {"app/tools.py": tools.replace("LIST", "[quote, send_image]")}) + result = run(repo, base, head, "--scope", "app") + # Main printed `compared` with no rows; the test double was the only agent. + assert result["comparison_status"] == "partial" + assert [a["name"] for a in result["head"]["agents"]] == ["Quote"] + assert result["head"]["excluded_tests"] == ["tests/test_turn.py"] + # Until the imported list resolves, it is named on Quote. + assert any( + g["agent"] == "Quote" and "QUOTE_TOOLS" in g["reason"] + for g in result["head"]["coverage_gaps"] + ) + + +def test_an_adk_agent_subclass_is_a_named_limit(repo): + adk = ( + "from google.adk.agents import LlmAgent\n" + "def lookup(query: str) -> str:\n return query\n" + "def execute(code: str) -> str:\n return code\n" + 'root_agent = LlmAgent(name="root", tools=[lookup])\n' + "class Helper(LlmAgent):\n pass\n" + 'helper = Helper(name="helper", tools=TOOLS)\n' + ) + base = commit(repo, {"agent.py": adk.replace("TOOLS", "[lookup]")}) + head = commit(repo, {"agent.py": adk.replace("TOOLS", "[lookup, execute]")}) + result = run(repo, base, head) + # Main and this branch before the port printed `compared` with no rows. + assert result["comparison_status"] == "partial" + assert any( + "agent.py:7" in g["reason"] and "'Helper'" in g["reason"] + for g in result["head"]["coverage_gaps"] + ) From 0a550d8f8fdfd752bbf25f0f807627c4530bb24d Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Sun, 27 Sep 2026 12:01:24 -0700 Subject: [PATCH 4/7] fix(#876): limit a duplicate tool to its name; keep a tool both constructions bind Two over-broad limits from the first #876 pass, both fail-closed but dropping true rows: - A tool name one application file defines twice dropped the whole file, so every other agent that file builds lost its rows. The name is now removed from that source's tools and binding observations and named as a gap on that name in that file (ported from PR #880); the file's other tools are still compared. - One agent name constructed at two sites (tensorflow#128063's root agent and builder) made every tool both sites bind an "Ambiguous tool identity", so the true ADDED row disappeared. A tool bound with the same meaning by each construction of an identity the reader already names as constructed more than once is now kept; a differing one is still ambiguous. Regression tests fail on the previous head. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +- docs/application-comparison.md | 13 +-- src/agents_shipgate/cli/application_diff.py | 89 +++++++++++++++------ tests/test_application_diff_unobserved.py | 42 ++++++++++ 4 files changed, 117 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f4bbb4c..b572cfd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,8 @@ - **Changes after construction.** A change to an agent's tools, handoffs or MCP servers — assigning, extending or slicing `x.tools`, a list method on it, `setattr`/`delattr` by name, or a handle `t = x.tools` that is itself changed later (a handle only read, like `len(request.tools)`, is nothing) — is read in the scope where it happens: on an agent the reader constructed (directly, through `self.agent`, or through a module function that returns it) it is a limit on that agent, and a change in place is one on every agent built from the same list object; on a value proven not to be an agent (`Settings()`, a literal, `type(...)()`, the instance's own `self.tools`) it is nothing; on anything else it is a limit on the file. In a module that is not read as an SDK source — a Google ADK source included, whose reader does not follow a change after construction either — a copy is a named limit on it, and so is a change to any object's tools once the module imports anything from the scope, or is itself a Google ADK source — through an alias, a loop, a parameter or a call's result, as in an SDK file — unless the object is plainly not an agent (a literal, or an instance of a class the scope defines that is not an `Agent` subclass); a module that imports nothing from the scope has another library's `.tools`. A module that imports an SDK `Agent` subclass from the scope, directly or through a package that re-exports it (`from app.core import *`), is limited too, never one that only spells its name in a string or imports a vendor class of the same name. - **Rows and verdicts that change.** A construction that used to be unread and silently absent is now either read (rows appear) or named (the result, and a `scan` of the same code, becomes `partial` / `insufficient_evidence` instead of complete). A Google ADK agent name constructed at two sites in one file is `insufficient_evidence` for `scan` too, where it was `review_required`. A copy passing no capabilities of its own, and a subclass nothing instantiates, are not limits. - **Test files are not the application.** Test files (the discovery convention, relative to the scope: `test`/`tests` directories, `test_*.py`, `*_test.py`, `conftest.py`, `test.py`, `tests.py`) are listed per side in `excluded_tests`, printed in the text output, and not read as agent sources, so a test double cannot establish a scope and a test file's own defects are not application gaps. - - **A duplicate tool definition limits one file.** A file defining one tool name twice refused the whole comparison with exit 2, and the message asked for the test file to be edited. It is now a named limit on that file; its agents are not compared, and every other file's are. Seen on alliance-genome/agr_ai_curation#842 and usestrix/strix#1103. + - **A duplicate tool definition limits one name.** A file defining one tool name twice refused the whole comparison with exit 2, and the message asked for the test file to be edited. It is now a named limit on that name in that file: no agent binds either definition, and the file's other tools and every other file are still compared. Seen on alliance-genome/agr_ai_curation#842 and usestrix/strix#1103. + - **One identity built twice keeps what both sites bind.** When one agent name is constructed at two sites (tensorflow#128063's root agent and builder), a tool both sites bind to the same callable is still a row of that agent, beside the limit that names the constructions; only a tool they bind differently is an ambiguous identity. - Additive on the unreleased `0.1` advisory result (`excluded_tests` per side); no schema or contract bump. - `diff --application` no longer refuses a repository over a link or submodule its application reader never opens, and reads a Google ADK agent imported from the package root. (#871 follow-up, measured for #868) - **The problem.** On 2026-09-25, against open third-party pull requests that edit SDK/ADK tool wiring, 15 of the first 27 runs at the default root scope exited 2 with `Git tree contains unsupported external binding`. The path named was never application source: `CLAUDE.md -> AGENTS.md` (dlt-hub/dlt#4417, jaegertracing/jaeger#9636, omnigent-ai/omnigent#6611, wandb/weave#7948), a linked `.claude/skills/…` or `.agents/skills/…` directory (asterinas#3834, vllm#57322, kagent-dev/kagent#2788), `agent/VERSION -> ../../VERSION` (TencentCloud/CubeSandbox#1508), `.pylintrc` (tensorflow#128063), and a vendored submodule (temporalio/sdk-python#1868). The root scope took the unscoped archive route, which refuses every link and gitlink. `from google.adk import Agent` was not read as an agent, so CubeSandbox#1508's `root_agent` gave `not_established` ("No supported application agents were established"). diff --git a/docs/application-comparison.md b/docs/application-comparison.md index 212e3527..f76a6072 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -85,9 +85,10 @@ agents' rows in the file stand: - one identity constructed at more than one site in a file with different tools or handoffs (two `return Agent(name="Quote", ...)` branches, or a literal `name` equal to another agent's variable) — the binding graph would - merge them, so the tools are attributed to neither; constructions that bind - exactly the same tools and handoffs are one agent. The same holds for a - Google ADK agent name; + merge them, so which construction binds which tool is not established; + constructions that bind exactly the same tools and handoffs are one agent. + A tool every such construction binds identically is still a row of that + agent, beside the limit. The same holds for a Google ADK agent name; - a construction with `**` keyword unpacking or positional arguments after its name, which can carry `tools` or `handoffs`; - an agent whose `tools`, `handoffs` or `mcp_servers` are changed after @@ -155,8 +156,10 @@ a parse failure) are not the application's gaps. A product module that only looks like a test is excluded too, which the printed list makes visible; the import resolver still follows a tool the application imports from such a file. -A file that defines one tool name twice is a named limit on that file, and the -agents it constructs are not compared; every other file in the scope still is. +A file that defines one tool name twice is a named limit on that name in that +file: no agent binds either definition, so every binding of the name there is +`not_established`, while the file's other tools, and every other file in the +scope, are still compared. Framework identity follows the import, not the spelling. `Agent` and `function_tool` are the OpenAI Agents SDK's only when imported from the absolute diff --git a/src/agents_shipgate/cli/application_diff.py b/src/agents_shipgate/cli/application_diff.py index 036c50e1..872273bf 100644 --- a/src/agents_shipgate/cli/application_diff.py +++ b/src/agents_shipgate/cli/application_diff.py @@ -679,19 +679,15 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) ) attributed.add(warning) result.handoff_only |= handoff_targets - constructed - try: - tools, warnings = _build_canonical_tools(loaded) - except InputParseError as exc: - if exc.details.get("failure") != DUPLICATE_TOOL_IN_SOURCE: - raise - # One file's duplicate is a limit on that file, not a refusal of every - # other agent in the scope (#876). - result.gap( - f"{source.path} defines the tool {exc.details.get('tool_name')!r} more " - "than once, so the agents it constructs are not compared.", - source=source.path, - ) - return + # One identity a reader observed at more than one construction site. The + # reader already names it as incomplete; a tool both sites bind the same + # way is one binding of it, not an ambiguous one (#876 review). + sites: dict[tuple[str, str], int] = {} + for item in loaded: + for observation in item.binding_observations: + site = (_source_path(root, observation.source), observation.agent) + sites[site] = sites.get(site, 0) + 1 + tools, warnings = _canonical_tools(result, source, loaded) for warning in warnings: result.gap(warning, source=source.path) graph, _ = resolve_agent_binding_graph(None, tools, bag, loaded) @@ -742,17 +738,7 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) binding_key = (*key, tool.name) if binding_key in ambiguous_bindings: continue - if binding_key in result.bindings: - result.gap( - f"Ambiguous tool identity: {binding_key}", - source=key[0], - agent=key[1], - tool=tool.name, - ) - result.bindings.pop(binding_key) - ambiguous_bindings.add(binding_key) - continue - result.bindings[binding_key] = { + binding = { "agent": key[1], "agent_source": key[0], "tool": tool.name, @@ -765,6 +751,24 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) "evidence_basis": edge.provenance_kind, **_import_path(tool, key[0]), } + if binding_key in result.bindings: + if sites.get(key, 0) > 1 and _meaning(result.bindings[binding_key]) == _meaning( + binding + ): + # Both constructions of one merged identity bind this callable + # identically: a true binding of that identity, kept beside the + # gap that already names its constructions. + continue + result.gap( + f"Ambiguous tool identity: {binding_key}", + source=key[0], + agent=key[1], + tool=tool.name, + ) + result.bindings.pop(binding_key) + ambiguous_bindings.add(binding_key) + continue + result.bindings[binding_key] = binding for edge in graph.handoff_edges: source, target = agent_keys[edge.source_agent_id], agent_keys[edge.target_agent_id] if source in ambiguous_agents or target in ambiguous_agents: @@ -781,6 +785,43 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) } +def _canonical_tools( + result: Observations, source: ToolSourceConfig, loaded: list[Any] +) -> tuple[list[Any], list[str]]: + """Build one source's catalog; a tool name defined twice is a limit on that name. + + The catalog refuses a duplicate definition, which is right for a reviewed + manifest and wrong here: one file defining `_tool` twice refused every + other agent's comparison (#876). The name is dropped from this source's + tools and binding observations, so no agent binds either definition, and + it is a gap over every binding of that name in this file; the agents' + other tools are still compared. Ported from PR #880. + """ + + while True: + try: + return _build_canonical_tools(loaded) + except InputParseError as exc: + if exc.details.get("failure") != DUPLICATE_TOOL_IN_SOURCE: + raise + name = exc.details.get("tool_name") + before = sum(len(item.tools) for item in loaded) + for item in loaded: + item.tools = [tool for tool in item.tools if tool.name != name] + for observation in item.binding_observations: + observation.tool_names = [n for n in observation.tool_names if n != name] + observation.tool_locators.pop(name, None) + observation.tool_issues.pop(name, None) + if not isinstance(name, str) or sum(len(item.tools) for item in loaded) == before: + raise + result.gap( + f"{source.path} defines the tool {name!r} more than once; which " + "definition an agent binds is not established.", + source=source.path, + tool=name, + ) + + def _reconcile_submodules(base: Observations, head: Observations) -> None: """Name each submodule the comparison did not read, by what it can hide. diff --git a/tests/test_application_diff_unobserved.py b/tests/test_application_diff_unobserved.py index d099a60f..8e67414e 100644 --- a/tests/test_application_diff_unobserved.py +++ b/tests/test_application_diff_unobserved.py @@ -152,6 +152,27 @@ def test_a_duplicate_tool_in_application_code_limits_only_that_file(repo): ) +def test_a_duplicate_tool_limits_only_that_name_in_its_file(repo): + # Review of #876: the whole file was dropped, so another agent the same + # file builds lost its rows. Only the duplicated name is uncertain. + duplicated = _agents( + "@function_tool\ndef dup(q: str) -> str:\n return q\n" + "@function_tool\ndef dup(q: str) -> str:\n return q + q\n" + 'other = Agent(name="other", tools=[dup])\n' + 'second = Agent(name="second", tools=TOOLS)\n' + ) + base = commit(repo, {"other.py": duplicated.replace("TOOLS", "[quote]")}) + head = commit(repo, {"other.py": duplicated.replace("TOOLS", "[quote, send_image]")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert _pairs(result) == [("second", "send_image", "added")] + assert { + (g["source"], g["agent"], g["tool"]) + for g in result["head"]["coverage_gaps"] + if "defines the tool" in g["reason"] + } == {("other.py", None, "dup")} + + def test_a_livekit_agent_subclass_is_not_the_sdk(repo): livekit = ( "from livekit.agents import Agent\n" @@ -240,6 +261,27 @@ def test_one_identity_constructed_twice_is_not_merged(repo): assert any("constructed more than once" in limit for limit in result["head"]["limits"]) +def test_a_tool_both_constructions_bind_is_kept(repo): + # tensorflow#128063: a root agent and a builder share one `name=` and both + # bind `details`. That binding is true of either construction; it was + # dropped as an ambiguous identity. The agent stays named as incomplete. + body = ( + "def make_root():\n" + ' return Agent(name="reviewer", tools=[quote, send_image])\n' + "def make_review_agent():\n" + ' return Agent(name="reviewer", tools=[quote])\n' + ) + base = commit(repo, {"README.md": "empty"}) + head = commit(repo, {"agent.py": _agents(body)}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert sorted(_pairs(result)) == [ + ("reviewer", "quote", "added"), + ("reviewer", "send_image", "added"), + ] + assert any("constructed more than once" in limit for limit in result["head"]["limits"]) + + def test_a_literal_name_equal_to_a_variable_name_is_not_merged(repo): body = ( 'assistant = Agent(name="Helper", tools=HELPER)\n' From 4f6a2c50c70fc4898eb2afe92fc822e0aadc07f7 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Sun, 27 Sep 2026 12:03:02 -0700 Subject: [PATCH 5/7] test(#876): run the SDK reader scaling check in the serial perf step `test_many_module_lists_are_read_in_linear_time` (added by this #876 change, not by #864) asserted a wall-clock ratio inside the parallel correctness gate. Alone it reads 27-31x for ten times the lists (the same on main); under `-n 4` load it measured 51x against a 40x bound and failed, which would stall the serial merge train. It moves to `tests/test_latency_budget.py` under the `perf` marker, so it runs only in CI's serial "Latency budget" step and `-m "not perf"` never collects it. The bound becomes 60x with a 2 s floor: the per-list rescan it guards against is ~100x and ~18 s at 1,000 lists. Co-Authored-By: Claude Opus 5.5 --- tests/test_application_diff_unobserved.py | 26 --------------- tests/test_latency_budget.py | 39 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/tests/test_application_diff_unobserved.py b/tests/test_application_diff_unobserved.py index 8e67414e..6098a4a5 100644 --- a/tests/test_application_diff_unobserved.py +++ b/tests/test_application_diff_unobserved.py @@ -699,32 +699,6 @@ def test_identical_constructions_of_one_identity_are_one_agent(repo): assert _pairs(result) == [("agent", "send_image", "added")] -def test_many_module_lists_are_read_in_linear_time(tmp_path): - import time - - from agents_shipgate.inputs.openai_sdk_static import load_openai_sdk_static_tools - from agents_shipgate.schemas.manifest import ToolSourceConfig - - def fastest(count: int) -> float: - lines = [TOOLS, "from agents import Agent\n"] - for index in range(count): - lines.append(f"t_{index} = [quote]\na_{index} = Agent(name='A_{index}', tools=t_{index})\n") - path = tmp_path / f"big_{count}.py" - path.write_text("".join(lines)) - source = ToolSourceConfig(id="sdk", type="openai_agents_sdk", path=path.name) - best = float("inf") - for _ in range(3): - started = time.perf_counter() - load_openai_sdk_static_tools(source, None, tmp_path) - best = min(best, time.perf_counter() - started) - return best - - small, large = fastest(100), fastest(1000) - # Ten times the lists may cost about ten times as much, never the hundred - # times a rescan of the file per list cost (41 s at 1,500 lists). - assert large < max(small * 40, 1.0) - - # --------------------------------------------------------------------------- # #876 review, round 3: reads that change nothing, uses spelled only in text, # and identities that must survive a rename. diff --git a/tests/test_latency_budget.py b/tests/test_latency_budget.py index a99217d8..5b6baf15 100644 --- a/tests/test_latency_budget.py +++ b/tests/test_latency_budget.py @@ -257,3 +257,42 @@ def test_scenarios_scale_sublinearly(perf_session: None, tmp_path: Path) -> None " python scripts/run_benchmarks.py --scenario large --json\n" "and look for a phase whose share of total time grew vs. small.\n" ) + + +_SDK_TOOL = '''from agents import function_tool +@function_tool +def quote(item: str) -> str: + return item +''' + + +@pytest.mark.perf +def test_sdk_reader_reads_many_module_lists_without_a_rescan(tmp_path: Path) -> None: + """The OpenAI Agents SDK reader must not rescan its file per tool list (#876). + + A rescan per list was quadratic: 41 s at 1,500 lists. Here, not in the + parallel correctness gate, because it is a wall-clock ratio and a loaded + xdist worker once measured 51x for what reads 27-31x alone (1.0-1.2 s at + 1,000 lists, 0.03-0.04 s at 100, the same on main). A rescan is ~100x and + ~18 s at 1,000 lists, so 60x with a 2 s floor still fails it. + """ + + from agents_shipgate.inputs.openai_sdk_static import load_openai_sdk_static_tools + from agents_shipgate.schemas.manifest import ToolSourceConfig + + def fastest(count: int) -> float: + lines = [_SDK_TOOL, "from agents import Agent\n"] + for index in range(count): + lines.append(f"t_{index} = [quote]\na_{index} = Agent(name='A_{index}', tools=t_{index})\n") + path = tmp_path / f"big_{count}.py" + path.write_text("".join(lines)) + source = ToolSourceConfig(id="sdk", type="openai_agents_sdk", path=path.name) + best = float("inf") + for _ in range(3): + started = time.perf_counter() + load_openai_sdk_static_tools(source, None, tmp_path) + best = min(best, time.perf_counter() - started) + return best + + small, large = fastest(100), fastest(1000) + assert large < max(small * 60, 2.0), (small, large) From 147518e5867403607778a1902fdd27f88155582b Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Sun, 27 Sep 2026 12:04:26 -0700 Subject: [PATCH 6/7] docs(#876): register excluded_tests and unread-construction limits The `application_diff` registry row did not mention what #876 adds to the surface: the per-side `excluded_tests` field and the limits that name an agent construction the readers cannot read. The row now states both and why neither is a claim (a path fact about the comparison's own inputs, and limits that only turn `compared` into `partial`); the parity harness's `application_diff` entry records the same reasoning and still registers no claim. The ADK agent-subclass limit carried over from #880 is documented in `docs/application-comparison.md` and as a sub-bullet of the #876 CHANGELOG entry, including its effect on `scan` (the module's tool surface is no longer reported as enumerated). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + docs/application-comparison.md | 7 +++++++ docs/distribution-surfaces.md | 2 +- tests/test_distribution_surface_parity.py | 6 ++++++ 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b572cfd9..ac6124d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - `diff --application` no longer reports `compared` with no changes while an agent it could not see gained a tool. On kkmiecik-coder/CRM#5, the quoting agent `Wycena`, built by `return Agent(name="Wycena", tools=NARZEDZIA_WYCENY)` inside a function, gained `wyslij_obraz`; the only agent read was a test double, so the result said `compared` and printed nothing. (#876) - **Every SDK construction is observed or named.** The OpenAI Agents SDK reader read only `name = Agent(...)`. It now also reads `return Agent(...)`, `self.agent = Agent(...)`, agents inline in a list, `Agent("name")` and `Agent[Context](...)`, identified by the literal `name`. An agent assigned to a plain name keeps that name as its identity, so renaming its `name=` or moving it between scopes changes nothing; only a variable name assigned in more than one function or class body (two builders' local `agent`) gives way to each agent's literal `name`, so those are two agents, and a handoff to such a variable reaches its own. What it cannot read is a named limit on that agent, so other agents' rows stand: one identity constructed at two sites that bind different tools (identical constructions are one agent), including a Google ADK agent name; `**` or extra positional arguments; a copy (`clone`, `dataclasses.replace`, `copy.replace`) passing its own `tools`/`handoffs`/`mcp_servers`, or only `**` on a value known to be an agent; and an agent built from a same-module `Agent` subclass, including one made by `type("X", (Agent,), {})`. A construction without a literal name is a limit on its file. - **Changes after construction.** A change to an agent's tools, handoffs or MCP servers — assigning, extending or slicing `x.tools`, a list method on it, `setattr`/`delattr` by name, or a handle `t = x.tools` that is itself changed later (a handle only read, like `len(request.tools)`, is nothing) — is read in the scope where it happens: on an agent the reader constructed (directly, through `self.agent`, or through a module function that returns it) it is a limit on that agent, and a change in place is one on every agent built from the same list object; on a value proven not to be an agent (`Settings()`, a literal, `type(...)()`, the instance's own `self.tools`) it is nothing; on anything else it is a limit on the file. In a module that is not read as an SDK source — a Google ADK source included, whose reader does not follow a change after construction either — a copy is a named limit on it, and so is a change to any object's tools once the module imports anything from the scope, or is itself a Google ADK source — through an alias, a loop, a parameter or a call's result, as in an SDK file — unless the object is plainly not an agent (a literal, or an instance of a class the scope defines that is not an `Agent` subclass); a module that imports nothing from the scope has another library's `.tools`. A module that imports an SDK `Agent` subclass from the scope, directly or through a package that re-exports it (`from app.core import *`), is limited too, never one that only spells its name in a string or imports a vendor class of the same name. + - **Google ADK agent subclasses.** A class deriving from an ADK agent class (`class Helper(LlmAgent)`) is a named limit on the module that defines it. An agent built from it was never read, so a tool it gained could leave the result `compared` with no rows. `scan` no longer reports that module's tool surface as enumerated either. (Carried over from #880.) - **Rows and verdicts that change.** A construction that used to be unread and silently absent is now either read (rows appear) or named (the result, and a `scan` of the same code, becomes `partial` / `insufficient_evidence` instead of complete). A Google ADK agent name constructed at two sites in one file is `insufficient_evidence` for `scan` too, where it was `review_required`. A copy passing no capabilities of its own, and a subclass nothing instantiates, are not limits. - **Test files are not the application.** Test files (the discovery convention, relative to the scope: `test`/`tests` directories, `test_*.py`, `*_test.py`, `conftest.py`, `test.py`, `tests.py`) are listed per side in `excluded_tests`, printed in the text output, and not read as agent sources, so a test double cannot establish a scope and a test file's own defects are not application gaps. - **A duplicate tool definition limits one name.** A file defining one tool name twice refused the whole comparison with exit 2, and the message asked for the test file to be edited. It is now a named limit on that name in that file: no agent binds either definition, and the file's other tools and every other file are still compared. Seen on alliance-genome/agr_ai_curation#842 and usestrix/strix#1103. diff --git a/docs/application-comparison.md b/docs/application-comparison.md index f76a6072..ad7fc2c5 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -131,6 +131,13 @@ in a string is not. Each is a limit on the module where it appears. The scope's own package path counts as the scope (`from svc.app.x import …` under `--scope svc/app`). +The Google ADK reader reads each `Agent(...)` / `LlmAgent(...)` call, not a +subclass's constructor. A class deriving from an ADK agent class (`class +Helper(LlmAgent)`, its base imported from `google.adk`) is therefore a named +limit on the module that defines it, whether its instances are built there or +in another module, and that module's tool surface is not reported as +enumerated to `scan` either. + Not read at all: an `Agent` re-exported through a project module, `functools.partial(Agent, ...)`, or a subclass defined outside the scope. diff --git a/docs/distribution-surfaces.md b/docs/distribution-surfaces.md index 4090252e..7d874b8d 100644 --- a/docs/distribution-surfaces.md +++ b/docs/distribution-surfaces.md @@ -75,7 +75,7 @@ and this document are checked against each other by | `human_review_decision` | `docs/human-review-decision.md` | `release_decision_vocabulary` | `test_surface_enumerations_match_the_engine_vocabulary` | Host-neutral read-only evaluator; no GitHub acquisition, persistence or operation authority. | | `github_action` | `action.yml`, `scripts/github_action_outputs.py` | `merge_verdict_vocabulary` | `test_action_input_enumerates_engine_merge_verdicts`, `test_action_output_script_shares_the_engine_merge_verdicts` | The paired `shipgate_wheel`/`shipgate_wheel_sha256` inputs install a caller-supplied local wheel instead of a published version, so that route names no channel and claims no `executable_pin`; it is refused unless both halves are given, and it installs `--no-deps`. `tests/test_action_engine_install.py` proves the refusals. Every `python` the Action starts in the workspace runs with `-P` or as a script path, so a pull request's `pip/` or `agents_shipgate/` package cannot stand in for pip or the engine; the same file executes the install and merge-verdict steps against such a checkout. The `v1.0.0` tag predates that fix; the published `v1.1.0` carries it. | | `capability_diff` | `src/agents_shipgate/cli/diff.py`, `src/agents_shipgate/core/capability_diff_rows.py`, `src/agents_shipgate/core/host_comparison.py`, `src/agents_shipgate/report/host_comparison.py`, `src/agents_shipgate/core/unread_inputs.py`, `src/agents_shipgate/cli/verify/changed_inputs.py` | — | — | Answers no question the engine answers: it emits no verdict, no release decision and no pin. Every field is read from the drift payload the engine already produces — `risk` is the engine's severity and `expansion_signals` is the engine's word on widening — so there is no second implementation to drift. A `permission_mode` or `sandbox` row names the setting and its value as the file spells it (`enableAllProjectMcpServers: true`, `defaultMode: dontAsk`), recovered from the grant's published value and digest, and a Claude Code setting's `why` is the basis the engine's one setting table (`core/host_settings.py`) records for the value; that table also rates the grant and `check`'s violation, so a row's severity and the violation's risk give one answer (#827, `tests/test_prompt_disabling_settings.py`). `verify`/PR and `check` reuse the host comparator (#684, `tests/test_manifest_free_pr_rows.py`), and the source name each named reusable-workflow secret refers to, also non-widening, with a redacting name or target refused rather than compared, and an unreadable value neither compared nor named on this surface — only the host inventory and `audit --host` name its `job/destination`, as on `1.0.0` (#693, `tests/test_reusable_workflow_secret_mappings.py`); check retains argument redaction and its existing local-policy control. Missing comparison evidence never supplies empty comparable rows. Default host mode only (`--application` is registered separately below); workflow rows compare effective writes and reusable secret recipients (#685, `tests/test_workflow_capability_diff.py`) and each job's remote step action references, as a non-widening change (#771, `tests/test_workflow_step_action_references.py`); and each job's agent launches — a documented agent action's permission inputs, the permission flags of a `run:` that is one plain `claude -p` / `codex exec` command — and checkout refs, compared as text, as a change unless the job gains a documented widening rule, which the engine names in `expansion_signals` (`workflow_agent_widened_*`) — a rule read only from text the engine reads exactly (no shell is parsed; an argument input that is not a plain list of words is compared by a digest and read for no rule), and a gain the engine does not claim (a rule moved in from a job the launch left, one an unread step of the job rewritten as a read launch may already have met, or one the job's launch held before in an expression or an unread argument input) named in the `why` from the same engine function, never counted — with a note on a workflow row naming the untrusted-input trigger, write scopes, secrets and pull request checkout beside each agent step, read off the grant the engine published and moving no direction; an unread `run:` agent step (never compared, so never a row), an unreadable value or a setting published redacted is named only by the host inventory and `audit --host`, as for an unread secret value, an unread argument input or an unresolved launch is named there and in the `why` of a row reporting its launch, and a checkout ref holding credential-shaped text is refused as a redacting step reference is (#823, `tests/test_workflow_agent_launches.py`); every job id, step label, trigger and scope name those rows print is the label the engine published once where it built the grant, redacted, never re-derived here; `check`'s workflow evidence is derived from the raw declarations, which it still compares, and redacts job and scope names by the same rule; two distinct job ids or triggers in one workflow, or scope names in one `permissions` mapping, that publish alike are refused rather than compared, so while such a workflow exists `check` refuses on every run even when it is unchanged (#802, `tests/test_workflow_label_redaction.py`); artifact-only edits remain separate evidence. Tool-source subjects are #655. Where a partial or experimental surface is byte-identical on both sides, `diff` and `verify` compare the rest and name it in `unchanged_limits`; `check` keeps refusing, because its boundary result cannot carry a limit yet (#721). A surface the reader reaches through an in-tree link it reads through qualifies only when that link, a link with the same text at each link on the way, and the file it lands on, the same blob at the same path, are both unchanged, read from the base's Git tree entries against a commit's or, without following any link, the working tree's; any change to either is treated as before. The same proof decides which shared plugin-reference limits `check` leaves out, so behind such a link `check` compares, and publishes the rows it finds, exactly as for a limit at its own path, and a comparison `partial` only because of such a limit is `comparable` with it in `unchanged_limits` (#822, `tests/test_linked_unchanged_limits.py`). A hook row's `why` states the grant's loading basis, read from its published `source`, `access` and `risk` by the engine's `hook_loading_basis`; only a hook the host loads for this project earns an expansion signal — one a settings layer declares, or one a plugin selects that the repository's project settings enable from an in-repository marketplace — so a declared-only hook, or one a plugin selects without that enablement, is a row and never an expansion, and a removal names no basis (#714). `check` compares without a plugin-reference limit both sides share on an untouched source, which it cannot name and, untouched, does not route; a limit only one side carries makes its comparison incomparable. Those rows are not what routes a change: `check`, and the boundary check a manifest-backed `verify` runs, route a changed hook declaration of a plugin the project settings enable through the existing protected-surface rule, from the plugin hook reader's selection on both compared sides, and count a changed hook file such a plugin selects that the reader does not open as incomplete input; the rows beside either are unchanged (#809, `tests/test_enabled_plugin_hook_routing.py`). A partial clone that never fetched the base's objects is refused as `objects_missing`, exit `2`, never compared and never fetched; the refusal ends with the remediation sentence `verify` reports for the same reason, produced by the same function (#817, `tests/test_capability_diff_partial_clone.py`). The text of `diff`, `verify`, the PR comment and `check` reads the rows through one function, `review_changes`, and adds no row and changes no row value in any JSON projection (#795, `tests/test_host_diff_review_changes.py`): a permission rule is named with its disposition; an MCP server with the command name (never its path) or redacted URL, its package and argument digest (#819) and the env and header key names its grant already publishes, a URL printing only in the engine's sanitized scheme-and-host form and otherwise as `url not shown`, or, when none of those differ, a sentence naming what was compared and that the change is in a detail not shown, such as the command's path or another setting; a hook with each handler field that changed — its group's matcher, its command as its executable's name and digest, its timeout — before and after, a handler only one side declares, or the published handlers in a different order with a detail not shown that may also differ, and past the handler bound the same kind of sentence naming a handler past it, all read from the handlers its host-grants `0.7` grant publishes, which hold no command or argument text, and never re-derived here, and a declaration outside the documented hooks shape named as not shown rather than guessed (#819, `tests/test_hook_mcp_detail_fields.py`); those hook and MCP members display what `config_sha256` already binds, so grant equality and every inventory digest leave them out, a saved baseline holds none of them, and no row, row value, reason, digest or control answer moves; the PR comment gives the lines 1.1.0 printed their room first, the coverage block included, and prints an entry whole when the whole comment fits, otherwise cut to the widest length of at least 60 characters at which it does, or else in its shortest form (a difference cut after its name, an added or removed grant as its row), never longer than the entry 1.1.0 printed, with one line naming `verifier.json`, so no long entry hides a row, the coverage block, the change count, the review question, the reproduction or the advisory that 1.1.0 kept (#819 review, cycles 4 and 6); an allow rule the permission lattice decided another replaced (`widened` or `narrowed`), or the exact rule text that moved between dispositions in one host and source (`moved`), is one entry, never on the routes that redact rule arguments; and `diff` counts entries `from N rows` when one joins rows. Comparable results with entries end with one review question, naming the row count when an entry joins rows, and every result whose comparison names a base commit and a commit or working-tree head — a zero-row result and a refusal included (#812 follow-up, `tests/test_host_comparison_coverage.py`) — ends with the compared commits, the tool version and an `agents-shipgate diff --base ` reproduction, labelled `Inputs:` rather than `Compared:` where the comparison was refused, since that run compared nothing — and a refused comparison publishes no `review` object at all, so those two lines are the only place that run states its provenance, built from the `base_commit` it publishes beside the refusal; `check` and a provided diff print the question alone, and no result without a change asks a question. Every one of those facts is published beside the rows, so a machine consumer reads what a human reads (#795 slice 2, same test file): a row adds `disposition`, the `allow`/`ask`/`deny` list a permission rule is declared under and `null` for any other kind, on every route that publishes rows; and `review` in `diff --json` (capability diff `0.3`) and `host_comparison.review` in `verifier.json` (verifier `0.20`) — one object for one comparison — carry the presented changes, each naming the `row_indexes` it stands for, the `direction` the text uses (`widened`, `narrowed` and `moved` included, which no single row can carry), its cells, its `why` and one `expands`, plus a `summary` of `{rows, changes, widenings}` equal to `diff`'s summary line, the review question and the reproduction command. The block is refused unless its changes stand for every published row exactly once, its counters match and no joined change's two sides read alike, so the routes that redact rule arguments publish their rows alone and never a pair that reads `X → X`; `check`'s boundary result carries rows, with their dispositions, and no block. It is presentation, not a second opinion: it is the one `review_changes` projection the text prints, so the rows, their values, their count and every control answer are what they were. A comparison read back from JSON prints the changes it published, and one whose rows a caller sliced falls back to those rows. Each comparison also says what it established (#812, `tests/test_host_comparison_coverage.py`): `coverage` in `diff --json` (capability diff `0.3`) and `host_comparison.coverage` in `verifier.json` (verifier `0.20`) are the same object, printed as `What this run established` by `diff`, `verify` text and the PR comment. It is read off the grant changes, artifact changes, observed sources and blocking issues the comparator already computed: a file's rows, counting a source inside it (`#profiles.`, `#plugins.`); a file with no row and no artifact change called unchanged (`compared`, `0` rows) only when Git proves its blob identical, as the check `unchanged_limits` uses does, asked privately in one bounded batch and never published, because the artifact digest redacts `env` values and `apiKeyHelper`; a file that changed with no compared grant moving (`changed_without_grant_change`), whose artifact differs only in its digest or whose content Git shows differs while its artifact did not (never a difference a checkout line-ending conversion or a converting attribute explains, and no filter is run), never a plugin manifest or marketplace, a retargeted link or project settings while a hook's loading basis moved, worded as no compared grant changing and never as which fields changed; any other changed file with no row (`changed_without_rows`); a file Git neither proves identical nor shows differs — a provided diff, a link read, a redacted path, a working-tree file a checkout wrote with `CRLF` that Git reports unchanged — as `unchanged_not_proven`, never no change and never a change (#812 review cycle 3); the side that published a source, worded `published by` rather than `read in` for a plugin manifest or marketplace, which is published only while it declares hooks; and on a refused comparison each blocking source and its kind. Outside the bounded candidate rules below, a file no inventory observed is never an item and its absence is no claim, which the block states where it is read — one line under the heading and `read_sources_only` in the JSON — so a true list cannot be taken for the account of the change (#812 follow-up); a source already in `unchanged_limits` is not repeated; the list is capped at ten with `omitted_items`, ordered so what no row shows precedes a file's rows and, among blocking limits, by kind (`unreadable`, `parse_failed`, `unresolved_precedence`, then `unsupported`, `dynamic_source_excluded`, `remote_source_excluded`) — order, not severity, and a ranking of kinds rather than of items, since `unsupported` carries both a file this entry merely does not accept and one whose own text would not parse, so an item behind the count may still be one to repair; total down to every field an item is keyed by, the source name and then its side, limit and status — and counted in text as items not listed, ranked below those listed, and the PR comment lists only what fits in the room its entries, review question, reproduction, advisory, next action and evidence leave, at most 2000 characters, so the block never pushes out a line the comment prints without it (a row list that fills the comment by itself still truncates it, as on `1.0.0`); an instruction file's line carries no redacted-values note; sources are the inventory's redacted paths; `null` means not recorded, which is how a `0.19` verifier reads. It moves no row, reason, digest, baseline, control state or next action, and `check`'s boundary result and text carry none, so neither `check` nor a provided diff asks Git anything for it. The same list names the changed inputs this entry does not read (#821, `tests/test_unread_changed_inputs.py`): capability diff `0.4` and verifier `0.21` add a `changed_not_read` item, with the `candidate` rule that named it, for each path in the comparison's own changed-file set — the committed change, or the working tree's tracked and untracked changes — that a bounded, documented rule set recognises as plausibly agent configuration (`mcp.json` in a plugin directory, a plugin manifest's `mcpServers`, a Codex, Cursor or Copilot manifest's `hooks` and the hook files it names, a manifest or marketplace that does not parse, `.cursor/hooks.json`, host settings below the repository root, a marketplace entry's external `source`) and that no inventory published; a member is named whatever read its file, because no reader reads it. It is named from the path and, for a manifest or marketplace member, its text: nothing is fetched, run or read as a grant, so it is never a row, a widening, a `check` violation or a loading claim, and an external source is described redacted and never fetched. It ranks right after the blocking limits, inside the same cap; `read_sources_only` is `false` while one is named, and the first line says so instead; `unread_candidates` and `unread_candidates_not_examined` say whether the change set was examined and how many candidates were not — past the bound of 32, or because a file the rule needed was not read or did not parse, one count the text names both causes of. A manifest-free `verify` whose only host-relevant change is such an input, or a changed candidate it counts as not examined, publishes the comparison instead of the setup route, and `verify --preview` then names `audit --host` instead of `init --write`, in an agent-related workspace too; a `0.20` verifier reads with the search not recorded. A comparison refused only by plugin-reference limits, each bounded by its plugin directory, that no compared source depends on, is `partial` instead (#808, `tests/test_partial_host_comparison.py`); any other blocking limit it carries must be one both sides share on an unchanged source, named in `unchanged_limits` as on a comparable result. Capability diff `0.4` and verifier `0.21` publish `comparison_status: partial` with the refusal's `incomparable_reasons`, the rows, review and unchanged limits established outside those directories, and each directory (the outermost, where one holds another) as the reserved `coverage.items[].scope` on the `blocking_limit` items it bounds, and never call a changed project settings file without a row `changed_without_grant_change`, since the hooks whose loading basis it decides are not all compared; `diff`, `verify` text and the PR comment lead with `Partial comparison against …` or `Host capability comparison partial: …` and `Not compared: , …` before any entry, and a partial result with no entry is never printed as no change. Independence is read off the reader's reference graph, never off directory names: any other limit that is not unchanged, a reference leaving its plugin, a plugin at the root or holding project settings, a marketplace elsewhere declaring inline hooks for it, or a directory that does not publish as itself refuses as before. It answers no engine question and moves no control: a partial comparison is not comparable, `verify`'s control and route are the refusal's, the control envelope projects it as `incomparable` with no rows, and `check`, whose boundary result cannot name a directory, refuses its comparison and decides exactly as before. A `0.20` verifier claiming a partial comparison or a scope is refused. | -| `application_diff` | `src/agents_shipgate/cli/application_diff.py` | — | — | Advisory SDK/ADK source-wiring comparison through `diff --application`. Reuses framework observations and the binding graph; its comparator answers no release or merge verdict, activation verdict, executable pin, or declared authority question. Unlike host mode it computes per-agent source differences, not a projection of host drift. Scoped gaps and `not_established` candidates bound the answer; no deployment-root reachability is asserted. An agent the other side's file still names through a construction no reader supports is a gap there, never a removal or addition, and SDK identity follows the import, so LiveKit's `Agent`/`function_tool` are not read as the SDK's. `tests/test_application_diff.py`, `tests/test_application_diff_review.py` and `tests/test_application_diff_identity.py` prove the advisory boundary, isolation, uncertainty and evidence identity. Every scope, the root included, is materialized by the scoped verified materializer, so a link is recreated rather than refused and never read through; every link under the scope, a dangling one included, is censused and gapped only where it can hide application source (a `*.py` link not aliasing an input the scope reads, a directory link holding Python outside the scope, or an unresolved link where the other side reads source); a submodule is never read, named as a limit when its gitlink commit is unchanged and a coverage gap over its path (the whole scope at the scope itself) otherwise; the host-configuration census adds no application gap (`tests/test_application_diff_reach.py`). A tool an agent binds from another module inside the selected scope is followed by the SDK/ADK readers to its definition, never imported or run, and each module read is published with its digest as `import_path` evidence outside the compared meaning; an import they cannot follow stays a named gap scoped to its agent (#864, `tests/test_imported_tool_bindings.py`). | +| `application_diff` | `src/agents_shipgate/cli/application_diff.py` | — | — | Advisory SDK/ADK source-wiring comparison through `diff --application`. Reuses framework observations and the binding graph; its comparator answers no release or merge verdict, activation verdict, executable pin, or declared authority question. Unlike host mode it computes per-agent source differences, not a projection of host drift. Scoped gaps and `not_established` candidates bound the answer; no deployment-root reachability is asserted. An agent the other side's file still names through a construction no reader supports is a gap there, never a removal or addition, and SDK identity follows the import, so LiveKit's `Agent`/`function_tool` are not read as the SDK's. `tests/test_application_diff.py`, `tests/test_application_diff_review.py` and `tests/test_application_diff_identity.py` prove the advisory boundary, isolation, uncertainty and evidence identity. Every scope, the root included, is materialized by the scoped verified materializer, so a link is recreated rather than refused and never read through; every link under the scope, a dangling one included, is censused and gapped only where it can hide application source (a `*.py` link not aliasing an input the scope reads, a directory link holding Python outside the scope, or an unresolved link where the other side reads source); a submodule is never read, named as a limit when its gitlink commit is unchanged and a coverage gap over its path (the whole scope at the scope itself) otherwise; the host-configuration census adds no application gap (`tests/test_application_diff_reach.py`). A tool an agent binds from another module inside the selected scope is followed by the SDK/ADK readers to its definition, never imported or run, and each module read is published with its digest as `import_path` evidence outside the compared meaning; an import they cannot follow stays a named gap scoped to its agent (#864, `tests/test_imported_tool_bindings.py`). An unobserved agent is not no change (#876): every OpenAI Agents SDK construction in a file the scope reads is an observed agent or a named limit — `return Agent(...)`, `self.agent = Agent(...)`, an inline agent, `Agent[Context](...)` and a positional name are read under their literal `name`; `**` or extra positional arguments, a capability-passing copy, a change to an agent's tools after construction, one identity constructed twice with different tools, a construction without a literal name, and an agent built from an SDK `Agent` subclass or from a class deriving from a Google ADK agent class are limits — so `compared` holds no unaccounted construction site. Test code, by discovery's test-path convention relative to the scope, establishes nothing and is listed per side in the additive `excluded_tests` field, a list of the comparison's own unread input paths that restates no engine answer and adds no claim; a tool name one application file defines twice is a gap on that name, never a refusal (`tests/test_application_diff_unobserved.py`). | | `zero_install_detector` | `tools/shipgate-detect.py` | `agent_project_verdict` | `test_detector_verdict_matches_cli` | Emits no `diagnostics[]` and no `next_actions[]`; evidence strings and framework scores are simplified. See the script's own "Intentional simplifications". | | `emitted_ci_workflow` | `src/agents_shipgate/cli/discovery/ci_workflow.py` | `executable_pin` | `tests/test_adopter_pins_resolve.py::test_the_emitted_workflow_pins_the_release_and_not_the_source_tree`, `tests/test_release_source.py::test_candidate_workflow_uses_immutable_source_before_and_after_publication` | Ordinary/source/preview builds use the published fallback; a stamped candidate pins its verified Action SHA and package version. Before publication its smoke substitutes the exact local wheel inputs. Provenance asserts no qualification. | | `prompts` | `prompts/` | `contract_floor`, `executable_pin`, `placeholder_ownership`, `release_decision_vocabulary` | `test_executable_pin_resolves_in_a_published_channel`, `test_surface_enumerations_match_the_engine_vocabulary`, `test_surface_routes_human_owned_placeholders_to_a_human`, `tests/test_adopter_pins_resolve.py::test_every_pin_init_writes_into_an_adopter_repo_names_the_published_release`, `tests/test_adopter_pins_resolve.py::test_the_shipped_floor_is_decided_against_the_release_the_prompts_pin` | — | diff --git a/tests/test_distribution_surface_parity.py b/tests/test_distribution_surface_parity.py index 342a92c0..fe1a25ad 100644 --- a/tests/test_distribution_surface_parity.py +++ b/tests/test_distribution_surface_parity.py @@ -176,6 +176,12 @@ def paths(self) -> list[Path]: ("src/agents_shipgate/cli/application_diff.py",), # Advisory source-wiring comparison, not host drift or an engine verdict. # No release permission, declared authority, pin or root reachability claim. + # Its per-side `excluded_tests` list (#876) names test files the + # comparison did not read, a path fact about its own inputs, and its + # unread-construction limits (an SDK construction it cannot read, an + # ADK agent subclass) only turn `compared` into `partial`; neither + # restates an engine answer, so neither adds a claim. + # `tests/test_application_diff_unobserved.py` holds both. {}, ), Surface( From 4d765581083c9be2fcdc214bafbac08e9869dfab Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Sun, 27 Sep 2026 13:25:48 -0700 Subject: [PATCH 7/7] fix(#876): count an ADK subclass where it is used; identical ADK constructions are one agent; no census without an agent source Three over-broad limits from the #876 review, each fail-closed but hiding true rows or turning `not_established` into `partial`: - An ADK agent subclass nothing uses limited its module. `class Helper(LlmAgent): pass` beside `root_agent` made the module `partial`, hid root's ADDED row, and dropped `scan` to insufficient_evidence. The reader now names a subclass only when its module uses it again (a call, `functools.partial`, any reference other than a deeper subclass's base, or a decorator); a subclass of a subclass counts too. Another module that imports it is named by the comparison's census, as for an SDK subclass, so `from agent import Helper; Helper(...)` limits that module. - Identical ADK constructions of one name were "constructed more than once". The duplicate check now runs after every construction is read and compares what each site bound: the tool definitions (name and locator) and resolved `sub_agents`. A site is comparable only when read cleanly (every tool a definition, no toolset, no warning, no new issue, no `**`, no unresolved or non-literal `sub_agents`); any other site differs from every site. `root_agent` plus a builder returning its twin is one agent again, and `scan` is `passed` as on main. - A scope with no SDK/ADK source on either side became `partial` from the census reading the repository's own `.tools` (`artifacts.tools.append` in `inputs/n8n/_tools.py` under `--scope src`). Both sides are now discovered before either is read, and the census runs only when some side found an agent source. Every module is still parsed, so a parse failure is still a gap. `--scope src` of this repository is `not_established` again, in main's time. Regression tests fail on the previous head. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 +- docs/application-comparison.md | 26 ++- src/agents_shipgate/cli/application_diff.py | 233 ++++++++++++++------ src/agents_shipgate/inputs/google_adk.py | 211 ++++++++++++++---- tests/test_application_diff_unobserved.py | 224 +++++++++++++++++++ 5 files changed, 576 insertions(+), 123 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac6124d8..ef482872 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,9 @@ - `diff --application` no longer reports `compared` with no changes while an agent it could not see gained a tool. On kkmiecik-coder/CRM#5, the quoting agent `Wycena`, built by `return Agent(name="Wycena", tools=NARZEDZIA_WYCENY)` inside a function, gained `wyslij_obraz`; the only agent read was a test double, so the result said `compared` and printed nothing. (#876) - **Every SDK construction is observed or named.** The OpenAI Agents SDK reader read only `name = Agent(...)`. It now also reads `return Agent(...)`, `self.agent = Agent(...)`, agents inline in a list, `Agent("name")` and `Agent[Context](...)`, identified by the literal `name`. An agent assigned to a plain name keeps that name as its identity, so renaming its `name=` or moving it between scopes changes nothing; only a variable name assigned in more than one function or class body (two builders' local `agent`) gives way to each agent's literal `name`, so those are two agents, and a handoff to such a variable reaches its own. What it cannot read is a named limit on that agent, so other agents' rows stand: one identity constructed at two sites that bind different tools (identical constructions are one agent), including a Google ADK agent name; `**` or extra positional arguments; a copy (`clone`, `dataclasses.replace`, `copy.replace`) passing its own `tools`/`handoffs`/`mcp_servers`, or only `**` on a value known to be an agent; and an agent built from a same-module `Agent` subclass, including one made by `type("X", (Agent,), {})`. A construction without a literal name is a limit on its file. - **Changes after construction.** A change to an agent's tools, handoffs or MCP servers — assigning, extending or slicing `x.tools`, a list method on it, `setattr`/`delattr` by name, or a handle `t = x.tools` that is itself changed later (a handle only read, like `len(request.tools)`, is nothing) — is read in the scope where it happens: on an agent the reader constructed (directly, through `self.agent`, or through a module function that returns it) it is a limit on that agent, and a change in place is one on every agent built from the same list object; on a value proven not to be an agent (`Settings()`, a literal, `type(...)()`, the instance's own `self.tools`) it is nothing; on anything else it is a limit on the file. In a module that is not read as an SDK source — a Google ADK source included, whose reader does not follow a change after construction either — a copy is a named limit on it, and so is a change to any object's tools once the module imports anything from the scope, or is itself a Google ADK source — through an alias, a loop, a parameter or a call's result, as in an SDK file — unless the object is plainly not an agent (a literal, or an instance of a class the scope defines that is not an `Agent` subclass); a module that imports nothing from the scope has another library's `.tools`. A module that imports an SDK `Agent` subclass from the scope, directly or through a package that re-exports it (`from app.core import *`), is limited too, never one that only spells its name in a string or imports a vendor class of the same name. - - **Google ADK agent subclasses.** A class deriving from an ADK agent class (`class Helper(LlmAgent)`) is a named limit on the module that defines it. An agent built from it was never read, so a tool it gained could leave the result `compared` with no rows. `scan` no longer reports that module's tool surface as enumerated either. (Carried over from #880.) - - **Rows and verdicts that change.** A construction that used to be unread and silently absent is now either read (rows appear) or named (the result, and a `scan` of the same code, becomes `partial` / `insufficient_evidence` instead of complete). A Google ADK agent name constructed at two sites in one file is `insufficient_evidence` for `scan` too, where it was `review_required`. A copy passing no capabilities of its own, and a subclass nothing instantiates, are not limits. + - **Google ADK agent subclasses.** A class deriving from an ADK agent class (`class Helper(LlmAgent)`, or a class deriving from that one) is a named limit where it is used: on the module that defines it when that module builds, passes or decorates it, and on every other module in the scope that imports it. An agent built from it was never read, so a tool it gained could leave the result `compared` with no rows. `scan` no longer reports the defining module's tool surface as enumerated when that module uses the class. A subclass nothing uses is not a limit. (Carried over from #880.) + - **Rows and verdicts that change.** A construction that used to be unread and silently absent is now either read (rows appear) or named (the result, and a `scan` of the same code, becomes `partial` / `insufficient_evidence` instead of complete). A Google ADK agent name constructed at two sites in one file with differing tools or handoffs is `insufficient_evidence` for `scan` too, where it could be `passed` or `review_required`; constructions binding the same definitions and handoffs, each read cleanly, are one agent and keep their rows and verdict. A copy passing no capabilities of its own, and a subclass nothing uses, are not limits. + - **No agent source, no census.** The census of copies, changes after construction and subclasses runs only when some side's discovery found an OpenAI Agents SDK or Google ADK source. Without one no agent is read, so `--scope src` of this repository stays `not_established` instead of turning `partial` over its own `artifacts.tools.append(record)`; every module is still parsed, and a parse failure is still a gap. - **Test files are not the application.** Test files (the discovery convention, relative to the scope: `test`/`tests` directories, `test_*.py`, `*_test.py`, `conftest.py`, `test.py`, `tests.py`) are listed per side in `excluded_tests`, printed in the text output, and not read as agent sources, so a test double cannot establish a scope and a test file's own defects are not application gaps. - **A duplicate tool definition limits one name.** A file defining one tool name twice refused the whole comparison with exit 2, and the message asked for the test file to be edited. It is now a named limit on that name in that file: no agent binds either definition, and the file's other tools and every other file are still compared. Seen on alliance-genome/agr_ai_curation#842 and usestrix/strix#1103. - **One identity built twice keeps what both sites bind.** When one agent name is constructed at two sites (tensorflow#128063's root agent and builder), a tool both sites bind to the same callable is still a row of that agent, beside the limit that names the constructions; only a tool they bind differently is an ambiguous identity. diff --git a/docs/application-comparison.md b/docs/application-comparison.md index ad7fc2c5..673f0120 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -88,7 +88,11 @@ agents' rows in the file stand: merge them, so which construction binds which tool is not established; constructions that bind exactly the same tools and handoffs are one agent. A tool every such construction binds identically is still a row of that - agent, beside the limit. The same holds for a Google ADK agent name; + agent, beside the limit. The same holds for a Google ADK agent name: its + constructions are one agent only when each binds the same tool definitions + and handoffs and was read cleanly (no toolset, no `**`, no warning, no + unresolved or non-literal `sub_agents`), as `root_agent` and a builder + returning its twin do; - a construction with `**` keyword unpacking or positional arguments after its name, which can carry `tools` or `handoffs`; - an agent whose `tools`, `handoffs` or `mcp_servers` are changed after @@ -133,10 +137,22 @@ own package path counts as the scope (`from svc.app.x import …` under The Google ADK reader reads each `Agent(...)` / `LlmAgent(...)` call, not a subclass's constructor. A class deriving from an ADK agent class (`class -Helper(LlmAgent)`, its base imported from `google.adk`) is therefore a named -limit on the module that defines it, whether its instances are built there or -in another module, and that module's tool surface is not reported as -enumerated to `scan` either. +Helper(LlmAgent)`, its base imported from `google.adk`, or a class deriving +from that one) is therefore a named limit where it is used: on the module that +defines it when that module names it again — a call, `functools.partial`, any +other reference, or a decorator that could build it — and that module's tool +surface is then not reported as enumerated to `scan` either; and on every other +module in the scope that imports it, as for an SDK subclass. A subclass nothing +uses, even one another unused subclass derives from, is not a limit. `scan` +reads one declared module, so a subclass used only in another module does not +affect it. + +The census of copies, changes and subclasses runs only when some side's +discovery found an OpenAI Agents SDK or Google ADK source. Without one, no +agent is read on either side, and the result stays `not_established` rather +than turning `partial` over the repository's own `.tools` (a report builder's +`artifacts.tools.append(record)`). Every module is still parsed, and one that +cannot be is still a gap. Not read at all: an `Agent` re-exported through a project module, `functools.partial(Agent, ...)`, or a subclass defined outside the scope. diff --git a/src/agents_shipgate/cli/application_diff.py b/src/agents_shipgate/cli/application_diff.py index 872273bf..aa292e59 100644 --- a/src/agents_shipgate/cli/application_diff.py +++ b/src/agents_shipgate/cli/application_diff.py @@ -37,7 +37,7 @@ from agents_shipgate.core.errors import ConfigError, InputParseError from agents_shipgate.core.privacy import sanitize_report_payload from agents_shipgate.core.verification_identity import build_engine_requirement -from agents_shipgate.inputs.google_adk import load_google_adk_artifacts +from agents_shipgate.inputs.google_adk import adk_agent_subclasses, load_google_adk_artifacts from agents_shipgate.inputs.openai_sdk_static import ( census_module, load_openai_sdk_static_tools, @@ -341,13 +341,31 @@ def read(path: str) -> str | None: return RepositoryLayout("" if scope in {"", "."} else scope, entries, links, read) -def observe( +@dataclass +class _Discovered: + """One side after discovery, before any module is read (#876 review). + + Whether either side found an agent source decides whether the census + runs, so both sides are discovered before either is read. + """ + + result: Observations + root: Path | None = None + python_files: list[Path] = field(default_factory=list) + linked: set[str] = field(default_factory=set) + detected: Any = None + tests: set[str] = field(default_factory=set) + #: ``(framework, path)`` for every supported agent source to read. + entries: list[tuple[str, str]] = field(default_factory=list) + + +def discover( tree: Path, scope: str, *, max_python_files: int, gitlinks: dict[str, str] | None = None, -) -> Observations: +) -> _Discovered: result = Observations(scope) for path, commit in (gitlinks or {}).items(): # A gitlink outside the scope arrived with an in-scope link's target, @@ -365,7 +383,7 @@ def observe( f"Scope {scope!r} is absent in this tree. If the application moved, " "select its old path with --base-scope and new path with --scope." ) - return result + return _Discovered(result) if not root.is_dir() or root.is_symlink(): raise ConfigError(f"Application scope is not a regular directory: {scope}") root = root.resolve() @@ -388,7 +406,7 @@ def observe( result.gap(f"Python input exceeds {MAX_PYTHON_BYTES} bytes: {file.relative_to(root)}") if result.limits: result.status = "partial" - return result + return _Discovered(result) linked = _observe_links(result, tree.resolve(), root, python_files) detected = detect_workspace(root, max_python_files=max_python_files) if detected.python_parse_truncated: @@ -399,16 +417,90 @@ def observe( # conceal application source were censused above, by `_observe_links`. for item in detected.excluded_sources: result.gap(f"Excluded candidate: {item}", source=item.get("path")) + entries = sorted( + { + (f.type, p) + for f in detected.frameworks + if f.type in SUPPORTED + for p in f.candidate_files + if p not in linked and p not in tests + } + ) + return _Discovered(result, root, python_files, linked, detected, tests, entries) + + +def observe(found: _Discovered, *, max_python_files: int, census: bool) -> Observations: + """Read one discovered side: every module's parse, the census, its sources. + + ``census`` is False only when neither side found a supported agent source + (#876 review). No agent is then read on either side, so no census limit + could qualify a row: it would only turn ``not_established`` into + ``partial`` over the repository's own ``.tools`` (``artifacts.tools.append``). + """ + + result = found.result + if found.root is None: + return result + root, python_files, linked = found.root, found.python_files, found.linked # Discovery omits malformed Python; preserve that gap rather than an empty # candidate list becoming negative evidence. Bound this second parse too. if len(python_files) > max_python_files: result.gap(f"Python input census exceeds {max_python_files} files.") - # A copy of an agent that passes its own tools, or a change to an agent's - # tools, can live in a module no reader reads as an SDK source; and an - # agent subclass defined in one module can be instantiated in another - # (#876 review). + parsed: dict[str, tuple[ast.Module, str]] = {} + for file in python_files[:max_python_files]: + relative = file.relative_to(root).as_posix() + if relative in linked: + continue + try: + raw = file.read_bytes() + tree = ast.parse(raw) + except (SyntaxError, ValueError, RecursionError, OSError): + result.gap( + f"Python input could not be parsed: {relative}", + source=relative, + ) + continue + if census: + parsed[relative] = (tree, raw.decode("utf-8", errors="replace")) + for framework in found.detected.frameworks: + if framework.type not in SUPPORTED and framework.candidate_files: + for path in framework.candidate_files: + if path in found.tests: + continue + result.gap( + f"Application comparison does not yet support {framework.type}: {path}", + source=path, + ) + if census: + _census(result, root, parsed, python_files, found.detected, found.entries) + sources = [ + ToolSourceConfig(id=f"{kind}:{path}", type=kind, path=path) for kind, path in found.entries + ] + result.sources = [{"type": s.type, "path": s.path} for s in sources] + for source in sources: + _observe_source(result, root, source) + return result + + +def _census( + result: Observations, + root: Path, + parsed: dict[str, tuple[ast.Module, str]], + python_files: list[Path], + detected: Any, + entries: list[tuple[str, str]], +) -> None: + """What can change an agent outside the constructions its reader reads. + + A copy of an agent that passes its own tools, or a change to an agent's + tools, can live in a module no reader reads as an SDK source; and an + agent subclass defined in one module can be instantiated in another + (#876 review). + """ + censuses: dict[str, Any] = {} - texts: dict[str, str] = {} + #: Google ADK agent subclasses each module defines, by name, with their line. + adk_subclasses: dict[str, dict[str, int]] = {} # The SDK reader reads its own sources' copies and changes. read_as_sdk = { path @@ -431,50 +523,20 @@ def observe( # The scope, and every package above it, may be what its modules # import through (``from svc.app.agents_def import …``). | {root.name} - | set(PurePosixPath(scope).parts) + | set(PurePosixPath(result.scope).parts) ) - for file in python_files[:max_python_files]: - relative = file.relative_to(root).as_posix() - if relative in linked: - continue - try: - raw = file.read_bytes() - parsed = ast.parse(raw) - except (SyntaxError, ValueError, RecursionError, OSError): - result.gap( - f"Python input could not be parsed: {relative}", - source=relative, - ) - continue - texts[relative] = raw.decode("utf-8", errors="replace") + for relative, (tree, text) in parsed.items(): censuses[relative] = census_module( - parsed, - texts[relative], + tree, + text, local_modules, read_as_sdk=relative in read_as_sdk, read_as_adk=relative in read_as_adk, ) - for framework in detected.frameworks: - if framework.type not in SUPPORTED and framework.candidate_files: - for path in framework.candidate_files: - if path in tests: - continue - result.gap( - f"Application comparison does not yet support {framework.type}: {path}", - source=path, - ) - entries = sorted( - { - (f.type, p) - for f in detected.frameworks - if f.type in SUPPORTED - for p in f.candidate_files - if p not in linked and p not in tests - } - ) - sources = [ - ToolSourceConfig(id=f"{kind}:{path}", type=kind, path=path) for kind, path in entries - ] + if "google.adk" in text: + found = adk_agent_subclasses(tree) + if found: + adk_subclasses[relative] = found sdk_sources = {path for kind, path in entries if kind == "openai_agents_sdk"} for path, census in sorted(censuses.items()): if path in sdk_sources: @@ -497,31 +559,46 @@ def observe( "that; agents it reaches are not established.", source=path, ) - for defining, census in sorted(censuses.items()): - for name, line in sorted(census.subclasses.items()): - # A package that re-exports the class (``from app.core import *``) - # provides it too, transitively. - providers = {defining} - grown = True - while grown: - grown = False - for path, other in censuses.items(): - if path not in providers and any( - other.reexports(name, provider) for provider in providers - ): - providers.add(path) - grown = True - for path, other in sorted(censuses.items()): - if path != defining and any(other.uses(name, provider) for provider in providers): + # A subclass is read where it is used. The defining module's reader names + # an instance it builds there; every other module that uses the class is + # named here. A subclass nothing uses is no agent (#876 review). + defined = ( + ("an OpenAI Agents SDK", {path: census.subclasses for path, census in censuses.items()}), + ("a Google ADK", adk_subclasses), + ) + for framework, table in defined: + for defining, classes in sorted(table.items()): + for name, line in sorted(classes.items()): + for path in _subclass_users(name, defining, censuses): result.gap( - f"{path} uses {name}, an OpenAI Agents SDK agent subclass defined at " + f"{path} uses {name}, {framework} agent subclass defined at " f"{defining}:{line}; agents built from it are not read.", source=path, ) - result.sources = [{"type": s.type, "path": s.path} for s in sources] - for source in sources: - _observe_source(result, root, source) - return result + + +def _subclass_users(name: str, defining: str, censuses: dict[str, Any]) -> list[str]: + """Every other module whose code uses ``name`` from ``defining``. + + A package that re-exports the class (``from app.core import *``) provides + it too, transitively. + """ + + providers = {defining} + grown = True + while grown: + grown = False + for path, other in censuses.items(): + if path not in providers and any( + other.reexports(name, provider) for provider in providers + ): + providers.add(path) + grown = True + return [ + path + for path, other in sorted(censuses.items()) + if path != defining and any(other.uses(name, provider) for provider in providers) + ] def _plain_scope_class(built: tuple[str, str] | None, censuses: dict[str, Any]) -> bool: @@ -1206,17 +1283,27 @@ def in_scope(path: str, selected: str = selected_scope) -> bool: # Each side's imports are read against its own commit's tree: the # materialized scope alone cannot say whether ``from common.patches # import ...`` is the application's code or an installed package. - with repository_layout(_git_layout(workspace, base_commit, old_scope)): - old = observe( + old_layout = _git_layout(workspace, base_commit, old_scope) + new_layout = _git_layout(workspace, head_commit, scope) + with repository_layout(old_layout): + old_found = discover( scratch / "base", old_scope, max_python_files=max_python_files, gitlinks=gitlinks["base"], ) - with repository_layout(_git_layout(workspace, head_commit, scope)): - new = observe( + with repository_layout(new_layout): + new_found = discover( scratch / "head", scope, max_python_files=max_python_files, gitlinks=gitlinks["head"] ) + # The census names what can change an agent its reader did not read. + # With no agent source on either side there is no agent to qualify + # (#876 review). + census = bool(old_found.entries or new_found.entries) + with repository_layout(old_layout): + old = observe(old_found, max_python_files=max_python_files, census=census) + with repository_layout(new_layout): + new = observe(new_found, max_python_files=max_python_files, census=census) if old.status == new.status == "absent": raise ConfigError( f"Neither comparison tree contains the selected scopes: " diff --git a/src/agents_shipgate/inputs/google_adk.py b/src/agents_shipgate/inputs/google_adk.py index 97dd12d9..5da0f201 100644 --- a/src/agents_shipgate/inputs/google_adk.py +++ b/src/agents_shipgate/inputs/google_adk.py @@ -897,12 +897,18 @@ class _AdkAgentBinding: tool_issues: dict[str, str] = field(default_factory=dict) #: Why this agent's tool list is incomplete, when it is. issues: list[str] = field(default_factory=list) + #: Every ``(tool, locator)`` one construction of this name asked to bind, + #: bound before or not, while ``extract`` reads that construction (#876 + #: review); None between constructions. + recording: list[tuple[str, str | None]] | None = field(default=None, repr=False) def bind( self, tool_name: str, locator: str | None = None, location: str | None = None ) -> bool: """Add one tool to this agent; return False if it was already bound.""" + if self.recording is not None: + self.recording.append((tool_name, locator)) if tool_name in self.tool_names or tool_name in self.duplicated: return False self.tool_names.append(tool_name) @@ -1024,8 +1030,9 @@ def __init__( # ordinal within one agent's tool list, never a line number. self.inline_slot_counts: dict[str, int] = {} self.agent_bindings: dict[str, _AdkAgentBinding] = {} - #: ``agent name -> {id(call): line}`` for every construction site. - self.agent_sites: dict[str, dict[int, int]] = {} + #: ``agent name -> {id(call): (line, signature)}`` for every construction + #: site with a literal tool list; equal signatures bind the same (#876). + self.agent_sites: dict[str, dict[int, tuple[int, object]]] = {} # Reasons this module's tool surface was not proven complete (#393). # Empty at the end of ``extract`` is what earns ``SURFACE_ENUMERATED``. self.surface_gaps: list[str] = [] @@ -1073,7 +1080,9 @@ def extract(self) -> list[LoadedToolSource]: "tool_count": tool_count, } ) + handoffs_at = len(self.artifacts.sub_agents) self._record_agent_callbacks_plugins_subagents(call, agent_name) + handoffs = self.artifacts.sub_agents[handoffs_at:] if not isinstance(tools_expr, (ast.List, ast.Tuple)): if tools_expr is not None: self._surface_warning( @@ -1091,10 +1100,10 @@ def extract(self) -> list[LoadedToolSource]: ) continue binding = self._binding_for(agent_name, call) - for item in tools_expr.elts: - loaded_sources.extend( - self._extract_tool_expr(item, tools, agent_name, binding) - ) + loaded_sources.extend( + self._read_construction(call, agent_name, binding, tools_expr, tools, handoffs) + ) + self._record_duplicate_constructions() self._record_agent_subclasses() self._resolve_extraction_evidence(warnings_before, loaded_sources) return [ @@ -1108,38 +1117,121 @@ def extract(self) -> list[LoadedToolSource]: *loaded_sources, ] + def _read_construction( + self, + call: ast.Call, + agent_name: str, + binding: _AdkAgentBinding, + tools_expr: ast.List | ast.Tuple, + tools: list[Tool], + handoffs: list[dict[str, Any]], + ) -> list[LoadedToolSource]: + """Read one construction's tools, and note what it binds as its site. + + Every construction of one name shares a binding, so a second one's + tools are recorded as it asks for them, bound before or not. A site is + comparable only when it was read cleanly — every tool a definition, no + warning, no new issue, no toolset, no ``**`` and no unresolved or + dynamic handoff; any other site differs from every site (#876 review). + """ + + warnings_at = len(self.artifacts.warnings) + state_at = (list(binding.issues), dict(binding.tool_issues), set(binding.duplicated)) + binding.recording = [] + loaded: list[LoadedToolSource] = [] + try: + for item in tools_expr.elts: + loaded.extend(self._extract_tool_expr(item, tools, agent_name, binding)) + finally: + recorded, binding.recording = binding.recording, None + clean = ( + not loaded + and all(locator is not None for _, locator in recorded or ()) + and len(self.artifacts.warnings) == warnings_at + and (list(binding.issues), dict(binding.tool_issues), set(binding.duplicated)) + == state_at + and not call.args + and all(keyword.arg is not None for keyword in call.keywords) + and all( + handoff.get("sub_agent_count") is not None + and not handoff.get("unresolved_sub_agents") + for handoff in handoffs + ) + ) + signature: object = ( + ( + frozenset(recorded or ()), + tuple(sorted(name for handoff in handoffs for name in handoff["sub_agents"])), + ) + if clean + else call + ) + self.agent_sites.setdefault(agent_name, {})[id(call)] = (call.lineno, signature) + return loaded + + def _record_duplicate_constructions(self) -> None: + """Name an agent whose constructions in this module differ (#876). + + The binding graph merges every construction of one name, so which one + binds which tool is not established. Constructions that bind exactly + the same definitions and handoffs, each read cleanly, are one agent + (#876 review): ``root_agent`` and a builder returning its twin. + """ + + for agent_name, sites in self.agent_sites.items(): + if len(sites) < 2 or len({signature for _, signature in sites.values()}) == 1: + continue + lines = ", ".join(str(line) for line in sorted(line for line, _ in sites.values())) + reason = ( + f"Google ADK agent {agent_name!r} is constructed more than once in " + f"{self.source_ref} (lines {lines}); its tools are not attributed to " + "either construction." + ) + self._surface_warning(reason, SURFACE_GAP_DUPLICATE_AGENT_NAME) + self.agent_bindings[agent_name].issues.append(reason) + def _record_agent_subclasses(self) -> None: - """Name each class deriving from an ADK agent class (#876; ported from PR #880). + """Name each class deriving from an ADK agent class this module uses (#876). ``extract`` reads every ``Agent(...)`` call; an instance of a subclass is not one, so its wiring is a named limit rather than silently absent. - A base is ADK's only while its root name is bound by nothing but an - import, as ``_framework_symbol_is_proven`` requires of a call. + A class this module never names again — no call, ``partial`` or other + reference beyond being a deeper subclass's base, and no decorator that + could build it — is no agent here (#876 review); a module that imports + it is named by the comparison's census instead, as for an SDK subclass. """ - for node in ast.walk(self.tree): - if not isinstance(node, ast.ClassDef): + subclasses = adk_agent_subclasses(self.tree, self.aliases, self.name_bindings) + if not subclasses: + return + # A deeper subclass's base names its parent without building it; the + # deeper class is itself counted by its own uses. + bases = { + id(base) + for node in ast.walk(self.tree) + if isinstance(node, ast.ClassDef) and node.name in subclasses + for base in node.bases + } + referenced = { + node.id + for node in ast.walk(self.tree) + if isinstance(node, ast.Name) + and isinstance(node.ctx, ast.Load) + and id(node) not in bases + } + decorated = { + node.name + for node in ast.walk(self.tree) + if isinstance(node, ast.ClassDef) and node.decorator_list + } + for name, line in sorted(subclasses.items(), key=lambda item: item[1]): + if name not in referenced and name not in decorated: continue - for base in node.bases: - expression = base.value if isinstance(base, ast.Subscript) else base - root = expression - while isinstance(root, ast.Attribute): - root = root.value - if ( - isinstance(root, ast.Name) - and _qualified_name(expression, self.aliases) in AGENT_CLASS_NAMES - and all( - isinstance(binding, ast.alias) - for binding in self.name_bindings.get(root.id, []) - ) - ): - self._surface_warning( - f"Google ADK agent class {node.name!r} at {self.source_ref}:" - f"{node.lineno} derives from an agent class; agents built from it " - "are not read.", - SURFACE_GAP_AGENT_SUBCLASS, - ) - break + self._surface_warning( + f"Google ADK agent class {name!r} at {self.source_ref}:{line} derives " + "from an agent class; agents built from it are not read.", + SURFACE_GAP_AGENT_SUBCLASS, + ) def _surface_warning(self, message: str, reason: str) -> None: """Report a construct that leaves part of this module's surface unknown.""" @@ -1445,18 +1537,6 @@ def _binding_for(self, agent_name: str, call: ast.Call) -> _AdkAgentBinding: source_pointer=f"{self.source_ref}:{call.lineno}", ) self.agent_bindings[agent_name] = binding - sites = self.agent_sites.setdefault(agent_name, {}) - if id(call) not in sites: - sites[id(call)] = call.lineno - if len(sites) == 2: - lines = ", ".join(str(line) for line in sorted(sites.values())) - reason = ( - f"Google ADK agent {agent_name!r} is constructed more than once in " - f"{self.source_ref} (lines {lines}); its tools are not attributed to " - "either construction." - ) - self._surface_warning(reason, SURFACE_GAP_DUPLICATE_AGENT_NAME) - binding.issues.append(reason) return binding def _binding_observations(self) -> list[AgentBindingObservation]: @@ -2840,6 +2920,51 @@ def _qualified_name(node: ast.AST, aliases: dict[str, str]) -> str | None: return None +def adk_agent_subclasses( + tree: ast.Module, + aliases: dict[str, str] | None = None, + name_bindings: dict[str, list[ast.AST]] | None = None, +) -> dict[str, int]: + """Classes deriving from a Google ADK agent class, by name, with their line (#876). + + Transitively within the module: ``class Deeper(Helper)`` is one too. A + base is ADK's only while its root name is bound by nothing but an import, + as ``_framework_symbol_is_proven`` requires of a call. + """ + + aliases = _import_aliases(tree) if aliases is None else aliases + name_bindings = _name_binding_occurrences(tree) if name_bindings is None else name_bindings + classes = [node for node in ast.walk(tree) if isinstance(node, ast.ClassDef)] + found: dict[str, int] = {} + grown = True + while grown: + grown = False + for node in classes: + if node.name in found: + continue + for base in node.bases: + expression = base.value if isinstance(base, ast.Subscript) else base + root = expression + while isinstance(root, ast.Attribute): + root = root.value + if isinstance(expression, ast.Name) and expression.id in found: + derives = True + else: + derives = ( + isinstance(root, ast.Name) + and _qualified_name(expression, aliases) in AGENT_CLASS_NAMES + and all( + isinstance(binding, ast.alias) + for binding in name_bindings.get(root.id, []) + ) + ) + if derives: + found[node.name] = node.lineno + grown = True + break + return found + + def _simple_target_name(targets: list[ast.expr]) -> str | None: if len(targets) != 1: return None diff --git a/tests/test_application_diff_unobserved.py b/tests/test_application_diff_unobserved.py index 6098a4a5..b8c4f35d 100644 --- a/tests/test_application_diff_unobserved.py +++ b/tests/test_application_diff_unobserved.py @@ -999,3 +999,227 @@ def test_an_adk_agent_subclass_is_a_named_limit(repo): "agent.py:7" in g["reason"] and "'Helper'" in g["reason"] for g in result["head"]["coverage_gaps"] ) + + +# --------------------------------------------------------------------------- +# #876 review, final pass: a Google ADK subclass counts only where it is used, +# identical constructions of one ADK name are one agent, and a scope with no +# agent source on either side stays `not_established`. + +_ADK = ( + "from google.adk.agents import LlmAgent\n" + "def lookup(query: str) -> dict:\n" + ' """Look up one catalog record by its identifier and return its fields."""\n' + ' return {"query": query}\n' + "def search(query: str) -> dict:\n" + ' """Search the public catalog index for records whose title matches."""\n' + ' return {"query": query}\n' +) + + +def _scan_decision(tmp_path, source: str) -> str: + import json + + from typer.testing import CliRunner + + from agents_shipgate.cli.main import app + + (tmp_path / "agent.py").write_text(source) + (tmp_path / "shipgate.yaml").write_text( + 'version: "0.1"\nproject:\n name: p\nagent:\n name: a\n' + " declared_purpose:\n - look things up\nenvironment:\n target: local\n" + "tool_sources:\n - id: src\n type: google_adk\n path: agent.py\n" + "action_surface:\n actions:\n" + " - tool: lookup\n effect: read\n authority:\n mode: none\n" + " - tool: search\n effect: read\n authority:\n mode: none\n" + ) + out = tmp_path / "reports" + result = CliRunner().invoke( + app, ["scan", "-c", str(tmp_path / "shipgate.yaml"), "--out", str(out), "--format", "json"] + ) + assert result.exit_code == 0, result.output + return json.loads((out / "report.json").read_text())["release_decision"]["decision"] + + +_UNUSED_HELPER = ( + 'root_agent = LlmAgent(name="root", model="m", tools=TOOLS)\n' + "class Helper(LlmAgent):\n pass\n" +) + + +def test_an_adk_subclass_nothing_uses_is_not_a_limit(repo, tmp_path): + # The unused class made the module `partial`, hid the root agent's true + # row, and dropped `scan` to insufficient_evidence (#876 review). + base = commit(repo, {"agent.py": _ADK + _UNUSED_HELPER.replace("TOOLS", "[lookup]")}) + head = commit(repo, {"agent.py": _ADK + _UNUSED_HELPER.replace("TOOLS", "[lookup, search]")}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared", result["head"]["limits"] + assert _pairs(result) == [("root", "search", "added")] + scan_dir = tmp_path / "scan" + scan_dir.mkdir() + source = _ADK + _UNUSED_HELPER.replace("TOOLS", "[lookup, search]") + assert _scan_decision(scan_dir, source) == "passed" + + +@pytest.mark.parametrize( + ("use", "named"), + [ + ('helper = Helper(name="helper", model="m", tools=TOOLS)\n', "Helper"), + ( + "import functools\nmake = functools.partial(Helper, model='m')\n" + 'helper = make(name="helper", tools=TOOLS)\n', + "Helper", + ), + ( + 'class Deeper(Helper):\n pass\nhelper = Deeper(name="helper", model="m", tools=TOOLS)\n', + "Deeper", + ), + ('def build():\n return Helper(name="helper", model="m", tools=TOOLS)\n', "Helper"), + ("@register\nclass Registered(LlmAgent):\n pass\n", "Registered"), + ], + ids=["instantiated", "partial", "subclassed", "in-a-builder", "decorated"], +) +def test_an_adk_subclass_the_module_uses_is_a_limit(repo, use, named): + body = ( + _ADK + + "register = list\n" + + 'root_agent = LlmAgent(name="root", model="m", tools=[lookup])\n' + + "class Helper(LlmAgent):\n pass\n" + + use + ) + base = commit(repo, {"agent.py": body.replace("TOOLS", "[lookup]")}) + head = commit(repo, {"agent.py": body.replace("TOOLS", "[lookup, search]")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any(f"'{named}'" in limit for limit in result["head"]["limits"]), result["head"][ + "limits" + ] + + +def test_an_unused_chain_of_adk_subclasses_is_not_a_limit(repo): + chain = "class Deeper(Helper):\n pass\n" + base = commit(repo, {"agent.py": _ADK + _UNUSED_HELPER.replace("TOOLS", "[lookup]") + chain}) + head = commit( + repo, {"agent.py": _ADK + _UNUSED_HELPER.replace("TOOLS", "[lookup, search]") + chain} + ) + result = run(repo, base, head) + assert result["comparison_status"] == "compared", result["head"]["limits"] + assert _pairs(result) == [("root", "search", "added")] + + +@pytest.mark.parametrize( + ("wiring", "named"), + [ + ( + 'from agent import Helper, lookup\nhelper = Helper(name="helper", model="m", tools=[lookup])\n', + "Helper", + ), + ( + 'import agent\nhelper = agent.Helper(name="helper", model="m", tools=[agent.lookup])\n', + "Helper", + ), + ( + 'from agent import Deeper, lookup\nhelper = Deeper(name="helper", model="m", tools=[lookup])\n', + "Deeper", + ), + ], + ids=["imported-by-name", "module-attribute", "deeper-subclass"], +) +def test_an_adk_subclass_another_module_uses_is_a_limit_there(repo, wiring, named): + agent = _ADK + _UNUSED_HELPER.replace("TOOLS", "[lookup]") + "class Deeper(Helper):\n pass\n" + base = commit(repo, {"agent.py": agent, "make.py": wiring}) + head = commit(repo, {"make.py": wiring + "# touched\n"}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any( + g["source"] == "make.py" and f"uses {named}, a Google ADK" in g["reason"] + for g in result["head"]["coverage_gaps"] + ), result["head"]["coverage_gaps"] + # The defining module builds no instance of either class itself. + assert not any(g["source"] == "agent.py" for g in result["head"]["coverage_gaps"]) + + +_TWICE = ( + 'root_agent = LlmAgent(name="rev", model="m", tools=TOOLS)\n' + "def mk():\n" + ' return LlmAgent(name="rev", model="m", tools=TOOLS)\n' +) + + +def test_identical_adk_constructions_of_one_name_are_one_agent(repo, tmp_path): + # Two identical constructions were named "constructed more than once", + # which hid the true row and failed `scan` (#876 review). + base = commit(repo, {"agent.py": _ADK + _TWICE.replace("TOOLS", "[lookup]")}) + head = commit(repo, {"agent.py": _ADK + _TWICE.replace("TOOLS", "[lookup, search]")}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared", result["head"]["limits"] + assert _pairs(result) == [("rev", "search", "added")] + scan_dir = tmp_path / "scan" + scan_dir.mkdir() + assert _scan_decision(scan_dir, _ADK + _TWICE.replace("TOOLS", "[lookup, search]")) == "passed" + + +@pytest.mark.parametrize( + "second", + [ + ' return LlmAgent(name="rev", model="m", tools=[search])\n', + ' child = LlmAgent(name="child", model="m", tools=[search])\n' + ' return LlmAgent(name="rev", model="m", tools=[lookup], sub_agents=[child])\n', + ' return LlmAgent(name="rev", model="m", tools=[lookup], **OPTIONS)\n', + " from other import lookup\n" + ' return LlmAgent(name="rev", model="m", tools=[lookup])\n', + ], + ids=["other-tools", "other-handoffs", "keyword-unpacking", "other-definition"], +) +def test_differing_adk_constructions_of_one_name_stay_a_limit(repo, tmp_path, second): + body = ( + _ADK + + "OPTIONS = {}\n" + + 'root_agent = LlmAgent(name="rev", model="m", tools=[lookup])\n' + + "def mk():\n" + + second + ) + other = "def lookup(query: str) -> str:\n return query + '!'\n" + base = commit(repo, {"agent.py": body.replace('"rev"', '"solo"', 1), "other.py": other}) + head = commit(repo, {"agent.py": body}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert any( + "constructed more than once" in limit for limit in result["head"]["limits"] + ), result["head"]["limits"] + scan_dir = tmp_path / "scan" + scan_dir.mkdir() + (scan_dir / "other.py").write_text(other) + assert _scan_decision(scan_dir, body) == "insufficient_evidence" + + +_NO_AGENTS = { + "app/__init__.py": "", + "app/model.py": "class Artifacts:\n tools: list = []\n", + "app/report.py": ( + "from app.model import Artifacts\n\n\n" + "def record(artifacts, item):\n artifacts.tools.append(item)\n\n\n" + "def copy(artifacts, extra):\n return artifacts.clone(tools=extra)\n" + ), +} + + +def test_a_scope_with_no_agent_source_on_either_side_is_not_established(repo): + # `--scope src` of this repository read `partial` from the census of its + # own `artifacts.tools.append(...)`, with no agent source anywhere. + base = commit(repo, dict(_NO_AGENTS)) + head = commit(repo, {"app/report.py": _NO_AGENTS["app/report.py"] + "# touched\n"}) + result = run(repo, base, head, "--scope", "app") + assert result["comparison_status"] == "not_established", result["head"]["limits"] + assert result["head"]["limits"] == [] and result["base"]["limits"] == [] + + +def test_the_census_still_runs_when_either_side_has_an_agent_source(repo): + base = commit(repo, dict(_NO_AGENTS)) + head = commit( + repo, {"app/agent.py": _agents('quote_agent = Agent(name="Quote", tools=[quote])\n')} + ) + result = run(repo, base, head, "--scope", "app") + assert result["comparison_status"] == "partial" + assert any("report.py:5" in limit for limit in result["head"]["limits"]) + assert any("report.py:5" in limit for limit in result["base"]["limits"])