feat(readers): read returned tool factories with constructor ownership checks by pengfei-threemoonslab · Pull Request #964 · ThreeMoonsLab/agents-shipgate

pengfei-threemoonslab · 2026-10-04T03:17:14Z

Bounded local tool factories retain source members and construction locations in the OpenAI Agents SDK and Google ADK readers. Unproved constructor ownership remains a coverage gap; it never establishes deployed wiring or permits an inferred removal.

Review status: on hold; not ready to merge. This update publishes the reviewed source fixes. It does not carry forward the previous head's full-suite, development-corpus, coverage, or hosted timing qualification.

The follow-up addresses the review as follows:

Review concern	Current change and remaining work
Lost handoff/subagent edges and false removals	Readable candidate edges survive constructor gaps. Incomplete graph coverage withholds capability and finding-resolution claims. Application comparisons carry exact-binding uncertainty even when an incomplete handoff is present on both sides. Standalone source-predicate comparisons retain their own identity checks.
Import identity through search paths	Check repository import-search effects and provider dependencies; refuse unresolved search-path/provider ownership instead of treating it as established.
Canonical SDK/ADK precision	Support the direct ADK package initializer `from . import agent`. SDK Runner consumers and other opaque carriers remain coverage limits. Restoring the remaining canonical forms and requalifying the development corpus are still required before merge.
Repeated constructor analysis and deep attribute chains	Reuse source syntax with fresh semantic, provider, owner, budget and currency checks; bound attribute-chain and import-closure work. Final committed performance and hosted CI qualification are still pending.
CI budget and required checks	Main already has five shards. A separately reviewed integration/CI proposal preserves those shards and the time/coverage limits, fails aggregation if any shard fails, and requires exactly five valid coverage fragments. That protected workflow proposal is not applied. The live Protect main ruleset still omits `suite (4)` and `suite (5)`.
Refusal locations, changelog and timing provenance	Retain bounded opaque-route source locations, add the changelog entry, separate timing records from progress output, and generate provenance from the measured JUnit population. README/quickstart excerpts explicitly distinguish the published `v1.2.0` output from current source coverage limits.

Validation of the latest ordinary source overlay:

61 focused regression cases passed with no skips, including handoff identity, guard dependency verification, incomplete-graph comparisons, cache repair, and the two-channel documentation excerpts. Source, HEAD, index and engine guards matched before and after execution. The private Git ignore override changed shortly after this run's controller launch; that environment timing is retained as a provenance limitation.
Both previously failing local-settings Git fixture cases passed in a separate run with an isolated test Git configuration. No user or repository Git configuration changed.
Ruff and staged whitespace checks passed.
Independent reviewers cleared the ordinary changes and the private latest-main integration/coverage proposal. The proposal's Bash guards passed 18 controlled status/artifact checks; this establishes neither actual coverage percentage nor hosted timing margin.

Remaining before merge:

Obtain the required protected-workflow review and integrate current main without losing the reviewed reader changes; update the required shard checks.
Resolve the canonical-form precision work and the remote-binding comparison cases. Historical full-suite failures are not silently rewritten or counted as cleared.
Run the complete selected suite, static/latency checks and hosted CI on the final committed head; regenerate timing weights only from complete, identity-bound results.

Parent #963 is merged. This remains the second bounded increment of #874; returned closures (#965), foreign target-agent surfaces and typed runtime retention remain separate work. Application code is never imported or executed. No declarations, waivers, suppressions or frozen qualification rules are changed.

pengfei-threemoonslab · 2026-10-05T21:49:50Z

Review (2026-10-05): holding this PR, not merging. It is stacked on #963; see the review there.

