feat(skills): let SkillToolset pin registry skills to appear in the catalog - #7135
ArulJerald wants to merge 2 commits into
Conversation
|
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. |
7f05adc to
4c469f8
Compare
codebee-aoki
left a comment
There was a problem hiding this comment.
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.
-
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. -
End-to-end testing. The checklist says the change was manually tested end to end, but
GCPSkillRegistry.get_skill()currently fails on the?alt=media302 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. -
Test coverage gaps (inline on the test file): LAZY-mode
list_skillsoutput, no secondget_skillcall after prefetch, and the frontmatter-name-mismatch case. -
Docs/sample.
contributing/samples/integrations/gcp_skill_registry_agentwould be a natural place to showregistry_skills=[...], and theregistry_skillsdocstring could say that pinned skills appear inlist_skills/ the EAGER catalog and are fetched once per process (inline). -
Nit (inline): the
prefetch()calls inListSkillsTool.run_asyncandSearchSkillsTool.run_asyncare redundant.
From my side, 1 and 3 are the ones I'd want addressed before this lands; 2 I can help with directly.
|
E2E result against a real Agent Registry (project-owned catalog, location
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:
Consequence: after prefetch, (The second row is also why Google-published skills currently cannot be fetched at all: their ids contain dots, which |
|
Thank you @codebee-aoki for validating this against the real catalog and identifying the name mismatch re-download issue! Implemented your suggested alias map:
|
…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.
df9ac26 to
d4ba96a
Compare
|
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 |
Link to Issue or Description of Change
1. Link to an existing issue:
Problem:
When
SkillToolsetis backed by aSkillRegistry, skills in the registry are not part of the initial catalog. The model must perform asearch_skillsturn 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:
registry_skills: list[str] | None = Noneparameter toSkillToolsetto pin specific skills from the registry.async def prefetch(self) -> Noneguarded by anasyncio.Lockwith double-checked locking to fetch pinned skills concurrently into the local catalog.get_tools(),process_llm_request(),_get_or_fetch_skill(),ListSkillsTool, andSearchSkillsTool.registry_skillsacrossclone_with_updated_skills().Problem:
When
SkillToolsetis backed by aSkillRegistry, skills in the registry are not part of the initial catalog. The model must perform asearch_skillsturn 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:
registry_skills: list[str] | None = Noneparameter toSkillToolsetto pin specific skills from the registry.async def prefetch(self) -> Noneguarded by anasyncio.Lockwith double-checked locking to fetch pinned skills lazily into the local catalog.get_tools(),process_llm_request(), and_get_or_fetch_skill()._registry_skill_aliasesmapping to support resolution when registry resource IDs and frontmatter names differ, ensuringload_skillserves them locally without redundant registry re-downloads.SearchSkillsToolto filter out pinned registry skills matching either alias or frontmatter name.registry_skillsacrossclone_with_updated_skills().contributing/samples/integrations/gcp_skill_registry_agent/agent.pyto demonstrateregistry_skills.Known Limitations & Follow-ups:
_registry_skills_loadedremainsFalseand 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:
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