Skip to content

fix(skills): accept a skill.md manifest in GCS skill directories - #7843

Open
adityasingh2400 wants to merge 1 commit into
google:mainfrom
adityasingh2400:fix/gcs-skills-lowercase-manifest
Open

adityasingh2400 wants to merge 1 commit into
google:mainfrom
adityasingh2400:fix/gcs-skills-lowercase-manifest

Conversation

@adityasingh2400

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 (if applicable):

Problem:
The local directory loader (_parse_skill_md, _list_skills_in_dir) and the zip loader accept a skill manifest named SKILL.md or skill.md. _list_skills_in_gcs_dir and _load_skill_from_gcs_dir only looked for SKILL.md. A skill that loads fine locally therefore disappeared from the GCS listing, and loading it raised FileNotFoundError once it was stored in a bucket. The async wrappers delegate to these two functions, so they had the same gap.

Solution:
Add _find_gcs_skill_manifest, which checks SKILL.md and then skill.md (the order the other loaders use), and use it in both GCS functions. When SKILL.md exists it is the only blob looked up, exactly as before. The not-found error is unchanged.

Testing Plan

Unit Tests:

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

Added to tests/unittests/skills/test__utils.py, with the same mocked google.cloud.storage client the existing GCS tests use:

  • test__list_skills_in_gcs_dir_accepts_lowercase_manifest
  • test__load_skill_from_gcs_dir_accepts_lowercase_manifest: also asserts that SKILL.md is tried first.
# On main (before the fix)
AssertionError: assert [] == ['my-skill']
FileNotFoundError: SKILL.md not found at gs://my-bucket/skills/my-skill/SKILL.md

# With the fix
$ pytest tests/unittests/skills
133 passed

pre-commit run passes on the changed files, and mypy reports no issues in skills/_utils.py.

Manual End-to-End (E2E) Tests:

I don't have a GCS bucket for a live run. The tests drive both GCS functions through the mocked storage client and check which blobs are looked up.

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

#7760 changes _list_skills_in_dir in the same file. This PR only touches the GCS functions, which are further down, so the two don't conflict.

The local directory and zip loaders accept SKILL.md or skill.md, but
_list_skills_in_gcs_dir and _load_skill_from_gcs_dir only looked for
SKILL.md, so a skill that loads locally vanished from the listing and
failed to load once stored in a bucket. Look up the manifest under both
names, in the same order the other loaders use.

Fixes google#7842
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.

GCS skill loaders ignore a skill.md manifest that the local and zip loaders accept

2 participants