CI budget (blocker). The new tests add about 26.6% to hosted test time. suite (3) passed in 895 s against the 900 s cap on 1db868c27, and earlier runs on this branch lost 12 shard jobs at the cap. Re-weighting cannot fix this, because hosted per-shard speed varies from 0.5× to 0.9× of local. It needs a fifth shard or cheaper tests, for example by applying the frozen engine fixture to the other ownership modules. Separately, the "Protect main" ruleset does not require suite (4). coverage (needs: [suite] without if: always()) is skipped rather than failed when a shard fails, so a lone suite (4) failure would not block a merge today.
scan regressions. Constructor gaps erase handoff and sub-agent edges (google_adk.py:1259-1267, openai_sdk_static.py:651-652). As a result:
- ADK root selection fails, every tool becomes unbound, and SHIP-SCHEMA-FREEFORM-OUTPUT findings drop.
- scan --diff-from reports false removals. Rewriting a loop as sum(genexp) "removes" a destructive tool, and adding an if __name__ == "__main__": Runner.run_sync(...) block "removes" a handoff.
The identity proof is open through sys.path. A vendored fake agents package, or a patcher plugin loaded via sys.path.insert / site.addsitedir, is treated as an unread external package (python_imports.py:1411, :3162). Rows stay established although the runtime drops the tools. The doc (lines 140-142, 269-270) says this case is covered.
Precision loss beyond the documented narrowing. These canonical forms no longer establish:
- the SDK README example (await Runner.run(agent, …));
- the ADK quickstart layout (__init__.py: from . import agent);
- any module containing a generator expression or lambda, a pydantic model or dataclass, a RunContextWrapper annotation or os.environ[...].
The ADK __init__ refusal is not justified even under this PR's model, because Python's import system already binds the submodule. On the development corpus all 68 established rows are withheld.
Performance and DoS. _external_constructor_use re-runs for every construction over the whole import closure, and references() is O(d³) per attribute chain. 8 agent modules go from 14 s to 177 s; depth-320 attribute chains go from 6 s to 285 s. A pull request author can trigger the latter.
Minor.
- The refusal text omits the opaque route (no file:line) when exported_elsewhere fires.
- There is no CHANGELOG entry.
- Under -q, the timing observer's records are glued onto the progress dots.
- The measurement block in tests/shard_seconds.json is hand-written, so the next re-measure drops it.

…ing evidence

Resolve conflicts with #985 (tools bound as objects) and the CHANGELOG. - SDK reader: record per-member construction sites (#964) for object bindings too, through one note_sites helper, and keep object names in the published tool_sites. - ADK reader: record a returned list's factory call sites for object bindings as well as function tools. - Both readers: an unproved constructor identity now marks every binding of the agent, object bindings included, so an object row is never established under a constructor gap. - CHANGELOG: keep both sides' entries.

