Add Smart Closer agent sample designed to bridge natural language requests into complex execution workflows across Salesforce and SAP (S/4HANA) - #1745

…#371)

* fix(init): rank agent-name candidates instead of taking the first one

`_agent_name_candidates` appended `Agent(name=…)` literals in file-then-AST
order with no scoring, and all three consumers — the manifest renderer, the
`init` JSON summary, and the zero-install detector — took the first entry
with an accepted `source`. "First encountered" was the entire selection
policy, and it produced two different wrong identities.

In usestrix/strix it chose the one-character literal `t` over `Strix`
(#320). Reproducing against the real repository showed more than the issue
reported: *every* `Agent(name=…)` literal there lives in a test file —
production builds the agent with `SandboxAgent(name=name, …)`, a parameter
— so a length filter alone would pick `Strix` for the wrong reason.

In google/adk-recipes#1745 it chose `SalesforceAgent`, a real worker agent,
over the `App(root_agent=…)` coordinator that declares it (#324). That is
the one that survives review: the manifest was schema-valid and named a
genuine agent in the repository, just not the reviewed one.

Selection is now one ranking pass over four signals:

- hierarchy — an application root (`App(root_agent=…)`, or the conventional
  `root_agent` symbol) outranks an unqualified agent, which outranks a
  declared `sub_agents=[…]` / `handoffs=[…]` child;
- origin — product code outranks test code, which names fixtures;
- corroboration — a value the project name independently agrees with is two
  sources, not one (this is what makes `Strix` the right answer in a
  repository whose only literals are fixtures);
- a quality floor — values under three significant characters and generic
  scaffolding names are ranked last and made unselectable.

A rejected value is never written: `agent.name` keeps its `CHANGE_ME`
placeholder and the existing `placeholders[]` review action, rather than
asserting an identity nothing reliably declares.

`name=` given as a symbol resolves statically through one hop — a module
constant or an `os.environ.get("…", "…")` default in the same package. The
real sample imports it as `from config import AGENT_NAME` (absolute, which
works because ADK puts the agent directory on `sys.path`), so both that and
the relative spelling resolve to the sibling file. The hop never chains,
never leaves the workspace, and never imports user code; the environment
default is reported as overridable at runtime rather than as a fact.

Each `agent_name_candidates[]` entry now carries `role`, `path`,
`rank_score`, `selectable`, and a `rationale[]`, so a future ordering
regression is visible in `detect --json` instead of silently changing what
the manifest claims.

Two consequences for the duplicate-implementation problem this exposed:

- the rule now exists once (`select_agent_name`) rather than as a
  `source in {…}` set literal copied into the renderer and the JSON summary,
  which were two chances to disagree about the declared identity;
- `tools/shipgate-detect.py` was the third copy and has to stay one, being
  stdlib-only. It is ported and pinned byte for byte by the parity suite —
  the one field there that is not merely structural parity, because it names
  an agent rather than answering yes/no. Samples all carry a single
  unambiguous literal and could never have caught a ranking divergence, so
  the parity test builds a workspace that can.

No sample selection changed. The parse remains a single AST walk: roles are
assigned after the walk completes, because the `App(root_agent=…)` binding
that identifies a coordinator can appear after the construction it names.

Closes #320
Closes #324

Co-Authored-By: Claude Opus 5 <[email protected]>

* fix(init): read agent-name bindings the way Python does, or decline

Addresses the six blocking review findings on #371. Each was reproduced
against the pre-fix code before being fixed, and each fix has a test that
fails without it.

The ranking shipped in the previous commit read Python name binding with a
flattened `ast.walk` and first-write-wins heuristics, which produced
misleading `agent.name` values in several statically reproducible cases —
including the fail-closed case #324 explicitly requires.

**An unresolved application root now blocks selection entirely.** With
`root_agent = LlmAgent(name=f"{tier}-coordinator", sub_agents=[worker])`,
detection dropped the root and wrote `agent.name: WorkerAgent`. Letting the
rest of the field rank looks conservative, but everything remaining is by
construction *not* the root, so it silently declares a worker as the
reviewed identity. A dynamic name, a factory call such as
`App(root_agent=build_root())`, or a symbol no single construction defines
now all produce CHANGE_ME plus the reason. The previous test asserted the
old behaviour and is replaced — it encoded the bug.

The block is deliberately workspace-wide rather than scoped to the file
that declares the unresolved root. Scoping it would let a resolved root
elsewhere win, which is precisely the class of "assert an identity we
cannot justify" this PR exists to remove.

**References resolve to the reaching assignment.** For
`agent = Agent(name="OldWorker"); agent = Agent(name="ActualRoot");
App(root_agent=agent)`, both constructions were classified as roots and
source order picked the overwritten one. Bindings are now matched by
nearest scope then latest line before the reference.

**Scopes are no longer flattened.** A function-local `root_agent` was
promoted to application root and outranked every real module-level agent;
the ADK convention is about the *module* symbol `adk run` imports. A
helper-local `from other import AGENT_NAME` could likewise supply the value
a module-level construction reads from a different module. Imports are now
keyed by scope, and `from config import AGENT_NAME as NAME` looks up
`AGENT_NAME` in the target module rather than the alias.

**Constants require exactly one binding.** `setdefault` made
`NAME = "OldAgent"; NAME = "CurrentAgent"` resolve to `OldAgent`, a value
Python never passes, and a conditional or computed later write was ignored
rather than triggering the promised fail-closed result. A symbol bound more
than once anywhere in the file — later, conditional, computed, in another
scope, or as a parameter — is now never resolved. That single rule is what
makes reading module-level constants safe at all.

**Conflicting import roots leave the identity unresolved.** `from config
import AGENT_NAME` resolves against the agent directory or the workspace
root depending on `sys.path`; when both exist in-workspace and disagree,
which one Python picks is not ours to assume. Package `__init__.py` now
also precedes the module file, matching Python's own finder.

**Child edges require a verified agent constructor.** The `sub_agents=` /
`handoffs=` branch ran for any call, so `configure(handoffs=[coordinator])`
demoted the coordinator and handed the identity to an unrelated worker.

**The zero-install byte-parity claim now holds.** It did not: the script
walked the filesystem while canonical detection lists through Git, so a
`.gitignore`d module could win a ranking `init` can never produce, and a
Python symlink escaping the workspace both contributed a name and leaked
its outside absolute path into `path`. Both reproduced. The script now uses
the same bounded `git ls-files` inventory when Git can read the workspace,
containment-checks every fallback-walk path, and bounds Python *parses*
rather than the whole inventory — a global file cap let an asset-heavy
repository exhaust its budget before reaching any source. Three parity
tests cover the three cases and all three fail on the previous script.

Two consistency fixes from the review body:

- `cli/detect.py` printed candidate zero rather than calling
  `select_agent_name`, so with only `Agent(name="t")` the human-readable
  `detect` reported `t` while `init` wrote CHANGE_ME. It now reports what
  `init` will write, and prints the rejection reasons when nothing is
  selectable. That was the fourth copy of the selection rule.
- `docs/agent-recipes.md` claimed `project_name_candidates[]` shares the
  enriched shape; it remains `{value, source}`.

Sample selections are unchanged, and both originally reported repositories
still resolve correctly (`Strix`, `SmartCloserAgent`).

Co-Authored-By: Claude Opus 5 <[email protected]>

* fix(init): model every binding, not just the ones that build agents

Addresses the four P1 and two P2 findings from the second review. Each was
reproduced against c102bec first, and each fix has a test that fails
without it.

The previous resolver modelled only assignments whose value was an agent
construction, resolved references by lexical position, and trusted a callee
spelling. All four P1s fall out of that: a binding model that cannot see a
non-agent write, a branch, a closure, or a shadowed import will confidently
report an identity the runtime never has.

Writes are now recorded for *every* binding — assignments, parameters,
imports, `global`, function definitions — each carrying its scope, line, and
whether it sits under a conditional. Four consequences:

**A root rebound to a non-agent value retires the previous one.**
`root_agent = Agent(name="StaleRoot")` followed by `root_agent =
build_root()` kept `StaleRoot` selectable; conventional-root resolution even
picked the factory call, which then produced no evidence and no marker.

**Branches are ambiguous, not "latest wins".** Under `if/else` both arms can
execute, so the previous rule always chose the lexically later one. Any
candidate binding under an `if`/`try`/loop now fails the lookup closed.

**Closures resolve against enclosing function scopes.** A nested function's
free name skipped straight to the module binding and read a different object
than the code does. Class bodies are deliberately *excluded* from that chain
— a method referencing a name bound in its class body raises `NameError`, so
treating it as reachable would resolve to an object Python never supplies.
That case was not in the review; it is the same defect one scope over, and
is covered by its own test.

**Unresolved root symbols block selection.** A root whose name is a symbol
that fails cross-module resolution was dropped silently, so
`NAME = "Old"; NAME = "Current"; root_agent = Agent(name=NAME,
sub_agents=[worker])` selected `WorkerAgent` — the documented rule says an
unresolved root makes every candidate non-selectable. The check now runs
where resolution happens, after every file's constants are known.

**Environment defaults must prove provenance.** Matching `getenv` by
spelling let a module's own `getenv(key, fallback)` fabricate an identity
from its fallback. The binding must now be an unshadowed stdlib import;
`import os as operating_system` still resolves, and a test pins that so the
proof does not degrade into recognising one spelling.

**Origin dominates the score instead of competing with it.** The documented
contract is that product code outranks test code, but additive scoring gave
a test `root_agent` 2.0 against a production unqualified agent's 1.0. The
test penalty is now strictly greater than the whole spread of the hierarchy
and corroboration signals. A test asserts that arithmetic directly, so a
future signal cannot widen the spread past it unnoticed.

**The zero-install Git bound is now a real bound.** `capture_output=True`
buffered the entire inventory before the 16 MiB check, making the cap
decorative, and overrunning it fell back to an uncapped filesystem walk —
the very work the bound exists to refuse, answered from a different
inventory than `init` used. It now streams in chunks, kills the child on
overrun, and exits non-zero exactly as canonical discovery raises
`DiscoveryError`.

All of it is mirrored in `tools/shipgate-detect.py` and verified by
byte-parity across every reproducer. Sample selections are unchanged, and
both originally reported repositories still resolve correctly (`Strix`,
`SmartCloserAgent`).

Co-Authored-By: Claude Opus 5 <[email protected]>

* fix(init): resolve constructors and bindings, or decline

Addresses the six P1 and three P2 findings from the third review. Each
reproduced against f0aab48 first, and each fix has a test that fails
without it.

Four of the six P1s were one gap: the model recorded assignments and
trusted spellings, so any binding it could not see — and any name it could
not attribute — produced a confident, wrong identity.

**Constructors resolve through their bindings.** Recognition matched the
terminal spelling, so a local `def Agent(...)` supplied `FabricatedRoot`
while the real `RealApp(root_agent=RealAgent(name="ActualRoot"))` went
unseen. The reaching binding decides instead: an import of a framework
class under any alias is a constructor, a spelling bound to a local
`def`/`class` is not. Aliases are only honoured from framework modules —
`from mypkg.agents import Agent as Wrapped` renames somebody else's class,
and my first attempt widened recognition to it. My own test caught that.

**Every binding form is modelled, not just assignments.** `del`, `class`,
`except … as`, and `case` all rebind or unbind a name without producing a
Store `Name`, so a retired `root_agent` kept its role. `from x import *`
binds an unknowable set, so a file carrying one can prove nothing about any
of its names.

**`global`/`nonlocal` are declarations, not writes.** Recorded as a local
write, a `global root_agent` store was invisible to module-level
resolution, so a stale `OldRoot` kept the role while the runtime exposed
`NewRoot`. Writes are now routed to the scope they actually bind, and
marked conditional: whether the function runs, and when relative to the
module body, is not something the file says.

**Enclosing writes are not ordered by the nested reference's line.** A
function body executes when called, not where written, so a module-level
rebinding *below* a nested reference still happens before the call. Only a
single unconditional binding is provable in an enclosing scope.

**Conditional provenance reaches inline roots.** `agent_calls` kept only
`(call, scope)`, so two `App(root_agent=Agent(...))` constructions in
`if`/`else` were both selectable roots and the first won.

**The detector keeps the logical path of contained symlinks.** Returning
the resolved path renamed the entry: with `agent.py -> source.txt` the
`.py` suffix disappeared, the script reported zero Python files and
`is_agent_project: false` where the CLI reported an agent project. That is
the go/no-go verdict, not just the ranking.

P2s: `DetectResult(agent_name_candidates=[NameCandidate(...)])` works again
— both the instance and the legacy dict are upgraded at the model boundary
with the rule that decided selection before ranking existed, instead of
raising or silently landing on `selectable: false`. `tests.py`/`test.py`
count as test code. The non-Git fallback walk has a documented ceiling that
raises rather than truncating, because a partial inventory is a partial
scope verdict.

Two behaviour changes worth calling out, both consequences of resolving
provenance rather than spellings:

- `samples/_anti_patterns/dynamic_toolset_factory` defines a local
  `class Agent`; its literal is no longer offered as a name candidate. The
  fixture comment and README documented the old behaviour and are updated.
- `test_an_unrelated_agent_class_does_not_define_a_project` (from #370)
  asserted the same literal stayed a "low-confidence name suggestion". It
  claimed a provenance it never had, and `init` could write it as the
  reviewed identity, so the assertion is inverted. The test's actual
  subject — that the module does not define a *project* — is unchanged.

All of it is mirrored in `tools/shipgate-detect.py` and verified by byte
parity across every reproducer. Sample selections are unchanged, and both
originally reported repositories still resolve correctly.

Co-Authored-By: Claude Opus 5 <[email protected]>

* fix(init): ask the binding that reaches the call site

Addresses the seven P1 and one P2 findings from the fourth review, plus the
EOF whitespace `git diff --check` reported. Each reproduced against
f10d0d3 first, and each fix has a test that fails without it.

The P1s share a premise I had wrong: provenance is a question about a
*location*, not about a file. The previous pass asked "is this spelling
bound to a framework import somewhere in this module", which is true in
places where it does not apply.

**The binding must reach the call site.** `_reaching_binding()` took the
highest-line binding, so `from google.adk.agents import Agent` at the
bottom of a file retroactively validated a decoy `def Agent` call above it,
and a binding inside an `if` counted as certain. It now takes a line number
and shares one implementation with agent-symbol resolution — the same rule
answered both questions in two copies before.

**Dotted spellings prove their head.** The provenance check only ran for
`len(parts) == 1`, so `fake.Agent(...)` borrowed the terminal name. The
head must now resolve to a framework module, which also fixed the
`plain_imports` bookkeeping: `import a.b.c` binds `a` denoting `a`, not the
full path, and storing the latter made `a.b.c.Agent` resolve to `a.b.c.b.c`
and never match.

**Attribute rebinding retires provenance.** `adk.Agent = fake` and
`os.getenv = fake` replace a constructor and a stdlib lookup without
binding any name, so the import that "proved" them was still there. Those
mutations are recorded and both `_constructor_role` and
`_is_stdlib_env_lookup` decline after one.

**Scopes now follow Python's.** Comprehensions have their own, so
`[App for App in ()]` no longer shadows a module-level `App` and drops the
App/root edge. Definition headers — defaults, decorators, annotations,
class bases and keywords — are walked in the enclosing scope, because that
is where they are evaluated; treating them as body let a parameter shadow
the constructor its own default had just used. And a scope introduced
inside a branch carries that contingency into everything it declares, so
two `def build()` arms no longer resolve to whichever came first.

**A wildcard import suspends constructor provenance too.** It was consulted
for symbol lookups but not for constructor roles, so
`from replacement import *` could legally replace both constructors while
detection still called the result a proven inline root. The rule is
source-ordered: a later explicit binding restores what a spelling means.

P2: legacy `NameCandidate` inputs work for every sequence form the old
field accepted — a tuple of instances raised and a tuple of legacy dicts
landed silently on `selectable=False` — and each legacy entry is validated
as a `NameCandidate` before being enriched, so missing values, wrong types,
and keys `extra="forbid"` rejects are no longer upgraded into a well-formed
lie.

Mirrored in `tools/shipgate-detect.py` and verified by byte parity across
all 19 reproducers. Sample selections are unchanged, and both originally
reported repositories still resolve correctly