From a608156dd26ddeda56fd7c65ff047fa8a38881c4 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Mon, 28 Sep 2026 15:21:26 -0700 Subject: [PATCH 1/4] feat(diff --application): derive the comparison scope from the change when --scope is omitted (#875) Without --scope, each changed Python file is related to the OpenAI Agents SDK and Google ADK agent files that import it, or that it imports, within six import hops. It is read from the two commits' objects on each side. A package's __init__.py and a literal importlib.import_module count. An agent file does one of these: - constructs or subclasses an agent class; - copies an agent with capabilities of its own; - changes an agent's capabilities after construction; - builds its agent through another file's factory. Each related group is compared in the outermost package that holds the change, its agents and what they import. Vesta#58 derives backend/app. - Independent applications become separate comparisons under `comparisons`, never the repository root. - A relocated application is one comparison. - A change that touches no supported agent is an explicit not_established answer naming the files considered. - These are named in scope_selection.limits and make the result partial: - a changed file unrelated to an agent-building module, with why; - a changed link or submodule; - an agent outside the scopes that reaches the change; - a consumer: a module that imports the change and an on-request builder and builds, copies or rewires an agent, calls repository code with its own arguments, or sets its module state. It is exempt only when one compared scope holds all three. - any bound reached. `scope_selection` records the mode, the scopes and why. An explicit --scope always wins, `--scope .` keeps the root, and --base-scope needs --scope. Partial clones are refused with the established hydration message before anything reads the tree. On the pinned 48-PR corpus, derived scopes establish the same 296 rows as the root. The changed-wiring directory establishes 18. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + README.md | 3 +- docs/application-comparison.md | 94 +- docs/distribution-surfaces.md | 2 +- src/agents_shipgate/cli/application_diff.py | 357 +++-- src/agents_shipgate/cli/application_scope.py | 1314 ++++++++++++++++++ src/agents_shipgate/cli/diff.py | 4 +- src/agents_shipgate/cli/discovery/signals.py | 23 +- tests/test_application_diff.py | 12 +- tests/test_application_diff_reach.py | 2 +- tests/test_application_diff_review.py | 2 +- tests/test_application_diff_unobserved.py | 7 +- tests/test_application_scope.py | 1121 +++++++++++++++ tests/test_distribution_surface_parity.py | 2 +- tests/test_imported_tool_review.py | 2 +- 15 files changed, 2840 insertions(+), 106 deletions(-) create mode 100644 src/agents_shipgate/cli/application_scope.py create mode 100644 tests/test_application_scope.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c2ca74469..48c517316 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Application comparison without prior setup - Add `diff --application` for OpenAI Agents SDK and Google ADK source-observed per-agent wiring, with exact base/head refs and independently selected scopes. No manifest, saved baseline or authored declarations are needed. The advisory `application_comparison_schema_version: "0.1"` result records before/after evidence, scoped coverage gaps and explicit uncertain candidates; it supplies no release verdict or merge permission. Includes scoped Git materialization, partial-clone recovery, and definition lookup using reader-resolved Python symbols and locations. See [application comparison](docs/application-comparison.md). (#871) +- `diff --application` without `--scope` now derives the scope from the change instead of comparing the repository root. Each changed Python file is related to the OpenAI Agents SDK and Google ADK files that import it, or that it imports, within six import hops (a package's `__init__.py` included); a changed test relates only when an agent imports it. Agent files are those that construct an agent class under any name it is imported as, subclass one, copy one with capabilities of its own, or change an agent's capabilities after construction. A changed file not related to a module that builds an agent is named, with why (no import path, a path longer than six hops, only a rewiring module, a file too large to read), and makes the result `partial` even inside a compared scope; so does a changed link to a module or directory, a changed submodule, an agent outside the compared scopes whose imports go past the bound, or a module that imports the change (directly, through a package or agent file's re-export, or through other modules), imports a builder whose agent's capabilities are not a fixed list of its own names, and builds, copies or rewires an agent itself, calls the repository's code with its own arguments, or sets its module state, unless one compared scope holds all three. A module that builds its agent through a builder's factory is an agent file. A relocated application is one comparison, whatever it holds. Each related group is compared in the outermost package that holds it. On mezgoodle/Vesta#58 a change to `backend/app/services/` is compared in `backend/app`, where its agents are, rather than in the changed directory, which holds none. Independent applications in a monorepo are separate comparisons under `comparisons`, never the root. A change that touches no supported agent is an explicit `not_established` answer naming the files considered. A bound reached is named and makes the result `partial`. `--json` records `scope_selection` (`derived` or `explicit`, the scopes and why). `--scope .` keeps the root, an explicit `--scope` always wins, and `--base-scope` now needs `--scope`. On the pinned corpus of 48 application PRs, derived scopes establish the same 296 rows the repository root does, where the changed-wiring directory establishes 18. The derived scope is narrower than the root for 20 PRs. For 7 PRs it answers that the change touches no supported agent; the root reported unrelated agents there. The median run takes 13 s, the same as the root. (#875) - `diff --application` no longer reads another library's `Agent` or `function_tool` as the OpenAI Agents SDK's, and no longer reports an agent it stopped observing as removed. On speechmatics/speechmatics-academy#142, a LiveKit voice agent (`from livekit.agents import Agent, function_tool`) moved its tools into an `Agent` subclass that passes them through `super().__init__`. The comparison printed `compared` with four `REMOVED agent → …` rows although the same four tools were still bound. It now prints `not_established`: no supported application agent on either side. (#871 follow-up; related #580, #864, #865) - **Framework identity.** Discovery and the SDK reader count `function_tool` and `Agent` as the SDK's only when the name is imported from the absolute `agents`/`openai_agents` package, or is not imported at all (the existing reading, unless a wildcard import from another module could supply it). The name is resolved in the scope that uses it, as Python resolves it, so an import in a sibling function never decides it, and a parameter or local assignment of that name is not the SDK's. A name imported from anywhere else — `livekit.agents`, a relative `.agents` package — is another library's. A LiveKit file is no longer an SDK candidate, and a file using both reads only the SDK's symbols. A relative import is no longer any framework's import signal. `scan` of a declared SDK source reads the same way. - **Unobserved is not removed.** One side may observe an agent while the other side's same file still assigns or imports its name (`from factory import agent`), or passes it as `name=`, through a construction the reader does not support: an `Agent` subclass, a factory, `Agent[Context](...)` or `.clone()`. That side then records a coverage gap for the agent. The result is `partial`, and its rows are `not_established` with `candidate_change` `removed` or `added`. An agent referenced only in another agent's `handoffs` is not an observed construction. An agent whose name is gone from the file is still an established removal, and rows for other agents are unaffected. No schema changes. diff --git a/README.md b/README.md index ba791e6ea..a304f9219 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,8 @@ agents-shipgate diff --application --workspace /path/to/repo --base BASE_SHA --h ``` It shows source-observed changes per agent, including before/after signatures -and source locations. See [application comparison](docs/application-comparison.md) +and source locations, in the package that holds the changed code's agents unless +`--scope` names one. See [application comparison](docs/application-comparison.md) for scoped applications, moves, exact refs and coverage limits. Version availability is recorded in the [CHANGELOG entry](CHANGELOG.md#application-comparison-without-prior-setup); while it is under Unreleased, use a source build containing the feature. diff --git a/docs/application-comparison.md b/docs/application-comparison.md index d2de0d992..d8eae0864 100644 --- a/docs/application-comparison.md +++ b/docs/application-comparison.md @@ -13,8 +13,9 @@ agents-shipgate diff --application --workspace /path/to/repository \ No `init`, manifest, saved baseline or authored declaration is required. The command writes no files in the subject repository and runs no application code. -Fetch the refs first; the command never fetches. `--scope` defaults to the -repository root; `--head` defaults to committed `HEAD`, excluding dirty edits. +Fetch the refs first; the command never fetches. Without `--scope` the scope +is derived from the change (below); `--scope .` selects the repository root. +`--head` defaults to committed `HEAD`, excluding dirty edits. The base is the requested base ref's merge base with the selected head. The result identifies each observed agent object and its added, removed or @@ -33,6 +34,92 @@ A reviewer can inspect the new callable and decide whether that agent should receive it. A body change is a request to review the implementation; it does not establish widening, narrowing, business impact or runtime behavior. +## A scope derived from the change + +Without `--scope`, the comparison does not read the whole repository, nor only +the directory the change touched. Each changed Python file is related to the +agent files — files discovery scores as OpenAI Agents SDK or Google ADK sources +that construct an agent class (under any name they import it as), subclass +one or copy one with capabilities of its own, and a module that changes an +agent's capabilities after construction (`support.tools.append(x)`) and +imports one of those, or that builds its agent through one's factory +(`root_agent = make("bot", [tool])` with `make` returning an `Agent`); a +module that only defines tools is not one — that +import it, or that it imports, up to six import hops away, on each side of the +change. A module under a directory discovery skips (`build/`, `fixtures/`) is +related like any other, never an agent file. Importing `pkg.impl` runs `pkg/__init__.py` +first, so a package's imports count, and so does a literal +`importlib.import_module("app.tools")`. A file named like a test is related +when an agent imports it; it is never an agent file, and a changed test that +imports an agent does not widen a scope. A changed Python file not related to +a module that builds an agent — no import path, one longer than six hops, only +a module that rewires an agent, a file too large to read — is named in +`scope_selection.limits` with why, and makes the result `partial`, even when a +compared scope holds it: its consumer may be elsewhere. So is a changed link +to a module or a directory, or a changed submodule, whose content is not read, +and so is an agent outside the compared scopes whose imports go past the hop +bound. A module that imports the change — directly, through a package's +re-export, an agent file's, or other modules, up to six levels — and imports +within six hops an agent builder that builds on request — one that +constructs an agent whose tools, handoffs, MCP servers or sub-agents are not a +fixed list of the module's own names (`tools=tools`, `list(REGISTRY.items)`, +`self.tools`, `**config`) or gives it such capabilities afterwards (a +rewire, or a copy such as `BASE.clone(tools=tools)`), or subclasses an agent +class — is named too, +however it builds its agent from them, when it builds, copies +(`clone(tools=...)`, `clone(update={"tools": ...})`) or rewires an agent +itself, calls the repository's code with arguments of its own, or sets the +repository's module state (`factory.TOOLS = [...]`, +`setattr(factory, ...)`), unless +one compared scope holds it, the change and that builder. A module that only +imports another module's agent, or only calls an entry point (`main()`), is +not one. A module that imports the change and imports onward past six hops +without reaching a builder is named as not established, and so is the search +itself when modules outside the compared scopes still import onward after six +levels. A rename's two paths are one file. + +```text +scope: backend/app (derived: backend/app: backend/app/services/gemini_tools.py is +related to agent file backend/app/agents/support.py through …) +``` + +Each changed file is compared in the outermost package holding it and every +agent that reaches it — with what those agents import from the repository and +the path entry a namespace package is imported through (`src` for +`myapp.tools` in `src/myapp/`) — so the reader can follow each chain. A +change to `backend/app/services/gemini_tools.py` that +`backend/app/agents/support.py` imports is compared in `backend/app`. When only +the repository root holds them all — a library module its own agents and an +example app elsewhere both import — the change is compared where its nearest +agents are, the others are named in `outside` and in `scope_selection.limits`, +and the answer is `partial`, never a silent `compared`. + +Independent applications of a monorepo are separate comparisons, never the +repository root; `--json` then holds each one under `comparisons`, with every +path spelled from the repository root, and `comparison_status` is `compared` +only when each one is. A Python module moved inside one package is one +comparison of that package; an application directory moved whole — its old +path gone from the head, its new one absent from the base — is one relocation, +compared old path to new path as `--base-scope`/`--scope` would, with every +scope inside it; a renamed +non-Python file joins nothing. A change that touches no supported agent — a +README, or a script no agent imports — is `not_established` with the reason +naming the files considered; nothing is compared, and that is an answer, not a +failure. An absolute import is found where the importer's own path would find +it, or at one path entry holding a package of that name (`libs/shared`); a +standard-library name is the standard library unless a module on the importer's +path shadows it. + +Everything is read from the two commits' objects, never run. The reading is +bounded per side: six import hops, 2000 files read to relate the change, and +5000 files or 64 MB read to find the agent files. A bound reached is named in +`scope_selection.limits` and makes the result `partial` — so does a +no-agent answer whose import following stopped at the hop bound — and it never +falls back to the root. `--json` records `scope_selection`: `mode` (`derived` +or `explicit`), `scopes`, the `reason`, the changed files considered and each +scope's relations (`base_scope` when it moved, `outside` agents). An explicit +`--scope` always wins, and `--base-scope` needs it. + ## Scopes, moves and incomplete inputs For an application directory moved by the PR, select its old location separately: @@ -54,7 +141,8 @@ exit 2. A removal describes the selected source path, not the entire repository. `--json` emits `application_comparison_schema_version: "0.1"`, engine identity, requested and compared refs/tree IDs, per-side scope/coverage, rows, source -correspondence, and a deterministic `comparison_id`. This is a separate advisory +correspondence, `scope_selection`, `comparisons` when a derived change spans +more than one application, and a deterministic `comparison_id`. This is a separate advisory artifact from the existing host diff JSON and verifier receipt. - `compared`: the selected supported source observations were compared. An diff --git a/docs/distribution-surfaces.md b/docs/distribution-surfaces.md index 35907460d..f2d80fcf4 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` | — | — | Advisory SDK/ADK source-wiring comparison through `diff --application`. Reuses framework observations and the binding graph; its comparator answers no release or merge verdict, activation verdict, executable pin, or declared authority question. Unlike host mode it computes per-agent source differences, not a projection of host drift. Scoped gaps and `not_established` candidates bound the answer; no deployment-root reachability is asserted. An agent the other side's file still names through a construction no reader supports is a gap there, never a removal or addition, and SDK identity follows the import, so LiveKit's `Agent`/`function_tool` are not read as the SDK's. `tests/test_application_diff.py`, `tests/test_application_diff_review.py` and `tests/test_application_diff_identity.py` prove the advisory boundary, isolation, uncertainty and evidence identity. Every scope, the root included, is materialized by the scoped verified materializer, so a link is recreated rather than refused and never read through; every link under the scope, a dangling one included, is censused and gapped only where it can hide application source (a `*.py` link not aliasing an input the scope reads, a directory link holding Python outside the scope, or an unresolved link where the other side reads source); a submodule is never read, named as a limit when its gitlink commit is unchanged and a coverage gap over its path (the whole scope at the scope itself) otherwise; the host-configuration census adds no application gap (`tests/test_application_diff_reach.py`). A tool an agent binds from another module inside the selected scope is followed by the SDK/ADK readers to its definition, never imported or run, and each module read is published with its digest as `import_path` evidence outside the compared meaning; an import they cannot follow stays a named gap scoped to its agent (#864, `tests/test_imported_tool_bindings.py`). An unobserved agent is not no change (#876): every OpenAI Agents SDK construction in a file the scope reads is an observed agent or a named limit — `return Agent(...)`, `self.agent = Agent(...)`, an inline agent, `Agent[Context](...)` and a positional name are read under their literal `name`; `**` or extra positional arguments, a capability-passing copy, a change to an agent's tools after construction, one identity constructed twice with different tools, a construction without a literal name, and an agent built from an SDK `Agent` subclass or from a class deriving from a Google ADK agent class are limits — so `compared` holds no unaccounted construction site. Test code, by discovery's test-path convention relative to the scope, establishes nothing and is listed per side in the additive `excluded_tests` field, a list of the comparison's own unread input paths that restates no engine answer and adds no claim; a tool name one application file defines twice is a gap on that name, never a refusal (`tests/test_application_diff_unobserved.py`). | +| `application_diff` | `src/agents_shipgate/cli/application_diff.py`, `src/agents_shipgate/cli/application_scope.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`). | | `zero_install_detector` | `tools/shipgate-detect.py` | `agent_project_verdict` | `test_detector_verdict_matches_cli` | Emits no `diagnostics[]` and no `next_actions[]`; evidence strings and framework scores are simplified. See the script's own "Intentional simplifications". | | `emitted_ci_workflow` | `src/agents_shipgate/cli/discovery/ci_workflow.py` | `executable_pin` | `tests/test_adopter_pins_resolve.py::test_the_emitted_workflow_pins_the_release_and_not_the_source_tree`, `tests/test_release_source.py::test_candidate_workflow_uses_immutable_source_before_and_after_publication` | Ordinary/source/preview builds use the published fallback; a stamped candidate pins its verified Action SHA and package version. Before publication its smoke substitutes the exact local wheel inputs. Provenance asserts no qualification. | | `prompts` | `prompts/` | `contract_floor`, `executable_pin`, `placeholder_ownership`, `release_decision_vocabulary` | `test_executable_pin_resolves_in_a_published_channel`, `test_surface_enumerations_match_the_engine_vocabulary`, `test_surface_routes_human_owned_placeholders_to_a_human`, `tests/test_adopter_pins_resolve.py::test_every_pin_init_writes_into_an_adopter_repo_names_the_published_release`, `tests/test_adopter_pins_resolve.py::test_the_shipped_floor_is_decided_against_the_release_the_prompts_pin` | — | diff --git a/src/agents_shipgate/cli/application_diff.py b/src/agents_shipgate/cli/application_diff.py index e5d09c58f..e2830c0aa 100644 --- a/src/agents_shipgate/cli/application_diff.py +++ b/src/agents_shipgate/cli/application_diff.py @@ -18,6 +18,7 @@ import typer +from agents_shipgate.cli.application_scope import derive_scopes from agents_shipgate.cli.discovery import detect_workspace from agents_shipgate.cli.discovery.artifacts import _candidate_files, _skip_part from agents_shipgate.cli.discovery.signals import _is_test_path @@ -1256,15 +1257,19 @@ def run_application_diff( workspace: Path, base: str | None, head: str, - scope: str, + scope: str | None, base_scope: str | None, max_python_files: int, json_output: bool, ) -> int: - from agents_shipgate.cli.diff import _one_line, _refuse_objects_missing, _resolve_base + from agents_shipgate.cli.diff import _refuse_objects_missing, _resolve_base + from agents_shipgate.cli.verify.git import promised_objects_missing workspace = ensure_git_workspace(workspace) - scope, old_scope = _scope(scope), _scope(base_scope if base_scope is not None else scope) + if scope is None and base_scope is not None: + raise typer.BadParameter( + "--base-scope names the old path of an explicitly selected application; pass --scope too." + ) if not head.strip() or head.startswith("-"): raise typer.BadParameter("Head ref must be non-empty and cannot start with a dash.") head_commit = commit_sha(workspace, head) @@ -1287,13 +1292,96 @@ def run_application_diff( raise typer.BadParameter("Base ref must be non-empty and cannot start with a dash.") requested_base_commit = commit_sha(workspace, base_ref) _, base_commit = _resolve_base(workspace, requested_base_commit or base_ref, head_commit) + for side, ref, commit in (("base", base_ref, base_commit), ("head", head, head_commit)): + if promised_objects_missing(workspace, commit): + # A partial clone: name the hydration it needs before anything + # reads the tree, the derived scope included (#817, #875). + _refuse_objects_missing(workspace, ref, commit, side=side) engine = build_engine_requirement(plugins_enabled=False).model_dump(mode="json") + sides = { + "base": { + "requested_ref": base_ref, + "requested_commit": requested_base_commit, + "compared_commit": base_commit, + "tree": tree_sha(workspace, base_commit), + }, + "head": { + "requested_ref": head, + "compared_commit": head_commit, + "tree": tree_sha(workspace, head_commit), + }, + } + if scope is None: + # No --scope: the change chooses it (#875). + selection = derive_scopes(workspace, base_commit, head_commit) + pairs = list(selection.pairs) + scope_selection = selection.payload() + else: + scope, old_scope = _scope(scope), _scope(base_scope if base_scope is not None else scope) + pairs = [(old_scope, scope)] + scope_selection = { + "mode": "explicit", + "scopes": [scope], + "base_scope": old_scope, + "reason": "Selected with --scope.", + } + comparisons = [ + _compare_scopes( + workspace, + sides, + (base_ref, base_commit, old_scope), + (head, head_commit, new_scope), + max_python_files=max_python_files, + engine=engine, + derived=scope is None, + ) + for old_scope, new_scope in pairs + ] + if len(comparisons) == 1: + payload = {**comparisons[0], "scope_selection": scope_selection} + if scope_selection.get("limits") and payload["comparison_status"] == "compared": + # The derivation stopped at a bound: an agent it did not reach + # may be related too (#875). + payload["comparison_status"] = "partial" + else: + payload = _combined(comparisons, sides, scope_selection, engine, max_python_files) + payload = sanitize_report_payload(payload) + payload["comparison_id"] = _digest(payload) + if json_output: + typer.echo(json.dumps(payload, ensure_ascii=False, indent=2)) + else: + _print_comparison(payload, base_commit, head_commit) + return 0 + + +_LIMITS = [ + "Covers supported OpenAI Agents SDK and Google ADK source wiring only.", + "Deployment-root reachability, runtime behavior, indirect helper effects and business authority are not established.", + "This comparison is advisory evidence and supplies no release verdict or merge permission.", +] + + +def _compare_scopes( + workspace: Path, + sides: dict[str, dict[str, Any]], + base: tuple[str, str, str], + head: tuple[str, str, str], + *, + max_python_files: int, + engine: dict[str, Any], + derived: bool, +) -> dict[str, Any]: + """One comparison of ``base`` and ``head`` (``(ref, commit, scope)``).""" + + from agents_shipgate.cli.diff import _refuse_objects_missing + + (base_ref, base_commit, old_scope), (head_ref, head_commit, scope) = base, head with tempfile.TemporaryDirectory(prefix="shipgate-application-diff-") as raw: scratch = Path(raw) gitlinks: dict[str, dict[str, str]] = {} for side, ref, commit, selected_scope in ( ("base", base_ref, base_commit, old_scope), - ("head", head, head_commit, scope), + ("head", head_ref, head_commit, scope), ): def in_scope(path: str, selected: str = selected_scope) -> bool: @@ -1336,7 +1424,7 @@ def in_scope(path: str, selected: str = selected_scope) -> bool: old = observe(old_found, max_python_files=max_python_files, census=census) with repository_layout(new_layout): new = observe(new_found, max_python_files=max_python_files, census=census) - if old.status == new.status == "absent": + if old.status == new.status == "absent" and not derived: raise ConfigError( f"Neither comparison tree contains the selected scopes: " f"base={old_scope!r}, head={scope!r}. Check --scope/--base-scope." @@ -1355,97 +1443,198 @@ def in_scope(path: str, selected: str = selected_scope) -> bool: status = "partial" if "partial" in {old.status, new.status} else "compared" if not old.agents and not new.agents and status == "compared": status = "not_established" - payload = { + return { "application_comparison_schema_version": SCHEMA_VERSION, "comparison_status": status, "static_analysis_only": True, "comparison_basis": "source_observed_per_agent_wiring", "input_origin": "independent_tree_discovery", "engine": engine, - "base": { - "requested_ref": base_ref, - "requested_commit": requested_base_commit, - "compared_commit": base_commit, - "tree": tree_sha(workspace, base_commit), - **old.summary(), - }, - "head": { - "requested_ref": head, - "compared_commit": head_commit, - "tree": tree_sha(workspace, head_commit), - **new.summary(), - }, + "base": {**sides["base"], **old.summary()}, + "head": {**sides["head"], **new.summary()}, "options": {"max_python_files": max_python_files, "max_python_bytes": MAX_PYTHON_BYTES}, "source_correspondence": moves, "rows": rows, - "limits": [ - "Covers supported OpenAI Agents SDK and Google ADK source wiring only.", - "Deployment-root reachability, runtime behavior, indirect helper effects and business authority are not established.", - "This comparison is advisory evidence and supplies no release verdict or merge permission.", - ], + "limits": list(_LIMITS), } - payload = sanitize_report_payload(payload) - payload["comparison_id"] = _digest(payload) - if json_output: - typer.echo(json.dumps(payload, ensure_ascii=False, indent=2)) + + +def _combined( + comparisons: list[dict[str, Any]], + sides: dict[str, dict[str, Any]], + scope_selection: dict[str, Any], + engine: dict[str, Any], + max_python_files: int, +) -> dict[str, Any]: + """No scope, or several: one result that holds each comparison (#875). + + With none, the change touches no supported agent, and the answer says so; + it is not a failure. With several, rows are every comparison's, each + path spelled from the repository root.""" + + statuses = {item["comparison_status"] for item in comparisons} + if not comparisons or statuses == {"not_established"}: + status = "not_established" + elif statuses == {"compared"}: + status = "compared" else: - typer.echo(f"Application comparison: {status} ({base_commit[:12]} → {head_commit[:12]})") - for row in payload["rows"]: - typer.echo( - f"{row['change'].upper()} {_one_line(row['agent'])} → {_one_line(row['tool'])}" - ) - for side in ("before", "after"): - value = row[side] - if value is None: - typer.echo( - f" {side}: " - + ( - "binding presence not established" - if row["uncertainty"] - else "no observed binding" - ) + status = "partial" + gaps = scope_selection.get("limits", []) + if gaps: + # A bound reached or an agent left outside: the answer is incomplete, + # never "no agent" (#875). + status = "partial" + + def side(name: str) -> dict[str, Any]: + # The same fields one comparison's side has, every path spelled from + # the repository root; each comparison keeps its own detail. + def rooted(item: dict[str, Any], path: str | None) -> str | None: + return _location(item[name]["scope"], path) + + return { + **sides[name], + "scope": None, + "scopes": [item[name]["scope"] for item in comparisons], + "status": "not_selected" + if not comparisons + else "partial" + if any(item[name]["status"] == "partial" for item in comparisons) + else "complete", + "sources": [ + {**source, "path": rooted(item, source.get("path"))} + for item in comparisons + for source in item[name]["sources"] + ], + "agents": [ + { + **agent, + "source": rooted(item, agent.get("source")), + "location": rooted(item, agent.get("location")), + } + for item in comparisons + for agent in item[name]["agents"] + ], + "binding_count": sum(item[name]["binding_count"] for item in comparisons), + "limits": sorted( + {*gaps, *(f"{item[name]['scope']}: {limit}" for item in comparisons for limit in item[name]["limits"])} + ), + "coverage_gaps": [ + {**gap, "source": rooted(item, gap.get("source"))} + for item in comparisons + for gap in item[name]["coverage_gaps"] + ], + "excluded_tests": sorted( + str(rooted(item, path)) for item in comparisons for path in item[name].get("excluded_tests", []) + ), + } + + return { + "application_comparison_schema_version": SCHEMA_VERSION, + "comparison_status": status, + "static_analysis_only": True, + "comparison_basis": "source_observed_per_agent_wiring", + "input_origin": "independent_tree_discovery", + "engine": engine, + "base": side("base"), + "head": side("head"), + "options": {"max_python_files": max_python_files, "max_python_bytes": MAX_PYTHON_BYTES}, + "source_correspondence": [ + { + **move, + "base_source": _location(item["base"]["scope"], move["base_source"]), + "head_source": _location(item["head"]["scope"], move["head_source"]), + } + for item in comparisons + for move in item["source_correspondence"] + ], + "rows": [ + {**row, "agent_source": _location(item["head"]["scope"], row.get("agent_source"))} + for item in comparisons + for row in item["rows"] + ], + "comparisons": comparisons, + "scope_selection": scope_selection, + "limits": list(_LIMITS), + } + + +def _print_comparison(payload: dict[str, Any], base_commit: str, head_commit: str) -> None: + from agents_shipgate.cli.diff import _one_line + + status = payload["comparison_status"] + typer.echo(f"Application comparison: {status} ({base_commit[:12]} → {head_commit[:12]})") + selection = payload.get("scope_selection") or {} + if selection.get("mode") == "derived": + scopes = ", ".join(selection["scopes"]) or "none" + typer.echo(f"scope: {_one_line(scopes)} (derived: {_one_line(selection['reason'])})") + for limit in selection.get("limits", []): + typer.echo(f" scope limit: {_one_line(limit)}") + if payload.get("comparisons") == []: + typer.echo("The change touches no supported application agent; nothing was compared.") + for item in payload.get("comparisons", [payload]): + if "comparisons" in payload: + typer.echo(f"Scope {item['head']['scope']}: {item['comparison_status']}") + _print_rows(item, _one_line) + typer.echo(payload["limits"][-1]) + + +def _print_rows(payload: dict[str, Any], _one_line: Any) -> None: + status = payload["comparison_status"] + rows = payload["rows"] + for row in payload["rows"]: + typer.echo( + f"{row['change'].upper()} {_one_line(row['agent'])} → {_one_line(row['tool'])}" + ) + for side in ("before", "after"): + value = row[side] + if value is None: + typer.echo( + f" {side}: " + + ( + "binding presence not established" + if row["uncertainty"] + else "no observed binding" ) - else: - definition = value.get("definition", {}) + ) + else: + definition = value.get("definition", {}) + typer.echo( + f" {side}: {_one_line(value.get('signature') or value['tool'])} at {_one_line(value.get('binding_location'))}" + ) + if definition: typer.echo( - f" {side}: {_one_line(value.get('signature') or value['tool'])} at {_one_line(value.get('binding_location'))}" + f" implementation: {_one_line(definition['source'])}:{definition['line']} ({str(definition['implementation_sha256'])[:12]})" ) - if definition: - typer.echo( - f" implementation: {_one_line(definition['source'])}:{definition['line']} ({str(definition['implementation_sha256'])[:12]})" - ) - for path in value.get("import_path", []): - hops = " → ".join( - f"{step['path']}:{step['line']}" - for step in path["steps"] - if step.get("line") is not None - ) - typer.echo(f" imported: {_one_line(hops)}") - typer.echo(f" {_one_line(row['why'])}") - for side, reasons in row["uncertainty"].items(): - for reason in reasons: - typer.echo(f" {side} uncertainty: {_one_line(reason)}") - typer.echo(f" Review: {_one_line(row['review_question'])}") - if not rows: - typer.echo( - "No supported application agents were established." - if status == "not_established" - else "Incomplete comparison; this is not a no-change result." - if status == "partial" - else "No established binding/interface/implementation changes in the observed surface." - ) - for side in ("base", "head"): - for limit in payload[side]["limits"]: - typer.echo(f" {side} limit: {_one_line(limit)}") - # Test files are never the application (#876); say which were left - # out, so a product module that only looks like a test is visible. - excluded = sorted( - set(payload["base"].get("excluded_tests", [])) - | set(payload["head"].get("excluded_tests", [])) + for path in value.get("import_path", []): + hops = " → ".join( + f"{step['path']}:{step['line']}" + for step in path["steps"] + if step.get("line") is not None + ) + typer.echo(f" imported: {_one_line(hops)}") + typer.echo(f" {_one_line(row['why'])}") + for side, reasons in row["uncertainty"].items(): + for reason in reasons: + typer.echo(f" {side} uncertainty: {_one_line(reason)}") + typer.echo(f" Review: {_one_line(row['review_question'])}") + if not rows: + typer.echo( + "No supported application agents were established." + if status == "not_established" + else "Incomplete comparison; this is not a no-change result." + if status == "partial" + else "No established binding/interface/implementation changes in the observed surface." ) - if excluded: - shown = ", ".join(_one_line(path) for path in excluded[:5]) - more = f", and {len(excluded) - 5} more" if len(excluded) > 5 else "" - typer.echo(f"Test files not read as the application ({len(excluded)}): {shown}{more}") - typer.echo(payload["limits"][-1]) - return 0 + for side in ("base", "head"): + for limit in payload[side]["limits"]: + typer.echo(f" {side} limit: {_one_line(limit)}") + # Test files are never the application (#876); say which were left + # out, so a product module that only looks like a test is visible. + excluded = sorted( + set(payload["base"].get("excluded_tests", [])) + | set(payload["head"].get("excluded_tests", [])) + ) + if excluded: + shown = ", ".join(_one_line(path) for path in excluded[:5]) + more = f", and {len(excluded) - 5} more" if len(excluded) > 5 else "" + typer.echo(f"Test files not read as the application ({len(excluded)}): {shown}{more}") diff --git a/src/agents_shipgate/cli/application_scope.py b/src/agents_shipgate/cli/application_scope.py new file mode 100644 index 000000000..61f67a55c --- /dev/null +++ b/src/agents_shipgate/cli/application_scope.py @@ -0,0 +1,1314 @@ +"""The application comparison's scope, derived from the change (#875). + +``diff --application`` without ``--scope`` does not compare the whole +repository, nor the directory the change touched. It relates each changed +Python file to the OpenAI Agents SDK and Google ADK files that import it, or +that it imports, a few hops away, and compares the outermost package that +holds each related group: a change to ``backend/app/services/tools.py`` that +``backend/app/services/adk_service.py`` imports is compared in +``backend/app``. Two independent groups are two comparisons, never the root. + +Everything is read from the two commits' objects, never imported or run; a +file is an agent file by the same signals discovery scores +(:func:`application_frameworks`). +""" + +from __future__ import annotations + +import ast +import sys +from dataclasses import dataclass, field +from pathlib import Path, PurePosixPath +from typing import Any + +from agents_shipgate.cli.discovery.artifacts import _skip_part +from agents_shipgate.cli.discovery.signals import _is_test_path, application_frameworks +from agents_shipgate.cli.verify.git import _blob_contents, _run_git_bounded_output, _TreeBlob + +SUPPORTED = frozenset({"openai_agents_sdk", "google_adk"}) +#: Import hops a change is related to an agent file through. +MAX_IMPORT_DEPTH = 6 +#: Python files one side reads to relate the change; past it, a named limit. +MAX_RELATED_FILES = 2000 +#: Files, and bytes, one side reads to find its agent files; past either, a +#: named limit. +MAX_AGENT_CANDIDATES = 5000 +MAX_AGENT_CANDIDATE_BYTES = 64 * 1024 * 1024 +#: The largest Python file read, as the comparison itself bounds it. +MAX_PYTHON_BYTES = 2_000_000 +_LISTING_BYTES = 64 * 1024 * 1024 +_BATCH_BYTES = 16 * 1024 * 1024 +#: Text every supported agent file holds: an ``agents``/``openai_agents`` +#: import, or ``adk`` (``google.adk``, ``from google import adk``). +_AGENT_TEXT = r"(agents|adk|\.(tools|handoffs|mcp_servers|sub_agents))" +#: Classes whose construction, or a subclass of which, makes a file an agent +#: file for scope choice; a module that only defines tools is not one. +_AGENT_CLASSES = frozenset({"Agent", "LlmAgent", "SequentialAgent", "ParallelAgent", "LoopAgent", "BaseAgent"}) +#: Keywords a copy of an agent passes its own capabilities by. +_CAPABILITIES = frozenset({"tools", "handoffs", "mcp_servers", "sub_agents"}) + + +@dataclass +class ScopeSelection: + """The comparisons a change derives, and why: each ``(base scope, head + scope)``, the same path unless an application directory moved.""" + + pairs: list[tuple[str, str]] = field(default_factory=list) + changed: list[str] = field(default_factory=list) + relations: list[dict[str, Any]] = field(default_factory=list) + limits: list[str] = field(default_factory=list) + + @property + def scopes(self) -> list[str]: + return [new for _, new in self.pairs] + + def reason(self) -> str: + if not self.changed: + return "The change touches no Python source." + if not self.pairs and all(_is_test_path(path) for path in self.changed): + return "The change touches no Python source outside tests." + if not self.pairs: + shown = ", ".join(self.changed[:5]) + ( + f", and {len(self.changed) - 5} more" if len(self.changed) > 5 else "" + ) + verb = "is not an agent file, and is not" if len(self.changed) == 1 else "are not agent files, and are not" + bounded = " within the bounds read" if self.limits else "" + return ( + f"The change touches no OpenAI Agents SDK or Google ADK agent{bounded}: {shown} " + f"{verb} imported by one or importing one within {MAX_IMPORT_DEPTH} hops." + ) + parts = [] + for relation in self.relations: + changed, agent, via = relation["first"] + how = ( + f"{changed} is an agent file" + if changed == agent + else f"{changed} is related to agent file {agent} through " + " → ".join(via) + ) + if relation["base_scope"] != relation["scope"]: + how += f" (moved from {relation['base_scope']})" + if relation.get("outside"): + how += ( + f"; agent files elsewhere also import it and are not compared: " + f"{', '.join(relation['outside'][:3])}" + + (f", and {len(relation['outside']) - 3} more" if len(relation["outside"]) > 3 else "") + ) + parts.append(f"{relation['scope']}: {how}") + return "; ".join(parts) + + def payload(self) -> dict[str, Any]: + return { + "mode": "derived", + "scopes": self.scopes, + "reason": self.reason(), + "changed_files": self.changed[:50], + "relations": [ + {key: value for key, value in relation.items() if key != "first"} + for relation in self.relations + ], + "limits": self.limits, + } + + +class _Tree: + """One commit's Python files, read on demand from its objects.""" + + def __init__(self, workspace: Path, commit: str) -> None: + self.workspace = workspace + self.commit = commit + self.blobs: dict[str, _TreeBlob] = {} + self.inits: set[str] = set() + self.directories: set[str] = set() + #: ``path -> what`` of entries that are neither a regular file nor a + #: directory: a submodule, a link to a module. + self.special: dict[str, str] = {} + #: Modules under a directory discovery skips: related, never agents. + self.skipped: set[str] = set() + #: Agent files that construct, subclass or copy an agent — not the + #: modules that only rewire one (``server.py``). + self.builders: set[str] = set() + #: Files whose import reach stopped at the hop bound. + self.cut_from: set[str] = set() + #: The agent files found, once looked for. + self.agents: set[str] = set() + self.listed = self._list() + self._texts: dict[str, str | None] = {} + self._imports: dict[str, set[str]] = {} + #: The modules an import names, without the packages it runs on the + #: way: ``from app import x`` names ``app/__init__.py``; ``from + #: app.other import y`` only runs it. + self._direct: dict[str, set[str]] = {} + self._sink: set[str] | None = None + #: ``module -> (builders it imports within MAX_IMPORT_DEPTH, cut)``. + self._builders_reached: dict[str, tuple[set[str], bool]] = {} + #: ``(path, on_request) -> _builds_agent`` of that file's text. + self._builds: dict[tuple[str, bool], bool] = {} + #: The namespace path entries an importer's absolute imports go + #: through: ``src`` for ``myapp.tools`` in ``src/myapp`` without an + #: ``__init__.py``; the scope must hold them for the reader to follow. + self.import_roots: dict[str, set[str]] = {} + self._by_stem: dict[str, list[str]] | None = None + self.limits: list[str] = [] + self._related = 0 + #: Whether following imports stopped at ``MAX_IMPORT_DEPTH`` with more + #: to follow. + self.cut_short = False + + def _list(self) -> bool: + output = _run_git_bounded_output( + self.workspace, + ["ls-tree", "-r", "-z", "-l", self.commit], + max_output_bytes=_LISTING_BYTES, + ) + if output is None: + return False + for raw in output.split(b"\0"): + meta, _, name = raw.partition(b"\t") + fields = meta.split() + path = name.decode("utf-8", errors="replace") + if len(fields) == 4 and fields[0] == b"160000": + # A submodule: its content is not in this tree (#875 review). + self.special[path] = "a submodule, whose content is not read" + continue + if len(fields) == 4 and fields[0] == b"120000" and ( + path.endswith(".py") or not PurePosixPath(path).suffix + ): + # A link to a module or a directory: what it points at is not + # read (#875 review). + self.special[path] = "a link, which is not read" + continue + if len(fields) != 4 or fields[0] not in {b"100644", b"100755"}: + continue + self.directories.update(str(parent) for parent in PurePosixPath(path).parents if str(parent) != ".") + if not path.endswith(".py"): + continue + if any(_skip_part(part) for part in PurePosixPath(path).parts[:-1]): + # ``build/``, ``fixtures/``: never an agent file, but an agent + # may import from it (#875 review). + self.skipped.add(path) + try: + size = int(fields[3]) + except ValueError: + continue + self.blobs[path] = _TreeBlob(fields[0].decode(), fields[2].decode(), size) + if path.endswith("/__init__.py"): + self.inits.add(str(PurePosixPath(path).parent)) + return True + + def holds(self, directory: str) -> bool: + return not directory or directory in self.directories + + def application(self, path: str) -> bool: + """A Python file read to relate the change. A file named like a test + is read when an agent imports it; it is never an agent file.""" + + return path in self.blobs and self.blobs[path].size <= MAX_PYTHON_BYTES + + def texts(self, paths: list[str], *, candidates: bool = False) -> None: + """Read ``paths`` in bounded batches; past the budget, a named limit.""" + + wanted = [path for path in paths if path not in self._texts and self.application(path)] + if candidates: + bound, room = MAX_AGENT_CANDIDATES, MAX_AGENT_CANDIDATES + what = "to find its agent files" + total_bytes, kept = 0, [] + for path in wanted: + total_bytes += self.blobs[path].size + if total_bytes > MAX_AGENT_CANDIDATE_BYTES: + self._limit( + f"{self.commit[:12]} holds more than {MAX_AGENT_CANDIDATE_BYTES // (1024 * 1024)} MB " + f"of Python to read {what}; files past the bound were not read." + ) + break + kept.append(path) + wanted = kept + else: + bound, room = MAX_RELATED_FILES, MAX_RELATED_FILES - self._related + what = "to relate the change" + if len(wanted) > room: + self._limit( + f"{self.commit[:12]} holds more than {bound} Python files to read {what}; " + "files past the bound were not read." + ) + wanted = wanted[: max(room, 0)] + if not candidates: + self._related += len(wanted) + chunk: list[str] = [] + total = 0 + for path in [*wanted, None]: + size = self.blobs[path].size if path is not None else 0 + if path is not None and (not chunk or total + size <= _BATCH_BYTES): + chunk.append(path) + total += size + continue + contents = _blob_contents(self.workspace, [self.blobs[item] for item in chunk]) if chunk else {} + for item in chunk: + data = contents.get(self.blobs[item].oid) + self._texts[item] = data.decode("utf-8", errors="replace") if data is not None else None + if chunk and not contents: + self._limit(f"{len(chunk)} Python files of {self.commit[:12]} could not be read.") + chunk, total = ([path], size) if path is not None else ([], 0) + + def _limit(self, message: str) -> None: + if message not in self.limits: + self.limits.append(message) + + def text(self, path: str) -> str | None: + if path not in self._texts: + self.texts([path]) + return self._texts.get(path) + + def agent_files(self) -> set[str]: + """Files that build an agent: discovery scores them as SDK or ADK + sources, and they construct an agent class or subclass one. A module + that only defines tools is a changed file an agent may import.""" + + output = _run_git_bounded_output( + self.workspace, + ["grep", "-l", "-z", "-E", _AGENT_TEXT, self.commit, "--", "*.py"], + max_output_bytes=_LISTING_BYTES, + allowed_returncodes=(0, 1), + ) + prefix = f"{self.commit}:" + candidates = [ + item.decode("utf-8", errors="replace").removeprefix(prefix) + for item in (output or b"").split(b"\0") + if item + ] + candidates = [ + path + for path in candidates + if self.application(path) and not _is_test_path(path) and path not in self.skipped + ] + if output is None: + self._limit(f"The agent files of {self.commit[:12]} could not be searched.") + self.texts(candidates, candidates=True) + wiring: set[str] = set() + for path in candidates: + text = self._texts.get(path) + if text is None: + continue + if application_frameworks(path, text) & SUPPORTED and _builds_agent(text): + self.builders.add(path) + elif _rewires_agent(text): + wiring.add(path) + # A module that rewires an agent is agent wiring only when it imports a + # module that builds one: ``params.tools = ...`` on another library's + # object is not (#875 review). + self._consumers() + return self.builders | {path for path in wiring if self.imports(path) & self.builders} + + def _consumers(self) -> None: + """Add the modules that call a function they import from an agent + builder — ``root_agent = make('bot', [shared_tool])`` with ``make`` + from ``lib/factory.py`` — which build their agent through it (#875 + review).""" + + stems = sorted( + { + PurePosixPath(path).parent.name if path.endswith("/__init__.py") else PurePosixPath(path).stem + for path in self.builders + } + - {""} + ) + if not stems: + return + found: list[str] = [] + for start in range(0, len(stems), 200): + chunk = stems[start : start + 200] + output = _run_git_bounded_output( + self.workspace, + ["grep", "-l", "-z", "-F", *[item for stem in chunk for item in ("-e", stem)], self.commit, "--", "*.py"], + max_output_bytes=_LISTING_BYTES, + allowed_returncodes=(0, 1), + ) + if output is None: + self._limit(f"The modules importing the agent files of {self.commit[:12]} could not be searched.") + return + prefix = f"{self.commit}:" + found += [ + item.decode("utf-8", errors="replace").removeprefix(prefix) for item in output.split(b"\0") if item + ] + candidates = [ + path + for path in sorted(set(found)) + if path not in self.builders + and self.application(path) + and not _is_test_path(path) + and path not in self.skipped + ] + self.texts(candidates, candidates=True) + for path in candidates: + text = self._texts.get(path) + if text is None or not self.imports(path) & self.builders: + continue + try: + module = ast.parse(text) + except (SyntaxError, ValueError, RecursionError): + continue + directory = PurePosixPath(path).parent + callable_names: set[str] = set() + for node in ast.walk(module): + if isinstance(node, ast.ImportFrom): + for alias in node.names: + if node.level: + base = directory + for _ in range(node.level - 1): + base = base.parent + parts = [*base.parts, *(node.module.split(".") if node.module else [])] + module_targets = self._exact(parts, []) + name_targets = self._exact(parts, [alias.name]) - module_targets + elif node.module: + module_targets = self._absolute(node.module.split("."), [], path) + name_targets = self._absolute(node.module.split("."), [alias.name], path) - module_targets + else: + continue + bound = alias.asname or alias.name + # ``from lib.factory import make``: a function of it. + if alias.name in set().union(*(self._factories(item) for item in module_targets & self.builders)): + callable_names.add(bound) + # ``from lib import factory``: the module itself. + for item in name_targets & self.builders: + callable_names |= {f"{bound}.{name}" for name in self._factories(item)} + elif isinstance(node, ast.Import): + for alias in node.names: + modules = self._absolute(alias.name.split("."), [], path) & self.builders + factories = set().union(*(self._factories(item) for item in modules)) + if factories: + head = alias.asname or alias.name.split(".", 1)[0] + callable_names |= {f"{head}.{name}" for name in factories} + if any( + isinstance(node, ast.Call) + and ( + (isinstance(node.func, ast.Name) and node.func.id in callable_names) + or _dotted(node.func) in callable_names + ) + for node in ast.walk(module) + ): + self.builders.add(path) + + def importers_of(self, paths: set[str]) -> dict[str, set[str]] | None: + """``path -> the modules importing it`` for each of ``paths``, read from + one search of their module names; None past the bound.""" + + stems = sorted( + { + PurePosixPath(path).parent.name if path.endswith("/__init__.py") else PurePosixPath(path).stem + for path in paths + } + - {""} + ) + # An import spells the module's name as a word; a relative one + # (``from . import tool``) only inside the package holding it. + searches = [ + ["-w", *[item for stem in stems[start : start + 200] for item in ("-e", stem)], "--", "*.py"] + for start in range(0, len(stems), 200) + ] + packages = sorted({self.package_root(str(PurePosixPath(path).parent)) for path in paths} - {"", "."}) + for start in range(0, len(packages), 200): + searches.append( + ["-e", "from .", "--", *[f"{package}/*.py" for package in packages[start : start + 200]]] + ) + found: set[str] = set() + for search in searches: + output = _run_git_bounded_output( + self.workspace, + ["grep", "-l", "-z", "-F", *search[: search.index("--")], self.commit, *search[search.index("--") :]], + max_output_bytes=_LISTING_BYTES, + allowed_returncodes=(0, 1), + ) + if output is None: + return None + prefix = f"{self.commit}:" + found |= { + item.decode("utf-8", errors="replace").removeprefix(prefix) for item in output.split(b"\0") if item + } + candidates = sorted( + path for path in found if self.application(path) and not _is_test_path(path) and path not in paths + ) + if len(candidates) > MAX_RELATED_FILES: + return None + self.texts(candidates) + importers: dict[str, set[str]] = {path: set() for path in paths} + for candidate in candidates: + for target in self.direct_imports(candidate) & paths: + importers[target].add(candidate) + return importers + + def dependents(self, paths: set[str]) -> tuple[dict[str, set[str]], set[str]] | None: + """``module -> the files of paths it reaches`` for every module that + imports one of ``paths``, or a module that does, up to + ``MAX_IMPORT_DEPTH`` levels — through a package's re-export or a module + with no agent on the way (#875 review) — and the modules whose + importers were not searched at that bound. None past the bound.""" + + edges: dict[str, set[str]] = {} + seen = set(paths) + frontier = set(paths) + for _ in range(MAX_IMPORT_DEPTH): + importers = self.importers_of(frontier) + if importers is None: + return None + following: set[str] = set() + for target, modules in importers.items(): + for module in modules: + edges.setdefault(module, set()).add(target) + # Through agent files too: one may re-export the change, + # or hand on an agent another module copies (#875 review). + if module not in seen: + seen.add(module) + following.add(module) + frontier = following + if not frontier: + break + origins: dict[str, set[str]] = {path: {path} for path in paths} + grown = True + while grown: + grown = False + for module, targets in edges.items(): + reached = set().union(*(origins.get(target, set()) for target in targets)) + if not reached <= origins.get(module, set()): + origins[module] = origins.get(module, set()) | reached + grown = True + return {module: found for module, found in origins.items() if module not in paths}, frontier + + def builds(self, path: str, *, on_request: bool = False) -> bool: + """``_builds_agent`` of one file, read once.""" + + key = (path, on_request) + if key not in self._builds: + text = self.text(path) + self._builds[key] = text is not None and _builds_agent(text, on_request=on_request) + return self._builds[key] + + def hands_arguments(self, path: str) -> bool: + """Whether a module calls something it imports from the repository + with arguments of its own: what a module does to build an agent + through another's code (``make("bot", [tool])``). An entry script + (``from app.__main__ import main; main()``) does not (#875 review).""" + + text = self.text(path) + try: + module = ast.parse(text) if text is not None else None + except (SyntaxError, ValueError, RecursionError): + return True + if module is None: + return True + directory = PurePosixPath(path).parent + names: set[str] = set() + for node in ast.walk(module): + if isinstance(node, ast.ImportFrom): + for alias in node.names: + if node.level: + base = directory + for _ in range(node.level - 1): + base = base.parent + parts = [*base.parts, *(node.module.split(".") if node.module else [])] + found = self._exact(parts, [alias.name]) + elif node.module: + found = self._absolute(node.module.split("."), [alias.name], path) + else: + found = set() + if found: + names.add(alias.asname or alias.name) + elif isinstance(node, ast.Import): + for alias in node.names: + if self._absolute(alias.name.split("."), [], path): + names.add(alias.asname or alias.name.split(".", 1)[0]) + def rooted(node: ast.AST) -> bool: + while isinstance(node, ast.Attribute | ast.Call | ast.Subscript): + node = node.func if isinstance(node, ast.Call) else node.value + return isinstance(node, ast.Name) and node.id in names + + for node in ast.walk(module): + if isinstance(node, ast.Call) and (node.args or node.keywords) and rooted(node.func): + return True + # ``setattr(factory, "TOOLS", [tool])``. + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id in {"setattr", "delattr"} + and node.args + and rooted(node.args[0]) + ): + return True + # ``factory.TOOLS = [tool]``: state the builder reads (#875 review). + if isinstance(node, ast.Assign | ast.AugAssign | ast.AnnAssign | ast.Delete): + targets = node.targets if isinstance(node, ast.Assign | ast.Delete) else [node.target] + if any(isinstance(target, ast.Attribute | ast.Subscript) and rooted(target) for target in targets): + return True + return False + + def builders_reached(self, module: str) -> tuple[set[str], bool]: + """The agent builders ``module`` imports within ``MAX_IMPORT_DEPTH`` + hops, and whether its imports went deeper.""" + + cached = self._builders_reached.get(module) + if cached is not None: + return cached + seen, frontier, reached = {module}, [module], set() + for _ in range(MAX_IMPORT_DEPTH): + self.texts([target for item in frontier for target in self.imports(item)]) + following = [ + target + for item in frontier + for target in sorted(self.imports(item)) + if target not in seen and self.application(target) + ] + reached |= {target for target in following if target in self.builders} + seen.update(following) + frontier = following + result = (reached, bool(frontier) and any(self.imports(item) - seen for item in frontier)) + self._builders_reached[module] = result + return result + + def _factories(self, path: str) -> set[str]: + """The module-level functions of an agent builder that return an agent + they construct: ``def make(...): return Agent(...)``, or through a + local bound to one.""" + + text = self._texts.get(path) + try: + module = ast.parse(text) if text is not None else None + except (SyntaxError, ValueError, RecursionError): + module = None + if module is None: + return set() + found: set[str] = set() + for function in module.body: + if not isinstance(function, ast.FunctionDef | ast.AsyncFunctionDef): + continue + bound = { + target.id + for node in ast.walk(function) + if isinstance(node, ast.Assign | ast.AnnAssign) + and isinstance(node.value, ast.Call) + and _agent_call(node.value) + for target in (node.targets if isinstance(node, ast.Assign) else [node.target]) + if isinstance(target, ast.Name) + } + for node in ast.walk(function): + if isinstance(node, ast.Return) and ( + (isinstance(node.value, ast.Call) and _agent_call(node.value)) + or (isinstance(node.value, ast.Name) and node.value.id in bound) + ): + found.add(function.name) + return found + + def imports(self, path: str) -> set[str]: + """The repository files one module's imports name.""" + + if path in self._imports: + return self._imports[path] + found: set[str] = set() + self._imports[path] = found + direct = self._direct[path] = set() + text = self.text(path) + if text is None: + return found + try: + tree = ast.parse(text) + except (SyntaxError, ValueError, RecursionError): + return found + outer, self._sink = self._sink, direct + try: + self._import_targets(tree, path, found) + finally: + self._sink = outer + direct.discard(path) + found.discard(path) + return found + + def direct_imports(self, path: str) -> set[str]: + """The modules ``path``'s imports name, not the packages they run.""" + + self.imports(path) + return self._direct.get(path, set()) + + def _import_targets(self, tree: ast.Module, path: str, found: set[str]) -> None: + directory = PurePosixPath(path).parent + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom): + names = [alias.name for alias in node.names if alias.name != "*"] + if node.level: + base = directory + for _ in range(node.level - 1): + base = base.parent + parts = [*base.parts, *(node.module.split(".") if node.module else [])] + found |= self._exact(parts, names) + elif node.module: + found |= self._absolute(node.module.split("."), names, path) + elif isinstance(node, ast.Import): + for alias in node.names: + found |= self._absolute(alias.name.split("."), [], path) + elif ( + isinstance(node, ast.Call) + and ( + (isinstance(node.func, ast.Attribute) and node.func.attr == "import_module") + or (isinstance(node.func, ast.Name) and node.func.id in {"import_module", "__import__"}) + ) + and node.args + and isinstance(node.args[0], ast.Constant) + and isinstance(node.args[0].value, str) + and not node.args[0].value.startswith(".") + ): + # ``importlib.import_module("app.tools")``: an import by name. + found |= self._absolute(node.args[0].value.split("."), [], path) + + def _module(self, parts: list[str]) -> list[str]: + """The module ``parts`` names, and every package ``__init__.py`` the + import runs on the way: ``pkg.impl`` runs ``pkg/__init__.py`` first.""" + + stem = "/".join(parts) + found = [item for item in (f"{stem}.py", f"{stem}/__init__.py") if item in self.blobs] + if self._sink is not None: + self._sink.update(found) + if found: + for length in range(1, len(parts)): + init = "/".join([*parts[:length], "__init__.py"]) + if init in self.blobs: + found.append(init) + return found + + def _exact(self, parts: list[str], names: list[str]) -> set[str]: + found = set(self._module(parts)) + for name in names: + found |= set(self._module([*parts, name])) + return found + + def _absolute(self, parts: list[str], names: list[str], importer: str) -> set[str]: + """``a.b.c`` wherever the importer's path would find it.""" + + if self._by_stem is None: + self._by_stem = {} + for path in self.blobs: + module = path[: -len("/__init__.py")] if path.endswith("/__init__.py") else path[:-3] + self._by_stem.setdefault(PurePosixPath(module).name, []).append(module) + own = str(PurePosixPath(importer).parent) if "/" in importer else "" + found: set[str] = set() + for spelling in [parts, *([*parts, name] for name in names)]: + suffix = "/".join(spelling) + roots = sorted( + { + module[: -len(suffix)].rstrip("/") + for module in self._by_stem.get(spelling[-1], []) + if module == suffix or module.endswith("/" + suffix) + } + ) + # A package directory is not a path entry — ``middlewares/logging.py`` + # in a package is ``tgbot.middlewares.logging``, never ``logging`` — + # except the importer's own directory, a script's first entry. + roots = [root for root in roots if not root or root not in self.inits or root == own] + if not roots: + continue + near = [root for root in roots if not root or root == own or importer.startswith(root + "/")] + if near: + chosen = [max(near, key=len)] + elif spelling[0] in sys.stdlib_module_names: + # ``import logging`` is the standard library unless a module + # on the importer's own path shadows it. + chosen = [] + else: + # Elsewhere, only one path entry holding a package of that + # name (``libs/shared/src/shared/__init__.py`` of a monorepo) + # is taken as the module; a loose same-named script is not + # what runs. + packaged = [ + root for root in roots if ("/".join([root, spelling[0]]) if root else spelling[0]) in self.inits + ] + chosen = packaged if len(packaged) == 1 and len(roots) == 1 else [] + for root in chosen: + modules = self._module([*([root] if root else []), *spelling]) + found |= set(modules) + top = "/".join([root, spelling[0]]) if root else spelling[0] + if modules and top not in self.inits: + # ``myapp.tools`` found through ``src/`` with ``src/myapp`` + # a namespace package: the reader needs ``src`` in scope. + self.import_roots.setdefault(importer, set()).add(root) + return found + + def package_root(self, directory: str) -> str: + """The outermost regular package holding ``directory``: relative + imports climb within it, and absolute ones name it.""" + + current = PurePosixPath(directory) if directory else PurePosixPath("") + if str(current) not in self.inits and directory: + return directory + while current.parts and str(current.parent) != "." and str(current.parent) in self.inits: + current = current.parent + return str(current) if current.parts else "" + + +def _agent_call(call: ast.Call) -> bool: + func = call.func.value if isinstance(call.func, ast.Subscript) else call.func + name = func.attr if isinstance(func, ast.Attribute) else func.id if isinstance(func, ast.Name) else None + return name in _AGENT_CLASSES + + +def _dotted(node: ast.AST) -> str | None: + if isinstance(node, ast.Attribute): + head = _dotted(node.value) + return f"{head}.{node.attr}" if head else None + return node.id if isinstance(node, ast.Name) else None + + +def _rewires(node: ast.AST) -> bool: + """``x.tools = ...``, ``x.tools += ...``, ``x.tools.append(...)``, + ``setattr(x, "tools", ...)``.""" + + if isinstance(node, ast.Assign | ast.AugAssign | ast.AnnAssign): + targets = node.targets if isinstance(node, ast.Assign) else [node.target] + return any(isinstance(item, ast.Attribute) and item.attr in _CAPABILITIES for item in targets) + if isinstance(node, ast.Call): + func = node.func + if ( + isinstance(func, ast.Attribute) + and func.attr in {"append", "extend", "insert", "remove", "pop", "clear"} + and isinstance(func.value, ast.Attribute) + and func.value.attr in _CAPABILITIES + ): + return True + return ( + isinstance(func, ast.Name) + and func.id in {"setattr", "delattr"} + and len(node.args) >= 2 + and isinstance(node.args[1], ast.Constant) + and node.args[1].value in _CAPABILITIES + ) + return False + + +def _rewired_values(node: ast.AST) -> list[ast.expr]: + """What a rewire (``_rewires``) gives the capability; a removal gives nothing.""" + + if isinstance(node, ast.Assign | ast.AugAssign | ast.AnnAssign): + return [node.value] if node.value is not None else [] + if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute): + if node.func.attr in {"append", "extend", "insert"}: + return list(node.args[-1:]) + [item.value for item in node.keywords] + if node.func.attr in {"remove", "pop", "clear"}: + return [] + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "setattr": + return list(node.args[2:3]) + return [] + + +def _copied_values(call: ast.Call) -> list[ast.expr]: + """The capabilities a copy passes: ``clone(tools=...)``, ``update={"tools": ...}``; + a non-literal ``update`` is its own value.""" + + found: list[ast.expr] = [] + for item in call.keywords: + if item.arg in _CAPABILITIES: + found.append(item.value) + elif item.arg == "update": + if isinstance(item.value, ast.Dict): + found += [ + value + for key, value in zip(item.value.keys, item.value.values, strict=False) + if key is None or not isinstance(key, ast.Constant) or key.value in _CAPABILITIES + ] + else: + found.append(item.value) + return found + + +def _rewires_agent(text: str) -> bool: + """Whether a module changes an agent's capabilities after construction: wiring, + whatever framework signal it carries (``server.py`` importing an app's + agent and appending a tool, #875 review).""" + + try: + tree = ast.parse(text) + except (SyntaxError, ValueError, RecursionError): + return False + return any(_rewires(node) for node in ast.walk(tree)) + + +def _builds_agent(text: str, *, on_request: bool = False) -> bool: + """Whether a module constructs an agent class (under any name it imports it + as), subclasses one, copies an agent with capabilities of its own + (``base.clone(tools=[...])``, ``replace(agent, tools=...)``), or changes an + agent's capabilities after construction (``support.tools.append(x)``) — + all of which the readers treat as agent wiring (#876, #875 review). + + ``on_request``: only inside a function or class, or by subclassing — an + agent another module can have built with its own tools, not only one the + module builds once (#875 review).""" + + try: + tree = ast.parse(text) + except (SyntaxError, ValueError, RecursionError): + return False + # ``from agents import Agent as SdkAgent``, ``from google.adk.agents import + # LlmAgent as Llm``. + aliases = { + alias.asname + for node in ast.walk(tree) + if isinstance(node, ast.ImportFrom) + and (node.module or "").split(".", 1)[0] in {"agents", "google"} + for alias in node.names + if alias.asname and alias.name in _AGENT_CLASSES + } + + #: Names the module binds at import: its functions, classes, imports and + #: top-level assignments. + module_names = set() + for node in tree.body: + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef): + module_names.add(node.name) + elif isinstance(node, ast.Import | ast.ImportFrom): + module_names |= {alias.asname or alias.name.split(".", 1)[0] for alias in node.names} + elif isinstance(node, ast.Assign | ast.AnnAssign): + for target in node.targets if isinstance(node, ast.Assign) else [node.target]: + if isinstance(target, ast.Name): + module_names.add(target.id) + + def named(node: ast.AST) -> bool: + if isinstance(node, ast.Subscript): + node = node.value + if isinstance(node, ast.Attribute): + return node.attr in _AGENT_CLASSES + return isinstance(node, ast.Name) and (node.id in _AGENT_CLASSES or node.id in aliases) + + agents = { + target.id + for node in ast.walk(tree) + if isinstance(node, ast.Assign) and isinstance(node.value, ast.Call) and named(node.value.func) + for target in node.targets + if isinstance(target, ast.Name) + } + + def copies(call: ast.Call) -> bool: + func = call.func + name = func.attr if isinstance(func, ast.Attribute) else func.id if isinstance(func, ast.Name) else None + if name not in {"clone", "replace", "model_copy"}: + return False + for item in call.keywords: + if item.arg in _CAPABILITIES: + return True + # Google ADK / pydantic: ``agent.clone(update={"tools": [...]})``; + # an ``update`` that is not a literal only on an agent the module + # constructs — a settings model's ``model_copy`` is not (#875 review). + if item.arg != "update": + continue + if isinstance(item.value, ast.Dict): + if any( + key is None or not isinstance(key, ast.Constant) or key.value in _CAPABILITIES + for key in item.value.keys + ): + return True + elif isinstance(func, ast.Attribute) and isinstance(func.value, ast.Name) and func.value.id in agents: + return True + return False + + def own(value: ast.expr) -> bool: + """A single name the module binds at import: ``agent.tools.append(lookup)``.""" + + return isinstance(value, ast.Name) and value.id in module_names + + def fixed(value: ast.expr) -> bool: + """``[lookup, search]``: a literal of names the module binds at import.""" + + return isinstance(value, ast.List | ast.Tuple) and all( + isinstance(item, ast.Name) and item.id in module_names for item in value.elts + ) + + if on_request: + # On request: an agent built with capabilities another module can + # supply or change — ``tools=tools``, ``list(REGISTRY.items)``, + # ``self.tools``, ``**config`` — or an agent subclass; one built with + # a fixed list of its own names (``tools=[lookup]``) takes nothing + # from a caller but a copy or a rewire, which is the caller's own + # construction (#875 review). + for node in ast.walk(tree): + if isinstance(node, ast.ClassDef) and any(named(base) for base in node.bases): + return True + if isinstance(node, ast.Call) and named(node.func) and any( + item.arg is None or (item.arg in _CAPABILITIES and not fixed(item.value)) for item in node.keywords + ): + return True + # Or given afterwards: a rewire or a copy whose capabilities are + # not the module's own (``agent.tools.append(extra)``, + # ``BASE.clone(tools=tools)``) (#875 review). + if _rewires(node) and not all(fixed(value) or own(value) for value in _rewired_values(node)): + return True + if isinstance(node, ast.Call) and copies(node) and not all( + fixed(value) or own(value) for value in _copied_values(node) + ): + return True + return False + roots: list[ast.AST] = [tree] + for root in roots: + for node in ast.walk(root): + if isinstance(node, ast.Call) and (named(node.func) or copies(node)): + return True + if _rewires(node) and not on_request: + return True + if isinstance(node, ast.ClassDef) and any(named(base) for base in node.bases): + return True + return False + + +def _common_directory(paths: set[str]) -> str: + parents = [PurePosixPath(path).parent.parts for path in paths] + common: list[str] = [] + for position, part in enumerate(parents[0]): + if all(len(item) > position and item[position] == part for item in parents): + common.append(part) + else: + break + return "/".join(common) + + +def _relate(tree: _Tree, changed: set[str], agents: set[str]) -> list[tuple[set[str], dict[str, Any]]]: + """Each changed file and an agent file an import path joins, with the + files a scope needs to read that agent: the path, the agents it imports, + the repository modules it imports, and the path entries they go through.""" + + groups: list[tuple[set[str], dict[str, Any]]] = [] + + def reach(start: str) -> dict[str, list[str]]: + paths = {start: [start]} + frontier = [start] + for _ in range(MAX_IMPORT_DEPTH): + following: list[str] = [] + tree.texts([target for item in frontier for target in tree.imports(item)]) + for item in frontier: + for target in sorted(tree.imports(item)): + if target not in paths and tree.application(target): + paths[target] = [*paths[item], target] + following.append(target) + frontier = following + if frontier and any(tree.imports(item) for item in frontier): + tree.cut_short = True + tree.cut_from.add(start) + return paths + + def needed(agent: str, members: set[str]) -> set[str]: + files = set(members) | {target for target in tree.imports(agent) if tree.application(target)} + # A path entry the reader must hold, as a file directly under it. + for item in list(files): + files |= {f"{root}/__entry__" if root else "__entry__" for root in tree.import_roots.get(item, ())} + return files + + reached_from: dict[str, dict[str, list[str]]] = {} + for path in sorted(changed & agents): + groups.append((needed(path, {path}), {"changed": path, "agent": path, "via": [path]})) + for agent in sorted(agents): + reached = reached_from[agent] = reach(agent) + for path in sorted(changed & set(reached)): + if path == agent: + continue + members = {*reached[path], *(item for item in reached if item in agents)} + groups.append((needed(agent, members), {"changed": path, "agent": agent, "via": reached[path]})) + for path in sorted(changed - agents): + if _is_test_path(path): + # A test that imports an agent is not the application: it relates + # only when an agent imports it (#875 review). + continue + reached = reach(path) + for agent in sorted(agents & set(reached)): + groups.append((needed(agent, set(reached[agent])), {"changed": path, "agent": agent, "via": reached[agent]})) + return groups + + +def derive_scopes(workspace: Path, base_commit: str, head_commit: str) -> ScopeSelection: + """The comparisons the change between two commits touches (#875).""" + + selection = ScopeSelection() + output = _run_git_bounded_output( + workspace, + ["diff", "--name-status", "-z", "-M", base_commit, head_commit], + max_output_bytes=_LISTING_BYTES, + ) + if output is None: + selection.limits.append( + "The change between the two commits could not be listed; pass --scope." + ) + return selection + fields = [item.decode("utf-8", errors="replace") for item in output.split(b"\0")] + changed_paths: set[str] = set() + renames: list[tuple[str, str]] = [] + index = 0 + while index < len(fields) and fields[index]: + status = fields[index] + if status[:1] in {"R", "C"} and index + 2 < len(fields): + old, new = fields[index + 1], fields[index + 2] + changed_paths |= {old, new} + if status[:1] == "R" and old.endswith(".py") and new.endswith(".py"): + renames.append((old, new)) + index += 3 + else: + changed_paths.add(fields[index + 1] if index + 1 < len(fields) else "") + index += 2 + # A file named like a test is related when an agent imports it + # (``app/test_runner.py``); it is never an agent file itself. + python = sorted(path for path in changed_paths if path.endswith(".py")) + selection.changed = python + #: ``(side, scope, relation, outside)`` for every scope a change needs. + found: list[tuple[str, str, dict[str, Any], list[str]]] = [] + trees: dict[str, _Tree] = {} + for side, commit in (("base", base_commit), ("head", head_commit)): + tree = trees[side] = _Tree(workspace, commit) + if not tree.listed: + selection.limits.append(f"The tree of {commit[:12]} could not be listed; pass --scope.") + continue + changed = {path for path in python if tree.application(path)} + if changed: + agents = tree.agent_files() + tree.agents = agents + by_changed: dict[str, list[tuple[str, dict[str, Any]]]] = {} + for members, relation in _relate(tree, changed, agents): + scope = tree.package_root(_common_directory(members)) + by_changed.setdefault(relation["changed"], []).append((scope, relation)) + for pairs in by_changed.values(): + union = _common_directory({f"{scope}/__entry__" if scope else "__entry__" for scope, _ in pairs}) + union = tree.package_root(union) + if union: + # Every agent the change reaches, in the package holding them. + found.extend((side, union, relation, []) for _, relation in pairs) + continue + # Only the repository root holds them all: compare where the + # change's nearest agents are, and name the others, which + # makes the answer partial rather than widening it (#875). + nearest = [ + item for item in pairs + if not any(other[0] != item[0] and _contains(item[0], other[0]) for other in pairs) + ] + kept = {scope for scope, _ in nearest} + outside = sorted( + { + relation["agent"] + for scope, relation in pairs + if scope not in kept and not any(_contains(item, relation["agent"]) for item in kept) + } + ) + for scope, relation in nearest: + found.append((side, scope, relation, outside)) + pairs = _pairs(found, renames, trees) + # A changed application file no agent is related to is not compared: name + # it, with why, rather than let the other scopes read as the whole change + # (#875 review). + # Related to a module that builds an agent, on either side, under either + # path of a rename; being inside a compared scope is not enough — its + # consumer may be outside it — nor is a module that only rewires an agent. + related = { + relation["changed"] + for side, _, relation, _ in found + if relation["agent"] in trees[side].builders + } + wired = {relation["changed"] for _, _, relation, _ in found} - related + for old, new in renames: + if old in related or new in related: + related |= {old, new} + unread: dict[str, str] = {} + for tree in trees.values(): + if not tree.listed: + continue + for path in sorted(changed_paths): + if path in tree.special and path not in unread: + unread[path] = f"it is {tree.special[path]}" + for path in python: + if path in unread or path in related or _is_test_path(path) or path not in tree.blobs: + continue + if not tree.application(path): + unread[path] = f"it exceeds {MAX_PYTHON_BYTES} bytes and is not read" + elif path in wired: + unread[path] = "only a module that rewires an agent relates to it" + elif pairs: + unread[path] = ( + f"no agent file reaches it within {MAX_IMPORT_DEPTH} import hops, and longer " + "paths were not followed" + if tree.cut_short + else "no agent file imports it, and it imports no agent file within " + f"{MAX_IMPORT_DEPTH} hops" + ) + # An agent outside every compared scope whose imports go deeper than the + # bound may reach the change past it (#875 review). + deep = sorted( + { + agent + for side, tree in trees.items() + for agent in tree.cut_from & tree.agents + if not any(_contains(new if side == "head" else old, agent) for old, new in pairs) + } + ) + if pairs and deep and python: + selection.limits.append( + f"Agent files outside the compared scopes import more than {MAX_IMPORT_DEPTH} hops deep, " + "so whether they reach the change is not established: " + + ", ".join(deep[:5]) + + (f", and {len(deep) - 5} more" if len(deep) > 5 else "") + + "." + ) + # A module that imports the change — directly, through a package's + # re-export or through other modules — and reaches an agent builder builds + # an agent from it, however it spells that: a wrapper class, a static + # method, a helper. It is compared only inside a scope that also holds the + # change and those builders (#875 review). + consumers: set[str] = set() + uncertain: set[str] = set() + for side, tree in trees.items(): + here = {path for path in python if path in tree.blobs and not _is_test_path(path)} + if not tree.listed or not pairs or not here: + continue + closure = tree.dependents(here) + if closure is None: + selection.limits.append( + f"The modules importing the change in {tree.commit[:12]} are more than the bound; " + "agents built outside the compared scopes may use it." + ) + continue + dependents, frontier = closure + compared = [new if side == "head" else old for old, new in pairs] + if any(not any(_contains(scope, module) for scope in compared) for module in frontier): + selection.limits.append( + f"Modules import the change in {tree.commit[:12]} through more than {MAX_IMPORT_DEPTH} " + "others, so whether agents built outside the compared scopes use it is not established." + ) + for module, origins in sorted(dependents.items()): + if module in tree.agents: + continue + builders, cut = tree.builders_reached(module) + # One that builds on request, or an agent this module builds, + # copies or changes itself: importing a module's agent is not + # building one (#875 review). + builders = {item for item in builders if tree.builds(item, on_request=True)} or ( + builders if tree.builds(module) else set() + ) + inside = [scope for scope in compared if _contains(scope, module)] + if builders and (tree.builds(module) or tree.hands_arguments(module)): + if not any(all(_contains(scope, item) for item in origins | builders) for scope in inside): + consumers.add(module) + elif cut and not inside: + uncertain.add(module) + if consumers: + shown_consumers = sorted(consumers) + selection.limits.append( + "Modules import the change and an agent builder that no compared scope holds together, " + "so the agents they build are not compared: " + + ", ".join(shown_consumers[:5]) + + (f", and {len(shown_consumers) - 5} more" if len(shown_consumers) > 5 else "") + + "." + ) + uncertain -= consumers + if uncertain: + shown_uncertain = sorted(uncertain) + selection.limits.append( + f"Modules import the change and more than {MAX_IMPORT_DEPTH} hops of other modules, so " + "whether they build an agent from it is not established: " + + ", ".join(shown_uncertain[:5]) + + (f", and {len(shown_uncertain) - 5} more" if len(shown_uncertain) > 5 else "") + + "." + ) + if unread: + shown = sorted(unread.items()) + selection.limits.append( + "Changed files are not related to a compared agent, so their effect is not " + "compared: " + + "; ".join(f"{path} ({reason})" for path, reason in shown[:5]) + + (f"; and {len(shown) - 5} more" if len(shown) > 5 else "") + + "." + ) + if not pairs and any(tree.cut_short for tree in trees.values()): + # "No agent" only as far as the imports were followed. + selection.limits.append( + f"Imports were followed {MAX_IMPORT_DEPTH} hops from each changed and agent file; " + "a longer path from the change to an agent is not established." + ) + # Every bound the trees hit, the searches after the relations included + # (#875 review). + for tree in trees.values(): + selection.limits.extend(item for item in tree.limits if item not in selection.limits) + selection.pairs = [(old or ".", new or ".") for old, new in sorted(pairs)] + for old_scope, new_scope in sorted(pairs): + related = [ + (relation, outside) + for side, scope, relation, outside in found + if _contains(new_scope if side == "head" else old_scope, scope) + ] + outside = sorted({path for _, names in related for path in names if not any(_contains(new, path) for _, new in pairs)}) + first = related[0][0] if related else {"changed": "", "agent": "", "via": []} + selection.relations.append( + { + "scope": new_scope or ".", + "base_scope": old_scope or ".", + "changed": sorted({relation["changed"] for relation, _ in related}), + "agent_files": sorted({relation["agent"] for relation, _ in related}), + "via": first["via"], + "outside": outside, + "first": (first["changed"], first["agent"], first["via"]), + } + ) + if outside: + limit = ( + f"Agent files outside {new_scope or '.'} also import the change and are not compared: " + + ", ".join(outside[:5]) + + (f", and {len(outside) - 5} more" if len(outside) > 5 else "") + + "." + ) + if limit not in selection.limits: + selection.limits.append(limit) + return selection + + +def _pairs( + found: list[tuple[str, str, dict[str, Any], list[str]]], + renames: list[tuple[str, str]], + trees: dict[str, _Tree], +) -> list[tuple[str, str]]: + """The ``(base scope, head scope)`` comparisons: the same path, reduced to + the outermost of nested ones, except where a module moved. + + A module moved inside one package is one comparison of that package. An + application directory moved as a whole — its old scope gone from the head, + its new one absent from the base — is one relocation, compared old path to + new path. A module moved between two applications that both remain is a + change to each, never a root-wide scope (#875).""" + + scopes = {scope for _, scope, _, _ in found} + relocations: set[tuple[str, str]] = set() + base_tree, head_tree = trees.get("base"), trees.get("head") + for old, new in renames: + old_scope = next((scope for side, scope, _, _ in found if side == "base" and _contains(scope, old)), None) + new_scope = next((scope for side, scope, _, _ in found if side == "head" and _contains(scope, new)), None) + if old_scope is None and new_scope is None: + continue + joint = (head_tree or base_tree).package_root(_common_directory({old, new})) if (head_tree or base_tree) else "" + if joint: + scopes.add(joint) + continue + if ( + old_scope is not None + and new_scope is not None + and old_scope != new_scope + and head_tree is not None + and base_tree is not None + and not head_tree.holds(old_scope) + and not base_tree.holds(new_scope) + ): + relocations.add((old_scope, new_scope)) + # ``apps/foo -> services/foo`` holds ``apps/foo/sub -> services/foo/sub``: + # one comparison, not two that repeat its rows (#875 review). + relocations = { + (old, new) + for old, new in relocations + if not any( + (outer_old, outer_new) != (old, new) + and _contains(outer_old, old) + and _contains(outer_new, new) + for outer_old, outer_new in relocations + ) + } + moved = {scope for pair in relocations for scope in pair} + kept: list[str] = [] + for scope in sorted(scopes - moved, key=lambda item: (len(PurePosixPath(item).parts) if item else 0, item)): + # A scope inside a relocated one is compared with it. + if not any(_contains(outer, scope) for outer in [*kept, *moved]): + kept.append(scope) + return [(scope, scope) for scope in kept] + sorted(relocations) + + +def _contains(outer: str, inner: str) -> bool: + return not outer or inner == outer or inner.startswith(outer + "/") diff --git a/src/agents_shipgate/cli/diff.py b/src/agents_shipgate/cli/diff.py index 401ad6470..a44755a2a 100644 --- a/src/agents_shipgate/cli/diff.py +++ b/src/agents_shipgate/cli/diff.py @@ -495,7 +495,7 @@ def diff( ), application: bool = typer.Option(False, "--application", help="Compare source-observed application agent wiring without setup (SDK/ADK)."), head: str | None = typer.Option(None, "--head", help="Application comparison head ref; defaults to committed HEAD."), - scope: str = typer.Option(".", "--scope", help="Application directory relative to the repository."), + scope: str | None = typer.Option(None, "--scope", help="Application directory relative to the repository; derived from the change when omitted."), base_scope: str | None = typer.Option(None, "--base-scope", help="Old application directory for an explicitly selected scope move."), max_python_files: int | None = typer.Option(None, "--max-python-files", min=1, help="Application discovery parse bound."), json_output: bool = typer.Option(False, "--json", help="Emit the rows as JSON."), @@ -515,7 +515,7 @@ def diff( emit_agent_mode_error("input_parse_error" if isinstance(exc, InputParseError) else "config_error", message=str(exc), exit_code=2) raise typer.Exit(2) from exc raise typer.Exit(code) - if head is not None or scope != "." or base_scope is not None or max_python_files is not None: + if head is not None or scope is not None or base_scope is not None or max_python_files is not None: raise typer.BadParameter("--head, --scope, --base-scope and --max-python-files require --application.") raise typer.Exit( run_capability_diff(workspace=workspace, base=base, json_output=json_output) diff --git a/src/agents_shipgate/cli/discovery/signals.py b/src/agents_shipgate/cli/discovery/signals.py index cf856da41..b6c7db308 100644 --- a/src/agents_shipgate/cli/discovery/signals.py +++ b/src/agents_shipgate/cli/discovery/signals.py @@ -901,12 +901,31 @@ def _parse_python_facts(path: Path, workspace: Path) -> _PyFacts | None: source = path.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): return None + return _python_facts_from_source(source, path, _relative(path, workspace)) + + +def application_frameworks(rel_path: str, source: str) -> frozenset[str]: + """The frameworks one Python file's own code claims, by the same signals + discovery scores: ``openai_agents_sdk`` for an ``agents`` import or an + SDK ``@function_tool``, ``google_adk`` for a ``google.adk`` import, and so + on (#875). Read from text, so a commit's blob answers without a checkout. + """ + + facts = _python_facts_from_source(source, Path(rel_path), rel_path) + if facts is None: + return frozenset() + scores = _initial_framework_scores() + _score_python_signals_inner(facts, scores) + return frozenset(name for name, state in scores.items() if state.candidate_files) + + +def _python_facts_from_source(source: str, path: Path, rel_path: str) -> _PyFacts | None: try: tree = ast.parse(source, filename=str(path)) - except SyntaxError: + except (SyntaxError, ValueError, RecursionError): return None - facts = _PyFacts(path=path, rel_path=_relative(path, workspace)) + facts = _PyFacts(path=path, rel_path=rel_path) scopes = _walk_scoped(tree) nodes = scopes.nodes facts.scope_parents = scopes.parents diff --git a/tests/test_application_diff.py b/tests/test_application_diff.py index 212712c70..2f3b20996 100644 --- a/tests/test_application_diff.py +++ b/tests/test_application_diff.py @@ -184,7 +184,7 @@ def test_identical_git_rename_preserves_binding_identity(repo): source = SDK.replace('TOOLS', '[lookup]') base = commit(repo, {'before/agent.py': source}) head = commit(repo, {'before/agent.py': None, 'after/agent.py': source}) - result = run(repo, base, head) + result = run(repo, base, head, '--scope', '.') assert result['rows'] == [] assert result['source_correspondence'] == [{'base_source': 'before/agent.py', 'head_source': 'after/agent.py', 'basis': 'git_rename_identical_blob'}] @@ -234,12 +234,12 @@ def test_gitlink_is_not_a_successful_comparison(repo): git(repo, 'update-index', '--add', '--cacheinfo', f'160000,{base},external') git(repo, '-c', 'user.name=Test', '-c', 'user.email=test@example.com', '-c', 'commit.gpgsign=false', 'commit', '-qm', 'gitlink') - result = run(repo, base, 'HEAD') + result = run(repo, base, 'HEAD', '--scope', '.') assert result['comparison_status'] == 'partial' assert [(g['source'], g['reason']) for g in result['head']['coverage_gaps']] == [ ('external', f'Submodule content is not read (commit {base[:12]}): external')] text = CliRunner().invoke(app, ['diff', '--application', '--workspace', str(repo), - '--base', base, '--head', 'HEAD']) + '--base', base, '--head', 'HEAD', '--scope', '.']) assert 'not a no-change result' in text.output @@ -248,7 +248,7 @@ def test_python_size_bound_precedes_discovery(repo, monkeypatch): ref = commit(repo, {'agent.py': SDK.replace('TOOLS', '[lookup]')}) monkeypatch.setattr(module, 'MAX_PYTHON_BYTES', 8) monkeypatch.setattr(module, 'detect_workspace', lambda *a, **kw: pytest.fail('oversized input parsed')) - result = run(repo, ref, ref) + result = run(repo, ref, ref, '--scope', '.') assert result['comparison_status'] == 'partial' assert result['rows'] == [] @@ -285,7 +285,7 @@ def test_nonidentical_unpaired_move_does_not_invent_new_capabilities(repo): source = SDK.replace('TOOLS', '[lookup]') base = commit(repo, {'old/agent.py': source}) head = commit(repo, {'old/agent.py': None, 'new/agent.py': '# changed comment\n' + source}) - result = run(repo, base, head) + result = run(repo, base, head, '--scope', '.') assert result['comparison_status'] == 'partial' assert result['rows'] assert all(row['change'] == 'not_established' for row in result['rows']) @@ -311,7 +311,7 @@ def test_diagnostic_limits_use_shared_redaction(repo, monkeypatch, json_output): secret = 'sk-privacyaaaaaaaaaaaaaaaa' monkeypatch.setattr(module, 'observe', lambda *a, **kw: module.Observations('.', status='partial', limits=[f'unresolved {secret}'])) - args = ['diff', '--application', '--workspace', str(repo), '--base', ref] + args = ['diff', '--application', '--workspace', str(repo), '--base', ref, '--scope', '.'] if json_output: args.append('--json') result = CliRunner().invoke(app, args) diff --git a/tests/test_application_diff_reach.py b/tests/test_application_diff_reach.py index 83874c2f4..31d461c8b 100644 --- a/tests/test_application_diff_reach.py +++ b/tests/test_application_diff_reach.py @@ -61,7 +61,7 @@ def test_python_link_to_an_input_already_read_is_not_read_twice(repo): link(repo, "helper.py", "real/helper.py") base = commit(repo, {}) head = commit(repo, {"real/helper.py": source.replace("TOOLS", "[lookup, execute]")}) - result = run(repo, base, head) + result = run(repo, base, head, "--scope", ".") assert result["comparison_status"] == "compared", result["head"]["limits"] assert [(r["agent_source"], r["tool"], r["change"]) for r in result["rows"]] == [ ("real/helper.py", "execute", "added") diff --git a/tests/test_application_diff_review.py b/tests/test_application_diff_review.py index 914c6ae9d..b44dc8db0 100644 --- a/tests/test_application_diff_review.py +++ b/tests/test_application_diff_review.py @@ -329,7 +329,7 @@ def broken(*args, **kwargs): monkeypatch.setattr(module, "archive_tree", broken) monkeypatch.setattr(module, "promised_objects_missing", lambda *a: False) result = CliRunner().invoke(app, ["diff", "--application", "--workspace", str(repo), - "--base", ref, "--json"], + "--base", ref, "--json", "--scope", "."], env={"AGENTS_SHIPGATE_AGENT_MODE": "1"}) assert result.exit_code == 2 assert "config_error" in result.output diff --git a/tests/test_application_diff_unobserved.py b/tests/test_application_diff_unobserved.py index b8c4f35d5..0f857aefe 100644 --- a/tests/test_application_diff_unobserved.py +++ b/tests/test_application_diff_unobserved.py @@ -103,7 +103,7 @@ def test_a_test_double_does_not_establish_the_application(repo): }, ) head = commit(repo, {"bots/agents.py": _agents(real.replace("TOOLS", "[quote, send_image]"))}) - result = run(repo, base, head) + result = run(repo, base, head, "--scope", ".") assert _pairs(result) == [("Wycena", "send_image", "added")] for side in ("base", "head"): assert [a["name"] for a in result[side]["agents"]] == ["Wycena"] @@ -455,7 +455,7 @@ def test_a_copy_in_a_module_without_the_sdk_import_is_a_named_limit(repo): ) }, ) - result = run(repo, base, head) + result = run(repo, base, head, "--scope", ".") assert result["comparison_status"] == "partial" assert any("app/variants.py:3" in limit for limit in result["head"]["limits"]) @@ -1089,7 +1089,8 @@ def test_an_adk_subclass_the_module_uses_is_a_limit(repo, use, named): ) base = commit(repo, {"agent.py": body.replace("TOOLS", "[lookup]")}) head = commit(repo, {"agent.py": body.replace("TOOLS", "[lookup, search]")}) - result = run(repo, base, head) + # The reader's answer, not scope selection: ``decorated`` changes nothing. + result = run(repo, base, head, "--scope", ".") assert result["comparison_status"] == "partial" assert any(f"'{named}'" in limit for limit in result["head"]["limits"]), result["head"][ "limits" diff --git a/tests/test_application_scope.py b/tests/test_application_scope.py new file mode 100644 index 000000000..a3646cd9d --- /dev/null +++ b/tests/test_application_scope.py @@ -0,0 +1,1121 @@ +"""#875: without --scope, the application comparison derives its scope from the change.""" + +from __future__ import annotations + +import json +import os +import re +import subprocess + +import pytest +from test_application_diff import commit, run +from test_application_diff import repo as repo +from typer.testing import CliRunner + +from agents_shipgate.cli.main import app + +TOOLS = "def lookup(q: str) -> str:\n return BODY\n" +SUPPORT = ( + "from google.adk.agents import Agent\n\nfrom ..services.gemini_tools import lookup\n\n" + "support_agent = Agent(name='support', model='m', tools=[lookup])\n" +) +SERVICE = ( + "from google.adk.runners import Runner\n\nfrom ..agents.support import support_agent\n\n" + "runner = Runner(agent=support_agent, app_name='a', session_service=None)\n" +) + + +def _vesta() -> dict[str, str]: + """mezgoodle/Vesta#58's shape: tools in ``services``, agents in ``agents``.""" + + return { + "backend/app/__init__.py": "", + "backend/app/agents/__init__.py": "", + "backend/app/agents/support.py": SUPPORT, + "backend/app/services/__init__.py": "", + "backend/app/services/gemini_tools.py": TOOLS.replace("BODY", "q"), + "backend/app/services/adk_service.py": SERVICE, + "frontend/app.js": "console.log(1)\n", + "README.md": "x\n", + } + + +def _rows(result): + return [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] + + +def test_a_tool_module_change_is_compared_in_the_package_that_holds_its_agent(repo): + base = commit(repo, _vesta()) + head = commit(repo, {"backend/app/services/gemini_tools.py": TOOLS.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + selection = result["scope_selection"] + assert selection["mode"] == "derived" + assert selection["scopes"] == ["backend/app"] + assert result["head"]["scope"] == "backend/app" + assert "backend/app/services/gemini_tools.py" in selection["reason"] + assert _rows(result) == [("support", "lookup", "changed")] + + +def test_the_scope_is_reported_in_the_text_output(repo): + base = commit(repo, _vesta()) + head = commit(repo, {"backend/app/services/gemini_tools.py": TOOLS.replace("BODY", "q.upper()")}) + text = CliRunner().invoke( + app, ["diff", "--application", "--workspace", str(repo), "--base", base, "--head", head] + ) + assert text.exit_code == 0, text.output + assert "scope: backend/app (derived:" in text.output + + +def test_a_change_in_a_sibling_package_the_agent_imports_derives_the_package_holding_both(repo): + files = { + "svc/__init__.py": "", + "svc/agents/__init__.py": "", + "svc/agents/support.py": SUPPORT.replace("..services.gemini_tools", "svc.tools.lookups"), + "svc/tools/__init__.py": "", + "svc/tools/lookups.py": TOOLS.replace("BODY", "q"), + } + base = commit(repo, files) + head = commit(repo, {"svc/tools/lookups.py": TOOLS.replace("BODY", "q.strip()")}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["svc"] + assert _rows(result) == [("support", "lookup", "changed")] + + +def test_two_independent_applications_are_two_comparisons_never_the_root(repo): + files = {} + for name in ("alpha", "beta"): + files.update( + { + f"{name}/app/__init__.py": "", + f"{name}/app/tools.py": TOOLS.replace("BODY", "q"), + f"{name}/app/agent.py": ( + "from google.adk.agents import Agent\n\nfrom .tools import lookup\n\n" + f"root_agent = Agent(name='{name}', model='m', tools=[lookup])\n" + ), + } + ) + base = commit(repo, files) + head = commit( + repo, + { + "alpha/app/tools.py": TOOLS.replace("BODY", "q.upper()"), + "beta/app/tools.py": TOOLS.replace("BODY", "q.lower()"), + }, + ) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["alpha/app", "beta/app"] + assert [item["head"]["scope"] for item in result["comparisons"]] == ["alpha/app", "beta/app"] + assert sorted(_rows(result)) == [("alpha", "lookup", "changed"), ("beta", "lookup", "changed")] + # Every path in the joined answer is spelled from the repository root. + assert sorted(row["agent_source"] for row in result["rows"]) == ["alpha/app/agent.py", "beta/app/agent.py"] + assert result["head"]["scope"] is None + + +def test_a_change_that_touches_no_python_says_so(repo): + base = commit(repo, _vesta()) + head = commit(repo, {"README.md": "y\n"}) + result = run(repo, base, head) + assert result["comparison_status"] == "not_established" + assert result["rows"] == [] + assert result["scope_selection"]["scopes"] == [] + assert result["scope_selection"]["reason"] == "The change touches no Python source." + + +def test_a_python_change_no_agent_reaches_names_the_files_it_considered(repo): + base = commit(repo, {**_vesta(), "scripts/cleanup.py": "print(1)\n"}) + head = commit(repo, {"scripts/cleanup.py": "print(2)\n"}) + result = run(repo, base, head) + assert result["comparison_status"] == "not_established" + assert "scripts/cleanup.py" in result["scope_selection"]["reason"] + assert "touches no OpenAI Agents SDK or Google ADK agent" in result["scope_selection"]["reason"] + text = CliRunner().invoke( + app, ["diff", "--application", "--workspace", str(repo), "--base", base, "--head", head] + ) + assert "nothing was compared" in text.output + + +def test_a_module_the_package_init_imports_is_related_through_the_package(repo): + files = { + "pkg/__init__.py": "from . import impl, danger\n\nimpl.lookup = danger.dangerous\n", + "pkg/impl.py": TOOLS.replace("BODY", "q"), + "pkg/danger.py": "def dangerous(q: str) -> str:\n return q\n", + "agent.py": ( + "from google.adk.agents import Agent\nfrom pkg.impl import lookup\n\n" + "root_agent = Agent(name='app', model='m', tools=[lookup])\n" + ), + } + base = commit(repo, files) + head = commit(repo, {"pkg/danger.py": "import os\n\n\ndef dangerous(q: str) -> str:\n return os.system(q)\n"}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["."] + assert result["comparison_status"] == "partial" + + +def test_a_module_moved_between_directories_is_one_comparison(repo): + source = ( + "from google.adk.agents import Agent\n\nworker = Agent(name='worker', tools=[])\n" + "root_agent = Agent(name='root', tools=[], sub_agents=[worker])\n" + ) + base = commit(repo, {"agents/old/agent.py": source, "agents/__init__.py": ""}) + head = commit(repo, {"agents/old/agent.py": None, "agents/new/agent.py": source}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["agents"] + assert result["rows"] == [] + assert result["comparison_status"] == "compared" + + +def test_an_explicit_scope_always_wins(repo): + base = commit(repo, _vesta()) + head = commit(repo, {"backend/app/services/gemini_tools.py": TOOLS.replace("BODY", "q.upper()")}) + result = run(repo, base, head, "--scope", "backend") + assert result["scope_selection"] == { + "mode": "explicit", + "scopes": ["backend"], + "base_scope": "backend", + "reason": "Selected with --scope.", + } + assert result["head"]["scope"] == "backend" + + +def test_a_base_scope_needs_an_explicit_scope(repo): + base = commit(repo, _vesta()) + result = CliRunner().invoke( + app, + ["diff", "--application", "--workspace", str(repo), "--base", base, "--base-scope", "backend"], + ) + assert result.exit_code == 2 + plain = re.sub(r"\x1b\[[0-9;]*m", "", result.output) + assert "pass --scope too" in " ".join(plain.replace("│", " ").split()) + + +def test_a_bound_on_the_files_read_is_named_never_a_silent_root(repo, monkeypatch): + import agents_shipgate.cli.application_scope as module + + monkeypatch.setattr(module, "MAX_RELATED_FILES", 1) + base = commit(repo, _vesta()) + head = commit(repo, {"backend/app/services/gemini_tools.py": TOOLS.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + selection = result["scope_selection"] + assert any("more than 1 Python files" in limit for limit in selection["limits"]) + assert "." not in selection["scopes"] + assert result["comparison_status"] in {"partial", "not_established"} + + +def test_the_derived_answer_is_json_serialisable_and_identified(repo): + base = commit(repo, _vesta()) + head = commit(repo, {"backend/app/services/gemini_tools.py": TOOLS.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + identity = result.pop("comparison_id") + assert identity + json.dumps(result) + + +def test_an_example_app_that_imports_a_library_change_is_named_not_joined(repo): + """The library's own agents decide the scope; an example elsewhere that + imports the changed module is named, never widening it to the root.""" + + agent = ( + "from google.adk.agents import Agent\n\nfrom mylib.tools import lookup\n\n" + "root_agent = Agent(name='NAME', model='m', tools=[lookup])\n" + ) + files = { + "src/mylib/__init__.py": "", + "src/mylib/tools.py": TOOLS.replace("BODY", "q"), + "src/mylib/agents.py": agent.replace("NAME", "library"), + "examples/demo.py": agent.replace("NAME", "demo"), + } + base = commit(repo, files) + head = commit(repo, {"src/mylib/tools.py": TOOLS.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + selection = result["scope_selection"] + assert selection["scopes"] == ["src/mylib"] + assert selection["relations"][0]["outside"] == ["examples/demo.py"] + assert "examples/demo.py" in selection["reason"] + assert _rows(result) == [("library", "lookup", "changed")] + + +def test_a_module_named_like_the_standard_library_elsewhere_is_not_imported(repo): + """``import logging`` in the agent is the standard library, not + ``bot/middlewares/logging.py`` in a separate package.""" + + files = { + **_vesta(), + "backend/app/services/gemini_tools.py": "import logging\n\n\n" + TOOLS.replace("BODY", "q"), + "bot/__init__.py": "", + "bot/middlewares/__init__.py": "", + "bot/middlewares/logging.py": "X = 1\n", + } + base = commit(repo, files) + head = commit(repo, { + "backend/app/services/gemini_tools.py": "import logging\n\n\n" + TOOLS.replace("BODY", "q.upper()"), + "bot/middlewares/logging.py": "X = 2\n", + }) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["backend/app"] + + +# -- review round 1 ----------------------------------------------------------- + +ADK_TOOLS = "def lookup(q: str) -> str:\n \"\"\"Look up.\"\"\"\n return BODY\n\n\ndef ping() -> str:\n return 'p'\n" + + +def test_a_module_that_only_defines_tools_is_not_an_agent_file(repo): + """The agent at the root binds the tool: the root is compared, not the + tool module's package.""" + + tool = "from agents import function_tool\n\n\n@function_tool\ndef lookup(q: str) -> str:\n return BODY\n" + files = { + "main.py": "from agents import Agent\nfrom app.tools.search import lookup\n\nmain_agent = Agent(name='main', tools=[lookup])\n", + "app/__init__.py": "", + "app/tools/__init__.py": "", + "app/tools/search.py": tool.replace("BODY", "q"), + } + base = commit(repo, files) + head = commit(repo, {"app/tools/search.py": tool.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["."] + assert _rows(result) == [("main_agent", "lookup", "changed")] + + +def test_an_agent_left_outside_the_nearest_scope_makes_the_answer_partial(repo): + files = { + "app/__init__.py": "", + "app/tools.py": ADK_TOOLS.replace("BODY", "q"), + "app/health_agent.py": "from google.adk.agents import Agent\nfrom .tools import ping\n\nroot_agent = Agent(name='health', model='m', tools=[ping])\n", + "server.py": "from google.adk.agents import Agent\nfrom app.tools import lookup\n\nroot_agent = Agent(name='main', model='m', tools=[lookup])\n", + } + base = commit(repo, files) + head = commit(repo, {"app/tools.py": ADK_TOOLS.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert result["scope_selection"]["relations"][0]["outside"] == ["server.py"] + assert any("server.py" in limit for limit in result["scope_selection"]["limits"]) + + +def test_an_adk_module_imported_as_from_google_import_adk_is_an_agent_file(repo): + agent = "from google import adk\nfrom svc.tools import lookup, ping\n\nroot_agent = adk.Agent(name='svc', model='m', tools=[TOOLS])\n" + base = commit(repo, {"svc/agent.py": agent.replace("TOOLS", "lookup"), "svc/tools.py": ADK_TOOLS.replace("BODY", "q")}) + head = commit(repo, {"svc/agent.py": agent.replace("TOOLS", "lookup, ping")}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared" + assert _rows(result) == [("svc", "ping", "added")] + + +@pytest.mark.parametrize( + ("files", "scope"), + [ + ( + { + "src/myapp/agent.py": "from google.adk.agents import Agent\nfrom myapp.tools.search import lookup\n\n" + "root_agent = Agent(name='s', model='m', tools=[lookup])\n", + "src/myapp/tools/search.py": ADK_TOOLS.replace("BODY", "q"), + }, + "src", + ), + ( + { + "app/__init__.py": "", + "app/agent.py": "from google.adk.agents import Agent\nfrom app.tools import lookup\nfrom lib.other import ping\n\n" + "root_agent = Agent(name='s', model='m', tools=[lookup, ping])\n", + "app/tools.py": ADK_TOOLS.replace("BODY", "q"), + "lib/__init__.py": "", + "lib/other.py": "def ping() -> str:\n return 'p'\n", + }, + ".", + ), + ], + ids=["namespace-package-under-src", "an-import-outside-the-package"], +) +def test_the_scope_holds_what_the_agent_imports(repo, files, scope): + changed = next(path for path in files if path.endswith(("search.py", "app/tools.py"))) + base = commit(repo, files) + head = commit(repo, {changed: ADK_TOOLS.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == [scope] + assert result["comparison_status"] == "compared", result["head"]["limits"] + assert _rows(result) == [("s", "lookup", "changed")] + + +def test_a_renamed_non_python_file_never_joins_applications(repo): + files = {} + for name in ("alpha", "beta"): + files.update( + { + f"{name}/app/__init__.py": "", + f"{name}/app/tools.py": TOOLS.replace("BODY", "q"), + f"{name}/app/agent.py": "from google.adk.agents import Agent\nfrom .tools import lookup\n\n" + f"root_agent = Agent(name='{name}', model='m', tools=[lookup])\n", + } + ) + notes = "# notes\n" * 20 + base = commit(repo, {**files, "alpha/app/NOTES.md": notes}) + head = commit( + repo, + { + "alpha/app/tools.py": TOOLS.replace("BODY", "q.upper()"), + "beta/app/tools.py": TOOLS.replace("BODY", "q.lower()"), + "alpha/app/NOTES.md": None, + "docs/NOTES.md": notes, + }, + ) + assert run(repo, base, head)["scope_selection"]["scopes"] == ["alpha/app", "beta/app"] + + +def test_an_application_directory_moved_whole_is_one_relocation(repo): + agent = "from google.adk.agents import Agent\nfrom .tools import lookup\n\nroot_agent = Agent(name='foo', model='m', tools=[lookup])\n" + base = commit(repo, {"apps/foo/__init__.py": "", "apps/foo/agent.py": agent, "apps/foo/tools.py": TOOLS.replace("BODY", "q")}) + head = commit( + repo, + { + "apps/foo/__init__.py": None, "apps/foo/agent.py": None, "apps/foo/tools.py": None, + "services/foo/__init__.py": "", "services/foo/agent.py": agent, + "services/foo/tools.py": TOOLS.replace("BODY", "q"), + }, + ) + result = run(repo, base, head) + assert result["base"]["scope"] == "apps/foo" + assert result["head"]["scope"] == "services/foo" + assert result["comparison_status"] == "compared" + assert result["rows"] == [] + + +@pytest.mark.parametrize( + ("files", "changed"), + [ + ( + { + "app/__init__.py": "", + "app/agent.py": "from google.adk.agents import Agent\nfrom .a import lookup\n\nroot_agent = Agent(name='deep', model='m', tools=[lookup])\n", + "app/a.py": "from .b import lookup # noqa: F401\n", + "app/b.py": "from .c import lookup # noqa: F401\n", + "app/c.py": "from .d import lookup # noqa: F401\n", + "app/d.py": TOOLS.replace("BODY", "q"), + }, + "app/d.py", + ), + ( + { + "app/__init__.py": "", + "app/agent.py": "import importlib\n\nfrom google.adk.agents import Agent\n\n" + "lookup = importlib.import_module('app.tools').lookup\nroot_agent = Agent(name='deep', model='m', tools=[lookup])\n", + "app/tools.py": TOOLS.replace("BODY", "q"), + }, + "app/tools.py", + ), + ( + { + "app/__init__.py": "", + "app/agent.py": "from google.adk.agents import Agent\nfrom .test_runner import lookup\n\nroot_agent = Agent(name='deep', model='m', tools=[lookup])\n", + "app/test_runner.py": TOOLS.replace("BODY", "q"), + }, + "app/test_runner.py", + ), + ], + ids=["four-hops", "literal-dynamic-import", "module-named-like-a-test"], +) +def test_a_change_an_agent_reaches_is_related(repo, files, changed): + base = commit(repo, files) + head = commit(repo, {changed: TOOLS.replace("BODY", "q.upper()")}) + assert run(repo, base, head)["scope_selection"]["scopes"] == ["app"] + + +def test_no_agent_found_within_a_bound_is_partial_not_no_agent(repo, monkeypatch): + import agents_shipgate.cli.application_scope as module + + monkeypatch.setattr(module, "MAX_AGENT_CANDIDATES", 1) + files = {**_vesta(), "aaa/notes.py": "agents = 1\n", "aab/more.py": "agents = 2\n"} + base = commit(repo, files) + head = commit(repo, {"backend/app/services/gemini_tools.py": TOOLS.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial" + assert result["scope_selection"]["limits"] + + +def test_a_file_that_copies_an_agent_with_its_own_tools_is_an_agent_file(repo): + """A copy passing its own capabilities is an agent to the readers (#876), so + a change to it is compared, not answered as having no agent in reach.""" + + body = ( + "from agents import Agent, function_tool\nfrom shared import base_agent\n\n\n" + "@function_tool\ndef quote(item: str) -> str:\n return item\n\n\n" + "copy = base_agent.clone(tools=[quote])\n" + ) + base = commit(repo, {"app/agent.py": body}) + head = commit(repo, {"app/agent.py": body + "# touched\n"}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["app"] + assert result["comparison_status"] == "partial" + + +# --------------------------------------------------------------------------- +# #875 review, round 2: a changed file related to no agent is named beside the +# other scopes; an aliased agent class, and a module that rewires an agent, +# are agent files; a changed test does not widen a scope; a relocation holds +# the scopes inside it. + +_ADK_TOOL = 'def lookup(q: str) -> str:\n """Look up."""\n return BODY\n' + + +def _adk(name: str, imports: str, tools: str) -> str: + return f"from google.adk.agents import Agent\n{imports}\n\nroot_agent = Agent(name='{name}', model='m', tools=[{tools}])\n" + + +_OTHER = { + "other/__init__.py": "", + "other/agent.py": _adk("other", "from .tools import lookup", "lookup"), + "other/tools.py": _ADK_TOOL.replace("BODY", "q"), +} +_OTHER_TOUCHED = {"other/agent.py": "# note\n" + _adk("other", "from .tools import lookup", "lookup")} + + +def _chain(depth: int) -> dict[str, str]: + files = {"deep/__init__.py": "", "deep/agent.py": _adk("deep", "from deep.m0 import lookup", "lookup")} + for index in range(depth): + files[f"deep/m{index}.py"] = f"from deep.m{index + 1} import lookup\n" + files[f"deep/m{depth}.py"] = _ADK_TOOL.replace("BODY", "q") + return files + + +@pytest.mark.parametrize( + ("files", "changed", "named"), + [ + ( + { + "app/__init__.py": "", + "app/db.py": "def connect():\n return 1\n", + "scripts/migrate.py": "from app.db import connect\n\nconnect()\n", + }, + {"app/db.py": "def connect():\n return 2\n"}, + "app/db.py (no agent file imports it", + ), + (_chain(7), {"deep/m7.py": _ADK_TOOL.replace("BODY", "q.upper()")}, "deep/m7.py (no agent file reaches it within 6"), + ], + ids=["imported-by-a-script", "beyond-the-hop-bound"], +) +def test_a_changed_file_related_to_no_agent_is_named_beside_other_scopes(repo, files, changed, named): + base = commit(repo, {**files, **_OTHER}) + head = commit(repo, {**changed, **_OTHER_TOUCHED}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any(named in limit for limit in result["scope_selection"]["limits"]), result["scope_selection"] + + +@pytest.mark.parametrize( + "agent", + [ + "from agents import Agent as SdkAgent, function_tool\n\n\n@function_tool\ndef lookup(q: str) -> str:\n" + " return q\n\n\n@function_tool\ndef ping() -> str:\n return 'p'\n\n\nbot = SdkAgent(name='bot', tools=[TOOLS])\n", + ], + ids=["sdk-alias"], +) +def test_an_aliased_agent_class_is_an_agent_file(repo, agent): + base = commit(repo, {"svc/__init__.py": "", "svc/agent.py": agent.replace("TOOLS", "lookup")}) + head = commit(repo, {"svc/agent.py": agent.replace("TOOLS", "lookup, ping")}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared", result["scope_selection"] + assert [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] == [("bot", "ping", "added")] + + +def test_a_module_that_rewires_an_agent_is_an_agent_file(repo): + """``server.py`` appends a tool to an app's agent: a change to that tool is + related through it, never "touches no agent".""" + + files = { + "app/__init__.py": "", + "app/agents/__init__.py": "", + "app/agents/support.py": _adk("support", "from app.tools import lookup", "lookup").replace( + "root_agent", "support_agent" + ), + "app/tools.py": _ADK_TOOL.replace("BODY", "q"), + "app/danger.py": 'def danger(cmd: str) -> str:\n """D."""\n return cmd\n', + "server.py": "from app.agents.support import support_agent\nfrom app.danger import danger\n\n" + "support_agent.tools.append(danger)\n", + } + base = commit(repo, files) + head = commit(repo, {"app/danger.py": 'def danger(cmd: str) -> str:\n """D."""\n return cmd.upper()\n'}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["."], result["scope_selection"] + assert result["comparison_status"] == "partial" + + +def test_a_relocation_holds_the_scopes_inside_it(repo): + def app(top: str, body: str) -> dict[str, str | None]: + return { + f"{top}/foo/zagent.py": _adk("top", "from .ztools import lookup", "lookup"), + f"{top}/foo/ztools.py": _ADK_TOOL.replace("BODY", "q"), + f"{top}/foo/sub/aagent.py": _adk("sub", "from .atools import lookup", "lookup"), + f"{top}/foo/sub/atools.py": _ADK_TOOL.replace("BODY", body), + } + + base = commit(repo, {**app("apps", "q"), "keep/x.txt": "x\n"}) + head = commit(repo, {**dict.fromkeys(app("apps", "q")), **app("services", "q.upper()")}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["services/foo"], result["scope_selection"] + assert [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] == [("sub", "lookup", "changed")] + + +def test_a_changed_test_that_imports_an_agent_does_not_widen_the_scope(repo): + files = { + f"{name}/app/{item}": content + for name in ("alpha", "beta") + for item, content in ( + ("__init__.py", ""), + ("agent.py", _adk("assistant", "from .tools import lookup", "lookup")), + ("tools.py", _ADK_TOOL.replace("BODY", "q")), + ) + } + test = "from alpha.app.agent import root_agent\n\n\ndef test_agent():\n assert root_agent.name{extra}\n" + base = commit(repo, {**files, "tests/test_alpha.py": test.format(extra=" == 'assistant'")}) + head = commit( + repo, + {"beta/app/tools.py": _ADK_TOOL.replace("BODY", "q.lower()"), "tests/test_alpha.py": test.format(extra="")}, + ) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["beta/app"], result["scope_selection"] + + +# --------------------------------------------------------------------------- +# #875 review, round 3: a changed file is exempt only when related to a file +# that builds an agent — not for being inside a compared scope; a changed +# link or submodule is named; an agent's tools under a directory discovery +# skips are related. + +def _git(root, *args): + subprocess.run( + ["git", "-C", str(root), "-c", "user.name=T", "-c", "user.email=t@e.com", "-c", "commit.gpgsign=false", *args], + check=True, + capture_output=True, + ) + + +def _head(root) -> str: + return subprocess.run(["git", "-C", str(root), "rev-parse", "HEAD"], check=True, capture_output=True, text=True).stdout.strip() + + +def test_a_changed_file_inside_a_compared_scope_whose_consumer_is_outside_is_named(repo): + files = { + "app/__init__.py": "", + "app/agent.py": _adk("app", "from .tools import lookup", "lookup"), + "app/tools.py": _ADK_TOOL.replace("BODY", "q"), + "app/shared.py": _ADK_TOOL.replace("lookup", "shared_tool").replace("BODY", "q"), + "lib/__init__.py": "", + "lib/factory.py": "from google.adk.agents import Agent\n\n\ndef make(name, tools):\n" + " return Agent(name=name, model='m', tools=tools)\n", + "server.py": "from lib.factory import make\nfrom app.shared import shared_tool\n\nroot_agent = make('bot', [shared_tool])\n", + } + base = commit(repo, files) + head = commit( + repo, + { + "app/shared.py": _ADK_TOOL.replace("lookup", "shared_tool").replace("BODY", "q.upper()"), + "app/agent.py": "# note\n" + _adk("app", "from .tools import lookup", "lookup"), + }, + ) + result = run(repo, base, head) + # ``server.py`` builds its agent through ``lib/factory.make``: it is an + # agent file, so the change is compared where it and the change meet. + assert result["comparison_status"] == "partial", result["scope_selection"] + assert result["scope_selection"]["scopes"] == ["."], result["scope_selection"] + + +def test_a_changed_link_to_a_module_is_named(repo): + files = { + "app/__init__.py": "", + "app/agent.py": _adk("app", "from .tools import lookup", "lookup"), + "shared/safe.py": _ADK_TOOL.replace("BODY", "q"), + "shared/danger.py": _ADK_TOOL.replace("BODY", "q.upper()"), + **_OTHER, + } + for path, content in files.items(): + (repo / path).parent.mkdir(parents=True, exist_ok=True) + (repo / path).write_text(content) + os.symlink("../shared/safe.py", repo / "app/tools.py") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "base") + base = _head(repo) + (repo / "app/tools.py").unlink() + os.symlink("../shared/danger.py", repo / "app/tools.py") + head = commit(repo, _OTHER_TOUCHED) + assert base != head + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("app/tools.py (it is a link" in limit for limit in result["scope_selection"]["limits"]) + + +def test_a_changed_submodule_is_named(repo): + sub = repo / "vendor/tools" + sub.mkdir(parents=True) + subprocess.run(["git", "init", "-q", "-b", "main", str(sub)], check=True, capture_output=True) + (sub / "tools.py").write_text(_ADK_TOOL.replace("BODY", "q")) + _git(sub, "add", "-A") + _git(sub, "commit", "-qm", "one") + base = commit(repo, _OTHER) + (sub / "tools.py").write_text(_ADK_TOOL.replace("BODY", "q.upper()")) + _git(sub, "commit", "-qam", "two") + head = commit(repo, _OTHER_TOUCHED) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("vendor/tools (it is a submodule" in limit for limit in result["scope_selection"]["limits"]) + + +def test_an_agents_tools_under_a_skipped_directory_are_related(repo): + files = { + "app/__init__.py": "", + "app/build/__init__.py": "", + "app/build/tools.py": _ADK_TOOL.replace("BODY", "q"), + "app/agent.py": _adk("app", "from app.build.tools import lookup", "lookup"), + } + base = commit(repo, files) + head = commit(repo, {"app/build/tools.py": _ADK_TOOL.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert result["comparison_status"] == "compared", result["scope_selection"] + assert [(row["agent"], row["tool"], row["change"]) for row in result["rows"]] == [("app", "lookup", "changed")] + + +# --------------------------------------------------------------------------- +# #875 review, round 4: a changed link to a directory is named; a module that +# builds its agent through a builder's factory is an agent file; an agent +# outside the compared scopes whose imports go past the hop bound is named. + + +def test_a_changed_link_to_a_directory_is_named(repo): + files = { + "app/__init__.py": "", + "app/agent.py": _adk("app", "from app.lib.tools import lookup", "lookup"), + "shared_v1/__init__.py": "", + "shared_v1/tools.py": _ADK_TOOL.replace("BODY", "q"), + "shared_v2/__init__.py": "", + "shared_v2/tools.py": _ADK_TOOL.replace("BODY", "q.upper()"), + **_OTHER, + } + for path, content in files.items(): + (repo / path).parent.mkdir(parents=True, exist_ok=True) + (repo / path).write_text(content) + os.symlink("../shared_v1", repo / "app/lib") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "base") + base = _head(repo) + (repo / "app/lib").unlink() + os.symlink("../shared_v2", repo / "app/lib") + head = commit(repo, _OTHER_TOUCHED) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("app/lib (it is a link" in limit for limit in result["scope_selection"]["limits"]) + + +def test_a_module_building_its_agent_through_a_factory_is_an_agent_file(repo): + files = { + "app/__init__.py": "", + "app/agent.py": _adk("app", "from .tools import lookup\nfrom . import shared # noqa: F401", "lookup"), + "app/tools.py": _ADK_TOOL.replace("BODY", "q"), + "app/shared.py": _ADK_TOOL.replace("lookup", "shared_tool").replace("BODY", "q"), + "lib/__init__.py": "", + "lib/factory.py": "from google.adk.agents import Agent\n\n\ndef make(name, tools):\n" + " return Agent(name=name, model='m', tools=tools)\n", + "server.py": "from lib.factory import make\nfrom app.shared import shared_tool\n\nroot_agent = make('bot', [shared_tool])\n", + } + base = commit(repo, files) + head = commit(repo, {"app/shared.py": _ADK_TOOL.replace("lookup", "shared_tool").replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any( + "outside app also import the change" in limit and "server.py" in limit + for limit in result["scope_selection"]["limits"] + ), result["scope_selection"] + + +def test_an_agent_outside_the_scopes_importing_past_the_bound_is_named(repo): + files = {"app/__init__.py": "", "app/agent.py": _adk("app", "from .g import lookup", "lookup")} + files.update( + { + "deep/__init__.py": "", + "deep/agent.py": _adk("deep", "from deep.m0 import lookup", "lookup"), + **{f"deep/m{index}.py": f"from deep.m{index + 1} import lookup\n" for index in range(6)}, + "deep/m6.py": "from app.g import lookup\n", + "app/g.py": _ADK_TOOL.replace("BODY", "q"), + } + ) + base = commit(repo, files) + head = commit(repo, {"app/g.py": _ADK_TOOL.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("deep/agent.py" in limit for limit in result["scope_selection"]["limits"]) + + +# --------------------------------------------------------------------------- +# #875 review, round 5: a module outside the compared scopes that imports the +# change and an agent builder is named however it builds its agent. + +_FACTORY_BASE = { + "app/__init__.py": "", + "app/agent.py": _adk("app", "from .tools import lookup\nfrom . import shared # noqa: F401", "lookup"), + "app/tools.py": _ADK_TOOL.replace("BODY", "q"), + "app/shared.py": _ADK_TOOL.replace("lookup", "shared_tool").replace("BODY", "q"), + "lib/__init__.py": "", +} + + +@pytest.mark.parametrize( + ("factory", "server"), + [ + ( + "from google.adk.agents import Agent\n\n\nclass CustomAgent:\n def __init__(self, name, tools):\n" + " self.name, self.tools = name, tools\n\n def build(self):\n" + " return Agent(name=self.name, model='m', tools=self.tools)\n", + "from lib.factory import CustomAgent\nfrom app.shared import shared_tool\n\n" + "root_agent = CustomAgent('bot', [shared_tool]).build()\n", + ), + ( + "from google.adk.agents import Agent\n\n\ndef make(name, tools):\n return Agent(name=name, model='m', tools=tools)\n", + "from lib import factory\nfrom app.shared import shared_tool\n\nroot_agent = factory.make('bot', [shared_tool])\n", + ), + ( + "from google.adk.agents import Agent\n\n\nclass Factory:\n @staticmethod\n def make(name, tools):\n" + " return Agent(name=name, model='m', tools=tools)\n", + "from lib.factory import Factory\nfrom app.shared import shared_tool\n\nroot_agent = Factory.make('bot', [shared_tool])\n", + ), + ( + "from google.adk.agents import Agent\n\n\ndef _build(name, tools):\n return Agent(name=name, model='m', tools=tools)\n\n\n" + "def make(name, tools):\n return _build(name, tools)\n", + "from lib.factory import make\nfrom app.shared import shared_tool\n\nroot_agent = make('bot', [shared_tool])\n", + ), + ], + ids=["wrapper-class", "module-attribute", "static-method", "helper-hop"], +) +def test_a_consumer_outside_the_scopes_is_named_however_it_builds(repo, factory, server): + base = commit(repo, {**_FACTORY_BASE, "lib/factory.py": factory, "server.py": server}) + head = commit(repo, {"app/shared.py": _ADK_TOOL.replace("lookup", "shared_tool").replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("server.py" in limit for limit in result["scope_selection"]["limits"]), result["scope_selection"] + + +# --------------------------------------------------------------------------- +# #875 review, round 6: the consumer is found however far the change travels +# to it, and whatever scope it sits in; the bounds it hits are named. + +_WRAPPER = ( + "from google.adk.agents import Agent\n\n\nclass CustomAgent:\n def __init__(self, name, tools):\n" + " self.name, self.tools = name, list(tools)\n\n def build(self):\n" + " return Agent(name=self.name, model='m', tools=self.tools)\n" +) +_SHARED_HEAD = {"app/shared.py": _ADK_TOOL.replace("lookup", "shared_tool").replace("BODY", "q.upper()")} + + +def _consumer_limits(result): + return [limit for limit in result["scope_selection"]["limits"] if "import the change" in limit] + + +@pytest.mark.parametrize( + "extra", + [ + { + "svc/__init__.py": "", + "svc/c.py": "from lib.factory import CustomAgent\n\n\ndef build_c(tools):\n return CustomAgent('bot', tools).build()\n", + "svc/b.py": "from svc.c import build_c\n\n\ndef build_b(tools):\n return build_c(tools)\n", + "svc/a.py": "from svc.b import build_b\n\n\ndef build_a(tools):\n return build_b(tools)\n", + "server.py": "from svc.a import build_a\nfrom app.shared import shared_tool\n\nroot_agent = build_a([shared_tool])\n", + }, + { + "app/__init__.py": "from .shared import shared_tool # noqa: F401\n", + "server.py": "from lib.factory import CustomAgent\nfrom app import shared_tool\n\n" + "root_agent = CustomAgent('bot', [shared_tool]).build()\n", + }, + { + "bundles/__init__.py": "", + "bundles/tools.py": "from app.shared import shared_tool\n\nTOOLS = [shared_tool]\n", + "server.py": "from lib.factory import CustomAgent\nfrom bundles.tools import TOOLS\n\n" + "root_agent = CustomAgent('bot', TOOLS).build()\n", + }, + ], + ids=["builder-four-hops-away", "package-re-export", "through-another-module"], +) +def test_a_consumer_the_change_reaches_indirectly_is_named(repo, extra): + base = commit(repo, {**_FACTORY_BASE, "lib/factory.py": _WRAPPER, **extra}) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("server.py" in limit for limit in _consumer_limits(result)), result["scope_selection"] + + +def test_a_consumer_inside_another_compared_scope_is_named(repo): + base = commit( + repo, + { + **_FACTORY_BASE, + "lib/factory.py": _WRAPPER, + **_OTHER, + "other/server.py": "from lib.factory import CustomAgent\nfrom app.shared import shared_tool\n\n" + "bot_agent = CustomAgent('bot', [shared_tool]).build()\n", + }, + ) + head = commit(repo, {**_SHARED_HEAD, "other/agent.py": "# note\n" + _OTHER["other/agent.py"]}) + result = run(repo, base, head) + assert result["scope_selection"]["scopes"] == ["app", "other"] + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("other/server.py" in limit for limit in _consumer_limits(result)), result["scope_selection"] + + +def test_a_consumer_inside_the_scope_holding_the_change_and_its_builder_is_compared(repo): + base = commit( + repo, + { + **_FACTORY_BASE, + "app/factory.py": _WRAPPER, + "app/server.py": "from .factory import CustomAgent\nfrom .shared import shared_tool\n\n" + "bot_agent = CustomAgent('bot', [shared_tool]).build()\n", + }, + ) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert _consumer_limits(result) == [], result["scope_selection"] + + +def test_the_bound_the_consumer_search_hits_is_named(repo, monkeypatch): + from agents_shipgate.cli import application_scope + + # Relating the change reads less than the bound; searching for its + # consumers — past modules that mention it — reads more. + monkeypatch.setattr(application_scope, "MAX_RELATED_FILES", 20) + wide = {f"wide/mods/m{index}.py": "VALUE = 1\n" for index in range(10)} + noise = {f"aa_noise/n{index}.py": "# shared\nVALUE = 1\n" for index in range(15)} + base = commit( + repo, + { + **_FACTORY_BASE, + **noise, + "lib/factory.py": _WRAPPER, + "wide/__init__.py": "", + "wide/mods/__init__.py": "", + **wide, + "wide/agent.py": "from google.adk.agents import Agent\n" + + "".join(f"import wide.mods.m{index}\n" for index in range(10)) + + "\nroot_agent = Agent(name='wide', model='m', tools=[])\n", + "server.py": "from lib.factory import CustomAgent\nfrom app.shared import shared_tool\n\n" + "root_agent = CustomAgent('bot', [shared_tool]).build()\n", + }, + ) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("files past the bound were not read" in limit for limit in result["scope_selection"]["limits"]) + + +@pytest.mark.parametrize( + ("module", "named"), + [ + ( + # Importing a module's agent is not building one. + "from app.agent import root_agent\nfrom app.shared import shared_tool # noqa: F401\n\n\n" + "def get_agent():\n return root_agent\n", + False, + ), + ( + # Copying it with the changed tool is. + "from app.agent import root_agent\nfrom app.shared import shared_tool\n\n" + "bot_agent = root_agent.clone(tools=[shared_tool])\n", + True, + ), + ], + ids=["uses-the-agent", "copies-the-agent"], +) +def test_a_module_using_an_agent_is_named_only_when_it_builds_one(repo, module, named): + base = commit(repo, {**_FACTORY_BASE, "web/__init__.py": "", "web/deps.py": module}) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert any("web/deps.py" in limit for limit in result["scope_selection"]["limits"]) is named, result[ + "scope_selection" + ] + + +# --------------------------------------------------------------------------- +# #875 review, round 7: the search walks through agent files — one may +# re-export the change, or hand on an agent a consumer copies — while a +# module that only runs what it imports builds nothing. + + +@pytest.mark.parametrize( + "server", + [ + "from lib.factory import CustomAgent\nfrom app.agent import shared_tool\n\nbot = CustomAgent('bot', [shared_tool]).build()\n", + "from lib.factory import CustomAgent\nfrom app.agent import root_agent\n\nbot = CustomAgent('bot', list(root_agent.tools)).build()\n", + "from app.agent import root_agent, shared_tool\n\nbot = root_agent.clone(update={'name': 'bot', 'tools': [shared_tool]})\n", + ], + ids=["re-exported-by-the-agent-file", "copies-the-agents-tools", "clones-the-agent"], +) +def test_a_consumer_reaching_the_change_through_an_agent_file_is_named(repo, server): + agent = _adk("app", "from .tools import lookup\nfrom .shared import shared_tool", "lookup, shared_tool") + base = commit(repo, {**_FACTORY_BASE, "app/agent.py": agent, "lib/factory.py": _WRAPPER, "server.py": server}) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("server.py" in limit for limit in _consumer_limits(result)), result["scope_selection"] + + +@pytest.mark.parametrize( + ("extra", "agent"), + [ + ( + {"web/__init__.py": "", "web/routes.py": "from app.tools import lookup\nfrom web.deps import get_agent\n\n\n" + "def index():\n return lookup('q'), get_agent().name\n", + "web/deps.py": "from app.agent import root_agent\n\n\ndef get_agent():\n return root_agent\n"}, + "from google.adk.agents import Agent\nfrom .tools import lookup\n\n\ndef create():\n" + " return Agent(name='app', model='m', tools=[lookup])\n\n\nroot_agent = create()\n", + ), + ( + {"main.py": "from app.run import main\n\nif __name__ == '__main__':\n main()\n", + "app/run.py": "from app.agent import build\n\n\ndef main():\n return build('bot')\n"}, + "from google.adk.agents import Agent\nfrom .tools import lookup\n\n\ndef build(name):\n" + " return Agent(name=name, model='m', tools=[lookup])\n", + ), + ], + ids=["import-time-factory", "entry-script"], +) +def test_a_module_that_only_runs_the_compared_agents_is_not_a_consumer(repo, extra, agent): + base = commit( + repo, + {"app/__init__.py": "", "app/agent.py": agent, "app/tools.py": _ADK_TOOL.replace("BODY", "q"), **extra}, + ) + head = commit(repo, {"app/tools.py": _ADK_TOOL.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert _consumer_limits(result) == [], result["scope_selection"] + + +# --------------------------------------------------------------------------- +# #875 review, round 8: module state is something to build with, and setting +# it is giving the builder something; a settings model's copy is no agent's. + +_STATE_FACTORY = ( + "from google.adk.agents import Agent\n\nTOOLS = []\n\n\ndef register(tool):\n TOOLS.append(tool)\n\n\n" + "def _build():\n return Agent(name='bot', model='m', tools=list(TOOLS))\n\n\ndef make():\n return _build()\n" +) + + +@pytest.mark.parametrize( + ("factory", "server"), + [ + (_STATE_FACTORY, "from lib.factory import make, register\nfrom app.shared import shared_tool\n\nregister(shared_tool)\nbot = make()\n"), + (_STATE_FACTORY, "import lib.factory as factory\nfrom app.shared import shared_tool\n\nfactory.TOOLS = [shared_tool]\nbot = factory.make()\n"), + ( + "from google.adk.agents import Agent\n\nTOOLS = []\n\n\ndef _build(tools):\n" + " return Agent(name='bot', model='m', tools=tools)\n\n\ndef make(tools=None):\n return _build(tools or list(TOOLS))\n", + "import lib.factory as factory\nfrom app.shared import shared_tool\n\nfactory.TOOLS = [shared_tool]\nbot = factory.make()\n", + ), + ], + ids=["registry", "module-state", "state-read-by-a-factory-taking-arguments"], +) +def test_a_consumer_giving_a_builder_its_module_state_is_named(repo, factory, server): + base = commit(repo, {**_FACTORY_BASE, "lib/factory.py": factory, "server.py": server}) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("server.py" in limit for limit in _consumer_limits(result)), result["scope_selection"] + + +def test_a_settings_models_copy_is_not_an_agents(repo): + files = { + "app/__init__.py": "", + "app/agent.py": _adk("app", "from .tools import lookup", "lookup"), + "app/tools.py": "PAGE_SIZE = 5\n\n\n" + _ADK_TOOL.replace("BODY", "q"), + "web/__init__.py": "", + "web/deps.py": "from app.agent import root_agent\n\n\ndef get_agent():\n return root_agent\n", + "web/settings.py": "from pydantic import BaseModel\n\n\nclass Settings(BaseModel):\n port: int = 8000\n\n\nBASE = Settings()\n", + "web/routes.py": "from app.tools import PAGE_SIZE\nfrom web.deps import get_agent\nfrom web.settings import BASE\n\n\n" + "def index(overrides):\n cfg = BASE.model_copy(update=overrides)\n return PAGE_SIZE, cfg, get_agent().name\n", + } + base = commit(repo, files) + head = commit(repo, {"app/tools.py": "PAGE_SIZE = 5\n\n\n" + _ADK_TOOL.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert _consumer_limits(result) == [], result["scope_selection"] + + +# --------------------------------------------------------------------------- +# #875 review, round 9: a builder builds on request when an agent's +# capabilities are not a fixed list of the module's own names, however the +# state that feeds them is spelled; a fixed one takes nothing from a caller. + +_DYNAMIC = "from google.adk.agents import Agent\nSTATE\n\n\ndef _build():\n return Agent(name='bot', model='m', tools=TOOLS)\n\n\ndef make():\n return _build()\n" + + +@pytest.mark.parametrize( + ("factory", "server", "extra"), + [ + ( + _DYNAMIC.replace("STATE", "\n\nclass Config:\n tool_list = ()\n\n\nCONFIG = Config()").replace("TOOLS", "list(CONFIG.tool_list)"), + "import lib.factory as factory\nfrom app.shared import shared_tool\n\nfactory.CONFIG.tool_list = (shared_tool,)\nbot = factory.make()\n", + {}, + ), + ( + _DYNAMIC.replace("STATE", "from lib.registry import TOOLS").replace("TOOLS)", "list(TOOLS))"), + "from lib.factory import make\nfrom lib.registry import TOOLS\nfrom app.shared import shared_tool\n\nTOOLS.append(shared_tool)\nbot = make()\n", + {"lib/registry.py": "TOOLS = []\n"}, + ), + ( + _DYNAMIC.replace("STATE", "\nTOOLS = []").replace("TOOLS)", "list(TOOLS))"), + "import lib.factory as factory\nfrom app.shared import shared_tool\n\nsetattr(factory, 'TOOLS', [shared_tool])\nbot = factory.make()\n", + {}, + ), + ], + ids=["config-object", "imported-state", "setattr"], +) +def test_a_builder_fed_through_module_state_however_spelled_is_named(repo, factory, server, extra): + base = commit(repo, {**_FACTORY_BASE, "lib/factory.py": factory, "server.py": server, **extra}) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("server.py" in limit for limit in _consumer_limits(result)), result["scope_selection"] + + +@pytest.mark.parametrize( + "route", + [ + "from app.tools import PAGE_SIZE\nfrom app.agent import get_agent, render\n\n\ndef index(x):\n return PAGE_SIZE, render(x), get_agent().name\n", + "from app.tools import PAGE_SIZE\nfrom app import settings\nfrom app.agent import get_agent\n\nsettings.DEBUG = True\n\n\n" + "def index():\n return PAGE_SIZE, get_agent().name\n", + ], + ids=["cached-builder", "settings-assignment"], +) +def test_a_builder_with_fixed_tools_takes_nothing_from_a_caller(repo, route): + agent = ( + "from google.adk.agents import Agent\nfrom .tools import lookup\n\n_CACHE = {}\n\n\ndef get_agent():\n" + " if 'agent' not in _CACHE:\n _CACHE['agent'] = Agent(name='app', model='m', tools=[lookup])\n" + " return _CACHE['agent']\n\n\ndef render(x):\n return str(x)\n" + ) + files = { + "app/__init__.py": "", + "app/agent.py": agent, + "app/settings.py": "DEBUG = False\n", + "app/tools.py": "PAGE_SIZE = 5\n\n\n" + _ADK_TOOL.replace("BODY", "q"), + "web/__init__.py": "", + "web/routes.py": route, + } + base = commit(repo, files) + head = commit(repo, {"app/tools.py": "PAGE_SIZE = 5\n\n\n" + _ADK_TOOL.replace("BODY", "q.upper()")}) + result = run(repo, base, head) + assert _consumer_limits(result) == [], result["scope_selection"] + + +# --------------------------------------------------------------------------- +# #875 review, round 10: a builder that gives its agent the caller's +# capabilities after building it — a rewire or a copy — builds on request. + + +@pytest.mark.parametrize( + "factory", + [ + "from google.adk.agents import Agent\n\n\ndef _build(extra):\n agent = Agent(name='bot', model='m', tools=[])\n" + " agent.tools.append(extra)\n return agent\n\n\ndef make(extra):\n return _build(extra)\n", + "from google.adk.agents import Agent\n\n\ndef _build(extra):\n agent = Agent(name='bot', model='m')\n" + " agent.tools = [extra]\n return agent\n\n\ndef make(extra):\n return _build(extra)\n", + "from google.adk.agents import Agent\n\nBASE = Agent(name='base', model='m', tools=[])\n\n\n" + "def _build(tools):\n return BASE.clone(update={'name': 'bot', 'tools': tools})\n\n\ndef make(extra):\n return _build([extra])\n", + ], + ids=["appended", "assigned", "cloned"], +) +def test_a_builder_giving_its_agent_the_callers_capabilities_later_is_on_request(repo, factory): + server = "from lib.factory import make\nfrom app.shared import shared_tool\n\nbot = make(shared_tool)\n" + base = commit(repo, {**_FACTORY_BASE, "lib/factory.py": factory, "server.py": server}) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("server.py" in limit for limit in _consumer_limits(result)), result["scope_selection"] diff --git a/tests/test_distribution_surface_parity.py b/tests/test_distribution_surface_parity.py index 6a327ec4a..0e03573ba 100644 --- a/tests/test_distribution_surface_parity.py +++ b/tests/test_distribution_surface_parity.py @@ -173,7 +173,7 @@ def paths(self) -> list[Path]: ), Surface( "application_diff", - ("src/agents_shipgate/cli/application_diff.py",), + ("src/agents_shipgate/cli/application_diff.py", "src/agents_shipgate/cli/application_scope.py"), # Advisory source-wiring comparison, not host drift or an engine verdict. # No release permission, declared authority, pin or root reachability claim. # Its per-side `excluded_tests` list (#876) names test files the diff --git a/tests/test_imported_tool_review.py b/tests/test_imported_tool_review.py index fd983fb4a..cc42d90ad 100644 --- a/tests/test_imported_tool_review.py +++ b/tests/test_imported_tool_review.py @@ -756,7 +756,7 @@ def test_a_self_wrapping_function_tool_is_read_without_a_guess(repo): ) base = commit(repo, {"agent.py": source.replace("BODY", "q"), "notes.md": "a\n"}) unchanged = commit(repo, {"notes.md": "b\n"}) - result = run(repo, base, unchanged) + result = run(repo, base, unchanged, "--scope", ".") assert result["comparison_status"] == "compared" assert result["rows"] == [] head = commit(repo, {"agent.py": source.replace("BODY", "q.upper()")}) From 6376398deb7bc748f5883c8e914555d71acad795 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Mon, 28 Sep 2026 15:21:26 -0700 Subject: [PATCH 2/4] Count a capability change the way the SDK reader counts it (#875 review 11) The derived scope decided whether a module rewires an agent with its own predicate, which missed spellings the SDK reader already counts: a slice store into an agent's tools list, and a change made through a handle to that list. It now asks #880's `_capability_changes`, so the reader and the scope derivation agree on what changes an agent. On the 48-case corpus, one extra changed file in alliance-genome #842 and #860 is named as unrelated (`prompt_builder.py` reads `agent.tools` into a local); nothing else moves. Of 156 fixtures, the four this round added move to partial. Co-Authored-By: Claude Opus 5.5 --- src/agents_shipgate/cli/application_scope.py | 74 ++++++++------------ tests/test_application_scope.py | 37 ++++++++++ 2 files changed, 68 insertions(+), 43 deletions(-) diff --git a/src/agents_shipgate/cli/application_scope.py b/src/agents_shipgate/cli/application_scope.py index 61f67a55c..269636555 100644 --- a/src/agents_shipgate/cli/application_scope.py +++ b/src/agents_shipgate/cli/application_scope.py @@ -24,6 +24,7 @@ from agents_shipgate.cli.discovery.artifacts import _skip_part from agents_shipgate.cli.discovery.signals import _is_test_path, application_frameworks from agents_shipgate.cli.verify.git import _blob_contents, _run_git_bounded_output, _TreeBlob +from agents_shipgate.inputs.openai_sdk_static import _capability_changes SUPPORTED = frozenset({"openai_agents_sdk", "google_adk"}) #: Import hops a change is related to an agent file through. @@ -752,35 +753,24 @@ def _dotted(node: ast.AST) -> str | None: return node.id if isinstance(node, ast.Name) else None -def _rewires(node: ast.AST) -> bool: - """``x.tools = ...``, ``x.tools += ...``, ``x.tools.append(...)``, - ``setattr(x, "tools", ...)``.""" +def _changes_capabilities(tree: ast.Module) -> list[ast.AST]: + """Where a module changes an object's capability list, as the SDK reader's + census reads it — an attribute, item or slice store, a delete, a list + method, ``setattr``/``delattr`` by name, a handle changed later — so scope + derivation and the reader answer one question one way (#875 review).""" - if isinstance(node, ast.Assign | ast.AugAssign | ast.AnnAssign): - targets = node.targets if isinstance(node, ast.Assign) else [node.target] - return any(isinstance(item, ast.Attribute) and item.attr in _CAPABILITIES for item in targets) - if isinstance(node, ast.Call): - func = node.func - if ( - isinstance(func, ast.Attribute) - and func.attr in {"append", "extend", "insert", "remove", "pop", "clear"} - and isinstance(func.value, ast.Attribute) - and func.value.attr in _CAPABILITIES - ): - return True - return ( - isinstance(func, ast.Name) - and func.id in {"setattr", "delattr"} - and len(node.args) >= 2 - and isinstance(node.args[1], ast.Constant) - and node.args[1].value in _CAPABILITIES - ) - return False + try: + return [site for _, site in _capability_changes(tree, frozenset(_CAPABILITIES))] + except (RecursionError, ValueError): + return [tree] def _rewired_values(node: ast.AST) -> list[ast.expr]: - """What a rewire (``_rewires``) gives the capability; a removal gives nothing.""" + """What a capability change gives the capability; a removal gives nothing. + Anything else the reader counts as a change is its own value.""" + if isinstance(node, ast.Delete): + return [] if isinstance(node, ast.Assign | ast.AugAssign | ast.AnnAssign): return [node.value] if node.value is not None else [] if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute): @@ -788,9 +778,9 @@ def _rewired_values(node: ast.AST) -> list[ast.expr]: return list(node.args[-1:]) + [item.value for item in node.keywords] if node.func.attr in {"remove", "pop", "clear"}: return [] - if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "setattr": + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id in {"setattr", "delattr"}: return list(node.args[2:3]) - return [] + return [node] if isinstance(node, ast.expr) else [] def _copied_values(call: ast.Call) -> list[ast.expr]: @@ -822,7 +812,7 @@ def _rewires_agent(text: str) -> bool: tree = ast.parse(text) except (SyntaxError, ValueError, RecursionError): return False - return any(_rewires(node) for node in ast.walk(tree)) + return bool(_changes_capabilities(tree)) def _builds_agent(text: str, *, on_request: bool = False) -> bool: @@ -928,26 +918,24 @@ def fixed(value: ast.expr) -> bool: item.arg is None or (item.arg in _CAPABILITIES and not fixed(item.value)) for item in node.keywords ): return True - # Or given afterwards: a rewire or a copy whose capabilities are - # not the module's own (``agent.tools.append(extra)``, - # ``BASE.clone(tools=tools)``) (#875 review). - if _rewires(node) and not all(fixed(value) or own(value) for value in _rewired_values(node)): - return True + # Or given afterwards: a copy whose capabilities are not the + # module's own (``BASE.clone(tools=tools)``) (#875 review). if isinstance(node, ast.Call) and copies(node) and not all( fixed(value) or own(value) for value in _copied_values(node) ): return True - return False - roots: list[ast.AST] = [tree] - for root in roots: - for node in ast.walk(root): - if isinstance(node, ast.Call) and (named(node.func) or copies(node)): - return True - if _rewires(node) and not on_request: - return True - if isinstance(node, ast.ClassDef) and any(named(base) for base in node.bases): - return True - return False + # Or a capability change, as the reader counts one, with a value that + # is not the module's own (``agent.tools[:] = [extra]``). + return any( + not all(fixed(value) or own(value) for value in _rewired_values(site)) + for site in _changes_capabilities(tree) + ) + for node in ast.walk(tree): + if isinstance(node, ast.Call) and (named(node.func) or copies(node)): + return True + if isinstance(node, ast.ClassDef) and any(named(base) for base in node.bases): + return True + return bool(_changes_capabilities(tree)) def _common_directory(paths: set[str]) -> str: diff --git a/tests/test_application_scope.py b/tests/test_application_scope.py index a3646cd9d..b8dcd8269 100644 --- a/tests/test_application_scope.py +++ b/tests/test_application_scope.py @@ -1119,3 +1119,40 @@ def test_a_builder_giving_its_agent_the_callers_capabilities_later_is_on_request result = run(repo, base, head) assert result["comparison_status"] == "partial", result["scope_selection"] assert any("server.py" in limit for limit in _consumer_limits(result)), result["scope_selection"] + + +# --------------------------------------------------------------------------- +# #875 review, round 11: a capability change is what the reader counts as one +# — a slice store, a handle changed later — in the builder or its consumer. + +_FIXED_FACTORY = ( + "from google.adk.agents import Agent\n\n\ndef _build():\n return Agent(name='bot', model='m', tools=[])\n\n\n" + "def make():\n return _build()\n" +) + + +@pytest.mark.parametrize( + ("factory", "server"), + [ + ( + "from google.adk.agents import Agent\n\n\ndef _build(extra):\n agent = Agent(name='bot', model='m', tools=[])\n" + " agent.tools[:] = [extra]\n return agent\n\n\ndef make(extra):\n return _build(extra)\n", + "from lib.factory import make\nfrom app.shared import shared_tool\n\nbot = make(shared_tool)\n", + ), + ( + _FIXED_FACTORY, + "from lib.factory import make\nfrom app.shared import shared_tool\n\nbot = make()\nbot.tools[:] = [shared_tool]\n", + ), + ( + _FIXED_FACTORY, + "from lib.factory import make\nfrom app.shared import shared_tool\n\nbot = make()\nhandle = bot.tools\nhandle.append(shared_tool)\n", + ), + ], + ids=["slice-store-in-the-builder", "slice-store-by-the-consumer", "handle-changed-by-the-consumer"], +) +def test_a_capability_change_the_reader_counts_counts_for_the_scope(repo, factory, server): + base = commit(repo, {**_FACTORY_BASE, "lib/factory.py": factory, "server.py": server}) + head = commit(repo, _SHARED_HEAD) + result = run(repo, base, head) + assert result["comparison_status"] == "partial", result["scope_selection"] + assert any("server.py" in limit for limit in result["scope_selection"]["limits"]), result["scope_selection"] From caca73d1ee12d76ab153a067b0766186f1cf28b8 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Mon, 28 Sep 2026 16:54:29 -0700 Subject: [PATCH 3/4] ci: balance test shards by measured time per file The suite's three CI shards were balanced by collected item count. Item count is a poor proxy for time: a file of forty git-fixture tests costs more than a file of four hundred pure ones. - On `main`, shard 3 took 13 of its 15 minutes while shards 1 and 2 took 8. - Any new test file reshuffled most files. #904's one new file moved 298 of 363, which put shard 1 at 14 minutes and cancelled shard 3 at the cap. Shards are now balanced by measured seconds per file, in `tests/shard_seconds.json`. `scripts/measure_shard_seconds.py` writes that file from a `--junitxml` run. A file without a measurement costs its item count at the measured seconds per item. A stale measurement only unbalances, never drops a file. The union property, determinism and fail-loud rules are unchanged. On one full-suite measurement: - balancing by count gives main 25/30/45% of the work, matching CI's 6.8/6.8/11.6-minute shards; - balancing by time gives 33/33/33%, for main and for #904. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 7 +- ci_sharding.py | 70 +++++- conftest.py | 4 +- scripts/measure_shard_seconds.py | 78 +++++++ tests/shard_seconds.json | 368 +++++++++++++++++++++++++++++++ tests/test_shard_partition.py | 54 ++++- 6 files changed, 567 insertions(+), 14 deletions(-) create mode 100644 scripts/measure_shard_seconds.py create mode 100644 tests/shard_seconds.json diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f7149fd11..d038425e5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,7 +22,12 @@ jobs: # (add a shard). # # `conftest.py` assigns whole test *files* to shards, deterministically, - # from the collection alone — nothing is exchanged between these jobs. + # from the collection and the measured seconds per file in + # `tests/shard_seconds.json` — nothing is exchanged between these jobs. + # Balancing item counts instead left one shard at 13 of its 15 minutes on + # `main` while another took 8, and any new test file reshuffled the rest + # (#904). When a shard nears the cap, re-measure with + # `scripts/measure_shard_seconds.py` before adding a shard. # `tests/test_shard_partition.py` asserts the union of the shards is the # suite, because a partition that silently drops a file leaves every job # green while a test stops running. diff --git a/ci_sharding.py b/ci_sharding.py index 1f8a7360c..a582f7522 100644 --- a/ci_sharding.py +++ b/ci_sharding.py @@ -8,17 +8,50 @@ from anywhere and has one definition. Pure. It reads nothing, writes nothing, and takes no environment: the caller -supplies the collection and gets back an assignment. That is what lets -``tests/test_shard_partition.py`` assert the properties directly. +supplies the collection (and the measured seconds, when it has them) and gets +back an assignment. That is what lets ``tests/test_shard_partition.py`` assert +the properties directly. """ from __future__ import annotations +import json +import math from collections.abc import Mapping +from pathlib import Path +#: Measured seconds per test file, written by +#: ``scripts/measure_shard_seconds.py`` from a full ``--junitxml`` run. +SECONDS_FILE = Path(__file__).resolve().parent / "tests" / "shard_seconds.json" -def shard_assignment(paths: Mapping[str, int], shards: int) -> dict[str, int]: - """Assign whole test *files* to shards, balancing collected item counts. + +def load_seconds(path: Path = SECONDS_FILE) -> dict[str, float]: + """The measured seconds per file, or nothing when there is no measurement. + + A missing or unreadable file balances on item counts alone, as before: + the measurement only ever improves the balance, never the coverage. + """ + + try: + payload = json.loads(path.read_text(encoding="utf-8")) + except (OSError, ValueError): + return {} + files = payload.get("files") if isinstance(payload, dict) else None + if not isinstance(files, dict): + return {} + return { + str(name): float(value) + for name, value in files.items() + if isinstance(value, int | float) and not isinstance(value, bool) and value >= 0 + } + + +def shard_assignment( + paths: Mapping[str, int], + shards: int, + seconds: Mapping[str, float] | None = None, +) -> dict[str, int]: + """Assign whole test *files* to shards, balancing their expected time. Two properties, and both are load-bearing. @@ -32,15 +65,32 @@ def shard_assignment(paths: Mapping[str, int], shards: int) -> dict[str, int]: is exchanged between jobs, so the union of the shards is exactly the suite and no test can fall between two of them — which a test asserts. - Item count is a proxy for time, and an imperfect one; it is used because it - is free and needs no stored measurements to go stale. Balance is checked in - ``tests/test_shard_partition.py`` rather than assumed. + **Measured time where there is one.** Item count alone was a poor proxy: + a file of forty git-fixture tests costs more than a file of four hundred + pure ones. Balancing counts left one shard at 13 of its 15 minutes on + ``main``, while another took 8. Adding any test file reshuffled most files + between shards, so an unrelated PR could tip a shard past its cap (#904). + ``seconds`` holds each file's measured time. A file without a measurement + (new, or renamed since) costs its item count times the measured seconds + per item. A stale measurement only unbalances; it never drops a file. + Balance is checked in ``tests/test_shard_partition.py`` rather than + assumed. """ - load = [0] * shards + cost = _costs(paths, seconds or {}) + load = [0.0] * shards owner: dict[str, int] = {} - for path, count in sorted(paths.items(), key=lambda item: (-item[1], item[0])): + for path in sorted(paths, key=lambda item: (-cost[item], item)): target = min(range(shards), key=lambda index: (load[index], index)) owner[path] = target - load[target] += count + load[target] += cost[path] return owner + + +def _costs(paths: Mapping[str, int], seconds: Mapping[str, float]) -> dict[str, float]: + known = {path: seconds[path] for path in sorted(paths) if path in seconds} + known_items = sum(paths[path] for path in known) + # ``fsum`` over a sorted order: every shard computes the same rate, bit + # for bit, whatever order its collection listed the files in. + rate = math.fsum(known.values()) / known_items if known_items else 1.0 + return {path: known.get(path, paths[path] * rate) for path in paths} diff --git a/conftest.py b/conftest.py index 7076c36e9..9b9132b8e 100644 --- a/conftest.py +++ b/conftest.py @@ -27,7 +27,7 @@ import pytest # noqa: E402 -from ci_sharding import shard_assignment # noqa: E402 +from ci_sharding import load_seconds, shard_assignment # noqa: E402 @pytest.fixture(autouse=True) @@ -90,7 +90,7 @@ def pytest_collection_modifyitems(config, items) -> None: # noqa: ANN001 counts: dict[str, int] = {} for item in items: counts[item.location[0]] = counts.get(item.location[0], 0) + 1 - owner = shard_assignment(counts, shards) + owner = shard_assignment(counts, shards, load_seconds()) keep = [item for item in items if owner[item.location[0]] == index - 1] dropped = [item for item in items if owner[item.location[0]] != index - 1] if not keep: diff --git a/scripts/measure_shard_seconds.py b/scripts/measure_shard_seconds.py new file mode 100644 index 000000000..7eccd2fca --- /dev/null +++ b/scripts/measure_shard_seconds.py @@ -0,0 +1,78 @@ +"""Write ``tests/shard_seconds.json`` from a full-suite ``--junitxml`` report. + +The CI suite is split into shards by measured time per test file +(``ci_sharding.py``). Re-measure when a shard nears its ``timeout-minutes``: + + python -m pytest -n auto -m "not perf" --ignore=tests/test_adapter_static_only.py \\ + --junitxml=junit.xml + python scripts/measure_shard_seconds.py junit.xml + +The times are relative weights: what matters is how the files compare with +each other, so one machine's measurement balances another's runners. +""" + +from __future__ import annotations + +import argparse +import json +import subprocess +import sys +import xml.etree.ElementTree as ET +from collections import defaultdict +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +OUTPUT = REPO_ROOT / "tests" / "shard_seconds.json" + + +def file_seconds(report: Path, root: Path = REPO_ROOT) -> dict[str, float]: + """Seconds per test file: each test case's setup, call and teardown summed.""" + + totals: dict[str, float] = defaultdict(float) + for case in ET.parse(report).getroot().iter("testcase"): + name = case.get("file") or _file_from_classname(case.get("classname", ""), root) + if name: + totals[name] += float(case.get("time") or 0.0) + return {name: round(value, 1) for name, value in sorted(totals.items())} + + +def _file_from_classname(classname: str, root: Path) -> str | None: + """``tests.test_x.TestY`` → ``tests/test_x.py``, the file the case is in.""" + + parts = classname.split(".") + for end in range(len(parts), 0, -1): + candidate = root.joinpath(*parts[:end]).with_suffix(".py") + if candidate.is_file(): + return candidate.relative_to(root).as_posix() + return None + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("report", type=Path, help="a --junitxml report of the full CI suite") + parser.add_argument( + "--root", + type=Path, + default=REPO_ROOT, + help="the checkout the report was made in (default: this one)", + ) + args = parser.parse_args(argv) + files = file_seconds(args.report, args.root.resolve()) + if not files: + print(f"{args.report} holds no test cases", file=sys.stderr) + return 1 + commit = subprocess.run( + ["git", "rev-parse", "--short=12", "HEAD"], + cwd=args.root, + capture_output=True, + text=True, + check=False, + ).stdout.strip() + payload = {"measured_at": commit or None, "files": files} + OUTPUT.write_text(json.dumps(payload, indent=1, sort_keys=True) + "\n", encoding="utf-8") + print(f"wrote {len(files)} files, {sum(files.values()):.0f}s in total, to {OUTPUT}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/shard_seconds.json b/tests/shard_seconds.json new file mode 100644 index 000000000..05c4bbd95 --- /dev/null +++ b/tests/shard_seconds.json @@ -0,0 +1,368 @@ +{ + "files": { + "tests/harness/test_adversarial_guards.py": 0.0, + "tests/harness/test_claude_code_driver.py": 0.0, + "tests/harness/test_codex_driver.py": 1.2, + "tests/harness/test_cursor_driver.py": 0.0, + "tests/harness/test_cursor_manual_driver.py": 0.0, + "tests/harness/test_detectors.py": 0.2, + "tests/harness/test_exit_criteria.py": 0.0, + "tests/harness/test_harness_layout.py": 2.0, + "tests/harness/test_infrastructure_failures.py": 0.1, + "tests/harness/test_overlay_renderer.py": 0.0, + "tests/harness/test_pressure_ground_truth.py": 6.7, + "tests/harness/test_redaction.py": 0.0, + "tests/harness/test_run_preflight.py": 0.9, + "tests/harness/test_smoke.py": 0.6, + "tests/integration/github_action/test_agent_result.py": 0.0, + "tests/test_absent_input_messages.py": 3.6, + "tests/test_action_comment_fallback.py": 0.7, + "tests/test_action_engine_install.py": 6.8, + "tests/test_action_metadata.py": 0.4, + "tests/test_action_scope_domain.py": 0.0, + "tests/test_action_scope_projection.py": 3.0, + "tests/test_action_surface_diff.py": 0.9, + "tests/test_adapter_contracts.py": 0.2, + "tests/test_adapter_entry_point_discovery.py": 4.2, + "tests/test_adapter_registry.py": 0.1, + "tests/test_adk_remote_bindings.py": 1.2, + "tests/test_adopter_pins_resolve.py": 0.2, + "tests/test_adopter_vocabulary.py": 9.6, + "tests/test_adopters_registry.py": 0.5, + "tests/test_adoption_ladder.py": 0.8, + "tests/test_adoption_scorer_input_recovery.py": 0.0, + "tests/test_adoption_walk.py": 71.1, + "tests/test_advisory_cadence.py": 0.6, + "tests/test_agent_action_summary.py": 5.2, + "tests/test_agent_bindings.py": 0.0, + "tests/test_agent_boundary.py": 32.3, + "tests/test_agent_control_contract.py": 0.2, + "tests/test_agent_control_envelope.py": 91.2, + "tests/test_agent_control_envelope_rows.py": 34.9, + "tests/test_agent_control_reports_dir.py": 102.8, + "tests/test_agent_controls.py": 0.0, + "tests/test_agent_handoff.py": 0.0, + "tests/test_agent_identity_origin.py": 1.1, + "tests/test_agent_instructions_apply.py": 0.2, + "tests/test_agent_instructions_renderers.py": 0.0, + "tests/test_agent_mode.py": 18.9, + "tests/test_agent_name_recovery.py": 7.7, + "tests/test_agent_protocol.py": 0.2, + "tests/test_anthropic_api.py": 0.5, + "tests/test_application_diff.py": 57.6, + "tests/test_application_diff_identity.py": 23.0, + "tests/test_application_diff_reach.py": 64.5, + "tests/test_application_diff_review.py": 40.9, + "tests/test_application_diff_unobserved.py": 230.7, + "tests/test_application_scope.py": 190.2, + "tests/test_apply_patches.py": 3.5, + "tests/test_attest.py": 0.2, + "tests/test_authorization_cli.py": 1.8, + "tests/test_authorization_execution.py": 2.1, + "tests/test_authorization_verifier_scenarios.py": 0.0, + "tests/test_authorization_verify_integration.py": 96.8, + "tests/test_base_cache_engine_identity.py": 189.8, + "tests/test_base_cache_namespace.py": 177.5, + "tests/test_baseline_integrity.py": 2.9, + "tests/test_baseline_status.py": 0.0, + "tests/test_benchmark_results_privacy.py": 0.0, + "tests/test_beta_strata_inventory.py": 2.6, + "tests/test_bootstrap.py": 43.0, + "tests/test_boundary_diff_hunks.py": 2.7, + "tests/test_boundary_diff_paths.py": 21.1, + "tests/test_capability_change_schema_hash_parity.py": 0.1, + "tests/test_capability_delta.py": 0.0, + "tests/test_capability_delta_attestation.py": 13.9, + "tests/test_capability_diff.py": 39.7, + "tests/test_capability_diff_partial_clone.py": 50.7, + "tests/test_capability_domain.py": 0.0, + "tests/test_capability_lattice.py": 0.0, + "tests/test_capability_lock.py": 0.3, + "tests/test_capability_payload.py": 8.4, + "tests/test_capability_trace_evidence.py": 0.0, + "tests/test_check_default_comparison.py": 37.7, + "tests/test_check_unmodelled_host_config_keys.py": 232.8, + "tests/test_ci.py": 0.4, + "tests/test_ci_recipes.py": 0.3, + "tests/test_claude_code_plugin_package.py": 0.0, + "tests/test_claude_hook_loading_evidence.py": 80.8, + "tests/test_claude_hooks_source.py": 0.9, + "tests/test_claude_known_marketplaces.py": 4.7, + "tests/test_claude_permission_shapes.py": 0.1, + "tests/test_cli.py": 5.7, + "tests/test_cli_on_demand_loading.py": 2.5, + "tests/test_codex_boundary_check.py": 0.9, + "tests/test_codex_plugin.py": 0.7, + "tests/test_codex_plugin_default_identity.py": 158.5, + "tests/test_codex_plugin_input_identity.py": 96.8, + "tests/test_codex_plugin_launch_package.py": 0.0, + "tests/test_cold_reader_order.py": 18.2, + "tests/test_cold_start_replay.py": 40.8, + "tests/test_conductor.py": 0.8, + "tests/test_config.py": 0.8, + "tests/test_control_packs.py": 11.1, + "tests/test_coverage_recovery.py": 16.7, + "tests/test_crewai.py": 0.1, + "tests/test_cross_block_consistency.py": 7.4, + "tests/test_current_control.py": 138.1, + "tests/test_current_control_auxiliary_inputs.py": 91.1, + "tests/test_current_control_closure.py": 75.0, + "tests/test_current_control_directory_currency.py": 92.5, + "tests/test_current_control_input_currency.py": 31.5, + "tests/test_current_control_input_origins.py": 74.6, + "tests/test_cursor_rule_globs.py": 0.0, + "tests/test_declaration_authoring.py": 0.4, + "tests/test_declaration_confirmation_route.py": 193.8, + "tests/test_declaration_drift.py": 0.1, + "tests/test_declaration_monotonicity.py": 0.3, + "tests/test_declaration_questionnaire.py": 1.7, + "tests/test_declaration_review.py": 4.6, + "tests/test_declaration_scaffold.py": 0.3, + "tests/test_declared_manifest_input_identity.py": 44.6, + "tests/test_design_partner_pilot.py": 6.5, + "tests/test_detect.py": 6.8, + "tests/test_determinism_boundary.py": 0.1, + "tests/test_diagnostics.py": 0.0, + "tests/test_diff_input_status.py": 24.9, + "tests/test_directory_reader_identity.py": 1.1, + "tests/test_discovery_scope.py": 251.9, + "tests/test_distribution_surface_parity.py": 2.1, + "tests/test_docs_links.py": 0.1, + "tests/test_documentation_checks.py": 0.0, + "tests/test_documented_claude_frontmatter.py": 0.0, + "tests/test_e3_prime_compat.py": 0.0, + "tests/test_effect_coverage.py": 0.9, + "tests/test_effect_provenance_presentation.py": 6.2, + "tests/test_enabled_plugin_hook_routing.py": 116.7, + "tests/test_environment.py": 6.8, + "tests/test_evidence_backed_pass.py": 1.4, + "tests/test_evidence_gap_ranking.py": 0.4, + "tests/test_evidence_packet.py": 19.6, + "tests/test_exec_equivalent_permissions.py": 22.1, + "tests/test_explain_finding.py": 3.9, + "tests/test_fastmcp_injection_contract.py": 0.0, + "tests/test_feedback.py": 0.0, + "tests/test_finding_attribution.py": 33.9, + "tests/test_finding_comparison_evidence.py": 0.1, + "tests/test_finding_remediation.py": 1.7, + "tests/test_findings.py": 0.0, + "tests/test_fingerprint_compatibility.py": 0.3, + "tests/test_first_adoption.py": 56.2, + "tests/test_first_look.py": 11.1, + "tests/test_fix_task_contract.py": 0.0, + "tests/test_fixture.py": 22.7, + "tests/test_fixture_no_import.py": 3.7, + "tests/test_framework_common.py": 0.0, + "tests/test_github_action_annotations.py": 0.0, + "tests/test_github_action_outputs.py": 0.0, + "tests/test_github_check_run.py": 0.0, + "tests/test_go_tool_descriptions.py": 0.1, + "tests/test_google_adk.py": 4.3, + "tests/test_governance_benchmark.py": 52.2, + "tests/test_governance_benchmark_baseline.py": 39.7, + "tests/test_guard_dependency_verification.py": 80.3, + "tests/test_headline_ranking.py": 11.5, + "tests/test_heuristics.py": 0.0, + "tests/test_hook_benign_session.py": 34.1, + "tests/test_hook_mcp_detail_fields.py": 231.0, + "tests/test_hook_script_archive.py": 3.3, + "tests/test_hook_script_capture.py": 34.6, + "tests/test_hook_script_comparison_limits.py": 66.0, + "tests/test_hook_script_currency.py": 28.1, + "tests/test_hook_script_reference.py": 0.0, + "tests/test_hook_script_routing.py": 15.4, + "tests/test_host_audit.py": 0.8, + "tests/test_host_boundary_check.py": 0.4, + "tests/test_host_boundary_unread_surfaces.py": 9.5, + "tests/test_host_change_route_parity.py": 111.8, + "tests/test_host_comparison_coverage.py": 319.0, + "tests/test_host_config_oracle_controls.py": 3.4, + "tests/test_host_config_replay.py": 54.6, + "tests/test_host_diff_entry_docs.py": 18.8, + "tests/test_host_diff_permission_direction.py": 193.1, + "tests/test_host_diff_review_changes.py": 163.4, + "tests/test_host_directory_inputs.py": 3.1, + "tests/test_host_discovery.py": 7.6, + "tests/test_host_file_links.py": 1.8, + "tests/test_host_grant_direction.py": 125.4, + "tests/test_host_input_recovery.py": 0.2, + "tests/test_host_inventory_stability.py": 0.0, + "tests/test_host_link_read_through.py": 2.0, + "tests/test_host_local_precedence.py": 0.1, + "tests/test_host_only_advisory_recipe.py": 67.6, + "tests/test_host_path_privacy.py": 5.7, + "tests/test_host_settings_narrowing_review.py": 0.4, + "tests/test_human_authorization.py": 0.1, + "tests/test_human_authorization_signature_vector.py": 0.0, + "tests/test_human_review_decision.py": 110.3, + "tests/test_human_review_presentation.py": 7.4, + "tests/test_human_review_request.py": 14.9, + "tests/test_imported_tool_bindings.py": 8.8, + "tests/test_imported_tool_review.py": 405.3, + "tests/test_init_agent_instructions.py": 9.3, + "tests/test_init_auto.py": 15.9, + "tests/test_init_ci.py": 4.4, + "tests/test_init_claude_code.py": 4.7, + "tests/test_init_gitignore.py": 3.9, + "tests/test_init_scaffold_disclosure.py": 23.7, + "tests/test_inline_hook_allow.py": 29.2, + "tests/test_inputs.py": 0.2, + "tests/test_install_hooks.py": 81.1, + "tests/test_instruction_structure.py": 0.1, + "tests/test_instruction_structure_contracts.py": 4.3, + "tests/test_instruction_structure_hooks.py": 19.2, + "tests/test_instruction_structure_workflow.py": 35.1, + "tests/test_invocation_policy.py": 92.6, + "tests/test_labeling_guide_is_rater_safe.py": 0.0, + "tests/test_langchain.py": 0.1, + "tests/test_large_sample.py": 1.1, + "tests/test_linked_unchanged_limits.py": 182.2, + "tests/test_live_workspace_cause.py": 67.9, + "tests/test_local_contract.py": 0.0, + "tests/test_local_review.py": 23.2, + "tests/test_managed_block.py": 0.0, + "tests/test_manifest_consistency.py": 0.1, + "tests/test_manifest_free_pr_rows.py": 121.4, + "tests/test_manifest_schema_parity.py": 0.0, + "tests/test_manifest_scope.py": 0.0, + "tests/test_mcp_audit.py": 0.1, + "tests/test_mcp_idioms.py": 0.0, + "tests/test_mcp_launch_source.py": 39.2, + "tests/test_mcp_manifest.py": 0.2, + "tests/test_mcp_permissions.py": 0.2, + "tests/test_mcp_server.py": 0.1, + "tests/test_mcp_server_findings_table.py": 0.0, + "tests/test_mcp_server_source.py": 1.3, + "tests/test_mcp_source_annotations.py": 0.4, + "tests/test_mcp_url_capability_digest.py": 10.0, + "tests/test_metadata_loader.py": 0.1, + "tests/test_miner.py": 22.8, + "tests/test_miner_candidates.py": 1.3, + "tests/test_miner_constructed.py": 28.4, + "tests/test_miner_corpus.py": 0.1, + "tests/test_miner_labels.py": 0.0, + "tests/test_miner_reevaluate.py": 11.4, + "tests/test_miner_scope_inputs.py": 9.8, + "tests/test_n8n.py": 2.0, + "tests/test_next_action_chains_terminate.py": 50.3, + "tests/test_no_heuristics.py": 6.5, + "tests/test_nonregular_evidence_readers.py": 5.2, + "tests/test_openai_api.py": 0.4, + "tests/test_openapi_fuzz.py": 0.1, + "tests/test_openapi_operation_attribution.py": 2.0, + "tests/test_operation_attribution_verification.py": 21.4, + "tests/test_org_governance.py": 0.1, + "tests/test_out_path_resolution.py": 52.2, + "tests/test_output_directory_content.py": 382.7, + "tests/test_p0_binding_canaries.py": 0.1, + "tests/test_p0_policy_evidence_canaries.py": 0.1, + "tests/test_p0_safety_canaries.py": 2.6, + "tests/test_p0_verification_identity_canaries.py": 0.2, + "tests/test_packaging.py": 14.6, + "tests/test_partial_host_comparison.py": 83.3, + "tests/test_patch_generators.py": 1.1, + "tests/test_patches_model.py": 0.3, + "tests/test_permission_lattice.py": 1.1, + "tests/test_permission_residual.py": 8.2, + "tests/test_permission_review_guidance.py": 14.3, + "tests/test_plugin_validation.py": 4.5, + "tests/test_plugins.py": 0.6, + "tests/test_policy_evidence_architecture.py": 0.0, + "tests/test_policy_packs.py": 1.1, + "tests/test_policy_reason_code_split.py": 15.5, + "tests/test_preflight.py": 1.6, + "tests/test_preview_control_currency.py": 133.0, + "tests/test_privacy.py": 0.1, + "tests/test_product_hardening_gap_closure.py": 0.0, + "tests/test_prompt_disabling_settings.py": 217.4, + "tests/test_prompt_parity.py": 0.0, + "tests/test_property_loaders.py": 0.8, + "tests/test_provenance_kind.py": 2.7, + "tests/test_public_surface_contract.py": 2.9, + "tests/test_python_import_resolution.py": 0.3, + "tests/test_qualification_coverage_misses.py": 1.3, + "tests/test_rater_harness.py": 79.7, + "tests/test_reader_vocabulary.py": 21.7, + "tests/test_regenerate_goldens.py": 17.2, + "tests/test_registry.py": 0.0, + "tests/test_registry_ledger.py": 0.1, + "tests/test_release_advisory_pipeline.py": 3.4, + "tests/test_release_channel.py": 0.1, + "tests/test_release_channel_cadence.py": 4.9, + "tests/test_release_decision.py": 0.0, + "tests/test_release_engine_smoke.py": 0.3, + "tests/test_release_pipeline.py": 5.7, + "tests/test_release_source.py": 5.1, + "tests/test_remediation_metadata.py": 0.0, + "tests/test_report_1_0_compatibility_fixtures.py": 4.8, + "tests/test_report_1_0_contract.py": 0.0, + "tests/test_reports.py": 16.3, + "tests/test_required_source_availability.py": 5.7, + "tests/test_reusable_workflow_secret_mappings.py": 177.3, + "tests/test_reviewer_summary.py": 0.0, + "tests/test_risk_hints.py": 0.0, + "tests/test_safety_qualification.py": 2.2, + "tests/test_safety_qualification_release.py": 1.4, + "tests/test_sarif.py": 0.3, + "tests/test_scan.py": 7.2, + "tests/test_scan_context.py": 0.1, + "tests/test_scenario_suggest.py": 2.4, + "tests/test_schema_boundaries.py": 3.5, + "tests/test_schema_roundtrip.py": 2.0, + "tests/test_scoped_base_tree.py": 13.4, + "tests/test_sdk_boolean_source.py": 1.3, + "tests/test_sdk_guard_dependencies.py": 0.3, + "tests/test_self_approval_signal.py": 0.0, + "tests/test_self_check.py": 1.6, + "tests/test_semantic_assessment.py": 0.0, + "tests/test_semantic_cold_start.py": 0.1, + "tests/test_semantic_properties.py": 0.2, + "tests/test_setup_control.py": 37.3, + "tests/test_severity_override_floor.py": 0.0, + "tests/test_shard_partition.py": 32.9, + "tests/test_skill_review.py": 0.1, + "tests/test_source_authority.py": 3.8, + "tests/test_source_binding.py": 2.2, + "tests/test_source_head_identity.py": 7.1, + "tests/test_source_provenance.py": 0.7, + "tests/test_static_inputs.py": 0.0, + "tests/test_strata_inventory.py": 4.2, + "tests/test_subject_rollup.py": 1.4, + "tests/test_surface_exclusions.py": 12.1, + "tests/test_three_command_flow.py": 1.1, + "tests/test_tool_identity.py": 0.0, + "tests/test_tool_surface_diff.py": 0.0, + "tests/test_toolkit_bounds_check.py": 1.0, + "tests/test_trigger_command.py": 1.5, + "tests/test_trust_root.py": 1.3, + "tests/test_ts_tool_descriptions.py": 0.1, + "tests/test_unchanged_host_limits.py": 18.9, + "tests/test_unread_changed_inputs.py": 150.9, + "tests/test_unresolved_descriptions.py": 0.1, + "tests/test_v07_metadata_roundtrip.py": 1.6, + "tests/test_validation_evidence.py": 1.1, + "tests/test_verdict_contract.py": 0.0, + "tests/test_verification_git_snapshot.py": 3.2, + "tests/test_verifier_blocks.py": 0.6, + "tests/test_verifier_control_contract.py": 1.1, + "tests/test_verifier_scenarios.py": 28.8, + "tests/test_verify.py": 115.3, + "tests/test_verify_auto_base.py": 14.5, + "tests/test_verify_capability_scope.py": 0.7, + "tests/test_verify_config_binding.py": 0.7, + "tests/test_verify_orchestrator.py": 45.4, + "tests/test_verify_run.py": 0.0, + "tests/test_verify_weakening.py": 21.8, + "tests/test_vscode_mcp_jsonc.py": 0.0, + "tests/test_vscode_mcp_support.py": 0.0, + "tests/test_wheel_candidate_build.py": 2.1, + "tests/test_workflow_agent_launches.py": 79.2, + "tests/test_workflow_capability_diff.py": 3.1, + "tests/test_workflow_evidence.py": 0.1, + "tests/test_workflow_label_redaction.py": 55.4, + "tests/test_workflow_step_action_references.py": 42.6, + "tests/test_workspace_input_guard.py": 0.3, + "tests/test_zero_install_detector.py": 38.4 + }, + "measured_at": "e672648fc800" +} diff --git a/tests/test_shard_partition.py b/tests/test_shard_partition.py index 969e81029..03823a66f 100644 --- a/tests/test_shard_partition.py +++ b/tests/test_shard_partition.py @@ -17,7 +17,7 @@ import pytest -from ci_sharding import shard_assignment +from ci_sharding import SECONDS_FILE, load_seconds, shard_assignment REPO_ROOT = Path(__file__).resolve().parent.parent @@ -82,6 +82,58 @@ def test_a_dominant_file_does_not_leave_a_shard_idle() -> None: assert max(weighted.values()) / total < 0.55 +def test_measured_seconds_outweigh_item_counts() -> None: + """A slow file of few tests is balanced by its time, not its count (#904). + + By count, the 20-item file below is a twentieth of the 400-item one and + shares a shard. Measured, it takes most of the suite's time, and it gets a + shard to itself. + """ + + def sharing(owner: dict[str, int]) -> list[str]: + return [path for path, shard in owner.items() if shard == owner["tests/test_small_a.py"]] + + seconds = {"tests/test_small_a.py": 600.0, "tests/test_huge.py": 60.0, "tests/test_large.py": 60.0} + assert sharing(shard_assignment(_COLLECTION, 2)) != ["tests/test_small_a.py"] + assert sharing(shard_assignment(_COLLECTION, 2, seconds)) == ["tests/test_small_a.py"] + + +def test_an_unmeasured_file_costs_its_items_at_the_measured_rate() -> None: + """A new file weighs what its items would at the suite's seconds per item.""" + + seconds = {"tests/test_huge.py": 400.0} # one second per item + owner = shard_assignment({"tests/test_huge.py": 400, "tests/test_new.py": 400}, 2, seconds) + assert owner["tests/test_huge.py"] != owner["tests/test_new.py"] + + +@pytest.mark.parametrize("shards", [2, 3, 4]) +def test_the_timed_assignment_is_deterministic(shards: int) -> None: + seconds = {path: count * 0.37 for path, count in _COLLECTION.items() if "tiny" not in path} + first = shard_assignment(_COLLECTION, shards, seconds) + reordered = dict(reversed(list(_COLLECTION.items()))) + assert shard_assignment(reordered, shards, dict(reversed(list(seconds.items())))) == first + assert set(first) == set(_COLLECTION) + + +def test_a_missing_or_malformed_measurement_balances_by_count(tmp_path: Path) -> None: + assert load_seconds(tmp_path / "absent.json") == {} + broken = tmp_path / "broken.json" + broken.write_text("{not json") + assert load_seconds(broken) == {} + odd = tmp_path / "odd.json" + odd.write_text('{"files": {"tests/a.py": 3, "tests/b.py": -1, "tests/c.py": true, "tests/d.py": "x"}}') + assert load_seconds(odd) == {"tests/a.py": 3.0} + + +def test_the_committed_measurement_names_test_files() -> None: + """The measurement is a weight per test file, and every weight is usable.""" + + seconds = load_seconds(SECONDS_FILE) + assert seconds, f"{SECONDS_FILE} holds no measurement" + assert all(re.fullmatch(r"tests/\S+\.py", path) for path in seconds) + assert all(value >= 0 for value in seconds.values()) + + def _collect(shard: int | None, shards: int | None) -> dict[str, int]: """Collect the CI suite and return ``{file: item count}``. From 1a0a40b4d80e59860b689d6c07b59b3d45528996 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Mon, 28 Sep 2026 18:06:12 -0700 Subject: [PATCH 4/4] test: compare the whole tree where #865's cases change only a README Three #865 cases change only `README.md` and assert what a full-tree comparison says about an untouched agent. With the scope derived from the change (#875), a README-only change touches no agent and is `not_established`, so these cases now pass `--scope .`. Co-Authored-By: Claude Opus 5.5 --- tests/test_adk_tool_factories.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_adk_tool_factories.py b/tests/test_adk_tool_factories.py index 4611834d5..00d165f65 100644 --- a/tests/test_adk_tool_factories.py +++ b/tests/test_adk_tool_factories.py @@ -730,7 +730,8 @@ def test_an_unnamed_value_in_an_untouched_module_is_a_gap_not_a_row(tmp_path): _git(tmp_path, "init", "-q", "-b", "main") before = _commit(tmp_path, {"tools.py": SQL_FACTORY, "agent.py": body, "README.md": "a\n"}) after = _commit(tmp_path, {"README.md": "b\n"}) - result = _compare(tmp_path, before, after) + # The whole tree: a README-only change derives no scope of its own (#875). + result = _compare(tmp_path, before, after, "--scope", ".") assert result["rows"] == [] assert result["comparison_status"] == "partial" assert any("readonly=ro at agent.py:" in gap["reason"] for gap in result["head"]["coverage_gaps"]) @@ -758,7 +759,8 @@ def test_a_value_the_read_can_follow_is_data(tmp_path, factory, call): _git(tmp_path, "init", "-q", "-b", "main") before = _commit(tmp_path, {"tools.py": FACTORY + factory, "agent.py": agent, "README.md": "a\n"}) after = _commit(tmp_path, {"README.md": "b\n"}) - result = _compare(tmp_path, before, after) + # The whole tree: a README-only change derives no scope of its own (#875). + result = _compare(tmp_path, before, after, "--scope", ".") assert result["rows"] == [] and result["comparison_status"] == "compared", result["head"]["coverage_gaps"]