…or checks (#964) The constructor-ownership proof predates #985 and refused every framework object a tool list can hold as an opaque use of the framework namespace, which withheld the whole agent. Each shape is now accepted by its role, resolved by import identity, and leaves an owner obligation the list reader discharges: - ADK built-ins (google_search, load_memory, built_in_code_execution) as a member of tools=, or of a list that is followed to its uses - ADK McpToolset with its connection-parameter class (data arguments only); the toolset's import path is not its identity - hosted tools, MCP servers (also as the target of an async with) and AgentTool as the documented call with data arguments; SDK function_tool(f) - an agent held by AgentTool(agent=...) or exposed by agent.as_tool(...), across modules and through a helper returning it from its parameter - eager annotations of the agent class or a tool object on an undecorated function, when no module of the census reads annotation dictionaries - an inert dataclass of environment defaults and its instance (a new instance_data owner), os.environ[...] read into a local or a tool object's configuration, and a method call whose result is dropped on an Agent instance - a name an installed package provides is not shadowed by a sibling package of the module's own (src/app/agents is app.agents, not agents) Exports of a handle are read through their importers' uses only for a plain from-import and only for agent, instance, tool-object and wrapped-function owners. Adversarial controls for each route are in test_object_tool_ownership.

…sed import (#964) _unused_reflection_import discharged a builtins/inspect import when only the importing module's own Name loads were checked. A module that does 'from builtins import eval as execute' and is imported by a sibling that calls execute(...) was therefore read as free of executable machinery, and a returned-tool renamed through it was silent. The discharge now also requires that no other module of the read scope imports the module (from m import name, import m [as x], from m import *, or through a re-export chain), using the caller census's own retaining set; a census cut short does not discharge. Covers the __all__ + star export and three bridge-module re-export routes.

…l, and withhold only absence (#964) A remote McpToolset is never statically enumerable, so its graph is always partial, and the binding-comparison gate switched the whole capability comparison off: an ADK endpoint moved from a read-only host to an admin one (#538) was 'broadened' on main and silent here. Two causes. - The ownership proof refused 'os.environ[...]' inside connection-parameter data. A read that is data of an exact ADK or mcp connection class is now accepted, following enclosing calls only through such classes up to an accepted toolset's connection_params, including the nested StdioConnectionParams(server_params=StdioServerParameters(env={...})) form. Every other placement of the read stays an unread handle. - The gate. A toolset whose own construction was read and whose leaves cannot be listed now carries its own issue identity (source framework_toolset_inventory); it, and the graph_status:partial it alone causes, are not binding-graph incompleteness. And when a graph really is incomplete the gate withholds only what depends on absence (removed and narrowed rows and members, the controls of an absent tool, resolved findings) and still reports added, changed, broadened and unknown-direction rows. The binding diff stays withheld, and the requested-comparison gap still stands, found through the diff notes. Consumers checked: tool_surface compute, the binding diff in sanitization, release_decision's incomplete-comparison gap, the capability_change projection and the verify capability review notes.

… 'unresolved' (#964) The constructor-gap pass copied every already-matched sub-agent name into unresolved_sub_agents, which made the binding graph say the scan 'could not match' that agent to a definition. The names were read; the record's 'unread' already makes their edge incomplete, and the constructor issue is its own partial. Stop the copy, and bring the three stale list-expression assertions to the contract the PR chose: readable candidate edges survive a constructor gap, incomplete, with the constructor issue named (#964)

…ow stop at (#964) Two imported-tool review cases that store sys itself into the module table are still withheld as constructor candidates, but the import-search check reads the store first. Their expected reason is now that check's text; the namespace-read case keeps the reflective-access reason.

…filesystem (#964) _unprovided_root checked the absence of operator / operator.py with lstat, which finds Operator.py on a case-insensitive filesystem (macOS) and not on a case-sensitive one (Linux): the same tree was a proof of absence on one and not the other, and three tests failed only on hosted Linux. Absence now reads the exact directory listing the resolver already holds: any entry that case-folds to the provider or provider.py blocks, on every platform. The same fix is applied to the other probes whose answer depended on the host: a missing import-root component beside a differently cased entry, the above-scope namespace directory, and the pyvenv.cfg marker that decides whether a directory is read at all (and its currency check).

…ls (#964) 120 parametrizations across 15 test functions in test_builder_bindings.py expect a clean read of a namespace write the ownership proof does not yet attribute to an unrelated attribute, another module or a fresh/foreign dictionary. The reader refuses them, fail-closed, which is an over-refusal and never a missed change. The precision is not implemented in this PR; it is tracked in #1005, and exactly the failing parametrizations are strict xfails so closing the gap is noticed. No refusing parametrization is marked.

Over half of reader time in a constructor-ownership test was the import-search check (_no_import_patch -> _import_search_limit -> _import_search_issue), recomputed for every construction: about 90 calls a case, mostly for the same runner list or the same module. The answer is now remembered within one resolver read, by runner list (path and parsed tree) for _import_search_limit and by (parsed tree, sys/site carrier set) for the per-module refusal. Nothing is stored once any read bound has been reached, for a refusal that is itself a bound, or when a stop was raised, so a limit is met again rather than remembered. Behavior is unchanged: a plugin that recomputed every call with empty caches and compared it with the remembered answer found no difference across 113,669 calls in the reader test files.

Replace the projected shard and gate budgets with what the first rehearsals of the sharded layout measured on 2026-10-09: Advisory Release Rehearsal run 37871044408 took 290-542 s per shard job (slowest 9.0 min) and 42 s for the gate; Release Rehearsal run 37869869033 reached the qualification download in about 50 s and stopped there, because this repository sets no qualification variables. The 35- and 10-minute timeouts are unchanged; re-derive them from the first rehearsal after #964 lands.

…ring subset (#964) The Agents SDK and Google ADK readers share one constructor-ownership engine; a test's framework argument selects the reader. Crossing it with a route matrix (patch x layout x change x factory, ...) ran the same engine over the same routes twice, in the per-PR run. Per test function in the 20 engine test files, the ADK matrix stays whole and the Agents SDK keeps a covering subset: every value of every parameter is met at least once on the SDK reader, so each route and each reader-specific branch is still exercised there. The other SDK cases (1,291 of 22,803) are marked slow at collection (tests/framework_matrix.py, applied from conftest before -m), which the per-PR run excludes and the nightly run executes with -m slow. Nothing is deleted, and the marks are added at collection so the parameter ids other tests key on do not change. Registers the slow marker in pyproject.

…es (#964) Amends the existing #964 entry and the two README/quickstart sentences that said an incomplete binding graph withholds the whole capability comparison.

#964) tests/shard_seconds.json is re-measured from one CI-mirror run (-n auto, not perf and not slow, 21,522 cases, 424 files): the PR's new files had no weight and were guessed at by item count, which put git-fixture files in a shard as if they were pure and overran the 15-minute cap. At the measured weights the suite packs into 5 equal shards of about 3,573 weighted seconds each, or 6 of about 2,978. test_shard_partition.py now fails when a file the CI suite collects has no measured weight, naming the re-measure command; the two files that run outside the sharded suite are an explicit allow-list. The unsharded collection is taken once per process instead of twice.

The 'does another module import this one' question asked by the unused reflection-import discharge built a fresh caller census (directory inventory and source reads) for each module that asked. One census per resolver answers all of them (#964)

… verification (#1006) * ci: split the suite into six shards, add a slow marker and a nightly run for it The slowest of CI's five shards took about 11 of its 15 minutes on main (Test steps of 311, 405, 623, 508 and 538 seconds, about 40 minutes in all), and #964, which adds roughly 45% more test time, had a shard cancelled at the cap. The partition is computed from the collection and measured seconds, so a sixth shard needs no other change to conftest.py; the matrix, the shard count and the coverage job's expected fragment count move together and a test holds them equal. Simulated from tests/shard_seconds.json, each of six shards carries 1,846 weighted seconds against 2,215 for each of five. Register a `slow` marker. Pull-request shards select `-m "not perf and not slow"`; a new Slow Tests workflow runs `-m slow` nightly and on dispatch with the suite job's checkout and locked install, treating pytest's exit status 5 (nothing collected, today's state) as success and every other non-zero status as failure. Release verification keeps the full selection. The coverage job now starts after a failed shard and fails by name. A skipped required check counts as passing, and the ruleset requires suite (1) to suite (3) only, so a failure in shard 4, 5 or 6 left the job skipped and the pull request mergeable. It also requires exactly one fragment per shard. * ci: shard release verification's suite instead of raising its single-runner timeout Both release verifications ran the whole suite on one runner inside 60 minutes, a figure derived on 2026-09-14 from a ~25 minute suite. CI's five shards now take about 40 minutes of test time on main (1.5x headroom, not 2.4x) and #964 adds roughly 45%, about 58 minutes. A timeout there fails verification after the tag exists, and a budget of 2.5x that would be one job of about two and a half hours. Run the suite the way CI does instead: a six-shard `suite` job in each of release-verify.yml and release-advisory-verify.yml, partitioned by the same conftest.py from the same tests/shard_seconds.json, selecting `-m "not perf"` so the `slow` tests CI leaves out still run here. The `tests` job stays the gate and keeps its outputs (source_sha, qualified_wheel_sha256, qualification_sha256) and every integrity property: it needs `suite`, starts after a failed shard (`!cancelled()`) so its first step can fail it by name instead of being skipped, requires exactly one evidence record per shard each naming the commit it gates, and enforces the 85% coverage floor over the combined fragments. The artifact job is unchanged and still needs `tests`. Budgets are projected from CI's timings: 35 minutes per shard (about 14 projected, 2.5x) and 10 for the gate (about 3 projected). The runbook records both, and the derivation; replace them with a finished rehearsal's figures. Tests follow the move (the suite step is found in `jobs.suite`), pin the shard count in all three matrices and both fragment counts, hold the two release files' `suite` jobs identical, and run the gate's shell against built evidence (failed, skipped and cancelled results, a missing or surplus shard, a wrong commit, and real coverage fragments that clear the floor only together). * docs(release): record the first sharded rehearsal's timings Replace the projected shard and gate budgets with what the first rehearsals of the sharded layout measured on 2026-10-09: Advisory Release Rehearsal run 37871044408 took 290-542 s per shard job (slowest 9.0 min) and 42 s for the gate; Release Rehearsal run 37869869033 reached the qualification download in about 50 s and stopped there, because this repository sets no qualification variables. The 35- and 10-minute timeouts are unchanged; re-derive them from the first rehearsal after #964 lands.

Bring in #1006 (six shards, the slow marker and sharded release verification). The slow marker description now covers both the framework-matrix cases this branch marks and the nightly/release runs.

Measured from one CI-selection run (not perf and not slow) of cbff1ef, which adds #1006's two test files the guard named as unweighted. Six simulated shards weigh 2,468-2,489 s each.

…ery host (#964) test_operator_receiver_proof_reads_import_roots_above_the_caller_scope derived its expectation from the host filesystem, so it contradicted test_builder_calls' case providers once absence was decided from the listing with case folded: it passed on macOS and failed on Linux CI (suite (6), case_root and case_provider). Both now expect the same host-independent refusal. Checked under a simulated case-sensitive filesystem, which reproduces CI's four failures with the old expectation.

pengfei-threemoonslab marked this pull request as ready for review October 4, 2026 03:33

pengfei-threemoonslab marked this pull request as draft October 4, 2026 03:46

pengfei-threemoonslab marked this pull request as ready for review October 4, 2026 04:08

pengfei-threemoonslab marked this pull request as draft October 4, 2026 04:22

pengfei-threemoonslab marked this pull request as ready for review October 4, 2026 05:14

pengfei-threemoonslab mentioned this pull request Oct 4, 2026

feat(adk): read returned plain tool closures with invocation evidence #965

Open

pengfei-threemoonslab marked this pull request as draft October 4, 2026 06:08

pengfei-threemoonslab changed the title ~~feat(readers): follow returned tool lists and dictionaries~~ feat(readers): read returned tool factories with constructor ownership checks Oct 4, 2026

pengfei-threemoonslab marked this pull request as ready for review October 4, 2026 11:42

pengfei-threemoonslab marked this pull request as draft October 4, 2026 12:22

pengfei-threemoonslab marked this pull request as ready for review October 4, 2026 13:46

pengfei-threemoonslab marked this pull request as draft October 4, 2026 13:58

pengfei-threemoonslab marked this pull request as ready for review October 5, 2026 17:01

pengfei-threemoonslab force-pushed the codex/874-builder-call-contexts branch from 229e0cd to 2977342 Compare October 6, 2026 08:29

fix(readers): bound constructor analysis and preserve incomplete bind…

e8f997a

…ing evidence

pengfei-threemoonslab force-pushed the codex/874-returned-tool-lists branch from 1db868c to e8f997a Compare October 8, 2026 08:55

pengfei-threemoonslab changed the base branch from codex/874-builder-call-contexts to main October 8, 2026 08:56

pengfei-threemoonslab added 7 commits October 8, 2026 10:23

This was referenced Oct 9, 2026

Constructor ownership: namespace-write precision for unrelated attributes, foreign receivers and fresh dicts (#964 follow-up) #1005

Open

ci: six shards, a slow marker with a nightly run, and sharded release verification #1006

Merged

pengfei-threemoonslab added 2 commits October 8, 2026 18:15

pengfei-threemoonslab added 4 commits October 8, 2026 19:11

docs(changelog): describe the partial-graph comparison and reader fix…

8df57cd

…es (#964) Amends the existing #964 entry and the two README/quickstart sentences that said an incomplete binding graph withholds the whole capability comparison.

pengfei-threemoonslab added 3 commits October 8, 2026 20:46

Merge main into codex/874-returned-tool-lists

cbff1ef

Bring in #1006 (six shards, the slow marker and sharded release verification). The slow marker description now covers both the framework-matrix cases this branch marks and the nightly/release runs.

test(ci): re-measure shard weights on the merged tree (#964)

5b65f7b

Measured from one CI-selection run (not perf and not slow) of cbff1ef, which adds #1006's two test files the guard named as unweighted. Six simulated shards weigh 2,468-2,489 s each.

pengfei-threemoonslab merged commit 5a22373 into main Oct 9, 2026
15 checks passed

Navigation Menu

Repository navigation

feat(readers): read returned tool factories with constructor ownership checks - #964

feat(readers): read returned tool factories with constructor ownership checks#964
pengfei-threemoonslab merged 17 commits into
mainfrom
codex/874-returned-tool-lists

pengfei-threemoonslab commented Oct 4, 2026 •

edited

Loading

Uh oh!

pengfei-threemoonslab commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

pengfei-threemoonslab commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pengfei-threemoonslab commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pengfei-threemoonslab commented Oct 4, 2026 •

edited

Loading