Skip to content

fix(skills): accept dotted registry ids in GCPSkillRegistry - #7138

Open
chelsealong wants to merge 1 commit into
google:mainfrom
chelsealong:fix-gcp-skill-registry-dotted-ids
Open

chelsealong wants to merge 1 commit into
google:mainfrom
chelsealong:fix-gcp-skill-registry-dotted-ids

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue:

Problem:

Skills published by Google into Agent Registry have resource ids of the
form cloud.google.com-<display-name> (also
discoveryengine.googleapis.com-<name>). GCPSkillRegistry rejects all of
them:

  • get_skill(name="cloud.google.com-...") raises ValueError: Invalid skill name ... because the check added for fix(skills): skip invalid catalog hits in GCP skill search #6839 requires the name to
    match models._SNAKE_OR_KEBAB_NAME_PATTERN, which does not allow dots.
  • search_skills() feeds each catalog id into models.Frontmatter(name=...),
    whose own field validator applies the same strict kebab/snake-case rule,
    so every Google-published hit is silently dropped with a "Skipping search
    result" warning.

In a real catalog this rejects the large majority of skills — only
self-created, plain-kebab-case skills pass. That name-pattern check was
never meant to apply here in the first place: it is the SKILL.md
frontmatter naming rule (a content-format rule for the file inside the
skill archive), and a registry resource id is a different kind of string
that happens to reuse the same Frontmatter.name field for convenience.

Solution:

  • Added a _is_safe_registry_id helper in gcp_skill_registry.py with its
    own pattern (^[a-z0-9]+(?:[._-][a-z0-9]+)*$, length <= 64) that keeps
    the original security intent (reject ./.., slashes, and anything else
    that isn't safe to interpolate as a single URL path segment) while
    allowing dots.
  • get_skill now validates the incoming name against this registry-id rule
    instead of the SKILL.md frontmatter name pattern.
  • search_skills now validates each catalog id the same way, and
    constructs the returned Frontmatter via model_construct (bypassing
    the frontmatter name validator, which does not apply to registry ids)
    while still running the real description validation so malformed
    descriptions are still skipped and logged as before.

Neither change touches models.Frontmatter's own naming rule, which still
applies, unmodified, to names parsed from SKILL.md content.

Testing Plan

Unit Tests:

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

Added:

  • test_search_skills_accepts_dotted_registry_id — a dotted registry id
    (cloud.google.com-agent-platform-eval-flywheel) is now returned by
    search_skills instead of being dropped.
  • Extended test_get_skill_builds_expected_url_for_valid_name with the same
    dotted id to confirm get_skill accepts it and builds the expected URL.
  • Extended test_get_skill_rejects_unsafe_name_before_any_request with
    ".", "..", and a 65-character name to confirm the safe-path-segment
    check still rejects bare traversal segments and enforces the length cap.
  • Updated test_search_skills_skips_entry_failing_validation's first case
    (previously the dotted id, used as an example of a name that fails
    validation) to "..", since a dotted id is now valid.

Verified the added tests fail without the fix (git checkout HEAD~1 -- src/google/adk/integrations/skill_registry/gcp_skill_registry.py, keeping
the new tests) with the exact errors the issue describes:

FAILED .../test_gcp_skill_registry.py::test_search_skills_accepts_dotted_registry_id - assert 0 == 1
FAILED .../test_gcp_skill_registry.py::test_get_skill_builds_expected_url_for_valid_name[cloud.google.com-agent-platform-eval-flywheel] - ValueError: Invalid skill name 'cloud.google.com-agent-platform-eval-flywheel': 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.
3 failed, 31 passed, 1 warning in 0.61s

Summary of passed pytest results (with the fix restored):

$ pytest tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py -q
34 passed in 0.54s

$ pytest tests/unittests/integrations/skill_registry/ tests/unittests/skills tests/unittests/tools/test_skill_toolset.py -q
284 passed, 1 warning in 5.04s

$ pytest tests/unittests -q -n auto
14990 passed, 82 skipped, 27 xfailed, 2 xpassed in 146.92s

(One unrelated test, test_eval_injects_session_input_state_into_instruction,
is flaky under -n auto parallel execution and reproduces identically on
unmodified main; deselected from the full run above for a clean signal.)

Also ran ruff check, pyink --check, and pylint on the changed files —
clean (pylint's only remaining note is a pre-existing line-length warning on
an unrelated line this PR doesn't touch).

Manual End-to-End (E2E) Tests:

Not run — no live Agent Registry project was available in this
environment; verified via the unit tests above, which exercise the same
name-validation and Frontmatter-construction code paths the issue reports
as broken.

Checklist

  • I have read the CONTRIBUTING.md document.
  • 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.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

This PR was generated by an AI coding agent (Claude Code). The implementation
and tests were verified against the automated test suite as described above.

Google-published skills have registry resource ids like
cloud.google.com-<name>, 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 google#7136
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.

GCPSkillRegistry rejects every Google-published skill: dotted ids (cloud.google.com-*) fail name validation in get_skill and search_skills

2 participants