fix(skills): stop re-downloading pinned skills that declare no version

Review follow-ups:

- A pinned ref hit the cache and was then re-validated against SKILL.md
  frontmatter. Skills aren't required to declare `metadata.version`, so for
  those every resolution missed and re-downloaded. The cache already records the
  version it stored and `get_cached_path` matches the pin against it, so drop the
  second check there; project-local copies have no such record and still fall
  back to frontmatter
- Reject a whitespace-only version pin (`@org/name@ `) instead of forwarding it
- Check `get_skill` is callable, not merely present, before using a client
- Treat cache metadata that parses but isn't an object as a miss rather than
  raising AttributeError out of the cache lookup

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: joaomdmoura

lib/crewai/src/crewai/skills/cache.py

@@ -72,6 +72,9 @@ def _records_version(self, meta_file: Path, version: str) -> bool:
         except (OSError, json.JSONDecodeError):
             _logger.debug("Unreadable cache entry: %s", meta_file, exc_info=True)
             return False
+        if not isinstance(meta, dict):
+            _logger.debug("Malformed cache entry: %s", meta_file)
+            return False
         return versions_match(version, meta.get("version"))
 
     def store(

lib/crewai/src/crewai/skills/registry.py

@@ -67,6 +67,7 @@ def parse_skill_ref(ref: str) -> SkillRef:
     org, remainder = without_at.split("/", 1)
     # Skill names cannot contain '@', so the first one starts the version pin.
     name, separator, version = remainder.partition("@")
+    version = version.strip()
     if (
         not org
         or not name
@@ -133,20 +134,37 @@ def resolve_registry_ref(
 
     local_path = Path.cwd() / "skills" / name
     if local_path.is_dir() and (local_path / "SKILL.md").exists():
+        # A project-local copy has no installation metadata, so its frontmatter
+        # is the only thing a pin can be checked against.
         skill = _load_matching_skill(local_path, version)
         if skill is not None:
             return activate_skill(skill, source=source)
 
     cache = SkillCacheManager()
     cached_path = cache.get_cached_path(org, name, version=version)
     if cached_path is not None and (cached_path / "SKILL.md").exists():
-        skill = _load_matching_skill(cached_path, version)
+        # get_cached_path already matched the pin against the version the cache
+        # recorded when it stored the archive, which is authoritative. Checking
+        # the frontmatter as well would miss on every skill that doesn't declare
+        # metadata.version and re-download it on each resolution.
+        skill = _load_skill(cached_path)
         if skill is not None:
             return activate_skill(skill, source=source)
 
     return download_skill(org, name, source=source, version=version)
 
 
+def _load_skill(path: Path) -> Skill | None:  # type: ignore[name-defined]  # noqa: F821
+    """Load the skill at *path*, or None when it can't be read."""
+    from crewai.skills.parser import load_skill_metadata
+
+    try:
+        return load_skill_metadata(path)
+    except Exception:
+        _logger.debug("Failed to load skill at %s", path, exc_info=True)
+        return None
+
+
 def _load_matching_skill(
     path: Path,
     version: str | None,
@@ -156,15 +174,8 @@ def _load_matching_skill(
     Skills record their version in SKILL.md frontmatter under
     ``metadata.version``; a skill that declares no version can't satisfy a pin.
     """
-    from crewai.skills.parser import load_skill_metadata
-
-    try:
-        skill = load_skill_metadata(path)
-    except Exception:
-        _logger.debug("Failed to load skill at %s", path, exc_info=True)
-        return None
-
-    if version is None:
+    skill = _load_skill(path)
+    if skill is None or version is None:
         return skill
 
     declared = (skill.frontmatter.metadata or {}).get("version")
@@ -204,7 +215,7 @@ def build_default_client() -> Any:
         )
 
     client = resolve_plus_client(build_default_client)
-    if not hasattr(client, "get_skill"):
+    if not callable(getattr(client, "get_skill", None)):
         # A runtime older than the Skills Repository provides a client that
         # can't fetch skills; fall back so behavior matches a plain install.
         _logger.warning(

lib/crewai/tests/skills/test_cache.py

@@ -78,6 +78,16 @@ def test_get_cached_path_misses_a_different_version(self, tmp_path: Path) -> Non
 
         assert cache.get_cached_path("acme", "my-skill", version="2.0.0") is None
 
+    def test_get_cached_path_misses_when_metadata_is_not_an_object(
+        self, tmp_path: Path
+    ) -> None:
+        cache = SkillCacheManager(cache_root=tmp_path)
+        archive = _make_tar_gz({"SKILL.md": "---\nname: my-skill\n---\nHello"})
+        dest = cache.store("acme", "my-skill", "1.0.0", archive)
+        (dest / ".crewai_meta.json").write_text("[]", encoding="utf-8")
+
+        assert cache.get_cached_path("acme", "my-skill", version="1.0.0") is None
+
     def test_get_cached_path_misses_when_the_cached_version_is_unknown(
         self, tmp_path: Path
     ) -> None:

lib/crewai/tests/skills/test_registry.py

@@ -114,6 +114,13 @@ def test_empty_version(self) -> None:
         with pytest.raises(ValueError, match="version must be non-empty"):
             parse_skill_ref("@acme/my-skill@")
 
+    def test_whitespace_only_version(self) -> None:
+        with pytest.raises(ValueError, match="version must be non-empty"):
+            parse_skill_ref("@acme/my-skill@   ")
+
+    def test_strips_surrounding_whitespace_from_the_version(self) -> None:
+        assert parse_skill_ref("@acme/my-skill@ 1.2.0 ").version == "1.2.0"
+
     def test_round_trips_through_str(self) -> None:
         assert str(parse_skill_ref("@acme/my-skill@1.2.0")) == "@acme/my-skill@1.2.0"
         assert str(parse_skill_ref("@acme/my-skill")) == "@acme/my-skill"
@@ -247,6 +254,31 @@ def test_caches_the_pinned_version_so_it_resolves_without_a_second_download(
 
         assert api.get_skill.call_count == 1
 
+    def test_caches_a_pinned_skill_that_declares_no_version_of_its_own(
+        self, tmp_path: Path
+    ) -> None:
+        """The cache records the version it stored, so a skill whose SKILL.md
+        omits metadata.version must still resolve from the cache rather than
+        re-downloading on every resolution."""
+        cache = SkillCacheManager(cache_root=tmp_path / "cache")
+        api = MagicMock()
+        # No version in the archive's frontmatter, only in the API payload.
+        api.get_skill.return_value = _mock_skill_response("unversioned-skill")
+
+        with (
+            patch.object(Path, "cwd", return_value=tmp_path),
+            patch("crewai.auth.token.get_auth_token", return_value="saved-login"),
+            patch("crewai.skills.registry.SkillCacheManager", return_value=cache),
+            patch("crewai_core.plus_api.PlusAPI", return_value=api),
+        ):
+            from crewai.skills.registry import resolve_registry_ref
+
+            resolve_registry_ref("@acme/unversioned-skill@1.0.0")
+            skill = resolve_registry_ref("@acme/unversioned-skill@1.0.0")
+
+        assert skill.name == "unversioned-skill"
+        assert api.get_skill.call_count == 1
+
     def test_redownloads_when_the_cached_version_is_not_the_pinned_one(
         self, tmp_path: Path
     ) -> None:
@@ -407,6 +439,27 @@ def test_falls_back_when_the_installed_client_cannot_fetch_skills(
         assert skill.name == "my-skill"
         plus_api.assert_called_once_with(api_key="user-pat", organization_id=None)
 
+    def test_falls_back_when_get_skill_is_not_callable(
+        self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        cache = SkillCacheManager(cache_root=tmp_path / "cache")
+        api = MagicMock()
+        api.get_skill.return_value = _mock_skill_response("my-skill")
+        monkeypatch.setenv("CREWAI_USER_PAT", "user-pat")
+
+        odd_client = MagicMock(spec=["get_skill"])
+        odd_client.get_skill = "not-callable"
+        set_plus_client_factory(lambda: odd_client)
+
+        with (
+            patch("crewai.skills.registry.SkillCacheManager", return_value=cache),
+            patch("crewai_core.plus_api.PlusAPI", return_value=api) as plus_api,
+        ):
+            skill = download_skill("acme", "my-skill")
+
+        assert skill.name == "my-skill"
+        plus_api.assert_called_once_with(api_key="user-pat", organization_id=None)
+
     def test_reports_the_pinned_ref_when_the_download_fails(
         self, tmp_path: Path
     ) -> None: