Skip to content

feat(skills): let SkillToolset pin registry skills to appear in the catalog - #7135

Open
ArulJerald wants to merge 2 commits into
google:mainfrom
ArulJerald:feat/pinned-registry-skills-7130
Open

ArulJerald wants to merge 2 commits into
google:mainfrom
ArulJerald:feat/pinned-registry-skills-7130

Conversation

@ArulJerald

@ArulJerald ArulJerald commented Sep 16, 2026

Copy link
Copy Markdown

Link to Issue or Description of Change

1. Link to an existing issue:

  • Closes: feat(skills): let SkillToolset pin registry skills so they appear in list_skills / the catalog without a search_skills turn #7130
    Problem:
    When SkillToolset is backed by a SkillRegistry, skills in the registry are not part of the initial catalog. The model must perform a search_skills turn before it can discover and load them. For a known set of registry skills, this costs an extra turn, extra round-trips, and extra token overhead.
    Solution:
  • Added a registry_skills: list[str] | None = None parameter to SkillToolset to pin specific skills from the registry.
  • Added async def prefetch(self) -> None guarded by an asyncio.Lock with double-checked locking to fetch pinned skills concurrently into the local catalog.
  • Integrated lazy prefetch into get_tools(), process_llm_request(), _get_or_fetch_skill(), ListSkillsTool, and SearchSkillsTool.
  • Preserved registry_skills across clone_with_updated_skills().
  • Added warning and local precedence when a fetched registry skill name collides with a local skill.

Problem:
When SkillToolset is backed by a SkillRegistry, skills in the registry are not part of the initial catalog. The model must perform a search_skills turn before it can discover and load them. For a known set of registry skills, this costs an extra turn, extra round-trips, and extra token overhead.
Solution:

  • Added a registry_skills: list[str] | None = None parameter to SkillToolset to pin specific skills from the registry.
  • Added async def prefetch(self) -> None guarded by an asyncio.Lock with double-checked locking to fetch pinned skills lazily into the local catalog.
  • Integrated lazy prefetch into get_tools(), process_llm_request(), and _get_or_fetch_skill().
  • Added _registry_skill_aliases mapping to support resolution when registry resource IDs and frontmatter names differ, ensuring load_skill serves them locally without redundant registry re-downloads.
  • Updated SearchSkillsTool to filter out pinned registry skills matching either alias or frontmatter name.
  • Preserved registry_skills across clone_with_updated_skills().
  • Added collision handling and warning when a fetched registry skill name collides with a local skill.
  • Updated contributing/samples/integrations/gcp_skill_registry_agent/agent.py to demonstrate registry_skills.
    Known Limitations & Follow-ups:
  • Unbounded retry on permanent failures: If a pinned registry skill fails to load (e.g., 404 or permanent upstream issue), _registry_skills_loaded remains False and retries the fetch on subsequent requests with a warning logged. A follow-up improvement will bound retries to N attempts or distinguish transient network failures from permanent errors.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Summary of passed pytest results:

tests/unittests/tools/test_skill_toolset.py ................................... [100%] 185 passed in 5.25s

configfile: pyproject.toml
plugins: mock-3.15.1, xdist-3.8.0, asyncio-1.4.0, langsmith-0.12.4, anyio-4.15.1, hypothesis-6.168.0
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=function, asyncio_default_test_loop_scope=function
collected 189 items / 186 deselected / 3 selected

tests/unittests/tools/test_skill_toolset.py ... [100%]

====================== 3 passed, 186 deselected in 0.78s =======================

configfile: pyproject.toml
plugins: mock-3.15.1, xdist-3.8.0, asyncio-1.4.0, langsmith-0.12.4, anyio-4.15.1, hypothesis-6.168.0
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=function, asyncio_default_test_loop_scope=function
collected 189 items

tests/unittests/tools/test_skill_toolset.py ............................ [ 14%]
........................................................................ [ 52%]
........................................................................ [ 91%]
................. [100%]
============================= 189 passed in 5.40s ==============================

Checklist

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.

@google-cla

