From 7fe121e31da5bc609a602eaa98b9500c19658061 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Wed, 16 Sep 2026 10:03:45 +0000 Subject: [PATCH 1/2] fix(skills): accept dotted registry ids in GCPSkillRegistry Google-published skills have registry resource ids like cloud.google.com-, which are not SKILL.md frontmatter names and were never meant to be held to the frontmatter naming rule. get_skill rejected every such id outright, and search_skills silently dropped every matching catalog entry, making Google-published skills unreachable from ADK. Give registry ids their own safe-path-segment check (still rejecting traversal, slashes, and other unsafe characters) instead of routing them through Frontmatter's kebab/snake-case name validator, which is scoped to SKILL.md content. Fixes #7136 --- .../skill_registry/gcp_skill_registry.py | 56 ++++++++++++++----- .../skill_registry/test_gcp_skill_registry.py | 49 +++++++++++++++- 2 files changed, 87 insertions(+), 18 deletions(-) diff --git a/src/google/adk/integrations/skill_registry/gcp_skill_registry.py b/src/google/adk/integrations/skill_registry/gcp_skill_registry.py index a129cb427c4..229ee7b5d2f 100644 --- a/src/google/adk/integrations/skill_registry/gcp_skill_registry.py +++ b/src/google/adk/integrations/skill_registry/gcp_skill_registry.py @@ -19,6 +19,7 @@ import asyncio import logging import os +import re import ssl import tempfile from typing import Any @@ -36,10 +37,22 @@ from google.auth.transport import mtls from google.auth.transport import requests as auth_requests import httpx -from pydantic import ValidationError logger = logging.getLogger("google_adk." + __name__) +# Registry resource ids (e.g. "cloud.google.com-agent-platform-eval-flywheel" +# for Google-published skills) are a different namespace from SKILL.md +# frontmatter names: they are not required to be kebab/snake-case and may +# contain dots. They still need to be safe to interpolate as a single URL +# path segment, so they get their own, more permissive check instead of +# reusing the SKILL.md content naming rule. +_SAFE_REGISTRY_ID_PATTERN = re.compile(r"^[a-z0-9]+(?:[._-][a-z0-9]+)*$") + + +def _is_safe_registry_id(name: str) -> bool: + """True if `name` is safe to use as a single skill-registry path segment.""" + return len(name) <= 64 and bool(_SAFE_REGISTRY_ID_PATTERN.match(name)) + class GCPSkillRegistry(SkillRegistry): """GCP implementation of SkillRegistry using GCP Skill Registry API.""" @@ -175,15 +188,16 @@ async def get_skill(self, *, name: str) -> models.Skill: ValueError: If the name is not a valid skill name. """ # The name reaches here straight from a model-issued tool call, so it must - # be a single path segment before it is interpolated into the request URL. - # Accept the same character set skill names are already held to; the - # snake-or-kebab pattern is the superset of the two accepted spellings. - # pylint: disable-next=protected-access - if not models._SNAKE_OR_KEBAB_NAME_PATTERN.match(name): + # be a single, safe path segment before it is interpolated into the + # request URL. This is a registry resource id, not a SKILL.md frontmatter + # name, so it is held to its own safe-path-segment rule rather than the + # stricter kebab/snake-case naming rule SKILL.md content is held to. + if not _is_safe_registry_id(name): raise ValueError( - f"Invalid skill name {name!r}: name must be lowercase kebab-case" - " (a-z, 0-9, hyphens) or snake_case (a-z, 0-9, underscores), with" - " no leading, trailing, or consecutive delimiters." + f"Invalid skill name {name!r}: name must be a single safe path" + " segment of at most 64 characters (lowercase letters, digits," + " and non-consecutive '.', '_', '-' separators), with no leading," + " trailing, or consecutive delimiters." ) async with self._create_httpx_client() as client: @@ -246,18 +260,30 @@ async def search_skills(self, *, query: str) -> list[models.Frontmatter]: # fails validation below and takes the skip path. raw_name = s.get("name") name = raw_name.split("/")[-1] if isinstance(raw_name, str) else "" + # A registry id is not a SKILL.md frontmatter name (see get_skill), + # so it is checked against the safe-path-segment rule instead of + # Frontmatter's stricter kebab/snake-case name validator. + if not _is_safe_registry_id(name): + logger.warning( + "Skipping search result %r: not a safe registry id.", name + ) + continue try: - results.append( - models.Frontmatter( - name=name, - description=s.get("description", "") or "", - ) + # pylint: disable-next=protected-access + description = models.Frontmatter._validate_description( + s.get("description", "") or "" ) - except ValidationError as e: + except ValueError as e: logger.warning( "Skipping search result %r: it does not pass frontmatter" " validation: %s", name, e, ) + continue + results.append( + models.Frontmatter.model_construct( + name=name, description=description + ) + ) return results diff --git a/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py b/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py index 62c9f46e963..3de7f343db4 100644 --- a/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py +++ b/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py @@ -188,8 +188,9 @@ async def test_search_skills_success(): @pytest.mark.parametrize( "bad_name, bad_description", [ - # A real first-party catalog entry: dots are outside the name pattern. - ("cloud.google.com-agent-platform-eval-flywheel", "Description bad"), + # A bare traversal segment must still be rejected even though '.' is + # otherwise an allowed registry-id character. + ("..", "Description bad"), ("Skill-With-Caps", "Description bad"), ("a" * 65, "Description bad"), ("skill-no-description", ""), @@ -237,6 +238,37 @@ async def test_search_skills_skips_entry_failing_validation( assert bad_name in caplog.text +@pytest.mark.asyncio +async def test_search_skills_accepts_dotted_registry_id(): + """A Google-published registry id with dots must not be dropped. + + Regression test for https://github.com/google/adk-python/issues/7136: + ids like "cloud.google.com-" are registry resource ids, not + SKILL.md frontmatter names, so they must not be checked against the + stricter kebab/snake-case frontmatter naming rule. + """ + registry = gcp_skill_registry.GCPSkillRegistry() + + mock_response = mock.MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = { + "skills": [{ + "name": ( + "projects/test-project/locations/global/skills/" + "cloud.google.com-agent-platform-eval-flywheel" + ), + "description": "A Google-published skill.", + }] + } + + with mock.patch("httpx.AsyncClient.get", return_value=mock_response): + results = await registry.search_skills(query="query") + + assert len(results) == 1 + assert results[0].name == "cloud.google.com-agent-platform-eval-flywheel" + assert results[0].description == "A Google-published skill." + + @pytest.mark.parametrize("raw_name", [None, 7, ["a"]]) @pytest.mark.asyncio async def test_search_skills_skips_entry_whose_name_is_not_a_string( @@ -388,6 +420,9 @@ async def mock_get(url, *unused_args, **kwargs): "my-skill/revisions/rev-123", "My-Skill", "", + ".", + "..", + "a" * 65, ], ) @pytest.mark.asyncio @@ -402,7 +437,15 @@ async def test_get_skill_rejects_unsafe_name_before_any_request(unsafe_name): mock_get_called.assert_not_called() -@pytest.mark.parametrize("valid_name", ["my-skill", "my_skill", "skill2"]) +@pytest.mark.parametrize( + "valid_name", + [ + "my-skill", + "my_skill", + "skill2", + "cloud.google.com-agent-platform-eval-flywheel", + ], +) @pytest.mark.asyncio async def test_get_skill_builds_expected_url_for_valid_name(valid_name): """Verifies that a valid name is still interpolated verbatim into the URL.""" From 314172eed22dfc8eb039319548e574f9e5819503 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Thu, 17 Sep 2026 08:26:02 +0000 Subject: [PATCH 2/2] fix(skills): raise registry id length cap to 256 The 64-char cap on GCPSkillRegistry's registry-id check rejected real Agent Registry catalog ids (e.g. an 80-char and a 65-char Google-published id), since that limit came from the SKILL.md frontmatter naming rule, not from anything about URL-segment safety. Raise the cap to 256 and cover the two real catalog ids as regression tests. --- .../adk/integrations/skill_registry/gcp_skill_registry.py | 4 ++-- .../integrations/skill_registry/test_gcp_skill_registry.py | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/google/adk/integrations/skill_registry/gcp_skill_registry.py b/src/google/adk/integrations/skill_registry/gcp_skill_registry.py index 229ee7b5d2f..4c63e366b24 100644 --- a/src/google/adk/integrations/skill_registry/gcp_skill_registry.py +++ b/src/google/adk/integrations/skill_registry/gcp_skill_registry.py @@ -51,7 +51,7 @@ def _is_safe_registry_id(name: str) -> bool: """True if `name` is safe to use as a single skill-registry path segment.""" - return len(name) <= 64 and bool(_SAFE_REGISTRY_ID_PATTERN.match(name)) + return len(name) <= 256 and bool(_SAFE_REGISTRY_ID_PATTERN.match(name)) class GCPSkillRegistry(SkillRegistry): @@ -195,7 +195,7 @@ async def get_skill(self, *, name: str) -> models.Skill: if not _is_safe_registry_id(name): raise ValueError( f"Invalid skill name {name!r}: name must be a single safe path" - " segment of at most 64 characters (lowercase letters, digits," + " segment of at most 256 characters (lowercase letters, digits," " and non-consecutive '.', '_', '-' separators), with no leading," " trailing, or consecutive delimiters." ) diff --git a/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py b/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py index 3de7f343db4..d1b1989f3a0 100644 --- a/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py +++ b/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py @@ -192,7 +192,7 @@ async def test_search_skills_success(): # otherwise an allowed registry-id character. ("..", "Description bad"), ("Skill-With-Caps", "Description bad"), - ("a" * 65, "Description bad"), + ("a" * 257, "Description bad"), ("skill-no-description", ""), ], ) @@ -422,7 +422,7 @@ async def mock_get(url, *unused_args, **kwargs): "", ".", "..", - "a" * 65, + "a" * 257, ], ) @pytest.mark.asyncio @@ -444,6 +444,9 @@ async def test_get_skill_rejects_unsafe_name_before_any_request(unsafe_name): "my_skill", "skill2", "cloud.google.com-agent-platform-eval-flywheel", + # Real catalog ids longer than the old 64-char cap (80 and 65 chars). + "cloud.google.com-google-cloud-solution-agentic-analytics-spark-knowledge-catalog", + "cloud.google.com-gke-ai-troubleshooting-handle-disruption-gpu-tpu", ], ) @pytest.mark.asyncio