diff --git a/CHANGELOG.md b/CHANGELOG.md index f48ff6a6..40a81bcb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - **An empty preflight plan no longer grants merge or completion.** A plan that named no changed file, capability request or host permission request returned the shared `complete` state, whose permissions include `merge` and `report_complete`, with no verifier run behind it. Preflight `0.6` answers it with the new `planning_only` state, which owes no action and authorizes nothing, and every preflight route now denies every permission in both the model and the published schema; `complete` and `review_publishable` cannot appear. Docs-only plans still route to `verify`, and protected surfaces and drift still stop for a human. `0.5` stays frozen and readable as a base preflight. Runtime contract 41 → 42; `minimum_control_contract_version` stays 21. Hooks written by `install-hooks` before this change accept only preflight `0.5`, so their instruction-structure check fails closed until `install-hooks --write` is re-run. See the `planning-only preflight` migration note in `STABILITY.md`. (#610) - Commit the application review benchmark (`benchmark/application-q2/`): the 49 pinned development pull requests, the runner, the hand-scoring protocol, the 2026-09-30 ledger, reproduced from fresh clones with every status, row and comparison id identical (Q2 1/49; re-read, 12 members are not SDK/ADK wiring changes, so Q0 is 37/49, and MIS_TALENT#6 is Q1, so Q1 is 2/49), and a holdout selection rule, the definitions it uses and a recorded repository pool, frozen by digest before the reader changes they will judge. Each release now records `Q2: n/49 development, m/≥30 holdout`. Development evidence only; no success-rate claim. (#908) +- **`diff --application` reads tools lists built by an expression.** An OpenAI Agents SDK or Google ADK agent's `tools=` (and an SDK agent's `handoffs=`, an ADK agent's `sub_agents=`) built by a spread, `+`, a conditional, `or`, a filter or another module's list is read member by member instead of reported as one dynamic expression. A part it cannot read — a call, a builder's parameter, `self.tools` — is named with its location on that agent, which stays incomplete, and the members read are still compared. A member held only under a condition carries `bound_when`, and a change to the condition alone is a `changed` row whose direction is not established. `application_comparison_schema_version` 0.2 → 0.3. The Google ADK dynamic-tools limit now names its file and line and its agent, a starred element the ADK reader cannot read no longer leaves the agent's list read as complete, and an imported `handoffs=` list no longer reads as one handoff named after the list. The SDK recovery reason `sdk_literal_tool_list_concatenation_unsupported` is retired: that form is read (#584). For `scan`, an ADK list that came through a name or from another module is not counted as a proven surface, so no `scan` decision moves. On the 49-PR development corpus (`benchmark/application-q2/`), pull requests with any row go from 9 to 11 and those still carrying a tools-list limit from 28 to 24, most of the rest a builder's parameter (#874); MIS_TALENT#7 now reports the `Finance_Agent` row the 2026-09-30 ledger found missing. On 131 open SDK/ADK pull requests, every status is unchanged and two gain rows, all 16 checked against source. (#909) - Move the published-release pins, examples and adoption prompts to `v1.2.0` (contract 41) now that it is published, re-capture the README and quickstart `diff` answers from the published `1.2.0`, and re-measure the pilot ledger's Route H dry run on it. No schema or contract change. (#778) ## 1.2.0 - 2026-09-30 diff --git a/docs/application-comparison.md b/docs/application-comparison.md index 5313a139..0704fec1 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -139,12 +139,14 @@ An absent side names the missing scope and suggests `--base-scope`/`--scope` for relocation. If neither selected directory exists, the command refuses with exit 2. A removal describes the selected source path, not the entire repository. -`--json` emits `application_comparison_schema_version: "0.2"`, engine identity, +`--json` emits `application_comparison_schema_version: "0.3"`, engine identity, requested and compared refs/tree IDs, per-side scope/coverage, rows, source correspondence, `scope_selection`, `comparisons` when a derived change spans more than one application, and a deterministic `comparison_id`. Version 0.2 adds `reach`, `effect_evidence` and `construction_sites` to a row's sides (see -[What a bound tool reaches](#what-a-bound-tool-reaches)). This is a separate advisory +[What a bound tool reaches](#what-a-bound-tool-reaches)); version 0.3 adds +`bound_when` (see [Tools lists built by an expression](#tools-lists-built-by-an-expression)). +This is a separate advisory artifact from the existing host diff JSON and verifier receipt. - `compared`: the selected supported source observations were compared. An @@ -351,7 +353,8 @@ the agent's function — `tools = [a, b]` with `tools.append(c)`, `.extend([...])`, `.insert(i, c)` or `tools += [...]` — is read member by member up to the statement that builds the agent, which copies it; an addition under a condition or in a loop before it is named on the agent, never read as -bound, and any other use of the list keeps it a dynamic tools expression. +bound. A list used any other way is read as +[a tools list built by an expression](#tools-lists-built-by-an-expression) is. The boundary is narrow. Only regular `.py` files inside the selected scope are read, parsed with `ast` and never imported or run. Symbolic links are not @@ -473,7 +476,8 @@ wildcard import could bind the name. `x = FunctionTool(func=x)` right after An OpenAI Agents SDK `tools=NAME` or `handoffs=NAME` is read through the scope that binds `NAME` where the agent is constructed: a builder's own list, a class body's own list, or the module's. It is read only when that scope binds it -once, to a literal list, and every use of that binding in the file only reads +once, unconditionally, to a list [the expression reader](#tools-lists-built-by-an-expression) +reads, and every use of that binding in the module that binds it only reads it: iterated, indexed, compared, tested, formatted, spread (`[*TOOLS, x]`), handed to a read-only builtin, a standard-library reader (`json.dumps`) or a logger's method — each proven by its binding, so a `print` imported from the @@ -482,8 +486,8 @@ own `tools=`, or to a function whose every use of that parameter is such a read. A list method, `+=`, a `global` or `nonlocal` rebinding, a subscript store, a second name (also through `x or y`), a tuple, a return, `*args`, or anything in the module that reaches its names without spelling them (`globals()`, -`vars()`, `sys.modules`, importing the module by `__name__`) makes it a dynamic -tools expression. The names in a module-level list are read at module level, whatever +`vars()`, `sys.modules`, importing the module by `__name__`) leaves it unread, +named with why. The names in a module-level list are read at module level, whatever the function that builds the agent imports. A reference that does not reach one definition stays an unresolved tool, named @@ -506,6 +510,83 @@ module it lives in: moving a function is not a change. So retargeting a binding between two functions whose definitions are the same text in different modules shows no row, even when the modules differ in what the function body refers to. +## Tools lists built by an expression + +An agent's tools are often not written out in its construction: +`tools=[*FINANCE_TOOLS, *([prepare_handoff] if want_handoff else [])]`, +`tools=base_tools + (extra_tools or [])`, or a filter over another module's +list. An OpenAI Agents SDK or Google ADK agent's `tools=`, an SDK agent's +`handoffs=` and an ADK agent's `sub_agents=` are read member by member when +built from (#909): + +- a list or tuple literal, each `*` spread spliced in; +- `a + b`; +- `a if c else b` and `a or b`: every branch that can be the value, each + member *conditional* on the condition that selects it, or only the branch a + constant condition, or an operand known to be empty or not, selects; +- a comprehension that keeps its elements (`[t for t in TOOLS if keep(t)]`) + and `filter(f, TOOLS)`: the members of `TOOLS`, each shown as held only + when the filter keeps it. The filter is not evaluated, so the list may hold + fewer; +- `list(...)`, `tuple(...)` and `sorted(...)` of one of these; +- a name bound once to one of these — in the builder or at module level, and + through a repository-local import to the module that builds the list, whose + members are then read by that module's names. A binding inside a branch, a + loop or an `except` is read only by code in the same block; one inside + `with` or a `try` body is read as unconditional. + +A name is looked up where Python evaluates it: a default, decorator or +annotation in the scope around its definition, a comprehension's first +iterable around the comprehension. It is read only while nothing can change +the list after it is built: no use in its own module but a read, by the rule +for `tools=NAME` above; no change through an agent built with it +(`helper.tools.append(x)`); no wildcard import after it; and no change in a +module the import passes through or a package above the list's module, which +run first. A change made from a module the import never passes through is not +looked for. Anything else is named where it is, on the agent it belongs to, +with why — a call (`get_tools()`), a parameter of the builder (its value comes +from a caller), `self.tools`, a comprehension that builds new elements, a name +bound twice or changed in place, nesting deeper than 16 levels, or more than +1,000 members: + +```text +OpenAI Agents SDK agent 'finance' at agent.py:4 has a tools list it reads only in part; +its binding graph is incomplete. Not read: a call to `plugin_tools`, whose result is not +read (agent.py:4). +``` + +The agent stays incomplete, so the answer is never `compared`, but the members +that were read are still compared: a tool both sides bind keeps its `changed` +row, and nothing a part not read holds is reported as removed. A member another +module's list names is read by that module's names, and never bound by its +spelling alone: an unresolved one is a named limit, and a handoff spelled like +an agent this module builds is not taken for that agent. Nothing is imported or +run. A Google ADK `tools=` or `sub_agents=` list that came through a name, or +from another module, is not counted by `scan` as a proven surface, because +another module could change it; a literal spread into a literal is. + +A member held only under a condition carries it on its row's side as +`bound_when`, one entry per way it gets in, each built from the conditions' +source text (`` `wanted` ``, `` not `wanted` ``, `` `extra` is empty ``, +`` the filter `keep(t)` keeps it ``, joined by `and`): + +```text +ADDED finance → prepare_finance_handoff + before: no observed binding + after: prepare_finance_handoff(note) at app/Agent/financeAgent.py:100 + bound only when `want_handoff_tool` +``` + +A conditional member is never shown as unconditional, and a tool the list also +holds unconditionally has no `bound_when`. A condition is compared whole, never +shortened. When only the condition changed (`if handoffs` → `if +want_handoff_tool`, or a conditional member made unconditional), the row is +`changed`, and its `why` names both conditions: they are read, never evaluated, +so whether the agent now holds the tool more or less often is not established. +When a part of the agent's list was not read, or the agent is constructed more +than once differently, the row is `not_established` instead: the unread part +may hold the tool another way. + ## What a bound tool reaches A signature says what the model may pass, not what the call does. Each diff --git a/docs/distribution-surfaces.md b/docs/distribution-surfaces.md index a54a8746..89ddbb20 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, except that an allow rule the engine rates as reaching a [documented arbitrary-code launcher](engineering/exec-equivalent-permissions.md) is shown in table text (`Bash(npx *)`, or a wider rule's own prefix of it, `Bash(python3 *)`) and never with a user operand, the one rating `diff`, `audit --host`, `verify` and the PR comment also carry, and its existing local-policy control (#824, `tests/test_exec_equivalent_permissions.py`). 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`). An added or changed MCP row's `why` appends a launch-source note — `launch source is mutable`, or `launch source moved from pinned … to mutable …` when both sides establish a pin — read from the `launch_source` the engine published on the grant by a bounded declaration grammar ([mcp-launch-source-notes](engineering/mcp-launch-source-notes.md)); grant equality, the inventory digests and saved baselines leave that fact out, so the note adds no row and moves no direction, `expands`, severity, expansion signal or `check` decision (#825, `tests/test_mcp_launch_source.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). An added or changed Claude Code `PreToolUse` hook row whose basis is host configuration or a project-enabled plugin appends `inline allow auto-approves matched tool calls without a prompt (matcher …)` when a handler's published `inline_allow` is true — the engine's reading of a literal, unconditional allow on a broad matcher by a bounded declaration grammar ([inline-hook-allow-notes](engineering/inline-hook-allow-notes.md)), which grant equality, the inventory digests and saved baselines leave out — so the note adds no row and moves no direction, `expands`, severity, expansion signal or `check` decision (#826, `tests/test_inline_hook_allow.py`). `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. A selected hook's script bytes are a comparison input of its hook grant (#702, `tests/test_hook_script_capture.py`, `tests/test_hook_script_comparison_limits.py`): a script-only edit is one `changed` row on the declaring hook, never an expansion, whose `why` names the script and loading basis and whose change carries the digests, read from the `script_inputs` the engine published; the script's own coverage line is `changed_without_rows`, which the text words as the row of the hook that runs it naming it, and a script only one side's hooks select is `compared` on that side, its selection change being the hook's row. A script the reader could not read costs that script only: a shared limit Git proves unchanged, or a script Git proves is on neither side, is an `unchanged_limits` entry of kind `unreadable`, and any other makes the comparison `partial` with a `blocking_limit` item naming the script as both `source` and `scope`, and `Not compared: the bytes of …` in the text; a working-tree difference a line-ending conversion can explain is `unchanged_not_proven`, not a row. A selected hook whose script is not resolved is a `script_not_resolved` item naming each handler and its reason, while the change could touch it, never a row, widening or limit. `check` and the boundary check of a manifest-backed `verify` route a changed selected script as a protected change of the hosts selecting it, from both compared sides' declarations, reading the base's only when the change touches a declaration (`tests/test_hook_script_routing.py`); `check` and a provided diff, which name no limit, leave out a selected script the change does not touch, from their comparison and input coverage alike, and a provided diff that touches both a script and its declaring file compares the script's bytes as the diff states them (`tests/test_hook_script_comparison_limits.py`). | -| `application_diff` | `src/agents_shipgate/cli/application_diff.py`, `src/agents_shipgate/cli/application_scope.py`, `src/agents_shipgate/inputs/tool_reach.py` | — | — | Advisory SDK/ADK source-wiring comparison through `diff --application`. Without `--scope` the scope is derived from the change: the outermost package holding each changed Python file and the agent-building SDK/ADK files (discovery's own signals) that import it or that it imports within six hops, with what they import. Each independent application is its own comparison and never the root by default; agents only the root would join are named as `outside` and make the answer `partial`. A whole application directory moved is one relocation. A change no agent reaches is an explicit `not_established` answer, and a bound reached is named in `scope_selection` and makes the answer `partial` (#875, `tests/test_application_scope.py`). 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`). Each side of a row also carries `reach` (#872). It holds the outbound HTTP calls read statically from the tool's own code and its same-scope helpers, up to three calls deep. For each call it records the method, the URL template and literal request fields, which model-supplied parameters flow where, and the environment variables sent as credentials, by name, never a value. Every call it cannot follow is a named limit. `effect_evidence` is the engine's own `assess_tool_semantics` over that tool, with the reach as one more structural source, `source_http_call`, so it is not a second classifier. `read` is claimed only when every call was followed and every outbound call reads. Both are evidence outside the compared meaning, so they move no row or status. For a Google ADK name constructed more than once, `binding_location` names a construction that lists the tool, and `construction_sites` lists every one (`tests/test_application_diff_tool_reach.py`). | +| `application_diff` | `src/agents_shipgate/cli/application_diff.py`, `src/agents_shipgate/cli/application_scope.py`, `src/agents_shipgate/inputs/tool_reach.py` | — | — | Advisory SDK/ADK source-wiring comparison through `diff --application`. Without `--scope` the scope is derived from the change: the outermost package holding each changed Python file and the agent-building SDK/ADK files (discovery's own signals) that import it or that it imports within six hops, with what they import. Each independent application is its own comparison and never the root by default; agents only the root would join are named as `outside` and make the answer `partial`. A whole application directory moved is one relocation. A change no agent reaches is an explicit `not_established` answer, and a bound reached is named in `scope_selection` and makes the answer `partial` (#875, `tests/test_application_scope.py`). 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`). Each side of a row also carries `reach` (#872). It holds the outbound HTTP calls read statically from the tool's own code and its same-scope helpers, up to three calls deep. For each call it records the method, the URL template and literal request fields, which model-supplied parameters flow where, and the environment variables sent as credentials, by name, never a value. Every call it cannot follow is a named limit. `effect_evidence` is the engine's own `assess_tool_semantics` over that tool, with the reach as one more structural source, `source_http_call`, so it is not a second classifier. `read` is claimed only when every call was followed and every outbound call reads. Both are evidence outside the compared meaning, so they move no row or status. For a Google ADK name constructed more than once, `binding_location` names a construction that lists the tool, and `construction_sites` lists every one (`tests/test_application_diff_tool_reach.py`). A tools, handoffs or sub-agents list built by an expression — a spread, `+`, a conditional, `or`, a filter, another module's list — is read member by member through one resolver both readers share, and each part it cannot read is named, with its location, on the agent it belongs to, which stays incomplete. A member held only under a condition carries `bound_when`, the condition's source text, never evaluated; it restates no engine answer, and a change to it alone is a `changed` row whose direction is not established (#909, `tests/test_list_expressions.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/docs/qualification-coverage.md b/docs/qualification-coverage.md index 5a429b6e..73f6da63 100644 --- a/docs/qualification-coverage.md +++ b/docs/qualification-coverage.md @@ -31,7 +31,7 @@ report. Read `kind` and `reason`, not warning prose or `authorable_by`: | `kind` | SDK evidence in this first supported slice | Next step | |---|---|---| | `input_unavailable` | Configured SDK entrypoint not found (`sdk_entrypoint_not_found`). The SDK loader currently reports this warning for either required or optional entries; the required-source execution-contract mismatch is tracked in #585. | Restore the existing file named by `next_action.path`; a declaration does not replace it. | -| `reader_limitation` | Two literal lists of tool names joined by `+` (`sdk_literal_tool_list_concatenation_unsupported`). The binding reader has no branch for this static form. | Retain the source as a reproducer for an Agents Shipgate reader repair. This does not assert what the deployed agent can access. | +| `reader_limitation` | None at present. The one static form classified here, two literal lists joined by `+` (`sdk_literal_tool_list_concatenation_unsupported`), is read since #909, so that reason is no longer emitted. | Retain the source as a reproducer for an Agents Shipgate reader repair. This does not assert what the deployed agent can access. | | `unresolved` | An unresolved tools expression (`sdk_tools_expression_unresolved`), or distinct raw source warnings that collapse to one public identity (`ambiguous_warning_identity`). | Establish the missing input or reader limitation before assigning a repair owner. An ambiguous source gets no guessed path. | Other loaders and SDK warning shapes remain unclassified. A dynamic expression diff --git a/src/agents_shipgate/cli/application_diff.py b/src/agents_shipgate/cli/application_diff.py index e4ab5d16..9c9196ae 100644 --- a/src/agents_shipgate/cli/application_diff.py +++ b/src/agents_shipgate/cli/application_diff.py @@ -32,7 +32,7 @@ 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.agent_bindings import adk_unnamed_sub_agents, 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, InputParseError @@ -53,7 +53,7 @@ from agents_shipgate.schemas.manifest import ToolSourceConfig SUPPORTED = frozenset({"openai_agents_sdk", "google_adk"}) -SCHEMA_VERSION = "0.2" +SCHEMA_VERSION = "0.3" MAX_PYTHON_BYTES = 2_000_000 @@ -880,11 +880,29 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) # Where each tool is listed when one ADK name is constructed more than # once: the row points at a construction that lists it (#872). tool_sites: dict[tuple[str, str], dict[str, list[str]]] = {} + # What a tool or handoff is bound under when only under a condition + # (#909); ``None`` once any construction binds it unconditionally. + bound_when: dict[tuple[str, str], dict[str, list[str] | None]] = {} 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 tool_sites.setdefault(site, {}).update(observation.tool_sites) + held = bound_when.setdefault(site, {}) + for name in observation.tool_names: + _hold(held, name, observation.tool_conditions.get(name)) + for name in observation.handoff_names: + _hold(held, f"handoff:{name}", observation.handoff_conditions.get(name)) + # A Google ADK agent's sub-agents are its records', not its observation's. + for record in artifacts.sub_agents if artifacts is not None else []: + if "sub_agent_count" not in record or not isinstance(record.get("agent_name"), str): + continue + held = bound_when.setdefault( + (_source_path(root, str(record.get("source_ref") or "")), record["agent_name"]), {} + ) + conditions = record.get("conditions") or {} + for name in record.get("sub_agents") or []: + _hold(held, f"handoff:{name}", conditions.get(name)) tools, warnings = _canonical_tools(result, source, loaded) for warning in warnings: result.gap(warning, source=source.path) @@ -906,6 +924,38 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) "source": key[0], "location": agent.source_pointer, } + for record in artifacts.sub_agents if artifacts is not None else []: + if "unread" in record and isinstance(record.get("agent_name"), str): + # A sub-agent list read only in part: a limit on its agent, at its + # construction, not on every agent of the file (#909 review). + message = adk_unnamed_sub_agents(record["agent_name"], record) + result.gap( + message, + source=_source_path(root, str(record.get("source_ref") or "")), + agent=record["agent_name"], + ) + attributed.add(message) + if artifacts is not None: + # A Google ADK list read only in part is a limit its agent already + # carries; the toolset record beside it names no agent, and would + # otherwise cover every agent in the file (#909 review). + incomplete = { + (_source_path(root, observation.source), observation.agent) + for item in loaded + for observation in item.binding_observations + if not observation.tools_complete + } + dynamic = [toolset for toolset in artifacts.toolsets if toolset.dynamic or not toolset.resolved] + if dynamic and all( + toolset.kind == "dynamic" + and toolset.agent_name + and (_source_path(root, toolset.source_ref or ""), toolset.agent_name) in incomplete + for toolset in dynamic + ): + attributed.update( + f"Google ADK toolset {toolset.name or toolset.kind!r} is not statically enumerable." + for toolset in dynamic + ) for issue in graph.issues: if ( issue.kind in {"ambiguous_root_agent", "missing_binding_evidence"} @@ -957,6 +1007,9 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) "evidence_basis": edge.provenance_kind, **_import_path(tool, key[0]), } + condition = bound_when.get(key, {}).get(tool.name) + if condition: + binding["bound_when"] = condition listed = tool_sites.get(key, {}).get(tool.name) if listed: binding["binding_location"] = listed[0] @@ -999,6 +1052,22 @@ def _observe_source(result: Observations, root: Path, source: ToolSourceConfig) "binding_location": edge.source_pointer, "evidence_basis": edge.provenance_kind, } + condition = bound_when.get(source, {}).get(f"handoff:{target[1]}") + if condition: + result.bindings[key]["bound_when"] = condition + + +def _hold(held: dict[str, list[str] | None], name: str, alternatives: list[str] | None) -> None: + """Record one construction's condition for ``name``: unconditional wins.""" + + if name in held and held[name] is None: + return + held[name] = None if alternatives is None else sorted(set(held.get(name) or []) | set(alternatives)) + + +def _bound(binding: dict[str, Any]) -> str: + alternatives = binding.get("bound_when") + return "only when " + " or ".join(alternatives) if alternatives else "unconditionally" def _canonical_tools( @@ -1126,6 +1195,7 @@ def compare( before, after = base.bindings.get(key), head.bindings.get(key) uncertainty = {} kind = "added" if before is None else "removed" if after is None else "changed" + only_condition = False if before is None: reasons = base.absence_gaps(key, target_moves) if reasons: @@ -1148,6 +1218,18 @@ def compare( for side, value in (("base", before), ("head", after)): if "definition" in value and value["definition"]["implementation_sha256"] is None: uncertainty[side] = ["The bound callable's implementation could not be read."] + # Only the condition it is bound under changed (#909): a change, + # stated as one, whose direction is not established. + only_condition = before_meaning != _meaning(after) and ( + {**before_meaning, "bound_when": None} == {**_meaning(after), "bound_when": None} + ) + if only_condition: + # A part of the list that was not read may hold the tool some + # other way, so the condition is not established either. + for side, observed, moves in (("base", base, target_moves), ("head", head, None)): + reasons = observed.absence_gaps(key, moves) + if reasons: + uncertainty.setdefault(side, []).extend(reasons) if before_meaning != _meaning(after): # A factory value this read cannot name moved with the code # around it: a candidate change, not an established one. @@ -1174,15 +1256,26 @@ def compare( "uncertainty": uncertainty, "before": before, "after": after, - "why": { - "added": "The source now binds this callable to this agent.", - "removed": "The selected source path no longer binds this callable to this agent; check scope limits for relocation.", + "why": ( + f"Only the condition changed: bound {_bound(before)} at the base and " + f"{_bound(after)} at the head. Conditions are read as source text, never " + "evaluated, so whether the agent holds it more or less often is not established." + ) + if only_condition and not uncertainty + else { + "added": "The source now binds this callable to this agent" + + (f", {_bound(after)}." if after and after.get("bound_when") else "."), + "removed": "The selected source path no longer binds this callable to this agent" + + (f" (it was bound {_bound(before)})" if before and before.get("bound_when") else "") + + "; check scope limits for relocation.", "not_established": "This candidate change cannot be established from the affected inputs; it is not a no-change result.", "changed": "The bound callable's interface or implementation changed; authority direction is not established.", }[kind], "review_question": ( f"Resolve the named uncertainty before treating {key[1]}.{key[2]} as {candidate_change}." if uncertainty + else f"Should {key[1]} hold {key[2]} {_bound(after)} rather than {_bound(before)}?" + if only_condition else f"Should {key[1]} have this {kind} binding to {key[2]}? Review the before/after signature and implementation locations." ), } @@ -1857,6 +1950,8 @@ def _print_rows(payload: dict[str, Any], _one_line: Any) -> None: f" {side}: {_one_line(value.get('signature') or value['tool'])} at {_one_line(value.get('binding_location'))}" + (f" (also listed at {_one_line(', '.join(also))})" if also else "") ) + if value.get("bound_when"): + typer.echo(f" bound {_one_line(_bound(value))}") if definition: typer.echo( f" implementation: {_one_line(definition['source'])}:{definition['line']} ({str(definition['implementation_sha256'])[:12]})" diff --git a/src/agents_shipgate/core/agent_bindings.py b/src/agents_shipgate/core/agent_bindings.py index 92986b0c..3437afe0 100644 --- a/src/agents_shipgate/core/agent_bindings.py +++ b/src/agents_shipgate/core/agent_bindings.py @@ -895,7 +895,7 @@ def _observations( if not isinstance(declared_count, int) or declared_count > len( _string_list(record.get("sub_agents")) ) + len(unresolved): - partials.add(f"Google ADK agent {source_name!r} has sub-agents that were not statically named.") + partials.add(adk_unnamed_sub_agents(source_name, record)) for toolset in adk.toolsets: if toolset.dynamic or not toolset.resolved: partials.add(f"Google ADK toolset {toolset.name or toolset.kind!r} is not statically enumerable.") @@ -915,6 +915,17 @@ def _observations( return agents, edges, handoffs, partials, invalid_annotations +def adk_unnamed_sub_agents(agent: str | None, record: dict[str, Any]) -> str: + """The limit a Google ADK sub-agent list not wholly named carries. + + A list read only in part names each part not read and where it is (#909). + """ + + message = f"Google ADK agent {agent!r} has sub-agents that were not statically named." + unread = record.get("unread") + return f"{message} Not read: {unread}." if isinstance(unread, str) and unread else message + + #: Characters that mean a reader was writing a pattern, not a name. _GLOB_CHARACTERS = frozenset("*?[") diff --git a/src/agents_shipgate/core/domain.py b/src/agents_shipgate/core/domain.py index 12465af0..6f1e2cbb 100644 --- a/src/agents_shipgate/core/domain.py +++ b/src/agents_shipgate/core/domain.py @@ -707,7 +707,15 @@ class AgentBindingObservation(BaseModel): #: that list the tool, when one name is constructed more than once in the #: source (#872). Where each binding is, not which one runs. tool_sites: dict[str, list[str]] = Field(default_factory=dict) + #: ``tool_name -> [condition, ...]`` for a tool the agent's list holds only + #: under a condition -- a branch of ``a if c else b`` or ``a or b``, or a + #: comprehension's filter (#909). Each entry is one alternative, its + #: conditions joined with "and"; a tool the list also holds unconditionally + #: has no entry. Source text, never evaluated. + tool_conditions: dict[str, list[str]] = Field(default_factory=dict) handoff_names: list[str] = Field(default_factory=list) + #: ``handoff_name -> [condition, ...]``: the same, for a handoff target. + handoff_conditions: dict[str, list[str]] = Field(default_factory=dict) tools_complete: bool = True handoffs_complete: bool = True issues: list[str] = Field(default_factory=list) diff --git a/src/agents_shipgate/inputs/google_adk.py b/src/agents_shipgate/inputs/google_adk.py index 48e58e92..7443f2a2 100644 --- a/src/agents_shipgate/inputs/google_adk.py +++ b/src/agents_shipgate/inputs/google_adk.py @@ -38,6 +38,15 @@ stable_tool_id, ) from agents_shipgate.inputs.coverage import BoundaryCell, SourceCoverage +from agents_shipgate.inputs.list_expressions import ( + Conditions, + ListExpressions, + ListMember, + ListResolution, + source_text, + unread_list_reason, + unread_parts, +) from agents_shipgate.inputs.mcp import load_mcp_tools from agents_shipgate.inputs.openapi import load_openapi_tools from agents_shipgate.inputs.protocol import LoadedAdapterResult @@ -51,6 +60,7 @@ PythonModule, Resolution, ScopeIndex, + _module_bindings, local_binding_detail, reference_spelling, ) @@ -909,6 +919,8 @@ class _AdkAgentBinding: recording: list[tuple[str, str | None]] | None = field(default=None, repr=False) #: ``tool_name -> lines`` of the constructions that list it (#872). tool_sites: dict[str, list[int]] = field(default_factory=dict) + #: What each tool is bound under, when only under a condition (#909). + when: Conditions = field(default_factory=Conditions, repr=False) def bind( self, tool_name: str, locator: str | None = None, location: str | None = None @@ -1058,6 +1070,8 @@ def __init__( # reused: ``extract`` iterates it, and the sub-agent spelling map # below is built from it. self.agent_call_list = self._agent_calls() + # Built on first use: most constructions spell a literal list. + self._lists: ListExpressions | None = None self.parents = { child: node for node in ast.walk(tree) @@ -1065,6 +1079,23 @@ def __init__( } self.agent_names_by_variable = self._agent_names_by_variable() + @property + def lists(self) -> ListExpressions: + """Members of a ``tools=`` or ``sub_agents=`` expression (#909).""" + + if self._lists is None: + self._lists = ListExpressions( + ref=self.source_ref, + tree=self.tree, + scopes=self.scopes, + bindings=self.module.bindings if self.module is not None else _module_bindings(self.tree)[0], + module=self.module, + resolver=self.resolver, + agent_reads=lambda call, keyword: keyword in {"tools", "sub_agents"} + and self._is_agent_call(call), + ) + return self._lists + def extract(self) -> list[LoadedToolSource]: tools: list[Tool] = [] loaded_sources: list[LoadedToolSource] = [] @@ -1106,7 +1137,7 @@ def extract(self) -> list[LoadedToolSource]: binding = self._binding_for(agent_name, call) loaded_sources.extend( self._read_construction( - call, agent_name, binding, ast.List(elts=elements, ctx=ast.Load()), tools, handoffs + call, agent_name, binding, [ListMember(item, None) for item in elements], tools, handoffs ) ) for line in conditional: @@ -1121,25 +1152,50 @@ def extract(self) -> list[LoadedToolSource]: # Not the same agent as another construction of its name. self.agent_sites.setdefault(agent_name, {})[id(call)] = (call.lineno, call) continue - if not isinstance(tools_expr, (ast.List, ast.Tuple)): - if tools_expr is not None: - self._surface_warning( - f"Google ADK agent {agent_name!r} uses a dynamic tools expression.", - SURFACE_GAP_DYNAMIC_TOOLS, - ) - self.artifacts.toolsets.append( - GoogleAdkToolset( - kind="dynamic", - source_id=self.source_id, - source_ref=f"{self.source_ref}:{call.lineno}", - agent_name=agent_name, - dynamic=True, - ) - ) + if tools_expr is None: + # No tools, but a construction all the same: two of one name + # that hand off differently are not one agent (#909 review). + binding = self._binding_for(agent_name, call) + loaded_sources.extend( + self._read_construction(call, agent_name, binding, [], tools, handoffs) + ) continue + if isinstance(tools_expr, ast.List | ast.Tuple) and not any( + isinstance(item, ast.Starred) for item in tools_expr.elts + ): + members = [ListMember(item, None) for item in tools_expr.elts] + unread = None + else: + # ``[*BASE, *([extra] if wanted else [])]``, ``base + extra``, + # a module list, a filter (#909): every member it can hold. + listed = self.lists.resolve(tools_expr) + members = list(listed.members) + self.artifacts.agents[-1]["tool_count"] = len({id(member.expr) for member in members}) + unread = listed if listed.unresolved else None + if _at_risk(listed): + # Read, but not proven for ``scan``: a list is checked for + # changes only in the module that binds it, and another + # module could change it. No warning, so the comparison + # reads it. + self._note_surface_gap(SURFACE_GAP_DYNAMIC_TOOLS) binding = self._binding_for(agent_name, call) + if unread is not None: + pointer = f"{self.source_ref}:{call.lineno}" + message = unread_list_reason("Google ADK", agent_name, pointer, unread) + self._surface_warning(message, SURFACE_GAP_DYNAMIC_TOOLS) + if message not in binding.issues: + binding.issues.append(message) + self.artifacts.toolsets.append( + GoogleAdkToolset( + kind="dynamic", + source_id=self.source_id, + source_ref=pointer, + agent_name=agent_name, + dynamic=True, + ) + ) loaded_sources.extend( - self._read_construction(call, agent_name, binding, tools_expr, tools, handoffs) + self._read_construction(call, agent_name, binding, members, tools, handoffs) ) self._record_duplicate_constructions() self._record_agent_subclasses() @@ -1160,7 +1216,7 @@ def _read_construction( call: ast.Call, agent_name: str, binding: _AdkAgentBinding, - tools_expr: ast.List | ast.Tuple, + members: list[ListMember], tools: list[Tool], handoffs: list[dict[str, Any]], ) -> list[LoadedToolSource]: @@ -1177,9 +1233,19 @@ def _read_construction( state_at = (list(binding.issues), dict(binding.tool_issues), set(binding.duplicated)) binding.recording = [] loaded: list[LoadedToolSource] = [] + conditioned: set[tuple[str, str]] = set() + always: set[str] = set() try: - for item in tools_expr.elts: - loaded.extend(self._extract_tool_expr(item, tools, agent_name, binding)) + for member in members: + at = len(binding.recording) + loaded.extend(self._extract_member(member, tools, agent_name, binding)) + for tool_name, _ in binding.recording[at:]: + if tool_name in binding.tool_names: + binding.when.add(tool_name, member.conditions) + if member.conditions: + conditioned.add((tool_name, " and ".join(member.conditions))) + else: + always.add(tool_name) finally: recorded, binding.recording = binding.recording, None for tool_name, _ in recorded or (): @@ -1204,6 +1270,15 @@ def _read_construction( ( frozenset(recorded or ()), tuple(sorted(name for handoff in handoffs for name in handoff["sub_agents"])), + frozenset(conditioned), + frozenset(always), + tuple( + sorted( + (name, tuple(alternatives)) + for handoff in handoffs + for name, alternatives in (handoff.get("conditions") or {}).items() + ) + ), ) if clean else call @@ -1487,23 +1562,34 @@ def _is_imported_module_path(self, node: ast.AST) -> bool: bindings = self.name_bindings.get(current.id, []) return len(bindings) == 1 and isinstance(bindings[0], ast.alias) - def _name_is_proven(self, name: str) -> bool: + def _name_is_proven(self, name: str, module: PythonModule | None = None) -> bool: """Whether ``name`` unambiguously refers to what the flat maps say. True only when the module binds the name exactly once, at module scope, through a statement that is a direct child of the module body. A second binding of any kind — a parameter, a class, an import, a later assignment, a ``global`` declaration — means the resolution is a guess - about which one was in effect, and a guess is not a proof. + about which one was in effect, and a guess is not a proof. ``module`` + is where the name is written, when not here: a member of a list + another module builds (#909). """ - bindings = self.name_bindings.get(name, []) + if module is None or module is self.module: + bindings, tree, parents = self.name_bindings.get(name, []), self.tree, self.parents + else: + bindings = self._names_of(module)[1].get(name, []) + tree, parents = module.tree, self._scopes_for(module).parents if len(bindings) != 1: return False - binding = bindings[0] - if self._scope_of(binding) is not self.tree: + scope = parents.get(bindings[0]) + while scope is not None and not isinstance(scope, _SCOPE_NODES): + scope = parents.get(scope) + if scope is not tree: return False - return isinstance(self._top_level_statement(binding), _TOP_LEVEL_BINDING_STATEMENTS) + top: ast.AST = bindings[0] + while parents.get(top) is not None and parents.get(top) is not tree: + top = parents[top] + return isinstance(top, _TOP_LEVEL_BINDING_STATEMENTS) def _top_level_statement(self, node: ast.AST) -> ast.AST | None: """The direct child of the module body that contains ``node``.""" @@ -1649,7 +1735,7 @@ def _scope_of(self, node: ast.AST) -> ast.AST | None: current = self.parents.get(current) return current - def _sub_agent_name(self, variable: str, call: ast.Call) -> str | None: + def _sub_agent_name(self, variable: str, call: ast.AST) -> str | None: """Resolve one ``sub_agents`` element to an agent defined in this module. Walks scopes innermost-out from the referencing call, so a factory's @@ -1701,6 +1787,7 @@ def _binding_observations(self) -> list[AgentBindingObservation]: } if len(self.agent_sites.get(binding.agent, {})) > 1 else {}, + tool_conditions=binding.when.only_when(binding.duplicated), tools_complete=not binding.issues, issues=list(binding.issues), ) @@ -1787,6 +1874,35 @@ def _slot_for(self, call: ast.Call, agent_name: str) -> str: self.inline_slot_counts[agent_name] = count return f"#{count}" + def _extract_member( + self, + member: ListMember, + tools: list[Tool], + agent_name: str, + binding: _AdkAgentBinding, + ) -> list[LoadedToolSource]: + """One member of an agent's tools list, read where it is written (#909).""" + + module = member.module + if module is None: + return self._extract_tool_expr(member.expr, tools, agent_name, binding) + spelling = reference_spelling(member.expr) + if spelling is None or self.resolver is None: + message = ( + f"Google ADK agent {agent_name!r} binds `{source_text(member.expr)}`, written in " + f"{module.ref}:{member.expr.lineno}, which this reader does not resolve." + ) + self._surface_warning(message, SURFACE_GAP_UNRESOLVED_EXPRESSION) + if message not in binding.issues: + binding.issues.append(message) + return [] + resolution, long_running = self._resolve_reference(spelling, module) + if resolution is not None and resolution.resolved: + self._bind_resolved(resolution, tools, agent_name, binding, long_running, spelled_in=module) + else: + self._unresolved_reference(agent_name, spelling, resolution) + return [] + def _extract_tool_expr( self, expr: ast.AST, @@ -2311,16 +2427,22 @@ def _append_wrapper_tool( f"Google ADK tool wrapper {wrapper_name!r} has no statically resolvable function.", ) - def _resolve_reference(self, spelling: str) -> tuple[Resolution | None, bool]: + def _resolve_reference( + self, spelling: str, module: PythonModule | None = None + ) -> tuple[Resolution | None, bool]: """Follow ``spelling`` through this module's imports (#864). The flag is True when the chain went through a - ``LongRunningFunctionTool(...)`` built in another module. + ``LongRunningFunctionTool(...)`` built in another module. ``module`` + is where the spelling is written, when not here: a member of a list + another module builds (#909). """ if self.resolver is None or self.module is None: return None, False - resolution, long_running = self._through_wrapper(self.resolver.resolve(self.module, spelling)) + resolution, long_running = self._through_wrapper( + self.resolver.resolve(module or self.module, spelling) + ) value, home = resolution.value, resolution.module if resolution.resolved or not isinstance(value, ast.Call) or home is None: return resolution, long_running @@ -2975,6 +3097,8 @@ def _bind_resolved( agent_name: str, binding: _AdkAgentBinding, long_running: bool, + *, + spelled_in: PythonModule | None = None, ) -> None: """Bind the definition an import chain reached, once per definition.""" @@ -2983,8 +3107,10 @@ def _bind_resolved( # A factory's function: its call was proven where it was read (#865). made = any(step.get("binding") == "factory" for step in resolution.steps) if not made: - # The spelling this module used has to hold up like a local name. - self._require_proven_name(resolution.reference.split(".", 1)[0]) + # The spelling has to hold up like a local name where it is + # written: here, or in the module whose list holds it (#909). + if not self._name_is_proven(resolution.reference.split(".", 1)[0], spelled_in): + self._note_surface_gap(SURFACE_GAP_SHADOWED_DEFINITION) if any(step.get("module_getattr") for step in resolution.steps): # A package ``__getattr__`` could have answered before the # submodule did; the definition is named, not proven. @@ -3528,12 +3654,25 @@ def _record_agent_callbacks_plugins_subagents(self, call: ast.Call, agent_name: } ) elif keyword.arg == "sub_agents": - elements = ( - keyword.value.elts - if isinstance(keyword.value, ast.List | ast.Tuple) - else None - ) - sub_agent_count = len(elements) if elements is not None else None + value = keyword.value + if isinstance(value, ast.List | ast.Tuple) and not any( + isinstance(item, ast.Starred) for item in value.elts + ): + members = [ListMember(item, None) for item in value.elts] + complete = True + unread = None + else: + # A spread, concatenation, conditional or module list (#909). + # Read, not proven for ``scan``: another module could change + # the list, and the sub-agents it holds bring their tools. + listed = self.lists.resolve(value) + members, complete = list(listed.members), listed.complete + unread = unread_parts(listed) if listed.unresolved else None + if _at_risk(listed): + self._note_surface_gap(SURFACE_GAP_DYNAMIC_TOOLS) + # None, unless every member is read: the graph then names the + # list as not statically named. + sub_agent_count = len(members) if complete else None # Three outcomes per element, kept apart because they mean # different things to the binding graph. Resolved to an agent # this module defines: a real handoff target. Named but @@ -3545,11 +3684,23 @@ def _record_agent_callbacks_plugins_subagents(self, call: ast.Call, agent_name: # all — an inline construction, a call: left to the count. sub_agent_names: list[str] = [] unresolved_sub_agents: list[str] = [] - for item in elements or []: + when = Conditions() + for member in members: + item = member.expr + if member.module is not None: + # An agent another module's list names: not one this + # module defines, so not matched, as an import is not. + spelling = reference_spelling(item) + if spelling is not None: + unresolved_sub_agents.append(spelling) + self._note_surface_gap(SURFACE_GAP_UNRESOLVED_SUB_AGENT) + continue variable = _qualified_name(item, self.aliases) if variable is None: continue - resolved = self._sub_agent_name(variable, call) + # Read from where the member is written: a module list's + # names are the module's. + resolved = self._sub_agent_name(variable, item) if resolved is None: unresolved_sub_agents.append(variable) # A handoff target this module does not define owns @@ -3562,6 +3713,7 @@ def _record_agent_callbacks_plugins_subagents(self, call: ast.Call, agent_name: self._note_surface_gap(SURFACE_GAP_UNRESOLVED_SUB_AGENT) else: sub_agent_names.append(resolved) + when.add(resolved, member.conditions) self.artifacts.sub_agents.append( { "agent_name": agent_name, @@ -3573,6 +3725,11 @@ def _record_agent_callbacks_plugins_subagents(self, call: ast.Call, agent_name: "sub_agent_count": sub_agent_count, "sub_agents": sub_agent_names, "unresolved_sub_agents": unresolved_sub_agents, + # ``name -> [condition, ...]`` for a sub-agent held + # only under a condition (#909). + "conditions": when.only_when(), + # Each part of the list not read, and where it is. + **({"unread": unread} if unread else {}), "source_ref": f"{self.source_ref}:{call.lineno}", } ) @@ -4526,6 +4683,17 @@ def _top_statement(node: ast.AST, function: ast.AST, parents: dict[ast.AST, ast. return current +def _at_risk(listed: ListResolution) -> bool: + """Whether a list's reading depends on a binding another module could change. + + Following a name did, even to an empty list, and so did reading another + module's list; a literal spread into a literal (``[a, *[b]]``, + ``[a] + [b]``) did not. + """ + + return bool(listed.unresolved) or listed.followed + + _TOP_LEVEL_BINDING_STATEMENTS = ( ast.Assign, ast.AnnAssign, diff --git a/src/agents_shipgate/inputs/list_expressions.py b/src/agents_shipgate/inputs/list_expressions.py new file mode 100644 index 00000000..12835f6f --- /dev/null +++ b/src/agents_shipgate/inputs/list_expressions.py @@ -0,0 +1,1169 @@ +"""Static members of an agent's tools, handoffs or sub-agents expression (#909). + +Both application readers used to read a ``tools=`` argument only when it was a +literal list or a name bound to one. An agent whose list is built by an +expression -- ``[*BASE, *([handoff] if wanted else [])]``, ``base + (extra or +[])``, ``[t for t in TOOLS if keep(t)]`` -- read as "uses a dynamic tools +expression", and none of its bindings were compared, even when every member +was a plain tool the reader understands. + +This module resolves such an expression to the elements it can hold, without +importing or running anything: + +- a list or tuple literal, splicing each ``*`` spread; +- ``a + b``; +- ``a if c else b`` and ``a or b``: every branch, each member *conditional* on + the condition that selects it; +- ``[x for x in L if f]`` and ``filter(f, L)``: the members of ``L``, + conditional on the filter -- an over-approximation, and said to be one; +- ``list(L)``, ``tuple(L)`` and ``sorted(L)``; +- a name bound once -- in the enclosing function or at module level -- to any + of the above and never changed in place, followed through + repository-local imports. + +Whatever it cannot follow is an :class:`UnresolvedPart` with a reason and a +location. Resolving the rest never makes the expression complete. + +The in-place change test (``read_only_use``) moved here from the OpenAI Agents +SDK reader, which used it for literal lists only (#879 review): every use of a +list must be one that cannot change it -- iterated, indexed, compared, tested, +spread, handed to a read-only builtin, to an agent's own list argument, or to +a function that treats its parameter the same way. +""" + +from __future__ import annotations + +import ast +from collections.abc import Callable +from dataclasses import dataclass, field +from pathlib import PurePosixPath +from typing import Any + +from agents_shipgate.inputs.python_imports import ( + ImportResolver, + PythonModule, + Resolution, + ScopeIndex, + _Stop, + reference_spelling, + reflective_access, +) +from agents_shipgate.inputs.python_static import dotted_name + +MAX_DEPTH = 16 +#: Members one expression may hold before the reader stops counting them. +MAX_MEMBERS = 1000 +#: Expressions one resolution may visit: a list spread into itself many +#: times grows exponentially, so the work is bounded, not only the depth. +MAX_VISITS = 20000 + +#: Calls that read the values they are given and never change them. +READ_ONLY_CALLS = frozenset( + { + "len", "print", "repr", "str", "bool", "id", "hash", "isinstance", "type", + "list", "tuple", "set", "frozenset", "sorted", "reversed", "enumerate", "iter", + "any", "all", "sum", "min", "max", "zip", "map", "filter", + "copy.copy", "copy.deepcopy", "json.dumps", "pprint", "pprint.pprint", + "pprint.pformat", "pformat", + } +) +LOG_METHODS = frozenset({"debug", "info", "warning", "error", "exception", "critical", "log"}) + +#: ``(name, site) -> [(binding node, its statement)]``, empty when unbound. +BindingsAt = Callable[[str, ast.AST], list[tuple[ast.AST, ast.AST | None]]] +#: Whether passing a list to ``call`` (at a position or keyword) only reads it. +CallReads = Callable[[ast.Call, int | None, str | None], bool] +#: Whether ``call`` builds an agent that reads the list it is given as +#: ``keyword`` -- the framework's own constructors, supplied by its reader. +AgentReads = Callable[[ast.Call, str | None], bool] + +#: Builtins whose result holds the same members as their one argument. +_SAME_MEMBERS = frozenset({"list", "tuple", "sorted"}) +#: An agent's list arguments, and the attributes it keeps them under. +CAPABILITY_FIELDS = frozenset({"tools", "handoffs", "sub_agents", "mcp_servers"}) + + +@dataclass(frozen=True) +class ListMember: + """One element an expression can hold, written where ``module`` says.""" + + expr: ast.expr + #: ``None``: the module being read. Otherwise the module an imported list + #: is written in; its names are that module's, not the reader's. + module: PythonModule | None + #: Every condition that must hold for the member to be present; empty when + #: it always is. Conditions are source text, read and never evaluated. + conditions: tuple[str, ...] = () + #: Each list the member came through, outermost first: ``NAME (path:line)``. + via: tuple[str, ...] = () + + +@dataclass(frozen=True) +class UnresolvedPart: + reason: str + location: str + + +@dataclass(frozen=True) +class ListResolution: + members: tuple[ListMember, ...] = () + unresolved: tuple[UnresolvedPart, ...] = () + #: Whether reading it followed a name to its binding, even one that held + #: nothing: a binding some other module could change. + followed: bool = False + + @property + def complete(self) -> bool: + return not self.unresolved + + def __add__(self, other: ListResolution) -> ListResolution: + # The same element under the same conditions is one member, however + # many lists spread it: binding it twice binds nothing more. + seen = {_identity(member) for member in self.members} + added = [] + for member in other.members: + if _identity(member) not in seen: + seen.add(_identity(member)) + added.append(member) + return ListResolution( + self.members + tuple(added), + self.unresolved + other.unresolved, + self.followed or other.followed, + ) + + def under(self, condition: str) -> ListResolution: + return ListResolution( + tuple( + ListMember(m.expr, m.module, (condition, *m.conditions), m.via) + for m in self.members + ), + self.unresolved, + self.followed, + ) + + def through(self, step: str) -> ListResolution: + return ListResolution( + tuple(ListMember(m.expr, m.module, m.conditions, (step, *m.via)) for m in self.members), + self.unresolved, + True, + ) + + +def bindings_at(scopes: ScopeIndex, module_bindings: dict[str, list[Any]]) -> BindingsAt: + # ``from helpers import *`` may bind any name: none is proven unbound. + star = any(isinstance(node, ast.alias) and node.name == "*" for node in scopes.parents) + + def found(name: str, site: ast.AST) -> list[tuple[ast.AST, ast.AST | None]]: + local = scopes.enclosing_bindings(evaluation_site(scopes, site), name) + if local: + return [(item, scopes.statement_of(item)) for item in local] + module = [(item.node, item.statement) for item in module_bindings.get(name, [])] + return module or ([(site, None)] if star else []) + + return found + + +def leaves_arguments_alone(call: ast.Call, bindings: BindingsAt) -> bool: + """Whether ``call`` is a builtin, a standard-library reader or a logging + method, which only read what they are handed. + + The spelling proves nothing alone: a ``print`` imported from the + application's helpers, or an ``.info()`` on an object of its own, may + change the list (#879 review). A bare name is the builtin only when nothing + binds it, or the standard-library reader when it is imported from that + module; ``json.dumps`` only when ``json`` is the standard library's; a + logging method only on ``logging`` or a logger ``getLogger()`` returned. + """ + + name = dotted_name(call.func) + if name in READ_ONLY_CALLS: + head, _, rest = name.partition(".") + found = bindings(head, call) + if not found: + return not rest + if len(found) != 1: + return False + node, statement = found[0] + if not isinstance(node, ast.alias): + return False + if isinstance(statement, ast.ImportFrom): + # ``from pprint import pprint``. + return not rest and not statement.level and f"{statement.module}.{node.name}" in READ_ONLY_CALLS + # ``import json`` then ``json.dumps``. + return bool(rest) and isinstance(statement, ast.Import) and node.name == head and node.asname is None + if not (isinstance(call.func, ast.Attribute) and call.func.attr in LOG_METHODS): + return False + receiver = call.func.value + if not isinstance(receiver, ast.Name): + return False + found = bindings(receiver.id, call) + if len(found) != 1: + return False + node, statement = found[0] + if isinstance(node, ast.alias): + # ``logging.info(...)``. + return isinstance(statement, ast.Import) and node.name == "logging" and receiver.id == "logging" + value = getattr(statement, "value", None) + # ``logger = logging.getLogger(__name__)``. + return ( + isinstance(statement, ast.Assign | ast.AnnAssign) + and isinstance(value, ast.Call) + and (reference_spelling(value.func) or "").rsplit(".", 1)[-1] in {"getLogger", "get_logger"} + ) + + +def parameter_left_alone( + function: ast.FunctionDef | ast.AsyncFunctionDef, name: str, bindings: BindingsAt +) -> bool: + """Whether every use of parameter ``name`` in ``function`` only reads it. + + The same test as a module list's own uses, one level deep: handing it on + to any call but a read-only builtin or logging method is not a read. + ``bindings`` answers for the function's own module. + """ + + parents = {child: node for node in ast.walk(function) for child in ast.iter_child_nodes(node)} + for node in ast.walk(function): + if isinstance(node, ast.Global | ast.Nonlocal) and name in node.names: + return False + if isinstance(node, ast.Name) and node.id == name: + if not isinstance(node.ctx, ast.Load): + return False + if not read_only_use( + node, parents, lambda call, *_: leaves_arguments_alone(call, bindings) + ): + return False + return True + + +def read_only_use( + node: ast.expr, + parents: dict[ast.AST, ast.AST], + call_reads: CallReads, +) -> bool: + """Whether this load of a list can only read it, never change or hand it on.""" + + parent = parents.get(node) + if isinstance(parent, ast.keyword): + call = parents.get(parent) + return isinstance(call, ast.Call) and call_reads(call, None, parent.arg) + if isinstance(parent, ast.Call): + for position, arg in enumerate(parent.args): + if arg is node: + return call_reads(parent, position, None) + return False + if isinstance(parent, ast.For | ast.AsyncFor | ast.comprehension): + return parent.iter is node + if isinstance(parent, ast.Subscript): + return parent.value is node and isinstance(parent.ctx, ast.Load) + if isinstance(parent, ast.BoolOp) or ( + isinstance(parent, ast.IfExp) and parent.test is not node + ): + # ``TOOLS or [x]`` may be the list itself: its own use decides. + return read_only_use(parent, parents, call_reads) + if isinstance(parent, ast.If | ast.While | ast.IfExp | ast.Assert): + return parent.test is node + if isinstance(parent, ast.UnaryOp): + return isinstance(parent.op, ast.Not) + if isinstance(parent, ast.Starred): + # ``[*TOOLS, x]`` or ``f(*TOOLS)`` spreads the members; the list itself + # goes nowhere. + return parent.value is node + if isinstance(parent, ast.Compare | ast.FormattedValue | ast.Expr | ast.BinOp): + # A comparison, a string, a bare expression, or ``TOOLS + [x]`` (a new list). + return True + if isinstance(parent, ast.Attribute) and parent.value is node: + grand = parents.get(parent) + called = isinstance(grand, ast.Call) and grand.func is parent + if called: + return parent.attr in {"count", "index", "copy", "get", "keys", "values", "items"} + # ``helpers.TOOLS`` handed on: ``helpers`` is read, and the attribute's + # own use decides. A call (``helpers.register(x)``) may change anything. + return isinstance(parent.ctx, ast.Load) and read_only_use(parent, parents, call_reads) + return False + + +@dataclass +class _View: + """One module as this resolver reads it.""" + + ref: str + tree: ast.Module + scopes: ScopeIndex + bindings: dict[str, list[Any]] + module: PythonModule | None + #: Bindings some code changes in place: ``id(local binding)`` or + #: ``("module", name)``. + changed: set[object] + #: Whether this is the module the agent is built in: only there do the + #: framework's own constructions read the list they are handed. + entry: bool = False + #: Lines of the module's ``from x import *``, which may rebind any name. + star_lines: tuple[int, ...] = () + #: Imports whose binding some code changes in place, wherever in the + #: module: the list they import is changed, whatever name reaches it. + changed_imports: list[tuple[ast.alias, ast.stmt]] = field(default_factory=list) + #: ``bindings_at`` for this module, built once. + lookup: BindingsAt | None = None + + +@dataclass +class ListCache: + """What reading lists learns about modules, shared by every module one load reads. + + An imported module is indexed once however many agent files import it + (#909 review). Keys hold the tree's id and whether the view is the + module an agent is built in; the cache keeps every view, and so every + tree, alive for as long as the ids are used. + """ + + views: dict[tuple[int, bool], _View] = field(default_factory=dict) + memo: dict[tuple[int, bool, int], ListResolution] = field(default_factory=dict) + changes: dict[tuple[int, bool, int, str], int | None] = field(default_factory=dict) + callees: dict[tuple[int, str], bool] = field(default_factory=dict) + scopes: dict[int, ScopeIndex] = field(default_factory=dict) + + +class ListExpressions: + """Resolve list expressions read in one module, following its imports. + + ``agent_reads`` tells the in-place change test which calls are the + framework's own agent constructions, whose list arguments are reads. + """ + + def __init__( + self, + *, + ref: str, + tree: ast.Module, + scopes: ScopeIndex, + bindings: dict[str, list[Any]], + module: PythonModule | None, + resolver: ImportResolver | None, + agent_reads: AgentReads, + cache: ListCache | None = None, + ) -> None: + self._resolver = resolver + self._agent_reads = agent_reads + self._cache = cache if cache is not None else ListCache() + self._visits = 0 + self.entry = self._view(ref, tree, scopes, bindings, module, entry=True) + + # -- views ------------------------------------------------------------- + + def _view( + self, + ref: str, + tree: ast.Module, + scopes: ScopeIndex, + bindings: dict[str, list[Any]], + module: PythonModule | None, + *, + entry: bool = False, + ) -> _View: + star_lines = tuple( + statement.lineno + for statement in tree.body + if isinstance(statement, ast.ImportFrom) + and any(alias.name == "*" for alias in statement.names) + ) + key = (id(tree), entry) + cached = self._cache.views.get(key) + if cached is not None: + return cached + view = _View(ref, tree, scopes, bindings, module, set(), entry, star_lines) + view.lookup = bindings_at(scopes, bindings) + self._cache.views[key] = view + self._index_changes(view) + return view + + def _foreign(self, module: PythonModule) -> _View: + if module.tree is self.entry.tree: + return self.entry + cached = self._cache.views.get((id(module.tree), False)) + if cached is not None: + return cached + return self._view(module.ref, module.tree, self._scopes(module.tree), module.bindings, module) + + def _scopes(self, tree: ast.Module) -> ScopeIndex: + scopes = self._cache.scopes.get(id(tree)) + if scopes is None: + scopes = self._cache.scopes[id(tree)] = ScopeIndex(tree) + return scopes + + def _index_changes(self, view: _View) -> None: + """Mark every binding some code may change in place (#879 review).""" + + # A name the reader could read as a list: one bound to an expression + # it follows. A name bound to anything else is never read, so a change + # to it does not matter, and following its uses would cost a callee + # read per call (#909 review). + tracked = { + target.id + for node in ast.walk(view.tree) + if isinstance(node, ast.Assign | ast.AnnAssign) and _listish(node.value) + for target in (node.targets if isinstance(node, ast.Assign) else [node.target]) + if isinstance(target, ast.Name) + } | { + (alias.asname or alias.name).split(".", 1)[0] + for node in ast.walk(view.tree) + if isinstance(node, ast.Import | ast.ImportFrom) + for alias in node.names + if alias.name != "*" + } + if reflective_access(view.tree) is not None: + # ``globals()["TOOLS"]``, ``vars()`` and ``sys.modules[__name__]`` + # reach a module list without spelling its name. + view.changed.update(("module", name) for name in tracked) + call_reads = self._call_reads(view) + for node in ast.walk(view.tree): + if isinstance(node, ast.Global): + view.changed.update(("module", name) for name in node.names) + elif isinstance(node, ast.Nonlocal): + for name in node.names: + found = view.scopes.enclosing_bindings(node, name) + if found: + view.changed.add(id(found[0])) + elif isinstance(node, ast.Attribute) and node.attr in CAPABILITY_FIELDS: + # ``helper.tools.append(x)`` changes the list ``helper`` was + # built with, which other agents may read too, here or in any + # module importing it. ``helper.tools = [...]`` only replaces it. + parent = view.scopes.parents.get(node) + if isinstance(node.ctx, ast.Load): + changes = not read_only_use(node, view.scopes.parents, call_reads) + else: + changes = isinstance(parent, ast.AugAssign) + if changes: + view.changed.update(self._built_with(view, node.value)) + elif isinstance(node, ast.MatchMapping) and node.rest in tracked: + # ``case {**tools}:`` rebinds ``tools`` where it matches. + found = view.scopes.enclosing_bindings(evaluation_site(view.scopes, node), node.rest) + view.changed.update(id(binding) for binding in found) + view.changed.add(("module", node.rest)) + elif isinstance(node, ast.Name) and node.id in tracked: + parent = view.scopes.parents.get(node) + if isinstance(node.ctx, ast.Load): + unchanged = read_only_use(node, view.scopes.parents, call_reads) + else: + # A walrus rebinds the name in the scope around any + # comprehension it sits in, which no binding index sees. + unchanged = isinstance(node.ctx, ast.Store) and not isinstance( + parent, ast.AugAssign | ast.NamedExpr + ) + if not unchanged: + self._mark_changed(view, node) + + def _mark_changed(self, view: _View, node: ast.Name) -> None: + """Mark every binding a change at ``node`` may reach (#909 review). + + The name is looked up where Python evaluates it: a default, decorator + or annotation in the scope around its definition, a class body's code + in the class and then outside it, since the class may not have bound + the name yet. A walrus binds around the comprehensions it sits in. + """ + + site = evaluation_site(view.scopes, node) + found = view.scopes.enclosing_bindings(site, node.id) + view.changed.add(id(found[0]) if found else ("module", node.id)) + for binding in found: + view.changed.add(id(binding)) + statement = view.scopes.statement_of(binding) + if isinstance(binding, ast.alias) and isinstance(statement, ast.Import | ast.ImportFrom): + view.changed_imports.append((binding, statement)) + if not found: + for item in view.bindings.get(node.id, []): + if isinstance(item.node, ast.alias) and isinstance(item.statement, ast.Import | ast.ImportFrom): + view.changed_imports.append((item.node, item.statement)) + scope = _nearest_scope(view.scopes, site) + walrus = isinstance(view.scopes.parents.get(node), ast.NamedExpr) + if isinstance(scope, ast.ClassDef) or walrus: + outer = view.scopes.enclosing_bindings(scope, node.id) if scope is not None else [] + view.changed.add(id(outer[0]) if outer else ("module", node.id)) + for binding in outer: + view.changed.add(id(binding)) + if walrus: + view.changed.add(("module", node.id)) + + def _built_with(self, view: _View, receiver: ast.expr) -> set[object]: + """The list bindings an agent named ``receiver`` was built with.""" + + if not isinstance(receiver, ast.Name): + return set() + site = evaluation_site(view.scopes, receiver) + local = view.scopes.enclosing_bindings(site, receiver.id) + if local: + statements = [view.scopes.statement_of(binding) for binding in local] + else: + statements = [item.statement for item in view.bindings.get(receiver.id, [])] + keys: set[object] = set() + for statement in statements: + value = getattr(statement, "value", None) + if isinstance(value, ast.Call): + fields = [item.value for item in value.keywords if item.arg in CAPABILITY_FIELDS] + keys |= shared_lists(view.scopes, view.bindings, fields) + return keys + + def _call_reads(self, view: _View) -> CallReads: + bindings = view.lookup or bindings_at(view.scopes, view.bindings) + + def reads(call: ast.Call, position: int | None, keyword: str | None) -> bool: + if leaves_arguments_alone(call, bindings): + return True + # Another module's ``Agent`` may be its own class: only the module + # the agent is built in says which constructions are the framework's. + if view.entry and self._agent_reads(call, keyword): + return True + return self._callee_leaves_alone(view, call, position, keyword) + + return reads + + def _callee_leaves_alone( + self, view: _View, call: ast.Call, position: int | None, keyword: str | None + ) -> bool: + """Whether the function ``call`` names never changes the argument it passes.""" + + spelling = reference_spelling(call.func) + if spelling is None or self._resolver is None or view.module is None: + return False + if view.scopes.enclosing_bindings(evaluation_site(view.scopes, call), spelling.split(".", 1)[0]): + # A parameter, local or nested ``def`` of that name is what is + # called here, not the module's function. + return False + resolution = self._resolver.resolve(view.module, spelling) + function = resolution.definition if resolution.resolved else None + if function is None or function.decorator_list: + # A decorator may hand the list to code this does not read. + return False + positional = [*function.args.posonlyargs, *function.args.args] + if position is not None: + if position >= len(positional) or any( + isinstance(arg, ast.Starred) for arg in call.args[:position] + ): + return False + parameter = positional[position].arg + elif keyword in {arg.arg for arg in [*positional, *function.args.kwonlyargs]}: + parameter = str(keyword) + else: + return False + defining = resolution.module + assert defining is not None + key = (id(function), parameter) + if key not in self._cache.callees: + self._cache.callees[key] = parameter_left_alone( + function, parameter, bindings_at(self._scopes(defining.tree), defining.bindings) + ) + return self._cache.callees[key] + + # -- resolution -------------------------------------------------------- + + def resolve(self, expr: ast.expr | None) -> ListResolution: + """The members ``expr``, read in the entry module, can hold.""" + + if expr is None: + return ListResolution() + self._visits = 0 + result = self._resolve(expr, self.entry, 0, frozenset()) + if len(result.members) > MAX_MEMBERS: + return self._stop(self.entry, expr, f"the list holds more than {MAX_MEMBERS} members, more than the reader follows") + return result + + def _where(self, view: _View, node: ast.AST) -> str: + return f"{view.ref}:{getattr(node, 'lineno', '?')}" + + def _stop(self, view: _View, node: ast.AST, reason: str) -> ListResolution: + return ListResolution(unresolved=(UnresolvedPart(reason, self._where(view, node)),)) + + def _member(self, view: _View, node: ast.expr) -> ListResolution: + return ListResolution(members=(ListMember(node, None if view is self.entry else view.module),)) + + def _resolve(self, node: ast.expr, view: _View, depth: int, seen: frozenset) -> ListResolution: + self._visits += 1 + if self._visits > MAX_VISITS: + return self._stop(view, node, "the expression is larger than the reader follows") + if depth > MAX_DEPTH: + return self._stop(view, node, "the expression nests further than the reader follows") + if isinstance(node, ast.List | ast.Tuple): + result = ListResolution() + for item in node.elts: + if isinstance(item, ast.Starred): + result += self._resolve(item.value, view, depth + 1, seen) + else: + result += self._member(view, item) + return result + if isinstance(node, ast.Constant) and node.value is None: + return ListResolution() + if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add): + return self._resolve(node.left, view, depth + 1, seen) + self._resolve( + node.right, view, depth + 1, seen + ) + if isinstance(node, ast.IfExp): + return self._choice(node, view, depth, seen) + if isinstance(node, ast.BoolOp) and isinstance(node.op, ast.Or): + return self._either(node, view, depth, seen) + if isinstance(node, ast.ListComp | ast.GeneratorExp): + return self._filtered_comprehension(node, view, depth, seen) + if isinstance(node, ast.Call): + return self._call(node, view, depth, seen) + if isinstance(node, ast.Name): + return self._name(node, view, depth, seen) + if isinstance(node, ast.Attribute): + return self._attribute(node, view, depth, seen) + return self._stop(view, node, f"`{_source(node)}` is not an expression the reader follows") + + def _choice(self, node: ast.IfExp, view: _View, depth: int, seen: frozenset) -> ListResolution: + if isinstance(node.test, ast.Constant): + branch = node.body if node.test.value else node.orelse + return self._resolve(branch, view, depth + 1, seen) + test = _condition(node.test) + return self._resolve(node.body, view, depth + 1, seen).under(f"`{test}`") + self._resolve( + node.orelse, view, depth + 1, seen + ).under(f"not `{test}`") + + def _either(self, node: ast.BoolOp, view: _View, depth: int, seen: frozenset) -> ListResolution: + """``a or b or c``: the first operand that is not empty, else the last.""" + + result = ListResolution() + reached: tuple[str, ...] = () + last = len(node.values) - 1 + for index, value in enumerate(node.values): + part = self._resolve(value, view, depth + 1, seen) + fixed = part.complete and all(not member.conditions for member in part.members) + if index == last or _always_true(value, view): + # The last operand, or one that is true however empty its + # members are (a ``filter`` object, a generator): the value. + return result + _all_under(part, reached) + if fixed and part.members: + # Known not empty: it is the value, and nothing after it is reached. + return result + _all_under(part, reached) + if fixed: + # Known empty: never the value. + continue + text = _condition(value) + result += _all_under(part, (*reached, f"`{text}` is not empty")) + reached = (*reached, f"`{text}` is empty") + return result + + def _filtered_comprehension( + self, node: ast.ListComp | ast.GeneratorExp, view: _View, depth: int, seen: frozenset + ) -> ListResolution: + generators = node.generators + if not ( + len(generators) == 1 + and not generators[0].is_async + and isinstance(generators[0].target, ast.Name) + and isinstance(node.elt, ast.Name) + and node.elt.id == generators[0].target.id + ): + return self._stop(view, node, "a comprehension that builds new elements is not followed") + part = self._resolve(generators[0].iter, view, depth + 1, seen) + if not generators[0].ifs: + return part + kept = " and ".join(_condition(test) for test in generators[0].ifs) + return part.under(f"the filter `{kept}` keeps it") + + def _call(self, node: ast.Call, view: _View, depth: int, seen: frozenset) -> ListResolution: + func = node.func + builtin = ( + func.id + if isinstance(func, ast.Name) and not (view.lookup or bindings_at(view.scopes, view.bindings))(func.id, func) + else None + ) + if builtin in _SAME_MEMBERS and len(node.args) == 1 and not node.keywords: + return self._resolve(node.args[0], view, depth + 1, seen) + if builtin == "filter" and len(node.args) == 2 and not node.keywords: + predicate, iterable = node.args + part = self._resolve(iterable, view, depth + 1, seen) + if isinstance(predicate, ast.Constant) and predicate.value is None: + return part + return part.under(f"the filter `{_condition(predicate)}` keeps it") + return self._stop(view, node, f"a call to `{_source(func)}`, whose result is not read") + + def _name(self, node: ast.Name, view: _View, depth: int, seen: frozenset) -> ListResolution: + name = node.id + local = view.scopes.enclosing_bindings(evaluation_site(view.scopes, node), name) + if local: + if len(local) != 1: + return self._stop(view, node, f"`{name}` is bound more than once in its function") + binding = local[0] + if isinstance(binding, ast.arg): + function = _enclosing_function(view.scopes, binding) + where = f" of `{function.name}`" if function is not None else "" + return self._stop(view, node, f"`{name}` is a parameter{where}, so its value comes from a caller") + statement = view.scopes.statement_of(binding) + if isinstance(binding, ast.alias) and isinstance(statement, ast.Import | ast.ImportFrom): + if id(binding) in view.changed: + return self._stop(view, statement, f"`{name}` may be changed in place after it is imported") + return self._imported_local(node, binding, statement, view, depth, seen) + return self._assigned( + node, binding, statement, view, depth, seen, changed=id(binding) in view.changed + ) + bindings = view.bindings.get(name, []) + if not bindings: + return self._stop(view, node, f"`{name}` is not bound where the list is read") + if len(bindings) != 1 or not bindings[0].top_level: + return self._stop(view, node, f"`{name}` is bound more than once, or conditionally, in {view.ref}") + binding = bindings[0] + rebinding = _star_after(view, binding.statement) + if rebinding is not None: + return self._stop(view, node, f"`{name}` may be rebound by the wildcard import at {view.ref}:{rebinding}") + if isinstance(binding.node, ast.alias): + return self._imported(node, name, view, depth, seen) + return self._assigned( + node, binding.node, binding.statement, view, depth, seen, + changed=("module", name) in view.changed, + ) + + def _assigned( + self, + node: ast.Name, + binding: ast.AST, + statement: ast.AST | None, + view: _View, + depth: int, + seen: frozenset, + *, + changed: bool, + ) -> ListResolution: + name = node.id + if not ( + isinstance(binding, ast.Name) + and isinstance(statement, ast.Assign | ast.AnnAssign) + and statement.value is not None + and _single_target(statement) == name + ): + kind = "a function" if isinstance(binding, ast.FunctionDef | ast.AsyncFunctionDef) else "not a list" + return self._stop(view, node, f"`{name}` is {kind}, not a list of tools") + if _conditional_between(view.scopes, statement, node): + return self._stop(view, statement, f"`{name}` is bound only under a condition or in a loop") + if changed: + return self._stop(view, statement, f"`{name}` may be changed in place after it is built") + key = (id(view.tree), id(statement)) + if key in seen: + return self._stop(view, statement, f"`{name}` refers to itself") + step = f"{name} ({view.ref}:{statement.lineno})" + return self._value_of(key, statement.value, view, depth, seen).through(step) + + def _value_of( + self, key: tuple[int, int], value: ast.expr, view: _View, depth: int, seen: frozenset + ) -> ListResolution: + """A name's value, read once however many lists spread it.""" + + memo = (key[0], view.entry, key[1]) + cached = self._cache.memo.get(memo) + if cached is None: + cached = self._resolve(value, view, depth + 1, seen | {key}) + self._cache.memo[memo] = cached + return cached + + def _imported(self, node: ast.expr, spelling: str, view: _View, depth: int, seen: frozenset) -> ListResolution: + if self._resolver is None or view.module is None: + return self._stop(view, node, f"`{spelling}` is imported, and imports are not followed here") + resolution = self._resolver.resolve(view.module, spelling) + return self._from_resolution(node, spelling, resolution, view, depth, seen) + + def _imported_local( + self, + node: ast.Name, + alias: ast.alias, + statement: ast.Import | ast.ImportFrom, + view: _View, + depth: int, + seen: frozenset, + ) -> ListResolution: + if self._resolver is None or view.module is None: + return self._stop(view, node, f"`{node.id}` is imported, and imports are not followed here") + resolution = self._resolver.resolve_local_import(view.module, statement, alias, node.id) + return self._from_resolution(node, node.id, resolution, view, depth, seen) + + def _from_resolution(self, node, spelling, resolution, view, depth, seen) -> ListResolution: + if resolution.definition is not None: + return self._stop(view, node, f"`{spelling}` is a function, not a list of tools") + value, defining = resolution.value, resolution.module + if value is None or defining is None: + return self._stop(view, node, f"`{spelling}` is not resolved: {resolution.detail}") + if resolution.caveats: + return self._stop(view, node, f"`{spelling}` is not established: {'; '.join(resolution.caveats)}") + foreign = self._foreign(defining) + name = next( + (step["name"] for step in reversed(resolution.steps) if step.get("binding") == "value"), + None, + ) + if name is None: + return self._stop(view, node, f"`{spelling}` does not end at an assignment") + bindings = foreign.bindings.get(name, []) + if len(bindings) != 1 or not bindings[0].top_level or ("module", name) in foreign.changed: + return self._stop(view, node, f"`{name}` in {foreign.ref} may be changed in place or rebound") + rebinding = _star_after(foreign, bindings[0].statement) + if rebinding is not None: + return self._stop( + view, node, f"`{name}` may be rebound by the wildcard import at {foreign.ref}:{rebinding}" + ) + head = spelling.split(".", 1)[0] + if ("module", head) in view.changed: + return self._stop(view, node, f"`{head}` may be changed in place in {view.ref}") + for other in [view, *self._chain_views(resolution, defining, view)]: + changer = self._changed_through(other, defining, name) + if changer is not None: + return self._stop( + view, node, f"`{name}` in {foreign.ref} may be changed in place at {other.ref}:{changer}" + ) + statement = bindings[0].statement + key = (id(foreign.tree), id(statement)) + if key in seen: + return self._stop(view, node, f"`{spelling}` refers to itself") + step = f"{name} ({foreign.ref}:{statement.lineno})" + return self._value_of(key, value, foreign, depth, seen).through(step) + + def _chain_views(self, resolution: Resolution, defining: PythonModule, view: _View) -> list[_View]: + """The other modules importing the list runs: each one the import passes + through, and each package above the defining module, which runs first. + + A change from a module the import never passes through is not looked for. + """ + + assert self._resolver is not None + refs = {step["path"] for step in resolution.steps if step.get("binding") == "import"} + packages = PurePosixPath(defining.ref).parts[:-1] + refs.update(f"{'/'.join(packages[:depth])}/__init__.py" for depth in range(1, len(packages) + 1)) + views = [] + for ref in sorted(refs - {defining.ref, view.ref}): + path = self._resolver.scope_root / ref + if not path.is_file(): + continue + try: + views.append(self._foreign(self._resolver.module(path))) + except _Stop: + continue + return views + + def _changed_through(self, view: _View, defining: PythonModule, name: str) -> int | None: + """The line of an import ``view`` changes in place that reaches ``defining``. + + ``from tools import BASE as B; B.append(x)`` in a sibling function, or + ``import tools; tools.BASE.append(x)``, changes the list another + spelling of it reads. Any import of the defining module, or of a name + in it, whose binding is changed counts, which may also count a + different list of that module. + """ + + if self._resolver is None or view.module is None: + return None + key = (id(view.tree), view.entry, id(defining.tree), name) + if key not in self._cache.changes: + self._cache.changes[key] = self._first_change_reaching(view, defining, name) + return self._cache.changes[key] + + def _first_change_reaching(self, view: _View, defining: PythonModule, name: str) -> int | None: + """An import changed in ``view`` that is the list, or the module holding it.""" + + assert self._resolver is not None and view.module is not None + for alias, statement in view.changed_imports: + local = alias.asname or alias.name.split(".", 1)[0] + # ``from tools import BASE as B`` is the list; ``import tools`` is + # the module holding it, so ``tools.BASE`` reaches it. + for spelling in (local, f"{local}.{name}"): + if statement in view.tree.body: + resolution = self._resolver.resolve(view.module, spelling) + else: + resolution = self._resolver.resolve_local_import(view.module, statement, alias, spelling) + if resolution.module is defining and _value_name(resolution) == name: + return statement.lineno + return None + + def _attribute(self, node: ast.Attribute, view: _View, depth: int, seen: frozenset) -> ListResolution: + spelling = reference_spelling(node) + root = node + while isinstance(root, ast.Attribute): + root = root.value + if isinstance(root, ast.Name) and root.id in {"self", "cls"}: + return self._stop(view, node, f"`{_source(node)}` is an attribute of the object, set elsewhere") + if spelling is None or not isinstance(root, ast.Name): + return self._stop(view, node, f"`{_source(node)}` is not an expression the reader follows") + local = view.scopes.enclosing_bindings(evaluation_site(view.scopes, node), root.id) + if local: + # The function's own ``root``, not the module's: a parameter, a + # local value, or an import made in the function. + binding = local[0] + statement = view.scopes.statement_of(binding) + if ( + len(local) == 1 + and isinstance(binding, ast.alias) + and isinstance(statement, ast.Import | ast.ImportFrom) + and id(binding) not in view.changed + and self._resolver is not None + and view.module is not None + ): + resolution = self._resolver.resolve_local_import(view.module, statement, binding, spelling) + return self._from_resolution(node, spelling, resolution, view, depth, seen) + return self._stop( + view, node, f"`{root.id}` is bound in its function, so `{spelling}` is not the module's" + ) + return self._imported(node, spelling, view, depth, seen) + + +class Conditions: + """Which names a list holds only under a condition. + + Each alternative is one way the name gets in, its conditions joined with + "and". A name the list also holds unconditionally has none. + """ + + def __init__(self) -> None: + self._alternatives: dict[str, set[str]] = {} + self._always: set[str] = set() + + def add(self, name: str, conditions: tuple[str, ...]) -> None: + if conditions: + self._alternatives.setdefault(name, set()).add(" and ".join(conditions)) + else: + self._always.add(name) + + def only_when(self, dropped: set[str] | None = None) -> dict[str, list[str]]: + return { + name: sorted(alternatives) + for name, alternatives in self._alternatives.items() + if name not in self._always and name not in (dropped or set()) + } + + +def unread_parts(listed: ListResolution) -> str: + return "; ".join(f"{part.reason} ({part.location})" for part in listed.unresolved) + + +def unread_list_reason(framework: str, target: str, pointer: str, listed: ListResolution) -> str: + """One sentence naming every part of a tools list the reader could not read. + + A list read in part keeps its readable members; the agent stays incomplete + and each unread part is named where it is. + """ + + if listed.members: + return ( + f"{framework} agent {target!r} at {pointer} has a tools list it reads only in " + f"part; its binding graph is incomplete. Not read: {unread_parts(listed)}." + ) + return ( + f"{framework} agent {target!r} at {pointer} uses a dynamic tools expression; its " + f"binding graph is incomplete. Not read: {unread_parts(listed)}." + ) + + +def source_text(node: ast.AST) -> str: + return _source(node) + + +def evaluation_site(scopes: ScopeIndex, node: ast.AST) -> ast.AST: + """The node whose enclosing scope Python evaluates ``node`` in. + + A function's defaults, annotations and decorators, a class's bases, + keywords and decorators, and a comprehension's first iterable run in the + scope around them; ``ScopeIndex`` would look them up inside. They are + anchored at the definition, whose enclosing scope is the right one. + """ + + child, parent = node, scopes.parents.get(node) + via_iter = False + while parent is not None and not isinstance(parent, ast.Module): + if isinstance(parent, ast.comprehension): + via_iter = child is parent.iter + elif isinstance(parent, ast.ListComp | ast.SetComp | ast.DictComp | ast.GeneratorExp): + if via_iter and parent.generators and child is parent.generators[0]: + return evaluation_site(scopes, parent) + return node + elif isinstance(parent, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef): + return node if child in parent.body else evaluation_site(scopes, parent) + elif isinstance(parent, ast.Lambda): + return node if child is parent.body else evaluation_site(scopes, parent) + child, parent = parent, scopes.parents.get(parent) + return node + + +def _nearest_scope(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.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef | ast.Lambda + | ast.ListComp | ast.SetComp | ast.DictComp | ast.GeneratorExp, + ): + current = scopes.parents.get(current) + return current + + +def _identity(member: ListMember) -> tuple[int, int, tuple[str, ...]]: + return (id(member.expr), id(member.module), member.conditions) + + +def _condition(node: ast.AST) -> str: + """A condition's whole source text: it is compared, so never shortened.""" + + return ast.unparse(node) + + +def _always_true(node: ast.expr, view: _View) -> bool: + """A value true however empty it is: a ``filter``/``map`` object or a generator.""" + + if isinstance(node, ast.GeneratorExp): + return True + return ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id in {"filter", "map", "iter", "reversed", "zip", "enumerate"} + and not (view.lookup or bindings_at(view.scopes, view.bindings))(node.func.id, node.func) + ) + + +def _star_after(view: _View, statement: ast.AST | None) -> int | None: + """The line of a wildcard import after ``statement``, which may rebind its name.""" + + line = getattr(statement, "lineno", 0) + return next((star for star in view.star_lines if star > line), None) + + +def _all_under(part: ListResolution, conditions: tuple[str, ...]) -> ListResolution: + for condition in reversed(conditions): + part = part.under(condition) + return part + + +def _source(node: ast.AST) -> str: + text = ast.unparse(node) + return text if len(text) <= 160 else text[:157] + "..." + + +def _single_target(statement: ast.Assign | ast.AnnAssign) -> str | None: + targets = statement.targets if isinstance(statement, ast.Assign) else [statement.target] + return targets[0].id if len(targets) == 1 and isinstance(targets[0], ast.Name) else None + + +def _conditional_between(scopes: ScopeIndex, statement: ast.AST, use: ast.AST) -> bool: + """Whether ``statement`` runs under a condition or in a loop that ``use`` is outside of. + + A ``with`` block, or a ``try`` body, runs whenever the code around it does. + A branch, a loop body, a handler or a ``match`` case does not, unless the + use sits in the same branch: then the binding always precedes it. + """ + + child = statement + current = scopes.parents.get(statement) + while current is not None and not isinstance( + current, ast.Module | ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef | ast.Lambda + ): + branch = _branch_holding(current, child) + if branch is not None and not any(_within(scopes, use, item) for item in branch): + return True + child = current + current = scopes.parents.get(current) + return False + + +def _branch_holding(compound: ast.AST, child: ast.AST) -> list[ast.AST] | None: + """The statements of ``compound`` that run only on some paths and hold ``child``.""" + + if isinstance(compound, ast.If | ast.For | ast.AsyncFor | ast.While): + return compound.body if child in compound.body else compound.orelse + if isinstance(compound, ast.Try | ast.TryStar): + if child in compound.body or child in compound.finalbody: + return None + return compound.orelse if child in compound.orelse else [child] + if isinstance(compound, ast.ExceptHandler | ast.match_case): + return compound.body + if isinstance(compound, ast.Match): + return [child] + return None + + +def _within(scopes: ScopeIndex, node: ast.AST, ancestor: ast.AST) -> bool: + current: ast.AST | None = node + while current is not None: + if current is ancestor: + return True + current = scopes.parents.get(current) + return False + + +def _listish(value: ast.expr | None) -> bool: + """Whether a name bound to ``value`` is one the reader may read as a list.""" + + if isinstance(value, ast.Call): + return isinstance(value.func, ast.Name) and value.func.id in _SAME_MEMBERS | {"filter"} + return isinstance( + value, + ast.List | ast.Tuple | ast.ListComp | ast.GeneratorExp | ast.BinOp | ast.IfExp + | ast.BoolOp | ast.Name | ast.Attribute, + ) or (isinstance(value, ast.Constant) and value.value is None) + + +def _value_name(resolution: Resolution) -> str | None: + return next( + (step["name"] for step in reversed(resolution.steps) if step.get("binding") == "value"), + None, + ) + + +def shared_lists( + scopes: ScopeIndex, module_bindings: dict[str, list[Any]], values: list[ast.expr] +) -> set[object]: + """Every list binding these values may be the very object of (#909). + + ``BASE``, ``BASE or []`` and ``A if c else B`` are the list itself, not a + copy, and so is a name bound to one of them; a literal, ``+``, a + comprehension or ``list(...)`` builds a new one. Keys are the binding's + id, or ``("module", name)``. + """ + + found: set[object] = set() + pending = [(value, 0) for value in values] + while pending: + node, depth = pending.pop() + if depth > MAX_DEPTH: + continue + if isinstance(node, ast.BoolOp) and isinstance(node.op, ast.Or): + pending.extend((value, depth + 1) for value in node.values) + elif isinstance(node, ast.IfExp): + pending.extend([(node.body, depth + 1), (node.orelse, depth + 1)]) + elif isinstance(node, ast.Name): + local = scopes.enclosing_bindings(evaluation_site(scopes, node), node.id) + key: object = id(local[0]) if local else ("module", node.id) + if key in found: + continue + found.add(key) + found.update(id(binding) for binding in local) + if local: + statement = scopes.statement_of(local[0]) if len(local) == 1 else None + else: + bindings = module_bindings.get(node.id, []) + statement = bindings[0].statement if len(bindings) == 1 else None + value = getattr(statement, "value", None) + if isinstance(statement, ast.Assign | ast.AnnAssign) and value is not None: + # ``TOOLS = BASE``: one object under a second name. + pending.append((value, depth + 1)) + return found + + +def _enclosing_function(scopes: ScopeIndex, node: ast.AST) -> ast.FunctionDef | ast.AsyncFunctionDef | None: + current = scopes.parents.get(node) + while current is not None: + if isinstance(current, ast.FunctionDef | ast.AsyncFunctionDef): + return current + current = scopes.parents.get(current) + return None + + +__all__ = [ + "CAPABILITY_FIELDS", + "MAX_DEPTH", + "Conditions", + "ListCache", + "ListExpressions", + "ListMember", + "ListResolution", + "UnresolvedPart", + "bindings_at", + "evaluation_site", + "leaves_arguments_alone", + "read_only_use", + "shared_lists", + "source_text", + "unread_list_reason", + "unread_parts", +] diff --git a/src/agents_shipgate/inputs/openai_sdk_static.py b/src/agents_shipgate/inputs/openai_sdk_static.py index da11c803..e1bd4c38 100644 --- a/src/agents_shipgate/inputs/openai_sdk_static.py +++ b/src/agents_shipgate/inputs/openai_sdk_static.py @@ -22,6 +22,21 @@ ) from agents_shipgate.inputs.config_trace import trace_config_binding from agents_shipgate.inputs.coverage import BoundaryCell, SourceCoverage +from agents_shipgate.inputs.list_expressions import ( + Conditions, + ListCache, + ListExpressions, + ListMember, + shared_lists, + source_text, + unread_list_reason, + unread_parts, +) +from agents_shipgate.inputs.list_expressions import bindings_at as _bindings_at +from agents_shipgate.inputs.list_expressions import ( + leaves_arguments_alone as _leaves_arguments_alone, +) +from agents_shipgate.inputs.list_expressions import read_only_use as _read_only_use from agents_shipgate.inputs.protocol import LoadedAdapterResult from agents_shipgate.inputs.python_imports import ( NOT_BOUND, @@ -32,7 +47,6 @@ _module_bindings, local_binding_detail, reference_spelling, - reflective_access, ) from agents_shipgate.inputs.python_static import ( display_path, @@ -216,6 +230,9 @@ def _extract_agent_bindings( } ) imports = _ImportedTools(tools, source, base_dir) + # What reading one file's lists learns about a module it imports serves + # every other file of this load (#909 review). + list_cache = ListCache() for path in paths: text = load_text_file(path) tree = parse_python_file(path, label="OpenAI Agents SDK") @@ -259,19 +276,30 @@ def identity_of( 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, - module.bindings if module is not None else _module_bindings(tree)[0], - sdk_names=sdk_names, - resolve=( - (lambda spelling, module=module: imports.resolver.resolve(module, spelling)) - if module is not None - else None + module_bindings = module.bindings if module is not None else _module_bindings(tree)[0] + #: Every agent identity this file constructs: a handoff from another + #: module's list spelled the same would be read as this file's agent. + local_identities = { + identity + for node in ast.walk(tree) + if isinstance(node, ast.Call) and _denotes_agent(sdk_names, node) + for identity in (identity_of(node),) + if identity is not None + } + lists = ListExpressions( + ref=source_ref, + tree=tree, + scopes=scopes, + bindings=module_bindings, + module=module, + resolver=imports.resolver if module is not None else None, + agent_reads=lambda call, keyword, sdk_names=sdk_names, scopes=scopes, module_bindings=module_bindings: _agent_reads( + sdk_names, scopes, module_bindings, call, keyword ), + cache=list_cache if module is not None else None, ) subclasses = _agent_subclasses(tree, sdk_names) - values = _AgentValues(tree, scopes, tool_lists.module_bindings, sdk_names, subclasses) + values = _AgentValues(tree, scopes, module_bindings, sdk_names, subclasses) #: ``list binding -> identities`` of agents constructed with that list. list_holders: dict[object, set[str]] = {} copies: list[ast.Call] = [] @@ -352,14 +380,14 @@ def unread_agent( ) 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) + # The list objects this agent may hold: an in-place change through + # any agent sharing one reaches this one too. + for shared in shared_lists( + scopes, + module_bindings, + [value for value in (_keyword(call, "tools"), _keyword(call, "handoffs")) if value is not None], + ): + list_holders.setdefault(shared, set()).add(target) opaque = _opaque_arguments(call) if opaque is not None: reason = ( @@ -376,38 +404,52 @@ def unread_agent( ) continue tools_expr = _keyword(call, "tools") - references = tool_lists.references(tools_expr, call) + listed = lists.resolve(tools_expr) issues: list[str] = [] tools_complete = True names: list[str] = [] locators: dict[str, str] = {} tool_issues: dict[str, str] = {} - if references is None: - reason = ( - f"OpenAI Agents SDK agent {target!r} at {pointer} uses a " - "dynamic tools expression; its binding graph is incomplete." - ) + when = Conditions() + if listed.unresolved: + reason = unread_list_reason("OpenAI Agents SDK", target, pointer, listed) warnings.append(reason) issues.append(reason) - literal_concat = _literal_tool_list_concatenation(tools_expr) recovery_evidence.append(SourceRecoveryEvidence( warning=reason, source_id=source.id, source_type="openai_agents_sdk", source_ref=pointer, path=source_ref, - recovery=CoverageRecovery( - kind="reader_limitation" if literal_concat else "unresolved", - reason=( - "sdk_literal_tool_list_concatenation_unsupported" if literal_concat - else "sdk_tools_expression_unresolved" - ), - ), + recovery=CoverageRecovery(kind="unresolved", reason="sdk_tools_expression_unresolved"), )) tools_complete = False - else: - # Two different definitions under one tool name: the model - # sees one name for both, so neither is bound (#879 review). - duplicated: set[str] = set() - first_location: dict[str, str] = {} - for reference, element in references: + # Two different definitions under one tool name: the model + # sees one name for both, so neither is bound (#879 review). + duplicated: set[str] = set() + first_location: dict[str, str] = {} + for member in listed.members: + element = member.expr + reference = reference_spelling(element) + if reference is None: + reason = ( + f"OpenAI Agents SDK agent {target!r} at {pointer} binds the tool " + f"expression `{source_text(element)}`, which is not a reference this " + "reader resolves." + ) + warnings.append(reason) + issues.append(reason) + recovery_evidence.append(SourceRecoveryEvidence( + warning=reason, source_id=source.id, source_type="openai_agents_sdk", + source_ref=pointer, path=source_ref, + recovery=CoverageRecovery(kind="unresolved", reason="sdk_tools_expression_unresolved"), + )) + tools_complete = False + continue + if member.module is not None: + # A member of a list another module builds: its names are + # that module's (#909). + tool, detail = imports.tool_for( + reference, member.module, member.module.ref, tool_by_name, {} + ) + else: head = reference.split(".", 1)[0] # Read where the reference is written: a module-level # list's names are the module's, whatever the agent's @@ -450,61 +492,88 @@ def unread_agent( tool, detail = imports.tool_for( reference, module, source_ref, tool_by_name, import_aliases ) - if tool is None: - reason = ( - f"OpenAI Agents SDK agent {target!r} at {pointer} binds " - f"unresolved tool {reference!r}" - + (f": {detail}." if detail else ".") - ) - warnings.append(reason) - issues.append(reason) - tools_complete = False + if tool is None: + reason = ( + f"OpenAI Agents SDK agent {target!r} at {pointer} binds " + f"unresolved tool {reference!r}" + + (f": {detail}." if detail else ".") + ) + warnings.append(reason) + issues.append(reason) + tools_complete = False + if member.module is None: names.append(import_aliases.get(reference, reference)) - continue - locator = f"{tool.source_ref}#{tool.name}" if tool.source_ref else None - if tool.name in duplicated: - continue - bound = locators.get(tool.name) - if bound is not None and locator is not None and bound != locator: - reason = ( - f"OpenAI Agents SDK agent {target!r} at {pointer} binds two " - f"different functions named {tool.name!r} " - f"({first_location.get(tool.name, bound.split('#', 1)[0])} and " - f"{tool.source_location}); the model sees one tool name for " - "both, so neither is resolved." - ) - warnings.append(reason) - issues.append(reason) - tools_complete = False - duplicated.add(tool.name) - names = [name for name in names if name != tool.name] - locators.pop(tool.name, None) - tool_issues.pop(tool.name, None) - continue - names.append(tool.name) - if locator is not None: - locators[tool.name] = locator - first_location.setdefault(tool.name, tool.source_location or locator) - if detail: - # Named, never established: code that runs first is - # not read (#879 review). - reason = ( - f"OpenAI Agents SDK agent {target!r} at {pointer} binds " - f"{tool.name!r} ({tool.source_location}), but {detail}; the " - "definition read for it is not established as the one bound." - ) - warnings.append(reason) - tool_issues[tool.name] = reason - handoff_names = tool_lists.names( - _keyword(call, "handoffs"), call, import_aliases, identity_of, sdk_names - ) + when.add(names[-1], member.conditions) + continue + locator = f"{tool.source_ref}#{tool.name}" if tool.source_ref else None + if tool.name in duplicated: + continue + bound = locators.get(tool.name) + if bound is not None and locator is not None and bound != locator: + reason = ( + f"OpenAI Agents SDK agent {target!r} at {pointer} binds two " + f"different functions named {tool.name!r} " + f"({first_location.get(tool.name, bound.split('#', 1)[0])} and " + f"{tool.source_location}); the model sees one tool name for " + "both, so neither is resolved." + ) + warnings.append(reason) + issues.append(reason) + tools_complete = False + duplicated.add(tool.name) + names = [name for name in names if name != tool.name] + locators.pop(tool.name, None) + tool_issues.pop(tool.name, None) + continue + names.append(tool.name) + when.add(tool.name, member.conditions) + if locator is not None: + locators[tool.name] = locator + first_location.setdefault(tool.name, tool.source_location or locator) + if detail: + # Named, never established: code that runs first is + # not read (#879 review). + reason = ( + f"OpenAI Agents SDK agent {target!r} at {pointer} binds " + f"{tool.name!r} ({tool.source_location}), but {detail}; the " + "definition read for it is not established as the one bound." + ) + warnings.append(reason) + tool_issues[tool.name] = reason + tool_conditions = when.only_when(duplicated) + handoff_list = lists.resolve(_keyword(call, "handoffs")) handoffs_complete = True - if handoff_names is None: - reason = f"OpenAI Agents SDK agent {target!r} has dynamic handoffs at {pointer}." + handoff_names: list[str] = [] + handoff_when = Conditions() + if handoff_list.unresolved: + reason = ( + f"OpenAI Agents SDK agent {target!r} has dynamic handoffs at {pointer}. " + f"Not read: {unread_parts(handoff_list)}." + ) warnings.append(reason) issues.append(reason) handoffs_complete = False - handoff_names = [] + for member in handoff_list.members: + identity, why_not = _handoff_identity( + member, + scopes=scopes, + aliases=import_aliases, + identity_of=identity_of, + sdk_names=sdk_names, + resolver=imports.resolver if module is not None else None, + local_identities=local_identities, + ) + if identity is None: + reason = ( + f"OpenAI Agents SDK agent {target!r} at {pointer} hands off to " + f"`{source_text(member.expr)}`, which is not resolved to an agent: {why_not}." + ) + warnings.append(reason) + issues.append(reason) + handoffs_complete = False + continue + handoff_names.append(identity) + handoff_when.add(identity, member.conditions) file_observations.append( AgentBindingObservation( agent=target, @@ -514,7 +583,9 @@ def unread_agent( tool_names=names, tool_locators=locators, tool_issues=tool_issues, + tool_conditions=tool_conditions, handoff_names=handoff_names, + handoff_conditions=handoff_when.only_when(), tools_complete=tools_complete, handoffs_complete=handoffs_complete, issues=issues, @@ -594,6 +665,10 @@ def unread_agent( tuple(observation.tool_names), tuple(sorted(observation.tool_locators.items())), tuple(observation.handoff_names), + tuple(sorted((k, tuple(v)) for k, v in observation.tool_conditions.items())), + tuple( + sorted((k, tuple(v)) for k, v in observation.handoff_conditions.items()) + ), observation.tools_complete, observation.handoffs_complete, tuple(observation.issues), @@ -736,19 +811,110 @@ def tool_from_resolution(self, resolution: Resolution) -> tuple[Tool | None, str return tool, "; ".join(resolution.caveats) or None -def _literal_tool_list_concatenation(value: ast.AST | None) -> bool: - """One proven reader limitation, never a claim about deployed wiring. +def _agent_reads( + sdk_names: _SdkNames, + scopes: ScopeIndex, + module_bindings: dict[str, list[Any]], + call: ast.Call, + keyword: str | None, +) -> bool: + """Whether ``call`` builds an agent, or a copy of one, that reads its list argument. - Python defines addition of two literal lists, but this reader's name-list - resolver has no BinOp branch. Calls, unpacking and other expressions do - not prove a product-owned repair and deliberately remain unresolved. + The spelling proves nothing (#909 review): ``.clone`` counts only on a name + bound to the SDK's ``Agent``, and ``replace`` only when it is the standard + library's ``dataclasses``/``copy`` function. """ - return ( - isinstance(value, ast.BinOp) - and isinstance(value.op, ast.Add) - and isinstance(value.left, ast.List) - and isinstance(value.right, ast.List) - and all(isinstance(item, ast.Name) for item in [*value.left.elts, *value.right.elts]) + + if keyword not in {"tools", "handoffs", "mcp_servers"}: + return False + if sdk_names.denotes(dotted_name(call.func), call, "Agent", DEFAULT_AGENT_CONSTRUCTORS): + return True + if isinstance(call.func, ast.Attribute) and call.func.attr == "clone": + receiver = call.func.value + if not isinstance(receiver, ast.Name): + return False + local = scopes.enclosing_bindings(receiver, receiver.id) + if local: + statements = [scopes.statement_of(local[0])] if len(local) == 1 else [] + else: + found = module_bindings.get(receiver.id, []) + statements = [found[0].statement] if len(found) == 1 else [] + return any( + isinstance(getattr(statement, "value", None), ast.Call) + and _denotes_agent(sdk_names, statement.value) + for statement in statements + ) + name = dotted_name(call.func) + if name not in {"replace", "dataclasses.replace", "copy.replace"}: + return False + head = name.split(".", 1)[0] + found = _bindings_at(scopes, module_bindings)(head, call) + if len(found) != 1 or not isinstance(found[0][0], ast.alias): + return False + alias, statement = found[0] + if head == "replace": + return ( + isinstance(statement, ast.ImportFrom) + and not statement.level + and statement.module in {"dataclasses", "copy"} + and alias.name == "replace" + ) + return isinstance(statement, ast.Import) and alias.name == head and alias.asname is None + + +def _handoff_identity( + member: ListMember, + *, + scopes: ScopeIndex, + aliases: dict[str, str], + identity_of: Callable[[ast.Call], str | None], + sdk_names: _SdkNames, + resolver: ImportResolver | None, + local_identities: set[str], +) -> tuple[str | None, str]: + """The agent identity one handoff list member names, or why it names none. + + A member written in the module being read keeps the reader's rule: a name + its scope binds to an agent it constructs is that agent's identity (#876 + review); any other name is spelled through ``from`` import aliases. A + member of a list another module builds is followed into that module, to + the name its construction is assigned to (#909). + """ + + item = member.expr + if not isinstance(item, ast.Name): + return None, "it is not a name" + if member.module is None: + found = scopes.enclosing_bindings(item, item.id) + statement = scopes.statement_of(found[0]) if len(found) == 1 else None + value = getattr(statement, "value", None) + if ( + isinstance(found[0] if found else None, ast.Name) + and isinstance(value, ast.Call) + and _denotes_agent(sdk_names, value) + ): + identity = identity_of(value) + if identity is not None: + return identity, "" + return aliases.get(item.id, item.id), "" + if resolver is None: + return None, f"it is written in {member.module.ref}, and imports are not followed here" + resolution = resolver.resolve(member.module, item.id) + name = next( + (step["name"] for step in reversed(resolution.steps) if step.get("binding") == "value"), + None, + ) + if isinstance(resolution.value, ast.Call) and name is not None and not resolution.caveats: + if name in local_identities: + return None, ( + f"`{name}` in {member.module.ref} has the name of an agent this module builds, " + "so which agent it is is not established" + ) + return name, "" + return None, ( + f"`{item.id}` in {member.module.ref} does not end at an assignment of a construction" + + (f" ({resolution.detail})" if resolution.detail else "") + + (f" ({'; '.join(resolution.caveats)})" if resolution.caveats else "") ) @@ -1348,382 +1514,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) -#: Calls that read the values they are given and never change them. -_READ_ONLY_CALLS = frozenset( - { - "len", "print", "repr", "str", "bool", "id", "hash", "isinstance", "type", - "list", "tuple", "set", "frozenset", "sorted", "reversed", "enumerate", "iter", - "any", "all", "sum", "min", "max", "zip", "map", "filter", - "copy.copy", "copy.deepcopy", "json.dumps", "pprint", "pprint.pprint", - "pprint.pformat", "pformat", - } -) -_LOG_METHODS = frozenset({"debug", "info", "warning", "error", "exception", "critical", "log"}) - - -#: ``(name, site) -> [(binding node, its statement)]``, empty when unbound. -BindingsAt = Callable[[str, ast.AST], list[tuple[ast.AST, ast.AST | None]]] - - -def _bindings_at(scopes: ScopeIndex, module_bindings: dict[str, list[Any]]) -> BindingsAt: - # ``from helpers import *`` may bind any name: none is proven unbound. - star = any(isinstance(node, ast.alias) and node.name == "*" for node in scopes.parents) - - def found(name: str, site: ast.AST) -> list[tuple[ast.AST, ast.AST | None]]: - local = scopes.enclosing_bindings(site, name) - if local: - return [(item, scopes.statement_of(item)) for item in local] - module = [(item.node, item.statement) for item in module_bindings.get(name, [])] - return module or ([(site, None)] if star else []) - - return found - - -def _leaves_arguments_alone(call: ast.Call, bindings_at: BindingsAt) -> bool: - """Whether ``call`` is a builtin, a standard-library reader or a logging - method, which only read what they are handed. - - The spelling proves nothing alone: a ``print`` imported from the - application's helpers, or an ``.info()`` on an object of its own, may - change the list (#879 review). A bare name is the builtin only when nothing - binds it, or the standard-library reader when it is imported from that - module; ``json.dumps`` only when ``json`` is the standard library's; a - logging method only on ``logging`` or a logger ``getLogger()`` returned. - """ - - name = dotted_name(call.func) - if name in _READ_ONLY_CALLS: - head, _, rest = name.partition(".") - found = bindings_at(head, call) - if not found: - return not rest - if len(found) != 1: - return False - node, statement = found[0] - if not isinstance(node, ast.alias): - return False - if isinstance(statement, ast.ImportFrom): - # ``from pprint import pprint``. - return not rest and not statement.level and f"{statement.module}.{node.name}" in _READ_ONLY_CALLS - # ``import json`` then ``json.dumps``. - return bool(rest) and isinstance(statement, ast.Import) and node.name == head and node.asname is None - if not (isinstance(call.func, ast.Attribute) and call.func.attr in _LOG_METHODS): - return False - receiver = call.func.value - if not isinstance(receiver, ast.Name): - return False - found = bindings_at(receiver.id, call) - if len(found) != 1: - return False - node, statement = found[0] - if isinstance(node, ast.alias): - # ``logging.info(...)``. - return isinstance(statement, ast.Import) and node.name == "logging" and receiver.id == "logging" - value = getattr(statement, "value", None) - # ``logger = logging.getLogger(__name__)``. - return ( - isinstance(statement, ast.Assign | ast.AnnAssign) - and isinstance(value, ast.Call) - and (reference_spelling(value.func) or "").rsplit(".", 1)[-1] in {"getLogger", "get_logger"} - ) - - -def _parameter_left_alone( - function: ast.FunctionDef | ast.AsyncFunctionDef, name: str, bindings_at: BindingsAt -) -> bool: - """Whether every use of parameter ``name`` in ``function`` only reads it. - - The same test as a module list's own uses, one level deep: handing it on - to any call but a read-only builtin or logging method is not a read. - ``bindings_at`` answers for the function's own module. - """ - - parents = {child: node for node in ast.walk(function) for child in ast.iter_child_nodes(node)} - for node in ast.walk(function): - if isinstance(node, ast.Global | ast.Nonlocal) and name in node.names: - return False - if isinstance(node, ast.Name) and node.id == name: - if not isinstance(node.ctx, ast.Load): - return False - if not _read_only_use( - node, parents, lambda call, *_: _leaves_arguments_alone(call, bindings_at) - ): - return False - return True - - -def _read_only_use( - node: ast.expr, - parents: dict[ast.AST, ast.AST], - call_reads: Callable[[ast.Call, int | None, str | None], bool], -) -> bool: - """Whether this load of a list can only read it, never change or hand it on.""" - - parent = parents.get(node) - if isinstance(parent, ast.keyword): - call = parents.get(parent) - return isinstance(call, ast.Call) and call_reads(call, None, parent.arg) - if isinstance(parent, ast.Call): - for position, arg in enumerate(parent.args): - if arg is node: - return call_reads(parent, position, None) - return False - if isinstance(parent, ast.For | ast.AsyncFor | ast.comprehension): - return parent.iter is node - if isinstance(parent, ast.Subscript): - return parent.value is node and isinstance(parent.ctx, ast.Load) - if isinstance(parent, ast.BoolOp) or ( - isinstance(parent, ast.IfExp) and parent.test is not node - ): - # ``TOOLS or [x]`` may be the list itself: its own use decides. - return _read_only_use(parent, parents, call_reads) - if isinstance(parent, ast.If | ast.While | ast.IfExp | ast.Assert): - return parent.test is node - if isinstance(parent, ast.UnaryOp): - return isinstance(parent.op, ast.Not) - if isinstance(parent, ast.Starred): - # ``[*TOOLS, x]`` or ``f(*TOOLS)`` spreads the members; the list itself - # goes nowhere. - return parent.value is node - if isinstance(parent, ast.Compare | ast.FormattedValue | ast.Expr | ast.BinOp): - # A comparison, a string, a bare expression, or ``TOOLS + [x]`` (a new list). - return True - if isinstance(parent, ast.Attribute) and parent.value is node: - grand = parents.get(parent) - return ( - parent.attr in {"count", "index", "copy", "get", "keys", "values", "items"} - and isinstance(grand, ast.Call) - and grand.func is parent - ) - return False - - -class _ToolLists: - """Literal lists that a ``tools=NAME`` / ``handoffs=NAME`` refers to (#879 review). - - The name is read where the agent is constructed, through the scope that - binds it there — a builder's local ``tools = [...]`` is that builder's, - never another's; a class body's list is the class body's. It is read only - when that scope binds it once, to a literal list, and nothing in the file - changes that binding in place — ``.append`` from a nested function, a - ``global`` or ``nonlocal`` rebinding, a subscript store. Anything else is a - dynamic expression, never the last assignment. - - Every change site is indexed once, against the binding it changes, so a - lookup costs the depth of the scopes and not the size of the file. - """ - - def __init__( - self, - tree: ast.Module, - scopes: ScopeIndex, - module_bindings: dict[str, list[Any]], - *, - sdk_names: _SdkNames | None = None, - resolve: Callable[[str], Resolution] | None = None, - ) -> None: - self.scopes = scopes - self.module_bindings = module_bindings - self.sdk_names = sdk_names - self.resolve = resolve - self.changed: set[object] = set() - # Only a name bound to a literal list somewhere can be read as one. - listed = { - target.id - for node in ast.walk(tree) - if isinstance(node, ast.Assign | ast.AnnAssign) - and isinstance(node.value, ast.List | ast.Tuple) - for target in (node.targets if isinstance(node, ast.Assign) else [node.target]) - if isinstance(target, ast.Name) - } - # Every use of such a name must be a read that cannot change the list: - # iterated, indexed, compared, tested, handed to a read-only builtin, to - # an agent's own ``tools=``, or to a function that treats its parameter - # the same way. Any other use — a method call, ``+=``, a second name, a - # tuple, a return, ``*args`` — may change it (#879 review). - # ``globals()["TOOLS"]``, ``vars()`` and ``sys.modules[__name__]`` - # reach a module list without spelling its name (#879 review). - if reflective_access(tree) is not None: - self.changed.update(("module", name) for name in listed) - for node in ast.walk(tree): - if isinstance(node, ast.Global): - self.changed.update(("module", name) for name in node.names) - elif isinstance(node, ast.Nonlocal): - for name in node.names: - found = scopes.enclosing_bindings(node, name) - if found: - self.changed.add(id(found[0])) - elif isinstance(node, ast.Name) and node.id in listed: - parent = scopes.parents.get(node) - if isinstance(node.ctx, ast.Load): - unchanged = _read_only_use(node, scopes.parents, self._call_reads) - else: - unchanged = isinstance(node.ctx, ast.Store) and not isinstance( - parent, ast.AugAssign - ) - if not unchanged: - found = scopes.enclosing_bindings(node, node.id) - self.changed.add(id(found[0]) if found else ("module", node.id)) - - def _call_reads(self, call: ast.Call, position: int | None, keyword: str | None) -> bool: - """Whether ``call`` only reads the list it is passed at ``position``/``keyword``.""" - - if _leaves_arguments_alone(call, _bindings_at(self.scopes, self.module_bindings)): - return True - if keyword in {"tools", "handoffs", "mcp_servers"} and ( - ( - self.sdk_names is not None - and self.sdk_names.denotes( - dotted_name(call.func), call, "Agent", DEFAULT_AGENT_CONSTRUCTORS - ) - ) - or (isinstance(call.func, ast.Attribute) and call.func.attr == "clone") - or dotted_name(call.func) in {"replace", "dataclasses.replace", "copy.replace"} - ): - # An agent, or a copy of one, reads its own ``tools=``. - return True - return self._callee_leaves_alone(call, position, keyword) - - def _callee_leaves_alone( - self, call: ast.Call, position: int | None, keyword: str | None - ) -> bool: - """Whether the function ``call`` names never changes the argument it passes.""" - - spelling = reference_spelling(call.func) - if spelling is None or self.resolve is None: - return False - resolution = self.resolve(spelling) - function = resolution.definition if resolution.resolved else None - if function is None: - return False - positional = [*function.args.posonlyargs, *function.args.args] - if position is not None: - if position >= len(positional): - return False - parameter = positional[position].arg - elif keyword in {arg.arg for arg in [*positional, *function.args.kwonlyargs]}: - parameter = str(keyword) - else: - return False - defining = resolution.module - assert defining is not None - return _parameter_left_alone( - function, parameter, _bindings_at(ScopeIndex(defining.tree), defining.bindings) - ) - - def _literal(self, name: str, node: ast.AST) -> ast.List | ast.Tuple | None | bool: - """The one literal list ``name`` holds at ``node``. - - False: not a list variable — a function, an import — so the name is a - reference. None: bound in a way the reader cannot read as one list. - """ - - found = self.scopes.enclosing_bindings(node, name) - if found: - if len(found) != 1: - return None - local = found[0] - if isinstance(local, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef | ast.alias): - return False - statement = self.scopes.statement_of(local) - if ( - isinstance(local, ast.Name) - and isinstance(statement, ast.Assign | ast.AnnAssign) - and _assignment_target(statement) == name - and isinstance(statement.value, ast.List | ast.Tuple) - and id(local) not in self.changed - ): - return statement.value - return None - bindings = self.module_bindings.get(name, []) - if all( - isinstance(item.node, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef | ast.alias) - for item in bindings - ): - return False - if len(bindings) != 1 or not bindings[0].top_level: - return None - statement = bindings[0].statement - if ( - isinstance(statement, ast.Assign | ast.AnnAssign) - and _assignment_target(statement) == name - and isinstance(statement.value, ast.List | ast.Tuple) - and ("module", name) not in self.changed - ): - return statement.value - return None - - def _elements(self, value: ast.AST | None, node: ast.AST) -> list[ast.expr] | None: - if value is None: - return [] - if isinstance(value, ast.List | ast.Tuple): - literal: ast.List | ast.Tuple | None | bool = value - elif isinstance(value, ast.Name): - literal = self._literal(value.id, node) - if literal is False: - return [value] - else: - return None - if not isinstance(literal, ast.List | ast.Tuple) or any( - isinstance(item, ast.Starred) for item in literal.elts - ): - return None - return list(literal.elts) - - def references( - self, value: ast.AST | None, node: ast.AST - ) -> list[tuple[str, ast.expr]] | None: - """``(spelling, element)`` per listed tool; None when not a readable list.""" - - elements = self._elements(value, node) - if elements is None: - return None - references: list[tuple[str, ast.expr]] = [] - for item in elements: - spelling = reference_spelling(item) - if spelling is None: - return None - references.append((spelling, item)) - return references - - def names( - 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. - - 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 - 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 = ( ast.FunctionDef, ast.AsyncFunctionDef, diff --git a/tests/test_adk_tool_factories.py b/tests/test_adk_tool_factories.py index 00d165f6..fa11e462 100644 --- a/tests/test_adk_tool_factories.py +++ b/tests/test_adk_tool_factories.py @@ -347,28 +347,66 @@ def test_a_tool_added_under_a_condition_is_named_beside_the_list(tmp_path): assert observation.issues == [message] +def _dynamic(artifacts, line: int, unread: str) -> None: + """The limit names its agent, its construction and what was not read (#909).""" + + assert ( + f"Google ADK agent 'assign' at agent.py:{line} uses a dynamic tools expression; its " + f"binding graph is incomplete. Not read: {unread}." + ) in artifacts.warnings + + @pytest.mark.parametrize( - "body", + ("body", "unread"), [ - "tools = [intake]\nregister(tools)", - "tools = [intake]\ntools.remove(intake)", - "tools = [intake]\nother = tools\nother.append(route)", - "tools = [intake]\nhelper = lambda: tools.append(route)", - "tools = [intake]\ntools.extend(more_tools())", - "tools = [*base_tools(), intake]", - "if enabled:\n tools = [intake]\nelse:\n tools = [route]", + ("tools = [intake]\nregister(tools)", "`tools` may be changed in place after it is built (agent.py:7)"), + ("tools = [intake]\ntools.remove(intake)", "`tools` may be changed in place after it is built (agent.py:7)"), + ( + "tools = [intake]\nother = tools\nother.append(route)", + "`tools` may be changed in place after it is built (agent.py:7)", + ), + ( + "tools = [intake]\nhelper = lambda: tools.append(route)", + "`tools` may be changed in place after it is built (agent.py:7)", + ), + ( + "tools = [intake]\ntools.extend(more_tools())", + "`tools` may be changed in place after it is built (agent.py:7)", + ), + ( + "if enabled:\n tools = [intake]\nelse:\n tools = [route]", + "`tools` is bound more than once in its function (agent.py:11)", + ), ], - ids=["handed-to-a-call", "removed", "aliased", "nested-function", "extended-by-a-call", "starred", "rebound"], + ids=["handed-to-a-call", "removed", "aliased", "nested-function", "extended-by-a-call", "rebound"], ) -def test_any_other_use_of_the_list_keeps_it_dynamic(tmp_path, body): +def test_any_other_use_of_the_list_keeps_it_dynamic(tmp_path, body, unread): _write(tmp_path, _list_agent(body)) loaded, artifacts = _adk(tmp_path) assert _edges(loaded, artifacts) == [] - assert "Google ADK agent 'assign' uses a dynamic tools expression." in artifacts.warnings + _dynamic(artifacts, 8 + body.count("\n"), unread) + (observation,) = [item for source in loaded for item in source.binding_observations] + # A limit on this agent, at its construction, not on the file (#909). + assert observation.tools_complete is False + + +def test_a_starred_call_keeps_the_rest_of_the_list(tmp_path): + # #909: the readable member is bound; the call it cannot read is named. + _write(tmp_path, _list_agent("tools = [*base_tools(), intake]")) + loaded, artifacts = _adk(tmp_path) + assert [tool for _, tool, _ in _edges(loaded, artifacts)] == ["intake"] + assert ( + "Google ADK agent 'assign' at agent.py:8 has a tools list it reads only in part; its binding " + "graph is incomplete. Not read: a call to `base_tools`, whose result is not read (agent.py:7)." + ) in artifacts.warnings + (observation,) = [item for source in loaded for item in source.binding_observations] + assert observation.tools_complete is False -def test_a_module_level_tools_list_stays_dynamic(tmp_path): - # Another module may change a module's list: not read here. +def test_a_module_level_tools_list_is_read(tmp_path): + # #909: a module's list is read when nothing in that module changes it. + # Another module could, which is not looked for, so ``scan`` does not + # count the surface as proven. _write( tmp_path, { @@ -378,8 +416,24 @@ def test_a_module_level_tools_list_stays_dynamic(tmp_path): }, ) loaded, artifacts = _adk(tmp_path) + assert [tool for _, tool, _ in _edges(loaded, artifacts)] == ["intake"] + assert artifacts.warnings == [] + (tool,) = [tool for source in loaded for tool in source.tools] + assert "dynamic_tools_expression" in tool.extraction["surface_gaps"] + + +def test_a_module_level_tools_list_changed_in_its_module_stays_dynamic(tmp_path): + _write( + tmp_path, + { + "tools.py": LIST_TOOLS, + "agent.py": "from google.adk.agents import LlmAgent\nfrom tools import intake, route\n\n" + "TOOLS = [intake]\nTOOLS.append(route)\nroot = LlmAgent(name='assign', tools=TOOLS)\n", + }, + ) + loaded, artifacts = _adk(tmp_path) assert _edges(loaded, artifacts) == [] - assert "Google ADK agent 'assign' uses a dynamic tools expression." in artifacts.warnings + _dynamic(artifacts, 6, "`TOOLS` may be changed in place after it is built (agent.py:4)") def test_application_diff_does_not_add_a_conditional_tool(tmp_path): diff --git a/tests/test_coverage_recovery.py b/tests/test_coverage_recovery.py index db32c26a..736e7399 100644 --- a/tests/test_coverage_recovery.py +++ b/tests/test_coverage_recovery.py @@ -29,10 +29,6 @@ ROOT = Path(__file__).resolve().parents[1] CASES = [ (None, "input_unavailable", "sdk_entrypoint_not_found", "Restore the existing entrypoint"), - ( - "[read_tool] + [other_tool]", "reader_limitation", - "sdk_literal_tool_list_concatenation_unsupported", "needs a reader repair", - ), ( "get_tools()", "unresolved", "sdk_tools_expression_unresolved", "before choosing a remedy", @@ -244,10 +240,15 @@ def test_redaction_keeps_location_and_warning_private(): assert fact.recovery.kind == "reader_limitation" -@pytest.mark.parametrize("expression", ["[read_tool, other_tool]", "[read_tool]", "[]"]) +@pytest.mark.parametrize("expression", [ + "[read_tool, other_tool]", "[read_tool]", "[]", + # #584's reproducer: read since #909, so its recovery metadata is retired. + "[read_tool] + [other_tool]", +]) def test_supported_syntax_does_not_manufacture_a_recovery(tmp_path, expression): report = _scan(_project(tmp_path / "project", expression), tmp_path / "reports") assert all(gap.recovery is None for gap in report.release_decision.evidence_coverage.evidence_gaps) + assert not any("tools expression" in warning for warning in report.source_warnings) def test_required_sdk_source_uses_the_shared_input_error_contract(tmp_path, monkeypatch): @@ -275,7 +276,7 @@ def test_required_sdk_source_uses_the_shared_input_error_contract(tmp_path, monk def test_recovery_does_not_rewrite_space_bearing_source_locations(tmp_path, capsys): from test_evidence_gap_ranking import _verifier_with - project = _project(tmp_path / "project", "[read_tool] + [other_tool]") + project = _project(tmp_path / "project", "get_tools()") filename = "agent tools.py" (project / "agent.py").rename(project / filename) manifest = project / "shipgate.yaml" diff --git a/tests/test_distribution_surface_parity.py b/tests/test_distribution_surface_parity.py index ae8d1beb..1fd984bf 100644 --- a/tests/test_distribution_surface_parity.py +++ b/tests/test_distribution_surface_parity.py @@ -190,6 +190,10 @@ def paths(self) -> list[Path]: # `effect_evidence` is `assess_tool_semantics` itself over that tool — # the engine's one effect model, called, not restated — so neither adds # a claim either (`tests/test_application_diff_tool_reach.py`). + # A side's `bound_when` (#909) is the source text of the condition a + # list member is held under, read and never evaluated: a fact about the + # source, not an engine answer, so it adds no claim + # (`tests/test_list_expressions.py`). {}, ), Surface( diff --git a/tests/test_google_adk.py b/tests/test_google_adk.py index e5e88c69..d66c63a7 100644 --- a/tests/test_google_adk.py +++ b/tests/test_google_adk.py @@ -1891,7 +1891,9 @@ def test_a_proven_adk_surface_with_declared_actions_reaches_a_merge_verdict(tmp_ "preamble": "base_tools = []\n", "extra_tools": "\n *base_tools,", }, - "unresolved_tool_expression", + # Read since #909, but a module's list is checked for changes only in + # its own module, so the surface is not proven. + "dynamic_tools_expression", id="starred_tool_element", ), pytest.param( diff --git a/tests/test_list_expressions.py b/tests/test_list_expressions.py new file mode 100644 index 00000000..e9935e1f --- /dev/null +++ b/tests/test_list_expressions.py @@ -0,0 +1,1204 @@ +"""Tools lists built by an expression are read member by member (#909). + +An agent whose ``tools=`` is ``[*BASE, *([extra] if wanted else [])]``, +``base + (extra or [])`` or a filter over a list used to read as "uses a +dynamic tools expression": none of its bindings were compared, even when every +member was a plain tool the reader already understood. These tests pin the +shared resolver (``inputs/list_expressions.py``) per shape, each with the +negative case where an operand cannot be read, then the two readers and +``diff --application``: + +- a member that cannot be read keeps the agent incomplete and is named where + it is, and resolving the rest never makes the agent complete; +- a member held only under a condition is never shown as unconditional, and a + change to the condition alone is a ``changed`` row whose direction is not + established. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from agents_shipgate.cli.main import app +from agents_shipgate.inputs.list_expressions import MAX_DEPTH, ListExpressions +from agents_shipgate.inputs.python_imports import ScopeIndex, _module_bindings +from tests.test_imported_tool_bindings import _adk, _commit, _compare, _edges, _git, _sdk, _write + +# -- the resolver, one shape at a time ------------------------------------------ + + +def _resolve(source: str, expression: str = "TOOLS"): + """Resolve ``expression`` as the last line of ``source`` would read it.""" + + tree = ast.parse(f"{source}\nagent = Agent(tools={expression})\n") + lists = ListExpressions( + ref="agent.py", + tree=tree, + scopes=ScopeIndex(tree), + bindings=_module_bindings(tree)[0], + module=None, + resolver=None, + agent_reads=lambda call, keyword: keyword == "tools", + ) + call = tree.body[-1].value + assert isinstance(call, ast.Call) + return lists.resolve(call.keywords[0].value) + + +def _members(result) -> list[tuple[str, tuple[str, ...]]]: + return [(ast.unparse(member.expr), member.conditions) for member in result.members] + + +def _unread(result) -> list[str]: + return [f"{part.reason} ({part.location})" for part in result.unresolved] + + +def test_a_literal_list_and_its_spread_are_read(): + result = _resolve("BASE = [a, b]", "[*BASE, c]") + assert _members(result) == [("a", ()), ("b", ()), ("c", ())] + assert result.complete + assert result.members[0].via == ("BASE (agent.py:1)",) + + +def test_a_spread_of_a_call_is_named_and_the_rest_kept(): + result = _resolve("", "[*load_tools(), c]") + assert _members(result) == [("c", ())] + assert _unread(result) == ["a call to `load_tools`, whose result is not read (agent.py:2)"] + + +def test_concatenation_reads_both_operands(): + result = _resolve("BASE = [a]", "BASE + [b] + (c,)") + assert _members(result) == [("a", ()), ("b", ()), ("c", ())] + assert result.complete + + +def test_concatenation_with_an_unread_operand_names_it(): + result = _resolve("BASE = [a]", "BASE + extra_tools()") + assert _members(result) == [("a", ())] + assert _unread(result) == ["a call to `extra_tools`, whose result is not read (agent.py:2)"] + + +def test_a_conditional_member_carries_its_condition(): + result = _resolve("", "[a, *([b] if wanted else [c])]") + assert _members(result) == [("a", ()), ("b", ("`wanted`",)), ("c", ("not `wanted`",))] + assert result.complete + + +def test_a_constant_condition_picks_its_branch(): + assert _members(_resolve("", "[a] + ([b] if True else [c])")) == [("a", ()), ("b", ())] + + +def test_a_conditional_branch_that_cannot_be_read_is_named(): + result = _resolve("", "[a] + ([b] if wanted else more())") + assert _members(result) == [("a", ()), ("b", ("`wanted`",))] + assert _unread(result) == ["a call to `more`, whose result is not read (agent.py:2)"] + + +def test_or_takes_the_first_operand_known_not_empty(): + assert _members(_resolve("EXTRA = [b]", "[a] + (EXTRA or [c])")) == [("a", ()), ("b", ())] + # Known empty: never the value. + assert _members(_resolve("EXTRA = None", "[a] + (EXTRA or [c])")) == [("a", ()), ("c", ())] + # Not known: each operand, under the condition that selects it. + assert _members(_resolve("EXTRA = [b] if wanted else []", "[a] + (EXTRA or [c])")) == [ + ("a", ()), + ("b", ("`EXTRA` is not empty", "`wanted`")), + ("c", ("`EXTRA` is empty",)), + ] + + +def test_or_with_a_parameter_names_the_parameter(): + # mosnin/realestatecrm#612: ``base_tools + (extra_tools or [])``. + tree = ast.parse( + "def build(extra_tools):\n base_tools = [a]\n return Agent(tools=base_tools + (extra_tools or []))\n" + ) + lists = ListExpressions( + ref="chippi.py", tree=tree, scopes=ScopeIndex(tree), bindings=_module_bindings(tree)[0], + module=None, resolver=None, agent_reads=lambda call, keyword: keyword == "tools", + ) + call = tree.body[0].body[-1].value + result = lists.resolve(call.keywords[0].value) + assert _members(result) == [("a", ())] + assert _unread(result) == [ + "`extra_tools` is a parameter of `build`, so its value comes from a caller (chippi.py:3)" + ] + + +def test_a_filter_over_a_list_keeps_each_member_conditional(): + result = _resolve("BROKER_TOOLS = [a, b]", "[tool for tool in BROKER_TOOLS if tool.name in ALLOWED]") + condition = "the filter `tool.name in ALLOWED` keeps it" + assert _members(result) == [("a", (condition,)), ("b", (condition,))] + result = _resolve("BROKER_TOOLS = [a, b]", "list(filter(is_allowed, BROKER_TOOLS))") + assert _members(result) == [("a", ("the filter `is_allowed` keeps it",)), ("b", ("the filter `is_allowed` keeps it",))] + # ``list``, ``tuple`` and ``sorted`` hold the same members. + assert _members(_resolve("BASE = [a]", "sorted(tuple(BASE))")) == [("a", ())] + + +def test_a_comprehension_that_builds_new_elements_is_named(): + result = _resolve("BASE = [a, b]", "[wrap(tool) for tool in BASE]") + assert result.members == () + assert _unread(result) == ["a comprehension that builds new elements is not followed (agent.py:2)"] + + +def test_a_filter_over_an_unread_list_is_named(): + result = _resolve("", "[tool for tool in registry() if tool.enabled]") + assert _unread(result) == ["a call to `registry`, whose result is not read (agent.py:2)"] + + +def test_a_name_bound_once_in_the_function_is_read(): + tree = ast.parse( + "def build(wanted):\n" + " tools = [a, *([b] if wanted else [])]\n" + " return Agent(tools=tools)\n" + ) + lists = ListExpressions( + ref="agent.py", tree=tree, scopes=ScopeIndex(tree), bindings=_module_bindings(tree)[0], + module=None, resolver=None, agent_reads=lambda call, keyword: keyword == "tools", + ) + result = lists.resolve(tree.body[0].body[-1].value.keywords[0].value) + assert _members(result) == [("a", ()), ("b", ("`wanted`",))] + assert result.members[0].via == ("tools (agent.py:2)",) + + +@pytest.mark.parametrize( + ("source", "reason"), + [ + ("TOOLS = [a]\nTOOLS = [b]", "`TOOLS` is bound more than once, or conditionally, in agent.py (agent.py:3)"), + ("if wanted:\n TOOLS = [a]", "`TOOLS` is bound more than once, or conditionally, in agent.py (agent.py:3)"), + ("TOOLS = [a]\nTOOLS.append(b)", "`TOOLS` may be changed in place after it is built (agent.py:1)"), + ("TOOLS = [a]\nregister(TOOLS)", "`TOOLS` may be changed in place after it is built (agent.py:1)"), + ("TOOLS = [a]\nglobals()['TOOLS'] = [b]", "`TOOLS` may be changed in place after it is built (agent.py:1)"), + ("def TOOLS():\n return [a]", "`TOOLS` is a function, not a list of tools (agent.py:3)"), + ("", "`TOOLS` is not bound where the list is read (agent.py:2)"), + ], + ids=["rebound", "conditional", "appended", "handed-on", "reflective", "function", "unbound"], +) +def test_a_name_the_reader_cannot_hold_still_is_named(source, reason): + result = _resolve(source) + assert result.members == () + assert _unread(result) == [reason] + + +def test_a_list_only_read_stays_readable(): + # Iterated, measured and logged: none of these change it. + result = _resolve("TOOLS = [a]\nfor tool in TOOLS:\n print(tool)\nCOUNT = len(TOOLS)") + assert _members(result) == [("a", ())] + + +def test_an_attribute_of_the_object_is_named(): + tree = ast.parse("class Bot:\n def build(self):\n return Agent(tools=self.tools)\n") + lists = ListExpressions( + ref="bot.py", tree=tree, scopes=ScopeIndex(tree), bindings=_module_bindings(tree)[0], + module=None, resolver=None, agent_reads=lambda call, keyword: keyword == "tools", + ) + result = lists.resolve(tree.body[0].body[0].body[0].value.keywords[0].value) + assert _unread(result) == ["`self.tools` is an attribute of the object, set elsewhere (bot.py:3)"] + + +def test_nesting_and_self_reference_are_bounded(): + deep = "[a]" + for _ in range(MAX_DEPTH + 2): + deep = f"({deep} + [])" + result = _resolve("", deep) + assert any("nests further than the reader follows" in reason for reason in _unread(result)) + # ``A = B`` alone is a second name, which may change the list; a spread + # only reads it, so two lists spreading each other reach the cycle bound. + result = _resolve("A = [*B]\nB = [*A]", "A") + assert result.members == () + assert _unread(result) == ["`A` refers to itself (agent.py:1)"] + + +# -- the OpenAI Agents SDK reader ----------------------------------------------- + +SDK_TOOLS = '''from agents import function_tool + + +@function_tool +def load_and_validate(path: str) -> str: + """Load and validate.""" + return path + + +@function_tool +def summarize(text: str) -> str: + """Summarize.""" + return text + + +@function_tool +def prepare_finance_handoff(note: str) -> str: + """Prepare a handoff.""" + return note + + +FINANCE_TOOLS = [load_and_validate, summarize] +''' + + +def _sdk_agent(tools: str, preamble: str = "") -> dict[str, str]: + return { + "tools.py": SDK_TOOLS, + "agent.py": "from agents import Agent\n" + "from tools import FINANCE_TOOLS, load_and_validate, prepare_finance_handoff, summarize\n" + f"{preamble}\nfinance = Agent(name='Finance', tools={tools})\n", + } + + +def _observation(loaded): + (observation,) = loaded.binding_observations + return observation + + +def test_sdk_reads_a_spread_of_another_module_s_list(tmp_path): + # Tiendat2703/MIS_TALENT#7: ``[*FINANCE_TOOLS, *([handoff] if wanted else [])]``. + _write( + tmp_path, + _sdk_agent( + "[*FINANCE_TOOLS, *([prepare_finance_handoff] if want_handoff_tool else [])]", + "want_handoff_tool = True\n", + ), + ) + loaded = _sdk(tmp_path, "agent.py") + observation = _observation(loaded) + assert observation.tools_complete is True + assert observation.tool_names == ["load_and_validate", "summarize", "prepare_finance_handoff"] + assert observation.tool_conditions == {"prepare_finance_handoff": ["`want_handoff_tool`"]} + assert not any("tools expression" in warning for warning in loaded.warnings) + # Each member is the definition its own module names. + assert _edges([loaded]) == [ + ("finance", "load_and_validate", "tools.py:5"), + ("finance", "prepare_finance_handoff", "tools.py:17"), + ("finance", "summarize", "tools.py:11"), + ] + + +def test_sdk_concatenation_binds_what_the_one_list_form_binds(tmp_path): + # #584: ``[a] + [b]`` yields the edges ``[a, b]`` does, with no warning. + _write(tmp_path / "joined", _sdk_agent("[load_and_validate] + [summarize]")) + _write(tmp_path / "single", _sdk_agent("[load_and_validate, summarize]")) + joined, single = _sdk(tmp_path / "joined", "agent.py"), _sdk(tmp_path / "single", "agent.py") + assert _edges([joined]) == _edges([single]) + assert joined.warnings == single.warnings == [] + assert joined.recovery_evidence == [] + + +def test_sdk_partly_read_list_keeps_the_agent_incomplete(tmp_path): + _write(tmp_path, _sdk_agent("[*FINANCE_TOOLS, *plugin_tools()]")) + loaded = _sdk(tmp_path, "agent.py") + observation = _observation(loaded) + assert observation.tools_complete is False + assert observation.tool_names == ["load_and_validate", "summarize"] + (reason,) = observation.issues + assert reason == ( + "OpenAI Agents SDK agent 'finance' at agent.py:4 has a tools list it reads only in part; its " + "binding graph is incomplete. Not read: a call to `plugin_tools`, whose result is not read " + "(agent.py:4)." + ) + (fact,) = loaded.recovery_evidence + assert (fact.recovery.kind, fact.recovery.reason) == ("unresolved", "sdk_tools_expression_unresolved") + + +def test_sdk_imported_list_changed_in_its_module_is_named(tmp_path): + files = _sdk_agent("[*FINANCE_TOOLS]") + files["tools.py"] += "FINANCE_TOOLS.append(prepare_finance_handoff)\n" + _write(tmp_path, files) + observation = _observation(_sdk(tmp_path, "agent.py")) + assert observation.tools_complete is False + assert observation.tool_names == [] + assert "`FINANCE_TOOLS` in tools.py may be changed in place or rebound" in observation.issues[0] + + +def test_sdk_imported_handoff_list_names_its_agents_not_itself(tmp_path): + # ``handoffs=SPECIALISTS`` used to bind one handoff named ``SPECIALISTS``. + _write( + tmp_path, + { + "specialists.py": "from agents import Agent\n" + "billing = Agent(name='Billing')\nrefunds = Agent(name='Refunds')\n" + "SPECIALISTS = [billing, *([refunds] if REFUNDS_ENABLED else [])]\n", + "triage.py": "from agents import Agent\nfrom specialists import SPECIALISTS\n" + "triage = Agent(name='Triage', handoffs=SPECIALISTS)\n", + }, + ) + observation = _observation(_sdk(tmp_path, "triage.py")) + assert observation.handoff_names == ["billing", "refunds"] + assert observation.handoff_conditions == {"refunds": ["`REFUNDS_ENABLED`"]} + assert observation.handoffs_complete is True + + +def test_sdk_unread_handoffs_are_named_beside_the_read_ones(tmp_path): + _write( + tmp_path, + { + "triage.py": "from agents import Agent, handoff\n" + "billing = Agent(name='Billing')\n" + "triage = Agent(name='Triage', handoffs=[billing, handoff(billing), *more_agents()])\n", + }, + ) + (observation,) = [item for item in _sdk(tmp_path, "triage.py").binding_observations if item.agent == "triage"] + assert observation.handoff_names == ["billing"] + assert observation.handoffs_complete is False + assert observation.issues == [ + "OpenAI Agents SDK agent 'triage' has dynamic handoffs at triage.py:3. Not read: a call to " + "`more_agents`, whose result is not read (triage.py:3).", + "OpenAI Agents SDK agent 'triage' at triage.py:3 hands off to `handoff(billing)`, which is not " + "resolved to an agent: it is not a name.", + ] + + +def test_sdk_a_change_through_one_agent_reaches_every_agent_sharing_the_list(tmp_path): + # ``BASE or []`` is BASE itself when BASE is not empty: the same object. + _write( + tmp_path, + { + "tools.py": SDK_TOOLS, + "agent.py": "from agents import Agent\nfrom tools import load_and_validate, summarize\n" + "BASE = [load_and_validate]\n" + "first = Agent(name='First', tools=BASE or [])\n" + "second = Agent(name='Second', tools=BASE)\n" + "second.tools.append(summarize)\n", + }, + ) + observations = {item.agent: item for item in _sdk(tmp_path, "agent.py").binding_observations} + assert observations["second"].tools_complete is False + assert observations["first"].tools_complete is False + + +def test_sdk_constructions_that_differ_only_in_a_condition_are_not_one_agent(tmp_path): + _write( + tmp_path, + { + "tools.py": SDK_TOOLS, + "agent.py": "from agents import Agent\nfrom tools import load_and_validate\n" + "def build(premium):\n" + " if premium:\n" + " return Agent(name='Quote', tools=[load_and_validate])\n" + " return Agent(name='Quote', tools=[*([load_and_validate] if premium else [])])\n", + }, + ) + observations = _sdk(tmp_path, "agent.py").binding_observations + assert len(observations) == 2 + assert all("constructed more than once" in item.issues[-1] for item in observations) + + +# -- the Google ADK reader ------------------------------------------------------ + +ADK_TOOLS = '''def load_and_validate(path: str) -> str: + """Load and validate.""" + return path + + +def summarize(text: str) -> str: + """Summarize.""" + return text + + +FINANCE_TOOLS = [load_and_validate, summarize] +''' + + +def test_adk_reads_another_module_s_list_with_that_module_s_names(tmp_path): + _write( + tmp_path, + { + "tools.py": ADK_TOOLS, + "agent.py": "from google.adk.agents import Agent\nfrom tools import FINANCE_TOOLS\n\n" + "def escalate(case: str) -> str:\n return case\n\n" + "root_agent = Agent(name='finance', tools=[*FINANCE_TOOLS, *([escalate] if URGENT else [])])\n", + }, + ) + loaded, artifacts = _adk(tmp_path) + assert _edges(loaded, artifacts) == [ + ("finance", "escalate", "agent.py:4"), + ("finance", "load_and_validate", "tools.py:1"), + ("finance", "summarize", "tools.py:6"), + ] + (observation,) = [item for source in loaded for item in source.binding_observations] + assert observation.tools_complete is True + assert observation.tool_conditions == {"escalate": ["`URGENT`"]} + assert artifacts.warnings == [] + # Read, never proven for ``scan``: another module could change the list. + for source in loaded: + for tool in source.tools: + assert "dynamic_tools_expression" in tool.extraction["surface_gaps"] + + +def test_adk_unread_part_is_a_limit_on_its_agent_with_its_location(tmp_path): + _write( + tmp_path, + { + "tools.py": ADK_TOOLS, + "agent.py": "from google.adk.agents import Agent\nfrom tools import FINANCE_TOOLS\n\n" + "root_agent = Agent(name='finance', tools=FINANCE_TOOLS + plugin_tools())\n", + }, + ) + loaded, artifacts = _adk(tmp_path) + assert [tool for _, tool, _ in _edges(loaded, artifacts)] == ["load_and_validate", "summarize"] + message = ( + "Google ADK agent 'finance' at agent.py:4 has a tools list it reads only in part; its binding " + "graph is incomplete. Not read: a call to `plugin_tools`, whose result is not read (agent.py:4)." + ) + assert message in artifacts.warnings + (observation,) = [item for source in loaded for item in source.binding_observations] + assert observation.tools_complete is False + assert observation.issues == [message] + + +def test_adk_starred_element_no_longer_reads_complete(tmp_path): + # ``tools=[a, *TOOLS]`` with TOOLS unbound used to bind ``a`` and call the + # agent's list complete, naming the starred element only on the file. + _write( + tmp_path, + { + "agent.py": "from google.adk.agents import Agent\n\n" + "def lookup(q: str) -> str:\n return q\n\n" + "root_agent = Agent(name='helper', tools=[lookup, *EXTRA_TOOLS])\n", + }, + ) + loaded, artifacts = _adk(tmp_path) + (observation,) = [item for source in loaded for item in source.binding_observations] + assert observation.tool_names == ["lookup"] + assert observation.tools_complete is False + assert "`EXTRA_TOOLS` is not bound where the list is read (agent.py:6)" in observation.issues[0] + + +def test_adk_sub_agents_spread_and_condition(tmp_path): + _write( + tmp_path, + { + "agent.py": "from google.adk.agents import Agent\n\n" + "billing = Agent(name='billing')\nrefunds = Agent(name='refunds')\n" + "SPECIALISTS = [billing]\n" + "root_agent = Agent(name='triage', sub_agents=[*SPECIALISTS, *([refunds] if REFUNDS else [])])\n", + }, + ) + _, artifacts = _adk(tmp_path) + (record,) = [record for record in artifacts.sub_agents if record["agent_name"] == "triage"] + assert record["sub_agent_count"] == 2 + assert record["sub_agents"] == ["billing", "refunds"] + assert record["conditions"] == {"refunds": ["`REFUNDS`"]} + + +def test_adk_sub_agents_partly_read_stay_unnamed_in_count(tmp_path): + _write( + tmp_path, + { + "agent.py": "from google.adk.agents import Agent\n\n" + "billing = Agent(name='billing')\n" + "root_agent = Agent(name='triage', sub_agents=[billing, *more_agents()])\n", + }, + ) + _, artifacts = _adk(tmp_path) + (record,) = [record for record in artifacts.sub_agents if record["agent_name"] == "triage"] + # The graph reads a None count as "has sub-agents that were not statically named". + assert record["sub_agent_count"] is None + assert record["sub_agents"] == ["billing"] + + +# -- diff --application ----------------------------------------------------------- + + +def _rows(result) -> list[tuple[str, str, str]]: + return [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] + + +def test_application_diff_reports_a_changed_tool_held_through_a_spread(tmp_path): + # MIS_TALENT#7: ``load_and_validate`` changes, and ``Finance_Agent`` holds it + # through ``[*FINANCE_TOOLS, ...]``: its row now appears. + files = _sdk_agent( + "[*FINANCE_TOOLS, *([prepare_finance_handoff] if want_handoff_tool else [])]", + "want_handoff_tool = True\n", + ) + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, files) + head = _commit(tmp_path, {"tools.py": SDK_TOOLS.replace(" return path", " return path.strip()")}) + result = _compare(tmp_path, base, head) + + assert result["application_comparison_schema_version"] == "0.3" + assert result["comparison_status"] == "compared" + assert _rows(result) == [("finance", "load_and_validate", "changed")] + + +def test_application_diff_shows_a_conditional_addition_with_its_condition(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _sdk_agent("[*FINANCE_TOOLS]", "want_handoff_tool = True\n")) + head = _commit( + tmp_path, + _sdk_agent( + "[*FINANCE_TOOLS, *([prepare_finance_handoff] if want_handoff_tool else [])]", + "want_handoff_tool = True\n", + ), + ) + result = _compare(tmp_path, base, head) + + (row,) = result["rows"] + assert (row["tool"], row["change"]) == ("prepare_finance_handoff", "added") + assert row["after"]["bound_when"] == ["`want_handoff_tool`"] + assert row["why"] == ( + "The source now binds this callable to this agent, only when `want_handoff_tool`." + ) + + +@pytest.mark.parametrize( + ("head_tools", "after", "why"), + [ + ( + "[*FINANCE_TOOLS, *([prepare_finance_handoff] if not want_handoff_tool else [])]", + ["`not want_handoff_tool`"], + "only when `not want_handoff_tool` at the head", + ), + ("[*FINANCE_TOOLS, prepare_finance_handoff]", None, "unconditionally at the head"), + ], + ids=["condition-changed", "now-unconditional"], +) +def test_application_diff_a_condition_change_alone_is_a_change_without_direction( + tmp_path, head_tools, after, why +): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit( + tmp_path, + _sdk_agent( + "[*FINANCE_TOOLS, *([prepare_finance_handoff] if want_handoff_tool else [])]", + "want_handoff_tool = True\n", + ), + ) + head = _commit(tmp_path, _sdk_agent(head_tools, "want_handoff_tool = True\n")) + result = _compare(tmp_path, base, head) + + assert result["comparison_status"] == "compared" + (row,) = result["rows"] + assert (row["tool"], row["change"], row["uncertainty"]) == ("prepare_finance_handoff", "changed", {}) + assert row["before"]["bound_when"] == ["`want_handoff_tool`"] + assert row["after"].get("bound_when") == after + assert row["why"].startswith( + "Only the condition changed: bound only when `want_handoff_tool` at the base and " + ) + assert why in row["why"] + assert "is not established" in row["why"] + assert row["review_question"].startswith("Should finance hold prepare_finance_handoff ") + + +def test_application_diff_text_prints_the_condition(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _sdk_agent("[*FINANCE_TOOLS]", "want_handoff_tool = True\n")) + head = _commit( + tmp_path, + _sdk_agent( + "[*FINANCE_TOOLS, *([prepare_finance_handoff] if want_handoff_tool else [])]", + "want_handoff_tool = True\n", + ), + ) + result = CliRunner().invoke( + app, ["diff", "--application", "--workspace", str(tmp_path), "--base", base, "--head", head] + ) + assert result.exit_code == 0, result.output + assert " bound only when `want_handoff_tool`" in result.output + + +def test_application_diff_a_partly_read_list_is_never_compared(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _sdk_agent("[*FINANCE_TOOLS, *plugin_tools()]")) + head = _commit(tmp_path, {"tools.py": SDK_TOOLS.replace(" return text", " return text[:80]")}) + result = _compare(tmp_path, base, head) + + # Never ``compared`` while a member is unread, and the unread part is + # named on its agent. A tool both sides bind is still a row: what the + # unread part holds cannot unbind it. + assert result["comparison_status"] == "partial" + assert _rows(result) == [("finance", "summarize", "changed")] + assert any( + gap["agent"] == "finance" + and "a call to `plugin_tools`, whose result is not read (agent.py:4)" in gap["reason"] + for gap in result["head"]["coverage_gaps"] + ) + + +def test_application_diff_handoff_through_an_imported_list(tmp_path): + specialists = ( + "from agents import Agent\nbilling = Agent(name='Billing')\nrefunds = Agent(name='Refunds')\n" + "SPECIALISTS = [billing]\n" + ) + triage = ( + "from agents import Agent\nfrom specialists import SPECIALISTS\n" + "triage = Agent(name='Triage', handoffs=SPECIALISTS)\n" + ) + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, {"specialists.py": specialists, "triage.py": triage}) + head = _commit( + tmp_path, + {"specialists.py": specialists.replace("[billing]", "[billing, *([refunds] if REFUNDS else [])]")}, + ) + result = _compare(tmp_path, base, head) + + (row,) = result["rows"] + assert (row["agent"], row["change"]) == ("triage", "added") + assert row["tool"].endswith(":refunds") + assert row["after"]["bound_when"] == ["`REFUNDS`"] + + +def test_application_diff_adk_sub_agent_condition(tmp_path): + agent = ( + "from google.adk.agents import Agent\n\n" + "billing = Agent(name='billing')\nrefunds = Agent(name='refunds')\n" + "root_agent = Agent(name='triage', sub_agents=SUBS)\n" + ) + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, {"agent.py": agent.replace("SUBS", "[billing]")}) + head = _commit(tmp_path, {"agent.py": agent.replace("SUBS", "[billing, *([refunds] if REFUNDS else [])]")}) + result = _compare(tmp_path, base, head) + + added = [row for row in result["rows"] if row["change"] == "added"] + assert [row["tool"].rsplit(":", 1)[-1] for row in added] == ["refunds"] + assert added[0]["after"]["bound_when"] == ["`REFUNDS`"] + + +def test_application_diff_reads_the_concatenation_without_a_recovery_gap(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _sdk_agent("[load_and_validate]")) + head = _commit(tmp_path, _sdk_agent("[load_and_validate] + [summarize]")) + result = _compare(tmp_path, base, head) + + assert result["comparison_status"] == "compared" + assert _rows(result) == [("finance", "summarize", "added")] + + +def test_the_comparison_never_runs_the_list(tmp_path): + # A list built by running code is named, never evaluated. + _write(tmp_path, _sdk_agent("[load_and_validate, *eval('[summarize]')]")) + observation = _observation(_sdk(tmp_path, "agent.py")) + assert observation.tool_names == ["load_and_validate"] + assert observation.tools_complete is False + assert "a call to `eval`, whose result is not read (agent.py:4)" in observation.issues[0] + + +def test_sdk_reader_writes_nothing(tmp_path): + _write(tmp_path, _sdk_agent("[*FINANCE_TOOLS]")) + before = sorted(path.name for path in Path(tmp_path).rglob("*")) + _sdk(tmp_path, "agent.py") + assert sorted(path.name for path in Path(tmp_path).rglob("*")) == before + + +# -- #909 review: every way a list could change without the reader seeing it ----------- + + +@pytest.mark.parametrize( + "source", + [ + # Defaults, decorators and class bodies run in the scope around them. + "TOOLS = [a]\ndef register(tool, TOOLS=TOOLS):\n TOOLS.append(tool)\nregister(b)", + "TOOLS = [a]\nadd = lambda tool, TOOLS=TOOLS: TOOLS.append(tool)\nadd(b)", + "TOOLS = [a]\ndef register(tool, *, TOOLS=TOOLS):\n TOOLS.append(tool)", + "TOOLS = [a]\n@extend(TOOLS)\ndef helper(TOOLS):\n pass", + "TOOLS = [a]\nclass Config:\n TOOLS = TOOLS\n TOOLS.append(b)", + # A walrus in a comprehension rebinds the name around it. + "TOOLS = [a]\n_ = [(TOOLS := [b]) for _ in range(1)]", + # A wildcard import after the list may rebind it. + "TOOLS = [a]\nfrom helpers import *", + # A list handed to a function past a spread lands on another parameter. + "TOOLS = [a]\nNO_ARGS = ()\ndef add(target, label=None):\n target.append(b)\nadd(*NO_ARGS, TOOLS)", + # A decorated helper may hand the list to code that changes it. + "TOOLS = [a]\n@wrap\ndef show(items):\n print(items)\nshow(TOOLS)", + ], + ids=["default", "lambda-default", "kw-default", "decorator", "class-body", "walrus", + "star-import", "spread-argument", "decorated-callee"], +) +def test_a_change_the_old_index_missed_keeps_the_list_unread(source): + result = _resolve(source) + assert result.members == () + assert not result.complete + + +def test_a_condition_is_compared_whole_however_long(): + condition = "os.environ.get('X', '') in (" + ", ".join(f"'region-{n}'" for n in range(30)) + ")" + result = _resolve("", f"[a, *([b] if {condition} else [])]") + assert result.members[1].conditions == (f"`{condition}`",) + assert len(result.members[1].conditions[0]) > 160 + + +def test_a_list_spread_many_times_is_read_once(): + lines = ["L0 = [a]"] + [f"L{k} = [" + ", ".join([f"*L{k - 1}"] * 10) + "]" for k in range(1, 7)] + result = _resolve("\n".join(lines), "L6") + assert _members(result) == [("a", ())] + + +def test_a_class_body_comprehension_reads_the_class_list(): + tree = ast.parse( + "TOOLS = [a]\nclass Config:\n TOOLS = [b]\n agent = Agent(tools=[t for t in TOOLS if t])\n" + ) + lists = ListExpressions( + ref="agent.py", tree=tree, scopes=ScopeIndex(tree), bindings=_module_bindings(tree)[0], + module=None, resolver=None, agent_reads=lambda call, keyword: keyword == "tools", + ) + call = tree.body[1].body[1].value + assert [ast.unparse(member.expr) for member in lists.resolve(call.keywords[0].value).members] == ["b"] + + +def test_a_filter_object_is_true_however_empty(): + # ``filter(...) or [b]`` is the filter object, never ``b``. + assert _members(_resolve("", "list(filter(keep, []) or [b])")) == [] + + +def test_a_list_bound_in_a_with_block_is_read(): + tree = ast.parse( + "async def main():\n" + " async with server() as s:\n" + " tools = [a]\n" + " return Agent(tools=tools)\n" + ) + lists = ListExpressions( + ref="agent.py", tree=tree, scopes=ScopeIndex(tree), bindings=_module_bindings(tree)[0], + module=None, resolver=None, agent_reads=lambda call, keyword: keyword == "tools", + ) + call = tree.body[0].body[0].body[1].value + assert _members(lists.resolve(call.keywords[0].value)) == [("a", ())] + + +def test_a_list_bound_in_a_branch_is_read_only_inside_that_branch(): + tree = ast.parse( + "def build(premium):\n" + " if premium:\n" + " tools = [a]\n" + " first = Agent(tools=tools)\n" + " return Agent(tools=tools)\n" + ) + lists = ListExpressions( + ref="agent.py", tree=tree, scopes=ScopeIndex(tree), bindings=_module_bindings(tree)[0], + module=None, resolver=None, agent_reads=lambda call, keyword: keyword == "tools", + ) + inside = tree.body[0].body[0].body[1].value + after = tree.body[0].body[1].value + assert _members(lists.resolve(inside.keywords[0].value)) == [("a", ())] + assert not lists.resolve(after.keywords[0].value).complete + + +@pytest.mark.parametrize( + ("files", "agent_file"), + [ + ( # A builder's own import of the list, changed in place. + {"agent.py": "from agents import Agent\ndef build():\n from tools import FINANCE_TOOLS, prepare_finance_handoff\n" + " FINANCE_TOOLS.append(prepare_finance_handoff)\n return Agent(name='Finance', tools=[*FINANCE_TOOLS])\n"}, + "agent.py", + ), + ( # The same list changed through the module's own spelling. + {"agent.py": "import tools\nfrom agents import Agent\nfrom tools import FINANCE_TOOLS\n" + "tools.FINANCE_TOOLS.append(tools.prepare_finance_handoff)\nfinance = Agent(name='Finance', tools=[*FINANCE_TOOLS])\n"}, + "agent.py", + ), + ( # A package that re-exports the list changes it when imported. + {"pkg/__init__.py": "from pkg.tools import FINANCE_TOOLS, prepare_finance_handoff\nFINANCE_TOOLS.append(prepare_finance_handoff)\n", + "pkg/tools.py": SDK_TOOLS, + "agent.py": "from agents import Agent\nfrom pkg import FINANCE_TOOLS\nfinance = Agent(name='Finance', tools=[*FINANCE_TOOLS])\n"}, + "agent.py", + ), + ( # The list's own module changes it through an agent built with it. + {"tools.py": SDK_TOOLS + "from agents import Agent\nhelper = Agent(name='Helper', tools=FINANCE_TOOLS)\n" + "helper.tools.append(prepare_finance_handoff)\n", + "agent.py": "from agents import Agent\nfrom tools import FINANCE_TOOLS\nfinance = Agent(name='Finance', tools=FINANCE_TOOLS)\n"}, + "agent.py", + ), + ( # Another module's own ``Agent`` class is not the SDK's. + {"tools.py": SDK_TOOLS + "class Agent:\n def __init__(self, name, tools):\n tools.append(prepare_finance_handoff)\n" + "legacy = Agent(name='legacy', tools=FINANCE_TOOLS)\n", + "agent.py": "from agents import Agent\nfrom tools import FINANCE_TOOLS\nfinance = Agent(name='Finance', tools=[*FINANCE_TOOLS])\n"}, + "agent.py", + ), + ], + ids=["local-import", "module-spelling", "package-init", "through-an-agent", "foreign-agent-class"], +) +def test_sdk_an_imported_list_changed_elsewhere_is_named(tmp_path, files, agent_file): + _write(tmp_path, {"tools.py": SDK_TOOLS, **files}) + loaded = _sdk(tmp_path, agent_file) + (observation,) = [item for item in loaded.binding_observations if item.agent.lower() == "finance"] + assert observation.tools_complete is False + assert "prepare_finance_handoff" not in observation.tool_names + + +@pytest.mark.parametrize( + "call", + [ + "replace(tools=FINANCE_TOOLS)", # the application's own ``replace`` + "Registry().clone(tools=FINANCE_TOOLS)", # a ``clone`` of something not an agent + ], + ids=["user-replace", "non-agent-clone"], +) +def test_sdk_only_a_proven_agent_copy_reads_its_list(tmp_path, call): + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "from agents import Agent\nfrom tools import FINANCE_TOOLS\n" + "def replace(tools):\n tools.append(1)\n\nclass Registry:\n def clone(self, tools):\n tools.append(1)\n\n" + f"{call}\nfinance = Agent(name='Finance', tools=[*FINANCE_TOOLS])\n"}) + observation = _observation(_sdk(tmp_path, "agent.py")) + assert observation.tools_complete is False + + +def test_sdk_a_module_attribute_is_read_when_only_read(tmp_path): + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "import tools\nfrom agents import Agent\n" + "finance = Agent(name='Finance', tools=tools.FINANCE_TOOLS)\n"}) + observation = _observation(_sdk(tmp_path, "agent.py")) + assert observation.tools_complete is True + assert observation.tool_names == ["load_and_validate", "summarize"] + + +def test_sdk_a_parameter_shadowing_a_module_is_not_the_module(tmp_path): + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "import tools\nfrom agents import Agent\n" + "def build(tools):\n return Agent(name='Finance', tools=tools.FINANCE_TOOLS)\n"}) + observation = _observation(_sdk(tmp_path, "agent.py")) + assert observation.tools_complete is False + assert observation.tool_names == [] + + +def test_sdk_a_copy_made_after_a_change_through_an_agent_is_unread(tmp_path): + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "from agents import Agent\n" + "from tools import load_and_validate, prepare_finance_handoff\nBASE = [load_and_validate]\n" + "first = Agent(name='First', tools=BASE)\nfirst.tools.append(prepare_finance_handoff)\n" + "second = Agent(name='Second', tools=[*BASE])\n"}) + observations = {item.agent: item for item in _sdk(tmp_path, "agent.py").binding_observations} + assert observations["second"].tools_complete is False + + +def test_sdk_a_hosted_tool_keeps_its_recovery_evidence(tmp_path): + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "from agents import Agent, WebSearchTool\n" + "from tools import summarize\nfinance = Agent(name='Finance', tools=[WebSearchTool(), summarize])\n"}) + loaded = _sdk(tmp_path, "agent.py") + (fact,) = loaded.recovery_evidence + assert (fact.recovery.kind, fact.recovery.reason) == ("unresolved", "sdk_tools_expression_unresolved") + assert fact.source_ref == "agent.py:3" + + +def test_sdk_an_unread_member_of_another_module_is_not_bound_by_name(tmp_path): + # ``lookup`` in the other module's list is extlib's, not this module's. + _write(tmp_path, { + "registry.py": SDK_TOOLS + "from extlib import lookup\nTOOLS = [summarize, lookup]\n", + "agent.py": "from agents import Agent, function_tool\nfrom registry import TOOLS\n\n" + "@function_tool\ndef lookup(q: str) -> str:\n return q\n\nfinance = Agent(name='Finance', tools=[*TOOLS])\n", + }) + observation = _observation(_sdk(tmp_path, "agent.py")) + assert observation.tool_names == ["summarize"] + assert observation.tools_complete is False + + +def test_sdk_a_handoff_spelled_like_a_local_agent_is_not_that_agent(tmp_path): + _write(tmp_path, { + "specialists.py": "from agents import Agent\nrefunds = Agent(name='Refunds')\nSPECIALISTS = [refunds]\n", + "triage.py": "from agents import Agent\nfrom specialists import SPECIALISTS\n" + "refunds = Agent(name='Refunds')\ntriage = Agent(name='Triage', handoffs=[*SPECIALISTS])\n", + }) + (observation,) = [item for item in _sdk(tmp_path, "triage.py").binding_observations if item.agent == "triage"] + assert observation.handoff_names == [] + assert observation.handoffs_complete is False + + +def test_adk_a_sub_agent_list_read_from_an_expression_is_not_proven(tmp_path): + _write(tmp_path, {"agent.py": "from google.adk.agents import LlmAgent\n\n" + "def lookup(q: str) -> dict:\n return {}\n\n" + "helper = LlmAgent(name='helper', model='m', tools=[lookup])\n" + "SUBS = [helper]\nroot_agent = LlmAgent(name='root', model='m', sub_agents=SUBS)\n"}) + loaded, artifacts = _adk(tmp_path) + (record,) = [record for record in artifacts.sub_agents if record["agent_name"] == "root"] + assert record["sub_agents"] == ["helper"] + tools = [tool for source in loaded for tool in source.tools] + assert tools and all("dynamic_tools_expression" in tool.extraction["surface_gaps"] for tool in tools) + + +def test_adk_twins_differing_in_what_they_hold_unconditionally_are_two(tmp_path): + _write(tmp_path, {"agent.py": "from google.adk.agents import Agent\n\n" + "def lookup(q: str) -> str:\n return q\n\ndef search(q: str) -> str:\n return q\n\n" + "def export(q: str) -> str:\n return q\n\n" + "COMMON = [lookup, search]\nPREMIUM = [lookup, export]\n\n" + "def build(premium):\n return Agent(name='assistant', tools=[*COMMON, *(PREMIUM if premium else [])])\n\n" + "def other(premium):\n return Agent(name='assistant', tools=[search, *(PREMIUM if premium else [])])\n"}) + loaded, _ = _adk(tmp_path) + (observation,) = [item for source in loaded for item in source.binding_observations if item.agent == "assistant"] + assert observation.tools_complete is False + assert any("constructed more than once" in issue for issue in observation.issues) + + +def test_application_diff_an_adk_list_read_in_part_limits_only_its_agent(tmp_path): + agent = ( + "from google.adk.agents import Agent\n\n" + "def lookup(q: str) -> str:\n return q\n\ndef search(q: str) -> str:\n return q\n\n" + "a = Agent(name='a', tools=[lookup, *get_more()])\nb = Agent(name='b', tools=TOOLS)\n" + ) + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, {"agent.py": agent.replace("TOOLS", "[search]")}) + head = _commit(tmp_path, {"agent.py": agent.replace("TOOLS", "[]")}) + result = _compare(tmp_path, base, head) + + assert result["comparison_status"] == "partial" + assert _rows(result) == [("b", "search", "removed")] + + +def test_application_diff_a_condition_change_beside_an_unread_part_is_not_established(tmp_path): + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _sdk_agent("[load_and_validate, summarize, *plugin_tools()]")) + head = _commit(tmp_path, _sdk_agent("[load_and_validate, *([summarize] if ADMIN else []), *plugin_tools()]")) + result = _compare(tmp_path, base, head) + + (row,) = result["rows"] + assert (row["tool"], row["change"], row["candidate_change"]) == ("summarize", "not_established", "changed") + + +def test_application_diff_a_change_past_160_characters_of_a_condition_is_seen(tmp_path): + regions = ", ".join(f"'region-{n}'" for n in range(20)) + tools = "[*FINANCE_TOOLS, *([prepare_finance_handoff] if os.environ.get('X', '') in (REGIONS) else [])]" + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, _sdk_agent(tools.replace("REGIONS", regions), "import os\n")) + head = _commit(tmp_path, _sdk_agent(tools.replace("REGIONS", regions + ", '*'"), "import os\n")) + result = _compare(tmp_path, base, head) + + assert _rows(result) == [("finance", "prepare_finance_handoff", "changed")] + + +def test_adk_a_literal_spread_keeps_the_surface_proven(tmp_path): + # No name another module could change: nothing puts the proof at risk. + _write(tmp_path, {"agent.py": "from google.adk.agents import Agent\n\n" + "def lookup(q: str) -> str:\n return q\n\ndef search(q: str) -> str:\n return q\n\n" + "root_agent = Agent(name='root', tools=[lookup] + [*[search]])\n"}) + loaded, _ = _adk(tmp_path) + tools = [tool for source in loaded for tool in source.tools] + assert sorted(tool.name for tool in tools) == ["lookup", "search"] + assert all("dynamic_tools_expression" not in tool.extraction["surface_gaps"] for tool in tools) + + +def test_sdk_one_load_indexes_an_imported_module_once(tmp_path, monkeypatch): + files = {"tools.py": SDK_TOOLS} + for index in range(5): + files[f"agents/agent_{index}.py"] = ( + "from agents import Agent\nfrom tools import FINANCE_TOOLS\n" + f"agent_{index} = Agent(name='a{index}', tools=[*FINANCE_TOOLS])\n" + ) + _write(tmp_path, files) + indexed: list[str] = [] + original = ListExpressions._index_changes + + def counting(self, view): + indexed.append(view.ref) + return original(self, view) + + monkeypatch.setattr(ListExpressions, "_index_changes", counting) + from agents_shipgate.inputs.openai_sdk_static import load_openai_sdk_static_tools + from agents_shipgate.schemas.manifest import ToolSourceConfig + + loaded = load_openai_sdk_static_tools( + ToolSourceConfig(id="sdk", type="openai_agents_sdk", path="."), None, tmp_path + ) + assert indexed.count("tools.py") <= 2 # once as an imported module, once if read as an entry + assert all(observation.tools_complete for observation in loaded.binding_observations) + + +# -- #909 review: the guards, the condition rules and each reader's shapes --------------- + + +def test_sdk_a_list_appended_through_a_module_level_import_is_named(tmp_path): + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "from agents import Agent\n" + "from tools import FINANCE_TOOLS, prepare_finance_handoff\n" + "FINANCE_TOOLS.append(prepare_finance_handoff)\n" + "finance = Agent(name='Finance', tools=[*FINANCE_TOOLS])\n"}) + observation = _observation(_sdk(tmp_path, "agent.py")) + assert observation.tools_complete is False + assert observation.tool_names == [] + + +def test_sdk_a_list_whose_module_runs_unread_code_is_not_established(tmp_path): + # ``tools.py`` imports code above the read scope, which could rebind its list. + _write(tmp_path, { + "svc/common.py": "settings = {}\n", + "svc/app/__init__.py": "", + "svc/app/tools.py": "from ..common import settings # noqa: F401\n" + SDK_TOOLS, + "svc/app/agent.py": "from agents import Agent\nfrom .tools import FINANCE_TOOLS\n" + "finance = Agent(name='Finance', tools=[*FINANCE_TOOLS])\n", + }) + observation = _observation(_sdk(tmp_path / "svc" / "app", "agent.py")) + assert observation.tools_complete is False + assert "is not established" in observation.issues[0] + + +@pytest.mark.parametrize( + "first", + ["Agent(name='First', tools=BASE if FLAG else [])", "Agent(name='First', handoffs=SPECIALISTS)"], + ids=["conditional-holder", "handoffs-holder"], +) +def test_sdk_a_change_through_one_holder_reaches_the_other(tmp_path, first): + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "from agents import Agent\n" + "from tools import load_and_validate, prepare_finance_handoff\n" + "BASE = [load_and_validate]\nhelper = Agent(name='Helper')\nSPECIALISTS = [helper]\nFLAG = True\n" + f"first = {first}\n" + "second = Agent(name='Second', tools=BASE, handoffs=SPECIALISTS)\n" + "second.tools.append(prepare_finance_handoff)\nsecond.handoffs.append(helper)\n"}) + observations = {item.agent: item for item in _sdk(tmp_path, "agent.py").binding_observations} + assert observations["first"].tools_complete is False or observations["first"].handoffs_complete is False + + +def test_sdk_a_member_of_another_module_is_that_module_s_definition(tmp_path): + # agent.py's own ``summarize`` is another function: the list's is tools.py's. + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "from agents import Agent, function_tool\n" + "from tools import FINANCE_TOOLS\n\n" + "@function_tool\ndef summarize(text: str) -> str:\n return text.upper()\n\n" + "finance = Agent(name='Finance', tools=[*FINANCE_TOOLS])\n"}) + loaded = _sdk(tmp_path, "agent.py") + assert _edges([loaded]) == [ + ("finance", "load_and_validate", "tools.py:5"), + ("finance", "summarize", "tools.py:11"), + ] + + +def test_sdk_a_list_imported_inside_the_builder_is_read(tmp_path): + _write(tmp_path, {"tools.py": SDK_TOOLS, "agent.py": "from agents import Agent\n" + "def build():\n from tools import FINANCE_TOOLS\n" + " return Agent(name='Finance', tools=FINANCE_TOOLS)\n"}) + (observation,) = _sdk(tmp_path, "agent.py").binding_observations + assert observation.tools_complete is True + assert observation.tool_names == ["load_and_validate", "summarize"] + + +def test_adk_a_member_of_another_module_that_is_not_a_reference_is_named(tmp_path): + _write(tmp_path, { + "tools.py": "from google.adk.tools import FunctionTool\n\n" + ADK_TOOLS + + "WRAPPED = [FunctionTool(func=summarize)]\n", + "agent.py": "from google.adk.agents import Agent\nfrom tools import WRAPPED\n\n" + "root_agent = Agent(name='finance', tools=[*WRAPPED])\n", + }) + loaded, _ = _adk(tmp_path) + (observation,) = [item for source in loaded for item in source.binding_observations] + assert observation.tools_complete is False + assert "binds `FunctionTool(func=summarize)`, written in tools.py" in observation.issues[0] + + +def test_a_tool_the_list_also_holds_unconditionally_has_no_condition(tmp_path): + _write(tmp_path, _sdk_agent("[load_and_validate, *([load_and_validate] if X else [])]")) + observation = _observation(_sdk(tmp_path, "agent.py")) + assert observation.tool_conditions == {} + + +def test_hold_merges_constructions_unconditional_first(): + from agents_shipgate.cli.application_diff import _hold + + held: dict[str, list[str] | None] = {} + _hold(held, "t", ["`a`"]) + _hold(held, "t", ["`b`"]) + _hold(held, "u", ["`a`"]) + _hold(held, "u", None) + _hold(held, "u", ["`c`"]) + assert held == {"t": ["`a`", "`b`"], "u": None} + + +def test_sdk_handoff_twins_differing_only_in_a_condition_are_two(tmp_path): + _write(tmp_path, {"triage.py": "from agents import Agent\nbilling = Agent(name='Billing')\n" + "def build(premium):\n" + " if premium:\n return Agent(name='Triage', handoffs=[billing])\n" + " return Agent(name='Triage', handoffs=[*([billing] if premium else [])])\n"}) + observations = [item for item in _sdk(tmp_path, "triage.py").binding_observations if item.agent == "Triage"] + assert observations and all(not item.handoffs_complete or not item.tools_complete for item in observations) + + +def test_adk_twins_differing_only_in_a_condition_are_two(tmp_path): + _write(tmp_path, {"agent.py": "from google.adk.agents import Agent\n\n" + "def lookup(q: str) -> str:\n return q\n\n" + "def build(premium):\n if premium:\n return Agent(name='assistant', tools=[lookup])\n" + " return Agent(name='assistant', tools=[*([lookup] if premium else [])])\n"}) + loaded, _ = _adk(tmp_path) + (observation,) = [item for source in loaded for item in source.binding_observations] + assert observation.tools_complete is False + + +@pytest.mark.parametrize("expression", ["filter(keep, plugin_tools())", "list(plugin_tools())", "sorted(get())"]) +def test_a_filter_or_copy_of_an_unread_list_is_named(expression): + result = _resolve("", expression) + assert result.members == () + assert not result.complete + + +@pytest.mark.parametrize( + ("expression", "names", "conditions"), + [ + ("[lookup] + [search]", ["lookup", "search"], {}), + ("EXTRA or [lookup]", ["search", "lookup"], + {"search": ["`EXTRA` is not empty and `FLAG`"], "lookup": ["`EXTRA` is empty"]}), + ("[t for t in BOTH if keep(t)]", ["lookup", "search"], + {"lookup": ["the filter `keep(t)` keeps it"], "search": ["the filter `keep(t)` keeps it"]}), + ], + ids=["concatenation", "or", "filter"], +) +def test_adk_tools_shapes(tmp_path, expression, names, conditions): + _write(tmp_path, {"agent.py": "from google.adk.agents import Agent\n\n" + "def lookup(q: str) -> str:\n return q\n\ndef search(q: str) -> str:\n return q\n\n" + "EXTRA = [search] if FLAG else []\nBOTH = [lookup, search]\n" + f"root_agent = Agent(name='root', tools={expression})\n"}) + loaded, _ = _adk(tmp_path) + (observation,) = [item for source in loaded for item in source.binding_observations] + assert observation.tool_names == names + assert observation.tool_conditions == conditions + assert observation.tools_complete is True + + +@pytest.mark.parametrize( + ("expression", "names", "conditions"), + [ + ("[billing] + [refunds]", ["billing", "refunds"], {}), + ("EXTRA or [billing]", ["refunds", "billing"], + {"refunds": ["`EXTRA` is not empty and `FLAG`"], "billing": ["`EXTRA` is empty"]}), + ("[a for a in BOTH if a]", ["billing", "refunds"], + {"billing": ["the filter `a` keeps it"], "refunds": ["the filter `a` keeps it"]}), + ], + ids=["concatenation", "or", "filter"], +) +def test_adk_sub_agents_shapes(tmp_path, expression, names, conditions): + _write(tmp_path, {"agent.py": "from google.adk.agents import Agent\n\n" + "billing = Agent(name='billing')\nrefunds = Agent(name='refunds')\n" + "EXTRA = [refunds] if FLAG else []\nBOTH = [billing, refunds]\n" + f"root_agent = Agent(name='triage', sub_agents={expression})\n"}) + _, artifacts = _adk(tmp_path) + (record,) = [record for record in artifacts.sub_agents if record["agent_name"] == "triage"] + assert record["sub_agents"] == names + assert record["conditions"] == conditions + assert record["sub_agent_count"] == len(names) + + +def test_adk_sub_agents_from_another_module_are_not_this_module_s(tmp_path): + _write(tmp_path, { + "specialists.py": "from google.adk.agents import Agent\nbilling = Agent(name='billing')\nSPECIALISTS = [billing]\n", + "agent.py": "from google.adk.agents import Agent\nfrom specialists import SPECIALISTS\n\n" + "root_agent = Agent(name='triage', sub_agents=[*SPECIALISTS])\n", + }) + _, artifacts = _adk(tmp_path) + (record,) = [record for record in artifacts.sub_agents if record["agent_name"] == "triage"] + assert record["sub_agents"] == [] + assert record["unresolved_sub_agents"] == ["billing"] + + +def test_application_diff_an_unread_sub_agent_part_is_named_on_its_agent(tmp_path): + agent = ( + "from google.adk.agents import Agent\n\n" + "def lookup(q: str) -> str:\n return q\n\ndef search(q: str) -> str:\n return q\n\n" + "billing = Agent(name='billing')\n" + "root_agent = Agent(name='triage', sub_agents=[billing, *more_agents()])\n" + "helper = Agent(name='helper', tools=TOOLS)\n" + ) + _git(tmp_path, "init", "-q", "-b", "main") + base = _commit(tmp_path, {"agent.py": agent.replace("TOOLS", "[lookup]")}) + head = _commit(tmp_path, {"agent.py": agent.replace("TOOLS", "[lookup, search]")}) + result = _compare(tmp_path, base, head) + + assert result["comparison_status"] == "partial" + # The other agent's addition stands; the limit is the triage agent's. + assert ("helper", "search", "added") in _rows(result) + (gap,) = [gap for gap in result["head"]["coverage_gaps"] if "sub-agents" in gap["reason"]] + assert gap["agent"] == "triage" + assert "Not read: a call to `more_agents`, whose result is not read (agent.py:10)." in gap["reason"] + + +@pytest.mark.parametrize( + ("expression", "names"), + [("[billing] + [refunds]", ["billing", "refunds"]), ("EXTRA or [billing]", ["refunds", "billing"]), + ("[a for a in BOTH if a]", ["billing", "refunds"])], + ids=["concatenation", "or", "filter"], +) +def test_sdk_handoffs_shapes(tmp_path, expression, names): + _write(tmp_path, {"triage.py": "from agents import Agent\n" + "billing = Agent(name='Billing')\nrefunds = Agent(name='Refunds')\n" + "EXTRA = [refunds] if FLAG else []\nBOTH = [billing, refunds]\n" + f"triage = Agent(name='Triage', handoffs={expression})\n"}) + (observation,) = [item for item in _sdk(tmp_path, "triage.py").binding_observations if item.agent == "triage"] + assert observation.handoff_names == names + assert observation.handoffs_complete is True diff --git a/tests/test_qualification_coverage_misses.py b/tests/test_qualification_coverage_misses.py index 8de216ad..f5277d33 100644 --- a/tests/test_qualification_coverage_misses.py +++ b/tests/test_qualification_coverage_misses.py @@ -49,7 +49,6 @@ def test_actual_ie_keeps_named_gaps_and_unclassified_cases(tmp_path): @pytest.mark.parametrize("expression,expected", [ (None, "input_unavailable"), - ("[read_tool] + [other_tool]", "reader_limitation"), ("get_tools()", "unresolved"), ]) def test_real_sdk_recovery_survives_qualification_without_relabeling(tmp_path, expression, expected):