google-cla Bot commented Sep 16, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@codebee-aoki codebee-aoki left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ArulJerald, this matches the proposal in #7130 closely and the structure looks right: lazy fetch on first use, one fetch under the lock, local skills win on collision, and clone_with_updated_skills keeps the pinned list. A few points from reading the diff, roughly in priority order. The line-level ones are inline.

  1. Unbounded retry for a permanently failing skill (inline on prefetch()). Fine as a first increment, but worth stating in the PR description and tracking as a follow-up.

  2. End-to-end testing. The checklist says the change was manually tested end to end, but GCPSkillRegistry.get_skill() currently fails on the ?alt=media 302 until #6824 lands. Could you describe the setup you used? I can run this branch against a real Agent Registry catalog with the redirect fix applied and report back here, if useful.

  3. Test coverage gaps (inline on the test file): LAZY-mode list_skills output, no second get_skill call after prefetch, and the frontmatter-name-mismatch case.

  4. Docs/sample. contributing/samples/integrations/gcp_skill_registry_agent would be a natural place to show registry_skills=[...], and the registry_skills docstring could say that pinned skills appear in list_skills / the EAGER catalog and are fetched once per process (inline).

  5. Nit (inline): the prefetch() calls in ListSkillsTool.run_async and SearchSkillsTool.run_async are redundant.

From my side, 1 and 3 are the ones I'd want addressed before this lands; 2 I can help with directly.

Comment thread src/google/adk/tools/skill_toolset.py
Comment thread src/google/adk/tools/skill_toolset.py Outdated
Comment thread src/google/adk/tools/skill_toolset.py Outdated
Comment thread tests/unittests/tools/test_skill_toolset.py
@codebee-aoki

codebee-aoki commented Sep 16, 2026

Copy link
Copy Markdown

E2E result against a real Agent Registry (project-owned catalog, location global), running this branch at 4c469f8 with the redirect fix from #6824 cherry-picked on top so that get_skill() can actually download:

  • registry_skills=["private-evidence-page"], LAZY mode: the pinned skill shows up in the list_skills output, load_skill on it is served locally (4204 chars of instructions), and across two simulated invocations registry.get_skill was called exactly once. EAGER mode: the skill is in the injected <available_skills> catalog, again with a single fetch. So the core behaviour works end to end.

One finding worth acting on in this PR. In real catalogs the registry resource id and the archive's frontmatter name differ as a rule, not as an exception:

registry id (what you pin / what search_skills returns) frontmatter name (what ends up in the catalog)
private-evidence-page evidence-page
cloud.google.com-google-cloud-networking-observability google-cloud-networking-observability

Consequence: after prefetch, load_skill("private-evidence-page") (the name the model would get from search_skills) misses _skills, falls through to _get_or_fetch_skill, and re-downloads the archive (measured: 1 extra registry call per invocation). Suggest keeping a small alias map, e.g. self._registry_skill_aliases[requested_name] = skill.name, populated in prefetch() and consulted in _get_or_fetch_skill() before the registry fallback. That closes the gap without adding a second catalog entry.

(The second row is also why Google-published skills currently cannot be fetched at all: their ids contain dots, which get_skill()'s name validation and Frontmatter reject. That is independent of this PR; I'll file it separately.)

@ArulJerald

Copy link
Copy Markdown
Author

Thank you @codebee-aoki for validating this against the real catalog and identifying the name mismatch re-download issue!

Implemented your suggested alias map:

  • Added self._registry_skill_aliases: dict[str, str] = {} populated during prefetch() (self._registry_skill_aliases[name] = skill.name).
  • In _get_or_fetch_skill(), checked self._registry_skill_aliases before falling through to the registry fetch.
  • In SearchSkillsTool.run_async, filtered out registry search results matching either the stored name or self._registry_skill_aliases.
  • Added test_skill_toolset_search_skills_filters_aliased_registry_skill to assert that both the registry ID and frontmatter name are filtered from search results.

…atalog

Fetch pinned registry skills lazily via prefetch() so they appear in
list_skills and the EAGER catalog without requiring an extra search_skills
turn.

Closes: google#7130
- Add _registry_skill_aliases map to resolve pinned skills by registry ID
  when frontmatter name differs, avoiding re-downloads in load_skill.
- Filter out pinned skills in search_skills matching either alias or
  frontmatter name.
- Remove redundant prefetch calls from ListSkillsTool and SearchSkillsTool.
- Expand registry_skills docstring explaining cache lifecycle and naming.
- Demonstrate registry_skills in GCP skill registry sample agent.
- Add unit tests for LAZY list_skills, no-refetch caching, frontmatter
  name mismatches, and aliased search filtering.
@codebee-aoki

Copy link
Copy Markdown

Re-verified on d4ba96a with #6824 cherry-picked on top: 219 unit tests pass, and the E2E against the real catalog now shows zero extra registry calls when load_skill is invoked with the registry id (the alias map works). With #7138 applied as well, pinning cloud.google.com-google-cloud-networking-observability works end to end: it appears in the catalog under its frontmatter name, is served locally, and is fetched exactly once. Nothing further from my side.

@ArulJerald

Copy link
Copy Markdown
Author

Thank you @codebee-aoki for reviewing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(skills): let SkillToolset pin registry skills so they appear in list_skills / the catalog without a search_skills turn

3 participants