From a5e39723cefc6606ca7de9ffec5c2de540625ea5 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Fri, 25 Sep 2026 14:22:30 -0700 Subject: [PATCH 1/2] fix(diff --application): key SDK identity on imports; unobserved agents are not removals On speechmatics/speechmatics-academy#142, a LiveKit voice agent moved its tools into an Agent subclass that passes them through super().__init__. `diff --application` printed `compared` with four false REMOVED rows. Two defects: - Framework identity. Discovery counted any bare `@function_tool` as the OpenAI Agents SDK, and the SDK reader recognized `function_tool` and `Agent` by spelling alone, so `livekit.agents` symbols were read as the SDK's. Both now key on import provenance: the absolute `agents` / `openai_agents` package, or an unimported name (unless a foreign wildcard could supply it). Relative imports are no framework's signal. - Unobserved is not removed. An agent observed on one side whose file on the other side still assigns its name (or passes it as `name=`), via a construction no reader supports (subclass, factory, Agent[Ctx], clone), now records a scoped coverage gap for that agent: `partial`, rows `not_established`. Handoff-only references do not count as an observed construction. A genuinely deleted agent stays an established removal. Regression tests fail on the unfixed tree. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 + docs/application-comparison.md | 14 ++ docs/distribution-surfaces.md | 2 +- src/agents_shipgate/cli/application_diff.py | 74 +++++- src/agents_shipgate/cli/discovery/signals.py | 35 ++- .../inputs/openai_sdk_static.py | 84 +++++-- tests/test_application_diff_identity.py | 217 ++++++++++++++++++ 7 files changed, 406 insertions(+), 23 deletions(-) create mode 100644 tests/test_application_diff_identity.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 35db9261..588b3643 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ ### Application comparison without prior setup - Add `diff --application` for OpenAI Agents SDK and Google ADK source-observed per-agent wiring, with exact base/head refs and independently selected scopes. No manifest, saved baseline or authored declarations are needed. The advisory `application_comparison_schema_version: "0.1"` result records before/after evidence, scoped coverage gaps and explicit uncertain candidates; it supplies no release verdict or merge permission. Includes scoped Git materialization, partial-clone recovery, and definition lookup using reader-resolved Python symbols and locations. See [application comparison](docs/application-comparison.md). (#871) +- `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). 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 its name, 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. ### Changes diff --git a/docs/application-comparison.md b/docs/application-comparison.md index 527cfa0d..47e979be 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -66,6 +66,20 @@ 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. +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 the agent's name, 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. + +Framework identity follows the import, not the spelling. `Agent` and +`function_tool` are the OpenAI Agents SDK's only when imported from the absolute +`agents`/`openai_agents` package or not imported at all; LiveKit's +`livekit.agents` exports the same names and is not read as the SDK, and a +relative `.agents` import is the project's own package. + Discovery is bounded by `--max-python-files` (default 1000) and a 2 MB per-Python file limit. Partial discovery remains visible. The first increment uses existing SDK/ADK readers; unresolved imports, dynamic factories and built-ins remain diff --git a/docs/distribution-surfaces.md b/docs/distribution-surfaces.md index 73ee6e7e..fe077ca7 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. `tests/test_application_diff.py` and `tests/test_application_diff_review.py` prove the advisory boundary, isolation, uncertainty and evidence identity. | +| `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. | | `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/src/agents_shipgate/cli/application_diff.py b/src/agents_shipgate/cli/application_diff.py index ad9d8e37..646333df 100644 --- a/src/agents_shipgate/cli/application_diff.py +++ b/src/agents_shipgate/cli/application_diff.py @@ -59,6 +59,8 @@ class Observations: scope: str status: str = "complete" agents: dict[tuple[str, str], dict[str, Any]] = field(default_factory=dict) + # Agents known only as a handoff target: their own construction was not read. + handoff_only: set[tuple[str, str]] = field(default_factory=set) bindings: dict[tuple[str, str, str], dict[str, Any]] = field(default_factory=dict) limits: list[str] = field(default_factory=list) sources: list[dict[str, str]] = field(default_factory=list) @@ -266,8 +268,12 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) if artifacts is not None: bag.set("google_adk", artifacts) attributed = set() + constructed, handoff_targets = set(), set() for item in loaded: for observation in item.binding_observations: + path = _source_path(root, observation.source) + constructed.add((path, observation.agent)) + handoff_targets.update((path, name) for name in observation.handoff_names) if not observation.tools_complete or not observation.handoffs_complete: for message in observation.issues or ["Incomplete observed binding list."]: result.gap( @@ -287,6 +293,7 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) if warning not in attributed: result.gap(warning, source=source.path) attributed.add(warning) + result.handoff_only |= handoff_targets - constructed tools, warnings = _build_canonical_tools(loaded) for warning in warnings: result.gap(warning, source=source.path) @@ -516,6 +523,67 @@ def _align_exact_moves( ] +def _still_named_at(root: Path, path: str, name: str) -> int | None: + """First line at which ``path`` still assigns ``name`` or passes ``name=name``. + + Those are the two agent identities the readers key on: the SDK's assigned + variable and ADK's ``name=`` literal. + """ + file = root / path + if not path or not file.is_file() or not file.resolve().is_relative_to(root.resolve()): + return None + try: + tree = ast.parse(file.read_bytes()) + except (SyntaxError, ValueError, RecursionError, OSError): + return None # observe() already recorded this file as a gap. + lines = [ + node.lineno + for node in ast.walk(tree) + if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store) and node.id == name + ] + [ + node.value.lineno + for node in ast.walk(tree) + if isinstance(node, ast.keyword) + and node.arg == "name" + and isinstance(node.value, ast.Constant) + and node.value.value == name + ] + return min(lines, default=None) + + +def _unobserved_agent_gaps( + old: Observations, new: Observations, base_root: Path, head_root: Path, moves: dict[str, str] +) -> None: + """An agent observed on one side is absent from the other only if it is gone. + + Readers see only the constructions they support. An ``Agent`` subclass + passing ``tools`` through ``super().__init__``, a factory, ``Agent[Ctx]`` + or ``.clone()`` keeps the agent while hiding its wiring. When the other + side's file still names the agent, its bindings there are unobserved, + never removed or newly added. Called after ``_align_exact_moves``, so + ``old.bindings`` are keyed by head paths. + """ + unmoves = {head: base for base, head in moves.items()} + old_agents = { + (moves.get(path, path), name) for path, name in old.agents.keys() - old.handoff_only + } + for side, root, agents, bindings, to_side in ( + (new, head_root, new.agents.keys() - new.handoff_only, old.bindings, lambda path: path), + (old, base_root, old_agents, new.bindings, lambda path: unmoves.get(path, path)), + ): + for path, name in sorted({key[:2] for key in bindings} - agents): + line = _still_named_at(root, to_side(path), name) + if line is not None: + side.gap( + f"{to_side(path)}:{line} still names agent {name!r}, but no supported " + "agent construction was observed for it (for example an Agent " + "subclass, factory or clone); its bindings on this side are not " + "established.", + source=to_side(path), + agent=name, + ) + + def _location(scope: str, path: str | None) -> str | None: if path is None: return None @@ -600,7 +668,11 @@ def in_scope(path: str, selected: str = selected_scope) -> bool: f"base={old_scope!r}, head={scope!r}. Check --scope/--base-scope." ) moves = _align_exact_moves(workspace, base_commit, head_commit, old, new) - rows = compare(old, new, target_moves={m["base_source"]: m["head_source"] for m in moves}) + target_moves = {m["base_source"]: m["head_source"] for m in moves} + _unobserved_agent_gaps( + old, new, scratch / "base" / old_scope, scratch / "head" / scope, target_moves + ) + rows = compare(old, new, target_moves=target_moves) for row in rows: row["before"] = _published_binding(row["before"], old_scope) row["after"] = _published_binding(row["after"], scope) diff --git a/src/agents_shipgate/cli/discovery/signals.py b/src/agents_shipgate/cli/discovery/signals.py index ca0241e2..cf856da4 100644 --- a/src/agents_shipgate/cli/discovery/signals.py +++ b/src/agents_shipgate/cli/discovery/signals.py @@ -936,7 +936,9 @@ def _parse_python_facts(path: Path, workspace: Path) -> _PyFacts | None: alias.name if alias.asname else alias.name.split(".")[0] ) elif isinstance(node, ast.ImportFrom): - if node.module: + # `from .agents import x` names this project's own package, never + # an installed framework, so only absolute imports are signals. + if node.module and not node.level: facts.imports.add(node.module) for alias in node.names: facts.imports.add(f"{node.module}.{alias.name}") @@ -1685,13 +1687,42 @@ def _score_python_signals_inner( 2.0, "strong", f"{fact.rel_path}: openai-agents import" ) scores["openai_agents_sdk"].add_file(fact.rel_path) - if fact.decorators & OPENAI_AGENTS_SDK_DECORATORS: + if any( + _sdk_spelling(fact, decorator) + for decorator in fact.decorators & OPENAI_AGENTS_SDK_DECORATORS + ): scores["openai_agents_sdk"].add( 2.0, "strong", f"{fact.rel_path}: @function_tool decorator" ) scores["openai_agents_sdk"].add_file(fact.rel_path) +def _sdk_spelling(fact: _PyFacts, spelling: str) -> bool: + """Whether an SDK-shaped decorator spelling denotes the SDK's own. + + LiveKit exports ``function_tool`` from ``livekit.agents`` too, so the + spelling alone proves nothing. Its head decides: imported from the + absolute ``agents``/``openai_agents`` package, or not imported at all + (the terminal-name reading, unless a wildcard could supply it), it is the + SDK's; imported only from anywhere else — ``livekit.agents``, a relative + ``.agents`` — it is not. + """ + head = spelling.split(".", 1)[0] + origins = [ + (module, level) + for (_scope, bound), (module, level, _original) in fact.constant_imports.items() + if bound == head + ] + if head in fact.plain_imports: + origins.append((fact.plain_imports[head], 0)) + if not origins: + return not fact.star_import + return any( + not level and module.split(".", 1)[0] in OPENAI_AGENTS_SDK_IMPORT_MODULES + for module, level in origins + ) + + def _collect_package_tokens(workspace: Path) -> list[str]: tokens: list[str] = [] pyproject = workspace / "pyproject.toml" diff --git a/src/agents_shipgate/inputs/openai_sdk_static.py b/src/agents_shipgate/inputs/openai_sdk_static.py index 31f6f9df..6a965398 100644 --- a/src/agents_shipgate/inputs/openai_sdk_static.py +++ b/src/agents_shipgate/inputs/openai_sdk_static.py @@ -40,9 +40,11 @@ ToolSourceConfig, ) +SDK_MODULES = frozenset({"agents", "openai_agents"}) DEFAULT_FUNCTION_TOOL_DECORATORS = frozenset( {"function_tool", "agents.function_tool", "openai_agents.function_tool"} ) +DEFAULT_AGENT_CONSTRUCTORS = frozenset({"Agent", "agents.Agent", "openai_agents.Agent"}) def load_openai_sdk_static_tools( @@ -186,6 +188,7 @@ def _extract_agent_bindings( for path in paths: tree = parse_python_file(path, label="OpenAI Agents SDK") source_ref = display_path(path, base_dir) + sdk_names = _SdkNames(tree) list_vars: dict[str, list[str] | None] = {} import_aliases: dict[str, str] = {} for node in ast.walk(tree): @@ -202,7 +205,13 @@ def _extract_agent_bindings( continue target = _assignment_target(node) call = node.value if isinstance(node.value, ast.Call) else None - if not target or call is None or _last_name(call.func) != "Agent": + if ( + not target + or call is None + or not sdk_names.denotes( + dotted_name(call.func), "Agent", DEFAULT_AGENT_CONSTRUCTORS + ) + ): continue tools_expr = _keyword(call, "tools") names = _resolve_name_list(tools_expr, list_vars, import_aliases) @@ -291,14 +300,6 @@ def _assignment_target(node: ast.Assign | ast.AnnAssign) -> str | None: return targets[0].id if len(targets) == 1 and isinstance(targets[0], ast.Name) else None -def _last_name(node: ast.AST) -> str | None: - if isinstance(node, ast.Name): - return node.id - if isinstance(node, ast.Attribute): - return node.attr - return None - - def _keyword(call: ast.Call, name: str) -> ast.AST | None: return next((item.value for item in call.keywords if item.arg == name), None) @@ -330,18 +331,63 @@ def _resolve_name_list( return None +class _SdkNames: + """Which spellings in one module denote the SDK's own symbols. + + ``livekit.agents`` also exports ``Agent`` and ``function_tool``, so the + spelling alone proves nothing. A spelling's head decides: imported from + the absolute ``agents``/``openai_agents`` package, it is the SDK's; not + imported at all, the default spellings keep their terminal-name reading + unless a wildcard from another module could supply them; imported only + from anywhere else — ``livekit.agents``, a relative ``.agents`` — it is + another library's symbol and never read as the SDK's. + """ + + def __init__(self, tree: ast.Module) -> None: + self.origins: dict[str, set[str]] = {} + self.foreign_wildcard = False + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom): + module = "." * node.level + (node.module or "") + for alias in node.names: + path = f"{module}.{alias.name}" if node.module else module + alias.name + if alias.name == "*": + self.foreign_wildcard |= not _is_sdk_path(module) + else: + self.origins.setdefault(alias.asname or alias.name, set()).add(path) + elif isinstance(node, ast.Import): + for alias in node.names: + head = alias.name.split(".", 1)[0] + bound, path = (alias.asname, alias.name) if alias.asname else (head, head) + self.origins.setdefault(bound, set()).add(path) + + def denotes(self, spelling: str | None, symbol: str, defaults: frozenset[str]) -> bool: + if not spelling: + return False + head, _, rest = spelling.partition(".") + origins = self.origins.get(head) + if origins is None: + return spelling in defaults and not self.foreign_wildcard + return any( + _is_sdk_path(path) and (f"{path}.{rest}" if rest else path).rsplit(".", 1)[-1] == symbol + for path in origins + ) + + +def _is_sdk_path(path: str) -> bool: + return not path.startswith(".") and path.split(".", 1)[0] in SDK_MODULES + + def _function_tool_decorator_names(tree: ast.Module) -> set[str]: - names = set(DEFAULT_FUNCTION_TOOL_DECORATORS) + names = _SdkNames(tree) + spellings = set() for node in ast.walk(tree): - if isinstance(node, ast.ImportFrom) and node.module in {"agents", "openai_agents"}: - for alias in node.names: - if alias.name == "function_tool": - names.add(alias.asname or alias.name) - elif isinstance(node, ast.Import): - for alias in node.names: - if alias.name in {"agents", "openai_agents"}: - names.add(f"{alias.asname or alias.name}.function_tool") - return names + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + for decorator in node.decorator_list: + spelling = _decorator_name(decorator) + if names.denotes(spelling, "function_tool", DEFAULT_FUNCTION_TOOL_DECORATORS): + spellings.add(spelling) + return spellings def _is_function_tool(node: ast.AST, decorator_names: set[str]) -> bool: diff --git a/tests/test_application_diff_identity.py b/tests/test_application_diff_identity.py new file mode 100644 index 00000000..79550cac --- /dev/null +++ b/tests/test_application_diff_identity.py @@ -0,0 +1,217 @@ +"""Framework identity and unobserved agents must never read as removals. + +Regressions from speechmatics/speechmatics-academy#142: a LiveKit agent was +read as the OpenAI Agents SDK (its ``function_tool`` and ``Agent`` share the +SDK's spellings), and its refactor into an ``Agent`` subclass then reported +every bound tool as REMOVED under ``compared``. +""" + +import pytest +from test_application_diff import SDK, commit, run +from test_application_diff import repo as repo + +from agents_shipgate.cli.discovery import detect_workspace +from agents_shipgate.inputs.openai_sdk_static import load_openai_sdk_static_tools +from agents_shipgate.schemas.manifest import ToolSourceConfig + +# Reduced from the PR's main.py: tools are closures inside the entrypoint, and +# the head passes them to an Agent subclass through ``super().__init__``. +LIVEKIT_BASE = """from livekit import agents +from livekit.agents import Agent, AgentSession, function_tool + + +async def entrypoint(ctx: agents.JobContext) -> None: + @function_tool + async def focus_on_speaker(speaker_ids: list[str]) -> str: + return "focused" + + @function_tool + async def ignore_speaker(speaker_id: str) -> str: + return "ignored" + + agent = Agent(instructions="focus", tools=[focus_on_speaker, ignore_speaker]) + await AgentSession().start(agent=agent, room=ctx.room) +""" +LIVEKIT_HEAD = """from livekit import agents +from livekit.agents import Agent, AgentSession, function_tool + + +class FocusAgent(Agent): + def __init__(self, tools: list) -> None: + super().__init__(instructions="focus", tools=tools) + + +async def entrypoint(ctx: agents.JobContext) -> None: + @function_tool + async def focus_on_speaker(speaker_ids: list[str]) -> str: + return "focused" + + @function_tool + async def ignore_speaker(speaker_id: str) -> str: + return "ignored" + + agent = FocusAgent(tools=[focus_on_speaker, ignore_speaker]) + await AgentSession().start(agent=agent, room=ctx.room) +""" + +SDK_SUBCLASS = SDK.replace( + 'agent = Agent(name="assistant", tools=TOOLS)', + """class SupportAgent(Agent): + def __init__(self, tools): + super().__init__(name="assistant", tools=tools) + + +agent = SupportAgent(tools=TOOLS)""", +) +# Constructions the SDK reader does not read. Each keeps the agent's identity +# (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)', + ), + "clone": SDK.replace( + 'agent = Agent(name="assistant", tools=TOOLS)', + 'agent = Agent(name="assistant").clone(tools=TOOLS)', + ), +} + + +@pytest.mark.parametrize( + "spelling", + [ + LIVEKIT_BASE, + LIVEKIT_BASE.replace("@function_tool", "@agents.function_tool").replace( + "agent = Agent(", "agent = agents.Agent(" + ), + LIVEKIT_BASE.replace("from livekit.agents import", "from .agents import"), + ], + ids=["livekit-from-import", "livekit-module-attribute", "relative-agents-package"], +) +def test_other_agents_package_is_not_the_openai_sdk(tmp_path, spelling): + (tmp_path / "main.py").write_text(spelling) + detected = detect_workspace(tmp_path, max_python_files=10) + assert "openai_agents_sdk" not in {f.type for f in detected.frameworks if f.candidate_files} + # A manifest that declares the file as SDK still reads no SDK wiring from it. + source = ToolSourceConfig(id="sdk", type="openai_agents_sdk", path="main.py") + loaded = load_openai_sdk_static_tools(source, None, tmp_path) + assert loaded.tools == [] + assert loaded.binding_observations == [] + + +def test_livekit_subclass_refactor_is_not_an_sdk_removal(repo): + base = commit(repo, {"main.py": LIVEKIT_BASE}) + head = commit(repo, {"main.py": LIVEKIT_HEAD}) + result = run(repo, base, head) + assert result["rows"] == [] + assert result["comparison_status"] == "not_established" + assert result["base"]["sources"] == result["head"]["sources"] == [] + + +def test_sdk_tools_beside_a_livekit_agent_keep_their_own_identity(tmp_path): + (tmp_path / "main.py").write_text( + "from livekit import agents as lk\n" + + SDK.replace("TOOLS", "[lookup]") + + "@lk.function_tool\ndef hang_up() -> str:\n return 'bye'\n" + + "voice = lk.Agent(instructions='x', tools=[hang_up])\n" + ) + source = ToolSourceConfig(id="sdk", type="openai_agents_sdk", path="main.py") + loaded = load_openai_sdk_static_tools(source, None, tmp_path) + assert sorted(t.name for t in loaded.tools) == ["execute", "lookup"] + assert [(o.agent, o.tool_names) for o in loaded.binding_observations] == [("agent", ["lookup"])] + + +@pytest.mark.parametrize("construction", sorted(UNREAD_CONSTRUCTIONS)) +def test_unread_head_construction_is_not_a_removal(repo, construction): + base = commit(repo, {"agent.py": SDK.replace("TOOLS", "[lookup, execute]")}) + head = commit( + repo, {"agent.py": UNREAD_CONSTRUCTIONS[construction].replace("TOOLS", "[lookup, execute]")} + ) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert [ + (r["agent"], r["tool"], r["change"], r["candidate_change"]) for r in result["rows"] + ] == [ + ("agent", "execute", "not_established", "removed"), + ("agent", "lookup", "not_established", "removed"), + ] + assert all(list(r["uncertainty"]) == ["head"] for r in result["rows"]) + assert any( + g["agent"] == "agent" and g["source"] == "agent.py" for g in result["head"]["coverage_gaps"] + ) + + +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. + triage = '\ntriage = Agent(name="triage", handoffs=[worker])\n' + base = commit( + repo, + { + "agent.py": SDK.replace("TOOLS", "[lookup]") + + '\nworker = Agent(name="worker", tools=[execute])' + + triage + }, + ) + head = commit( + repo, + { + "agent.py": SDK.replace("TOOLS", "[lookup]") + + "\nclass Worker(Agent):\n pass\n" + + 'worker = Worker(name="worker", tools=[execute])' + + triage + }, + ) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert [(r["agent"], r["tool"], r["change"]) for r in result["rows"]] == [ + ("worker", "execute", "not_established") + ] + + +def test_unread_base_construction_is_not_an_addition(repo): + base = commit(repo, {"agent.py": SDK_SUBCLASS.replace("TOOLS", "[lookup, execute]")}) + head = commit(repo, {"agent.py": SDK.replace("TOOLS", "[lookup, execute]")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert {(r["change"], r["candidate_change"]) for r in result["rows"]} == { + ("not_established", "added") + } + assert all(list(r["uncertainty"]) == ["base"] for r in result["rows"]) + + +def test_adk_subclass_refactor_is_not_a_removal(repo): + source = """from google.adk.agents import Agent, LlmAgent + +def lookup(query: str) -> str: + return query +""" + base = commit( + repo, {"agent.py": source + 'root_agent = Agent(name="helper", tools=[lookup])\n'} + ) + head = commit( + repo, + { + "agent.py": source + + "class HelperAgent(LlmAgent):\n pass\n" + + 'root_agent = HelperAgent(name="helper", tools=[lookup])\n' + }, + ) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert [(r["agent"], r["change"], r["candidate_change"]) for r in result["rows"]] == [ + ("helper", "not_established", "removed") + ] + + +def test_deleted_agent_is_still_an_established_removal(repo): + worker = '\nworker = Agent(name="worker", tools=[execute])\n' + base = commit(repo, {"agent.py": SDK.replace("TOOLS", "[lookup]") + worker}) + head = commit(repo, {"agent.py": SDK.replace("TOOLS", "[lookup]")}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared" + assert [(r["agent"], r["tool"], r["change"]) for r in result["rows"]] == [ + ("worker", "execute", "removed") + ] From 0d313b97ec5613700207ac42b2cbb33730725a7f Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Fri, 25 Sep 2026 15:21:09 -0700 Subject: [PATCH 2/2] fix(diff --application): resolve SDK names per scope; imports keep an agent named Addresses the two P2 findings on #873. - Import provenance was flattened across lexical scopes: one file-wide import map, accepted if any origin was the SDK, so a LiveKit `Builder(...)` became an SDK agent when a sibling function imported the SDK under the same alias. `_SdkNames` now resolves each spelling in the scope that uses it (nearest binding scope, class bodies skipped for nested code, global/nonlocal obeyed; decorators and defaults in the enclosing scope). An import there must be the SDK's; a parameter or local assignment is not. `function_tool` is decided per decorator node, not as a file-wide set of spellings. The walk is iterative. - The missing-agent check ignored import bindings, so `from agent_factory import agent` (or `exported as agent`) still gave a definite removal. Import aliases now count as the file still naming the agent. Regression tests for both fail on the previous head. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 +- docs/application-comparison.md | 7 +- src/agents_shipgate/cli/application_diff.py | 13 +- .../inputs/openai_sdk_static.py | 167 +++++++++++++----- tests/test_application_diff_identity.py | 61 +++++++ 5 files changed, 198 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 588b3643..c0857b84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,8 @@ - Add `diff --application` for OpenAI Agents SDK and Google ADK source-observed per-agent wiring, with exact base/head refs and independently selected scopes. No manifest, saved baseline or authored declarations are needed. The advisory `application_comparison_schema_version: "0.1"` result records before/after evidence, scoped coverage gaps and explicit uncertain candidates; it supplies no release verdict or merge permission. Includes scoped Git materialization, partial-clone recovery, and definition lookup using reader-resolved Python symbols and locations. See [application comparison](docs/application-comparison.md). (#871) - `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). 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 its name, 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. + - **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. ### Changes diff --git a/docs/application-comparison.md b/docs/application-comparison.md index 47e979be..ab2932f1 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -67,8 +67,8 @@ artifact from the existing host diff JSON and verifier receipt. - Exit 2: refs/materialization/input could not be read. This is not no change. 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 the agent's name, or passes -it as `name=`, through a construction the reader does not support (an `Agent` +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 @@ -76,7 +76,8 @@ only in another agent's `handoffs` is not an observed construction. Framework identity follows the import, not the spelling. `Agent` and `function_tool` are the OpenAI Agents SDK's only when imported from the absolute -`agents`/`openai_agents` package or not imported at all; LiveKit's +`agents`/`openai_agents` package or not imported at all, resolved in the scope +that uses them, so an import in another function does not decide it; LiveKit's `livekit.agents` exports the same names and is not read as the SDK, and a relative `.agents` import is the project's own package. diff --git a/src/agents_shipgate/cli/application_diff.py b/src/agents_shipgate/cli/application_diff.py index 646333df..b377b9a2 100644 --- a/src/agents_shipgate/cli/application_diff.py +++ b/src/agents_shipgate/cli/application_diff.py @@ -524,10 +524,11 @@ def _align_exact_moves( def _still_named_at(root: Path, path: str, name: str) -> int | None: - """First line at which ``path`` still assigns ``name`` or passes ``name=name``. + """First line at which ``path`` still binds ``name`` or passes ``name=name``. - Those are the two agent identities the readers key on: the SDK's assigned - variable and ADK's ``name=`` literal. + Those are the two agent identities the readers key on: the SDK's bound + variable and ADK's ``name=`` literal. An import binds as an assignment + does, so ``from factory import agent`` keeps the name here. """ file = root / path if not path or not file.is_file() or not file.resolve().is_relative_to(root.resolve()): @@ -540,6 +541,12 @@ def _still_named_at(root: Path, path: str, name: str) -> int | None: node.lineno for node in ast.walk(tree) if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store) and node.id == name + ] + [ + node.lineno + for node in ast.walk(tree) + if isinstance(node, (ast.Import, ast.ImportFrom)) + for alias in node.names + if (alias.asname or alias.name.split(".", 1)[0]) == name ] + [ node.value.lineno for node in ast.walk(tree) diff --git a/src/agents_shipgate/inputs/openai_sdk_static.py b/src/agents_shipgate/inputs/openai_sdk_static.py index 6a965398..17a6a57e 100644 --- a/src/agents_shipgate/inputs/openai_sdk_static.py +++ b/src/agents_shipgate/inputs/openai_sdk_static.py @@ -147,9 +147,9 @@ def _load_python_file( ) raise InputParseError(message) from exc ref = display_path(path, base_dir) - decorator_names = _function_tool_decorator_names(tree) - definitions = [node for node in ast.walk(tree) if _is_function_tool(node, decorator_names)] - tools = [_function_to_tool(node, source, ref, decorator_names) for node in definitions] + sdk_decorators = _function_tool_decorators(tree) + definitions = [node for node in ast.walk(tree) if _is_function_tool(node, sdk_decorators)] + tools = [_function_to_tool(node, source, ref, sdk_decorators) for node in definitions] source_sha256, source_within_limits = guard_module_metadata(tree, source_text) guards = [ read_guard_dependency( @@ -209,7 +209,7 @@ def _extract_agent_bindings( not target or call is None or not sdk_names.denotes( - dotted_name(call.func), "Agent", DEFAULT_AGENT_CONSTRUCTORS + dotted_name(call.func), call, "Agent", DEFAULT_AGENT_CONSTRUCTORS ) ): continue @@ -331,46 +331,119 @@ def _resolve_name_list( return None +_SCOPE_NODES = ( + ast.FunctionDef, + ast.AsyncFunctionDef, + ast.Lambda, + ast.ClassDef, + ast.ListComp, + ast.SetComp, + ast.DictComp, + ast.GeneratorExp, +) + + class _SdkNames: - """Which spellings in one module denote the SDK's own symbols. + """Which spellings, where they are used, denote the SDK's own symbols. ``livekit.agents`` also exports ``Agent`` and ``function_tool``, so the - spelling alone proves nothing. A spelling's head decides: imported from - the absolute ``agents``/``openai_agents`` package, it is the SDK's; not - imported at all, the default spellings keep their terminal-name reading - unless a wildcard from another module could supply them; imported only - from anywhere else — ``livekit.agents``, a relative ``.agents`` — it is - another library's symbol and never read as the SDK's. + spelling alone proves nothing. A spelling's head is resolved where Python + resolves it: the nearest enclosing scope that binds it, with class bodies + skipped from code nested inside them and ``global``/``nonlocal`` obeyed, + so an import in a sibling function never decides this use. If that scope + imports the head, every import there must come from the absolute + ``agents``/``openai_agents`` package; if it binds the head only otherwise + (a parameter, an assignment, a local ``def``), it is not the SDK's. A head + no scope binds keeps the default spellings' terminal-name reading, unless + a wildcard from another module could supply it. """ def __init__(self, tree: ast.Module) -> None: - self.origins: dict[str, set[str]] = {} + self.module = tree + self.scope_of: dict[int, ast.AST] = {} + self.parent: dict[int, ast.AST | None] = {id(tree): None} + self.bindings: dict[int, dict[str, list[str | None]]] = {} + self.declared: dict[int, dict[str, str]] = {} self.foreign_wildcard = False - for node in ast.walk(tree): - if isinstance(node, ast.ImportFrom): + # Decorators, defaults, class bases and a comprehension's first + # iterable are evaluated in the scope that encloses their owner. + enclosing: dict[int, ast.AST] = {} + stack: list[tuple[ast.AST, ast.AST]] = [(tree, tree)] + while stack: + node, scope = stack.pop() + scope = enclosing.get(id(node), scope) + self.scope_of[id(node)] = scope + inner = scope + if isinstance(node, _SCOPE_NODES): + self.parent[id(node)] = scope + inner = node + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + self._bind(scope, node.name) + if isinstance(node, ast.ClassDef): + outer = [*node.decorator_list, *node.bases, *node.keywords] + elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda)): + outer = [ + *getattr(node, "decorator_list", []), + *node.args.defaults, + *(default for default in node.args.kw_defaults if default), + ] + else: + outer = [node.generators[0].iter] + enclosing.update((id(item), scope) for item in outer) + elif isinstance(node, ast.ImportFrom): module = "." * node.level + (node.module or "") for alias in node.names: - path = f"{module}.{alias.name}" if node.module else module + alias.name if alias.name == "*": self.foreign_wildcard |= not _is_sdk_path(module) else: - self.origins.setdefault(alias.asname or alias.name, set()).add(path) + path = f"{module}.{alias.name}" if node.module else module + alias.name + self._bind(scope, alias.asname or alias.name, path) elif isinstance(node, ast.Import): for alias in node.names: head = alias.name.split(".", 1)[0] bound, path = (alias.asname, alias.name) if alias.asname else (head, head) - self.origins.setdefault(bound, set()).add(path) + self._bind(scope, bound, path) + elif isinstance(node, ast.Name) and isinstance(node.ctx, (ast.Store, ast.Del)): + self._bind(scope, node.id) + elif isinstance(node, ast.arg): + self._bind(scope, node.arg) + elif isinstance(node, ast.ExceptHandler) and node.name: + self._bind(scope, node.name) + elif isinstance(node, (ast.Global, ast.Nonlocal)): + kind = "global" if isinstance(node, ast.Global) else "nonlocal" + self.declared.setdefault(id(scope), {}).update(dict.fromkeys(node.names, kind)) + stack.extend((child, inner) for child in ast.iter_child_nodes(node)) + + def _bind(self, scope: ast.AST, name: str, path: str | None = None) -> None: + self.bindings.setdefault(id(scope), {}).setdefault(name, []).append(path) + + def _resolve(self, name: str, node: ast.AST) -> list[str | None] | None: + scope: ast.AST | None = self.scope_of.get(id(node), self.module) + start = scope + while scope is not None: + declared = self.declared.get(id(scope), {}).get(name) + if declared == "global": + return self.bindings.get(id(self.module), {}).get(name) + if declared is None and (scope is start or not isinstance(scope, ast.ClassDef)): + bound = self.bindings.get(id(scope), {}).get(name) + if bound is not None: + return bound + scope = self.parent.get(id(scope)) + return None - def denotes(self, spelling: str | None, symbol: str, defaults: frozenset[str]) -> bool: + def denotes( + self, spelling: str | None, node: ast.AST, symbol: str, defaults: frozenset[str] + ) -> bool: if not spelling: return False head, _, rest = spelling.partition(".") - origins = self.origins.get(head) - if origins is None: + bound = self._resolve(head, node) + if bound is None: return spelling in defaults and not self.foreign_wildcard - return any( + paths = [path for path in bound if path is not None] + return bool(paths) and all( _is_sdk_path(path) and (f"{path}.{rest}" if rest else path).rsplit(".", 1)[-1] == symbol - for path in origins + for path in paths ) @@ -378,26 +451,28 @@ def _is_sdk_path(path: str) -> bool: return not path.startswith(".") and path.split(".", 1)[0] in SDK_MODULES -def _function_tool_decorator_names(tree: ast.Module) -> set[str]: +def _function_tool_decorators(tree: ast.Module) -> set[int]: + """The decorator nodes that are the SDK's ``function_tool``, by identity. + + Decided per node, not per spelling: the same ``@function_tool`` may be the + SDK's in one function and LiveKit's in its sibling. + """ names = _SdkNames(tree) - spellings = set() - for node in ast.walk(tree): - if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): - for decorator in node.decorator_list: - spelling = _decorator_name(decorator) - if names.denotes(spelling, "function_tool", DEFAULT_FUNCTION_TOOL_DECORATORS): - spellings.add(spelling) - return spellings + return { + id(decorator) + for node in ast.walk(tree) + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + for decorator in node.decorator_list + if names.denotes( + _decorator_name(decorator), decorator, "function_tool", DEFAULT_FUNCTION_TOOL_DECORATORS + ) + } -def _is_function_tool(node: ast.AST, decorator_names: set[str]) -> bool: +def _is_function_tool(node: ast.AST, sdk_decorators: set[int]) -> bool: if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): return False - for decorator in node.decorator_list: - name = _decorator_name(decorator) - if name in decorator_names: - return True - return False + return any(id(decorator) in sdk_decorators for decorator in node.decorator_list) def _decorator_name(decorator: ast.AST) -> str | None: @@ -410,11 +485,11 @@ def _function_to_tool( node: ast.FunctionDef | ast.AsyncFunctionDef, source: ToolSourceConfig, source_ref: str, - decorator_names: set[str], + sdk_decorators: set[int], ) -> Tool: - tool_name = _tool_name(node, decorator_names) + tool_name = _tool_name(node, sdk_decorators) input_schema, parameters = function_input_schema(node) - description = _description(node, decorator_names) or ast.get_docstring(node) + description = _description(node, sdk_decorators) or ast.get_docstring(node) return Tool( id=stable_tool_id(tool_name), name=tool_name, @@ -434,24 +509,24 @@ def _function_to_tool( ) -def _tool_name(node: ast.FunctionDef | ast.AsyncFunctionDef, decorator_names: set[str]) -> str: - return _decorator_kwarg_string(node, decorator_names, "name_override") or node.name +def _tool_name(node: ast.FunctionDef | ast.AsyncFunctionDef, sdk_decorators: set[int]) -> str: + return _decorator_kwarg_string(node, sdk_decorators, "name_override") or node.name def _description( - node: ast.FunctionDef | ast.AsyncFunctionDef, decorator_names: set[str] + node: ast.FunctionDef | ast.AsyncFunctionDef, sdk_decorators: set[int] ) -> str | None: - return _decorator_kwarg_string(node, decorator_names, "description_override") + return _decorator_kwarg_string(node, sdk_decorators, "description_override") def _decorator_kwarg_string( node: ast.FunctionDef | ast.AsyncFunctionDef, - decorator_names: set[str], + sdk_decorators: set[int], kwarg_name: str, ) -> str | None: for decorator in node.decorator_list: call = decorator if isinstance(decorator, ast.Call) else None - if not call or _decorator_name(call.func) not in decorator_names: + if not call or id(call) not in sdk_decorators: continue for keyword in call.keywords: if keyword.arg != kwarg_name or not isinstance(keyword.value, ast.Constant): diff --git a/tests/test_application_diff_identity.py b/tests/test_application_diff_identity.py index 79550cac..20a3d35b 100644 --- a/tests/test_application_diff_identity.py +++ b/tests/test_application_diff_identity.py @@ -123,6 +123,67 @@ def test_sdk_tools_beside_a_livekit_agent_keep_their_own_identity(tmp_path): assert [(o.agent, o.tool_names) for o in loaded.binding_observations] == [("agent", ["lookup"])] +FACTORY = '''def {name}(): + from {module} import Agent as Builder, function_tool + + @function_tool + def {name}_tool(query: str) -> str: + return query + + {name}_agent = Builder(name="{name}", tools=[{name}_tool]) + return {name}_agent +''' + + +@pytest.mark.parametrize( + "source", + [ + # PR #873 review: an import in a sibling function decided identity. + FACTORY.format(name="sdk", module="agents") + + "\n\n" + + FACTORY.format(name="voice", module="livekit.agents"), + # A function's own import shadows the module's SDK import. + "from agents import Agent as Builder, function_tool\n\n\n" + + FACTORY.format(name="voice", module="livekit.agents") + + "\n\n@function_tool\ndef sdk_tool(query: str) -> str:\n return query\n" + + 'sdk_agent = Builder(name="sdk", tools=[sdk_tool])\n', + ], + ids=["sibling-function-import", "inner-import-shadows-module"], +) +def test_import_provenance_is_resolved_in_the_enclosing_scope(tmp_path, source): + (tmp_path / "main.py").write_text(source) + loaded = load_openai_sdk_static_tools( + ToolSourceConfig(id="sdk", type="openai_agents_sdk", path="main.py"), None, tmp_path + ) + assert [t.name for t in loaded.tools] == ["sdk_tool"] + assert [(o.agent, o.tool_names) for o in loaded.binding_observations] == [ + ("sdk_agent", ["sdk_tool"]) + ] + assert loaded.warnings == [] + + +@pytest.mark.parametrize("spelling", ["agent", "exported as agent"]) +def test_import_bound_agent_name_is_not_a_removal(repo, spelling): + # PR #873 review: the head module still binds `agent`, through an import + # of an unsupported factory's result, so absence is not established. + exported = spelling.split()[0] + base = commit(repo, {"agent.py": SDK.replace("TOOLS", "[lookup]")}) + factory = SDK.replace( + 'agent = Agent(name="assistant", tools=TOOLS)', + f'def build():\n return Agent(name="assistant", tools=[lookup])\n{exported} = build()', + ) + head = commit( + repo, + {"agent_factory.py": factory, "agent.py": f"from agent_factory import {spelling}\n"}, + ) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert [ + (r["agent"], r["tool"], r["change"], r["candidate_change"]) for r in result["rows"] + ] == [("agent", "lookup", "not_established", "removed")] + assert list(result["rows"][0]["uncertainty"]) == ["head"] + + @pytest.mark.parametrize("construction", sorted(UNREAD_CONSTRUCTIONS)) def test_unread_head_construction_is_not_a_removal(repo, construction): base = commit(repo, {"agent.py": SDK.replace("TOOLS", "[lookup, execute]")})