feat(skills): let SkillToolset pin registry skills so they appear in list_skills / the catalog without a search_skills turn
- Lingua principale
- Python
- Stelle
- 21.5k
- Fork
- 4k
- Merge medio
- 1g 14h
- PR unite (30g)
- 37
Descrizione
## 🔴 Required Information
### Is your feature request related to a specific problem?
When `SkillToolset` is backed by a `SkillRegistry` (e.g. `GCPSkillRegistry` / Agent Registry), skills that live in the registry are never part of the L1 catalog the model sees. The model can only reach them through `search_skills` → `load_skill`, so a registry skill always costs at least one more model turn (plus one registry round-trip) than an equivalent local skill, even when the developer knows up front exactly which registry skills the agent should use.
Current behavior on `main` (ac0133a4):
- `list_skills` and the `` catalog are built from `SkillToolset._list_skills()`, which only returns the skills passed via `skills=[...]` (`src/google/adk/tools/skill_toolset.py:1594-1596`). The registry is never consulted for the catalog.
- The registry is only consulted from `_get_or_fetch_skill()` (`skill_toolset.py:1582,1592`), i.e. after the model has already named the skill. The system instruction tells the model to fall back to `search_skills` when local skills are not sufficient (`skill_toolset.py:1669-1675`).
- The new `SkillDiscoveryMode.EAGER` (#7092 / #7093) removes the discovery turn for local skills, but its docstring explicitly leaves registry skills out: "Registry skills are unaffected: they are still reachable only through `search_skills`." So after that change the gap is: local skills = 0 discovery turns, registry skills = always ≥ 1.
- As a secondary cost, a registry skill the model has activated is re-downloaded on every later invocation: session state only keeps the activated skill *names* (`_adk_activated_skill_`), and `_fetched_skill_cache` is keyed by `invocation_id` (`skill_toolset.py:1564`), so `_resolve_additional_tools_from_state()` calls `registry.get_skill()` again each turn.
Concretely, for a fixed set of skills that happen to be published in Agent Registry instead of shipped with the agent:
| | local skill | registry skill |
|---|---|---|
| turns before `load_skill` (LAZY) | 1 (`list_skills`) | 2 (`list_skills` → `search_skills`) |
| turns before `load_skill` (EAGER) | 0 | 1 (`search_skills`) |
| registry round-trips per later turn | 0 | 1 per activated skill |
The extra turn also adds the `search_skills` tool declaration, the search result payload and the model's reasoning about it to the context on every conversation, which is the token waste we want to avoid. Our use case is the common one where the skill *content* is managed centrally in the registry (its update cycle is decoupled from the agent's deploy cycle), but the *set* of skills an agent uses is decided at build time, exactly like local skills.
### Describe the Solution You'd Like
Let `SkillToolset` pin a known set of registry skills so they behave like local skills once fetched:
```python
toolset = SkillToolset(
skills=load_skills_from_dir("./skills"), # local, as today
registry=GCPSkillRegistry(project_id=..., location=...),
registry_skills=["shared-skill-a", "shared-skill-b"], # NEW
discovery_mode=SkillDiscoveryMode.EAGER,
)
```
Proposed semantics:
- `registry_skills: list[str] | None = None` (keyword-only, default `None`, fully backward compatible).
- The named skills are fetched lazily, once, on the first `get_tools()` / `process_llm_request()` call for the toolset (both are already `async`, and the agent module itself is usually imported synchronously, sometimes inside a running event loop such as `adk run` / `adk web`, so `asyncio.run` at import time is not an option). Fetches for the whole list run concurrently and are guarded by a lock so concurrent first requests only fetch once.
- After the fetch the skills are stored alongside the local ones, so they appear in `list_skills`, in the EAGER catalog, and `_get_or_fetch_skill` resolves them without touching the registry again for the lifetime of the process. `search_skills` keeps working for skills that are *not* pinned.
- A public `async def prefetch()` so callers that do have a startup hook (e.g. Agent Engine `AdkApp.set_up()`, which runs synchronously with no loop, or a FastAPI lifespan) can warm the toolset before the first request.
- Name collisions: a fetched registry skill whose `SKILL.md` frontmatter name already exists locally is skipped with a warning and the local skill wins. This matches the policy `SearchSkillsTool` already applies ("Skill naming conflict ... Registry skill is filtered."). Note the skill name comes from the archive's frontmatter, not from the registry resource name, so the check must happen after the fetch.
- Fetch failures are logged and leave the toolset usable (the `search_skills` path still works); the next request retries.
- `clone_with_updated_skills()` forwards `registry_skills`, so the pinned set survives cloning.
### Impact on your work
We run agents on Agent Engine that consume skills published in Agent Registry. Every conversation pays a `search_skills` turn before the skill can be loaded, and every subsequent turn re-downloads the skill archive. We currently work around this with a `SkillToolset` subclass that does the lazy prefetch described above, but it has to reach into private attributes (`_skills`, `_registry`) and is dropped by `clone_with_updated_skills()`, so we would much rather have it in the library. Not blocking, but it affects latency and token cost on every request.
### Willingness to contribute
Yes. Happy to open a PR with the parameter, the lazy prefetch, `prefetch()`, and unit tests, once the maintainers agree on the shape. (Related: #6908 / #6824 — `GCPSkillRegistry.get_skill()` currently fails on the media-download redirect, so the prefetch needs that fix to work end to end against the real Agent Registry.)
---
## 🟡 Recommended Information
### Describe Alternatives You've Considered
- **Fetch at startup and pass the result via `skills=[...]`.** Works in principle, but `get_skill()` is `async` and there is no sync counterpart, and the agent module is imported inside a running event loop by `adk run` / `adk web`, so `asyncio.run()` at module import is not portable.
- **Snapshot the registry skills into the local `skills/` directory at deploy time.** Removes the runtime dependency entirely, but couples skill updates to the agent's deploy cycle, which is the opposite of why the skills are in a registry.
- **Extend `SkillDiscoveryMode.EAGER` to include registry skills automatically.** Would require a "list all" operation on `SkillRegistry`, which the interface does not have (only `get_skill` and `search_skills`), and would inject an unbounded catalog. An explicit allow-list keeps the catalog bounded and the registry interface unchanged.
- **Cache fetched skills across invocations (key `_fetched_skill_cache` by skill name instead of `invocation_id`).** Solves the re-download cost but not the discovery turn; it would also be a useful, independent change and could be a separate PR.
### Proposed API / Implementation
Sketch of the core (names are placeholders):
```python
class SkillToolset(BaseToolset):
def __init__(self, ..., registry_skills: list[str] | None = None):
...
self._registry_skills = list(registry_skills or [])
self._registry_skills_loaded = False
self._registry_skills_lock = asyncio.Lock()
async def prefetch(self) -> None:
if self._registry_skills_loaded or not self._registry_skills:
return
async with self._registry_skills_lock:
if self._registry_skills_loaded:
return
missing = [n for n in self._registry_skills if n not in self._skills]
fetched = await asyncio.gather(
*(self._registry.get_skill(name=n) for n in missing)
)
for skill in fetched:
if skill.name in self._skills:
logger.warning("Registry skill %r collides with a local skill; keeping local", skill.name)
continue
self._skills[skill.name] = skill
self._registry_skills_loaded = True
async def get_tools(self, readonly_context=None):
await self.prefetch()
...
async def process_llm_request(self, *, tool_context, llm_request):
await self.prefetch()
...
```
### Additional Context
- `SkillDiscoveryMode` (#7092, #7093, merged as 3d3c1d90) — the local-skill half of the same problem.
- `GCPSkillRegistry.get_skill()` redirect bug (#6908, fix in #6824).
- ADK version: main @ ac0133a4 (also verified on 2.9.1 for the catalog / `search_skills` behavior).
Guida per i contributori
Apri la guida per i contributori
Valutazione
Questa issue non è ancora stata valutata.