Skip to content

Commit b03b3f8

Browse files
joaomdmouraclaude
andcommitted
fix(skills): address review comments on the promotion PR
- Back-compat shim now aliases the old submodules in sys.modules so `crewai.experimental.skills.registry/cache/events` imports (and patch targets) resolve to the real crewai.skills modules, not just the package-root re-exports. - `crewai skill publish` actually enforces the git-state check that --force claims to skip: unsynced repos block publishing (mirroring tool publish); standalone skill dirs outside any git repo publish without a check. - Explicit UTF-8 encoding on SKILL.md and cache-metadata reads/writes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 8c7346f commit b03b3f8

5 files changed

Lines changed: 122 additions & 9 deletions

File tree

lib/cli/src/crewai_cli/skills/main.py

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
from rich.console import Console
1414
from rich.table import Table
1515

16+
from crewai_cli import git
1617
from crewai_cli.command import BaseCommand, PlusAPIMixin
1718
from crewai_cli.config import Settings
1819
from crewai_cli.constants import DEFAULT_CREWAI_ENTERPRISE_URL
@@ -63,7 +64,7 @@ def create(self, name: str, in_project: bool = True) -> None:
6364
(skill_dir / "assets").mkdir()
6465

6566
skill_md = skill_dir / "SKILL.md"
66-
skill_md.write_text(_SKILL_MD_TEMPLATE.format(name=name))
67+
skill_md.write_text(_SKILL_MD_TEMPLATE.format(name=name), encoding="utf-8")
6768

6869
console.print(
6970
f"[green]Created skill [bold]{name}[/bold] at [bold]{skill_dir}[/bold].[/green]"
@@ -170,7 +171,9 @@ def install(self, ref: str) -> None:
170171
"version": version,
171172
"installed_at": datetime.now(tz=timezone.utc).isoformat(),
172173
}
173-
(cache_dir / ".crewai_meta.json").write_text(json.dumps(meta, indent=2))
174+
(cache_dir / ".crewai_meta.json").write_text(
175+
json.dumps(meta, indent=2), encoding="utf-8"
176+
)
174177
console.print(
175178
f"[green]Installed [bold]{ref}[/bold]{' (' + version + ')' if version else ''} to global cache.[/green]"
176179
)
@@ -189,8 +192,27 @@ def publish(self, org: str | None = None, force: bool = False) -> None:
189192
)
190193
raise SystemExit(1)
191194

195+
if not force:
196+
try:
197+
repository = git.Repository()
198+
except ValueError:
199+
# Standalone skill directories may live outside any git repo;
200+
# there is no git state to validate in that case.
201+
repository = None
202+
if repository is not None and not repository.is_synced():
203+
console.print(
204+
"[bold red]Failed to publish skill.[/bold red]\n"
205+
"Local changes need to be resolved before publishing. Please do the following:\n"
206+
"* [bold]Commit[/bold] your changes.\n"
207+
"* [bold]Push[/bold] to sync with the remote.\n"
208+
"* [bold]Pull[/bold] the latest changes from the remote.\n"
209+
"\nOnce your repository is up-to-date, retry publishing the skill "
210+
"(or pass --force to skip this check)."
211+
)
212+
raise SystemExit(1)
213+
192214
try:
193-
frontmatter = self._parse_frontmatter(skill_md.read_text())
215+
frontmatter = self._parse_frontmatter(skill_md.read_text(encoding="utf-8"))
194216
except ValueError as exc:
195217
console.print(f"[red]Failed to parse SKILL.md frontmatter: {exc}[/red]")
196218
raise SystemExit(1) from exc
@@ -281,7 +303,7 @@ def list_cached(self) -> None:
281303
meta_file = skill_dir / ".crewai_meta.json"
282304
if meta_file.exists():
283305
try:
284-
meta = json.loads(meta_file.read_text())
306+
meta = json.loads(meta_file.read_text(encoding="utf-8"))
285307
table.add_row(
286308
"cache",
287309
f"@{meta['org']}/{meta['name']}",
@@ -372,7 +394,7 @@ def _parse_frontmatter(self, content: str) -> dict[str, str]:
372394
def _read_version(self, skill_md: Path) -> str | None:
373395
"""Read the version from a SKILL.md file's metadata, or None."""
374396
try:
375-
fm = self._parse_frontmatter(skill_md.read_text())
397+
fm = self._parse_frontmatter(skill_md.read_text(encoding="utf-8"))
376398
raw_metadata = fm.get("metadata")
377399
if isinstance(raw_metadata, dict):
378400
return raw_metadata.get("version")

lib/cli/tests/skills/test_main.py

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,60 @@ def test_publish_calls_api(self, skill_command):
171171
# Skills are always org-scoped; there is no public visibility.
172172
assert call_kwargs.kwargs["is_public"] is False
173173

174+
def test_publish_blocked_when_git_not_synced(self, skill_command):
175+
with in_temp_dir():
176+
Path("SKILL.md").write_text(
177+
"---\nname: my-skill\ndescription: A test skill.\nmetadata:\n version: 1.0.0\n---\nInstructions."
178+
)
179+
skill_command.plus_api_client.publish_skill = MagicMock()
180+
with patch("crewai_cli.skills.main.git.Repository") as mock_repo_cls:
181+
mock_repo_cls.return_value.is_synced.return_value = False
182+
with pytest.raises(SystemExit):
183+
skill_command.publish(org="acme")
184+
skill_command.plus_api_client.publish_skill.assert_not_called()
185+
186+
def test_publish_force_skips_git_check(self, skill_command):
187+
with in_temp_dir():
188+
Path("SKILL.md").write_text(
189+
"---\nname: my-skill\ndescription: A test skill.\nmetadata:\n version: 1.0.0\n---\nInstructions."
190+
)
191+
mock_resp = MagicMock()
192+
mock_resp.is_success = True
193+
mock_resp.status_code = 200
194+
mock_resp.json.return_value = {}
195+
skill_command.plus_api_client.publish_skill = MagicMock(return_value=mock_resp)
196+
with (
197+
patch("crewai_cli.skills.main.git.Repository") as mock_repo_cls,
198+
patch("crewai_cli.skills.main.Settings") as mock_settings_cls,
199+
):
200+
mock_settings_cls.return_value.org_name = "acme"
201+
mock_settings_cls.return_value.enterprise_base_url = None
202+
skill_command.publish(org="acme", force=True)
203+
mock_repo_cls.assert_not_called()
204+
skill_command.plus_api_client.publish_skill.assert_called_once()
205+
206+
def test_publish_proceeds_outside_git_repo(self, skill_command):
207+
with in_temp_dir():
208+
Path("SKILL.md").write_text(
209+
"---\nname: my-skill\ndescription: A test skill.\nmetadata:\n version: 1.0.0\n---\nInstructions."
210+
)
211+
mock_resp = MagicMock()
212+
mock_resp.is_success = True
213+
mock_resp.status_code = 200
214+
mock_resp.json.return_value = {}
215+
skill_command.plus_api_client.publish_skill = MagicMock(return_value=mock_resp)
216+
with (
217+
patch(
218+
"crewai_cli.skills.main.git.Repository",
219+
side_effect=ValueError("not a Git repository"),
220+
),
221+
patch("crewai_cli.skills.main.Settings") as mock_settings_cls,
222+
):
223+
mock_settings_cls.return_value.org_name = "acme"
224+
mock_settings_cls.return_value.enterprise_base_url = None
225+
skill_command.publish(org="acme")
226+
skill_command.plus_api_client.publish_skill.assert_called_once()
227+
174228

175229
class TestSkillListCached:
176230
def test_list_cached_empty(self, skill_command, capsys):

lib/crewai/src/crewai/experimental/skills/__init__.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,14 @@
22
33
The registry-backed skills feature graduated out of experimental; it now
44
lives in `crewai.skills` alongside the filesystem-based loader. This module
5-
re-exports the public names so imports written against the experimental
6-
namespace keep working, and will be removed in a future release.
5+
re-exports the public names and aliases the old submodules so imports
6+
written against the experimental namespace keep working, and will be
7+
removed in a future release.
78
"""
89

10+
import sys
11+
12+
from crewai.skills import cache, events, registry
913
from crewai.skills.cache import SkillCacheManager
1014
from crewai.skills.registry import (
1115
SkillNotCachedError,
@@ -15,10 +19,19 @@
1519
)
1620

1721

22+
# Keep `crewai.experimental.skills.<module>` imports (and patch targets)
23+
# resolving to the real modules in crewai.skills.
24+
sys.modules[__name__ + ".cache"] = cache
25+
sys.modules[__name__ + ".events"] = events
26+
sys.modules[__name__ + ".registry"] = registry
27+
1828
__all__ = [
1929
"SkillCacheManager",
2030
"SkillNotCachedError",
31+
"cache",
32+
"events",
2133
"is_registry_ref",
2234
"parse_registry_ref",
35+
"registry",
2336
"resolve_registry_ref",
2437
]

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,9 @@ def store(
8888
"version": version,
8989
"installed_at": datetime.now(tz=timezone.utc).isoformat(),
9090
}
91-
(skill_dir / _META_FILENAME).write_text(json.dumps(meta, indent=2))
91+
(skill_dir / _META_FILENAME).write_text(
92+
json.dumps(meta, indent=2), encoding="utf-8"
93+
)
9294
return skill_dir
9395

9496
def list_cached(self) -> list[SkillMetadata]:
@@ -103,7 +105,9 @@ def list_cached(self) -> list[SkillMetadata]:
103105
meta_file = skill_dir / _META_FILENAME
104106
if meta_file.exists():
105107
try:
106-
results.append(json.loads(meta_file.read_text()))
108+
results.append(
109+
json.loads(meta_file.read_text(encoding="utf-8"))
110+
)
107111
except (json.JSONDecodeError, KeyError):
108112
_logger.debug(
109113
"Skipping malformed cache entry: %s",

lib/crewai/tests/skills/test_experimental_shim.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,23 @@ def test_experimental_namespace_reexports_public_names():
1616
assert experimental_skills.is_registry_ref is is_registry_ref
1717
assert experimental_skills.parse_registry_ref is parse_registry_ref
1818
assert experimental_skills.resolve_registry_ref is resolve_registry_ref
19+
20+
21+
def test_experimental_submodule_imports_alias_real_modules():
22+
"""Old submodule import style must resolve to the crewai.skills modules."""
23+
from crewai.experimental.skills import cache as shim_cache
24+
from crewai.experimental.skills import events as shim_events
25+
from crewai.experimental.skills import registry as shim_registry
26+
import crewai.experimental.skills.registry as shim_registry_direct
27+
from crewai.experimental.skills.cache import SkillCacheManager
28+
from crewai.experimental.skills.registry import resolve_registry_ref
29+
import crewai.skills.cache
30+
import crewai.skills.events
31+
import crewai.skills.registry
32+
33+
assert shim_cache is crewai.skills.cache
34+
assert shim_events is crewai.skills.events
35+
assert shim_registry is crewai.skills.registry
36+
assert shim_registry_direct is crewai.skills.registry
37+
assert SkillCacheManager is crewai.skills.cache.SkillCacheManager
38+
assert resolve_registry_ref is crewai.skills.registry.resolve_registry_ref

0 commit comments

Comments
 (0)