refactor: unify skill source model

This commit is contained in:
2026-07-30 12:11:25 +08:00
parent 076d87e303
commit 7d1994cf93
18 changed files with 669 additions and 527 deletions
@@ -9,7 +9,8 @@ import unittest
from pathlib import Path
from unittest.mock import patch
from skiff.registry import external_checkout_path, registry_repo
from skiff import catalog
from skiff.catalog import catalog_checkout_path, catalog_repo
REPO_ROOT = Path(__file__).resolve().parents[1]
@@ -26,7 +27,16 @@ def write_skill(root: Path, name: str) -> Path:
return skill
class RegistryCollectionTests(unittest.TestCase):
class CatalogCollectionTests(unittest.TestCase):
def test_load_catalog_falls_back_to_legacy_registry_file(self) -> None:
legacy = self.home / "legacy-registry.yaml"
legacy.write_text("legacy:\n repo: https://example.test/legacy.git\n")
with (
patch.object(catalog, "CATALOG_FILE", self.home / "missing-catalog.yaml"),
patch.object(catalog, "LEGACY_REGISTRY_FILE", legacy),
):
self.assertIn("legacy", catalog.load_catalog())
def setUp(self) -> None:
self.temp_dir = tempfile.TemporaryDirectory()
self.home = Path(self.temp_dir.name)
@@ -58,7 +68,7 @@ class RegistryCollectionTests(unittest.TestCase):
check=True,
capture_output=True,
)
self.skills_home.joinpath("registry.yaml").write_text(
self.skills_home.joinpath("catalog.yaml").write_text(
"test-pack:\n"
f" repo: {self.upstream}\n"
" ref: main\n"
@@ -82,14 +92,14 @@ class RegistryCollectionTests(unittest.TestCase):
check=False,
)
def test_registry_repo_expands_home_relative_local_path(self) -> None:
def test_catalog_repo_expands_home_relative_local_path(self) -> None:
with patch.dict(os.environ, {"HOME": str(self.home)}):
self.assertEqual(
registry_repo({"repo": "~/.skills"}),
catalog_repo({"repo": "~/.skills"}),
str(self.skills_home),
)
self.assertEqual(
external_checkout_path("skills", {"repo": "~/.skills"}),
catalog_checkout_path("skills", {"repo": "~/.skills"}),
self.skills_home.resolve(),
)
@@ -113,7 +123,23 @@ class RegistryCollectionTests(unittest.TestCase):
self.assertTrue((skill_dir / "second-skill").is_symlink())
self.assertTrue((skill_dir / "second-skill" / "SKILL.md").is_file())
def test_add_collection_with_registry_source_installs_all(self) -> None:
def test_add_collection_with_catalog_source_installs_all(self) -> None:
result = self.run_skiff(
"add",
"test-pack",
"--source",
"catalog",
"-g",
"-a",
"codex",
)
self.assertEqual(result.returncode, 0, result.stderr)
skill_dir = self.home / ".codex" / "skills"
self.assertTrue((skill_dir / "first-skill").is_symlink())
self.assertTrue((skill_dir / "second-skill").is_symlink())
def test_legacy_registry_source_alias_still_installs(self) -> None:
result = self.run_skiff(
"add",
"test-pack",
@@ -125,9 +151,9 @@ class RegistryCollectionTests(unittest.TestCase):
)
self.assertEqual(result.returncode, 0, result.stderr)
skill_dir = self.home / ".codex" / "skills"
self.assertTrue((skill_dir / "first-skill").is_symlink())
self.assertTrue((skill_dir / "second-skill").is_symlink())
self.assertTrue(
(self.home / ".codex" / "skills" / "first-skill").is_symlink()
)
def test_project_add_records_collection_for_sync(self) -> None:
project = self.home / "project"
@@ -153,7 +179,7 @@ class RegistryCollectionTests(unittest.TestCase):
)
self.assertEqual(added.returncode, 0, added.stderr)
self.assertIn('registry: "test-pack"', project.joinpath(".skills.yaml").read_text())
self.assertIn('source: "catalog:test-pack"', project.joinpath(".skills.yaml").read_text())
self.assertEqual(synced.returncode, 0, synced.stderr)
self.assertTrue(link.is_symlink())
self.assertTrue((link / "SKILL.md").is_file())
@@ -184,9 +210,9 @@ class RegistryCollectionTests(unittest.TestCase):
self.assertEqual(installed.returncode, 0, installed.stderr)
self.assertEqual(status.returncode, 0, status.stderr)
self.assertIn("[registry:test-pack] first-skill", status.stdout)
self.assertIn("[registry:test-pack] second-skill", status.stdout)
self.assertNotIn("[registry] test-pack\n (未 fetch)", status.stdout)
self.assertIn("[catalog:test-pack] first-skill", status.stdout)
self.assertIn("[catalog:test-pack] second-skill", status.stdout)
self.assertNotIn("[catalog] test-pack\n (未 fetch)", status.stdout)
if __name__ == "__main__":
+50 -7
View File
@@ -8,6 +8,7 @@ import unittest
from pathlib import Path
from unittest.mock import patch
from skiff.project import normalize_skill_entry
from skiff.sources import fetch_source
REPO_ROOT = Path(__file__).resolve().parents[1]
@@ -25,12 +26,24 @@ def write_skill(root: Path, name: str) -> Path:
class CustomSourceTests(unittest.TestCase):
def test_legacy_manifest_sources_normalize_to_new_model(self) -> None:
self.assertEqual(
normalize_skill_entry({"name": "ack", "source": "owned"})["source"],
"builtin",
)
self.assertEqual(
normalize_skill_entry(
{"name": "think", "source": "registry", "registry": "waza"}
)["source"],
"catalog:waza",
)
def setUp(self) -> None:
self.temp_dir = tempfile.TemporaryDirectory()
self.home = Path(self.temp_dir.name)
self.skills_home = self.home / ".skills"
(self.skills_home / "skills").mkdir(parents=True)
(self.skills_home / "registry.yaml").write_text("", encoding="utf-8")
(self.skills_home / "catalog.yaml").write_text("", encoding="utf-8")
def tearDown(self) -> None:
self.temp_dir.cleanup()
@@ -74,6 +87,36 @@ class CustomSourceTests(unittest.TestCase):
config = self.home / ".config" / "skiff" / "config.yaml"
self.assertIn("company:", config.read_text(encoding="utf-8"))
def test_local_source_can_expose_a_single_skill(self) -> None:
source = self.home / "single-source"
source.mkdir()
source.joinpath("SKILL.md").write_text(
"---\nname: solo\ndescription: Test single source.\n---\n",
encoding="utf-8",
)
added = self.run_skiff(
"source",
"add",
"solo",
"--local",
str(source),
"--skills-path",
".",
)
installed = self.run_skiff(
"add",
"solo",
"-g",
"-a",
"codex",
)
self.assertEqual(added.returncode, 0, added.stderr)
self.assertEqual(installed.returncode, 0, installed.stderr)
self.assertEqual(
(self.home / ".codex" / "skills" / "solo").resolve(),
source.resolve(),
)
def test_project_add_persists_resolved_source_in_manifest(self) -> None:
company = self.home / "company"
expected = write_skill(company / "skills", "code-review")
@@ -136,17 +179,17 @@ class CustomSourceTests(unittest.TestCase):
result = self.run_skiff("add", "code-review", "-g", "-a", "codex")
self.assertNotEqual(result.returncode, 0)
self.assertIn("owned/code-review", result.stderr)
self.assertIn("builtin/code-review", result.stderr)
self.assertIn("company/code-review", result.stderr)
def test_custom_source_namespace_is_not_shadowed_by_registry_collection(self) -> None:
def test_custom_source_namespace_is_not_shadowed_by_catalog_collection(self) -> None:
company = self.home / "company"
expected = write_skill(company / "skills", "code-review")
registry_repo = self.home / "registry-repo"
write_skill(registry_repo / "skills", "other-skill")
self.skills_home.joinpath("registry.yaml").write_text(
catalog_repo = self.home / "catalog-repo"
write_skill(catalog_repo / "skills", "other-skill")
self.skills_home.joinpath("catalog.yaml").write_text(
"company:\n"
f" repo: {registry_repo}\n"
f" repo: {catalog_repo}\n"
" ref: main\n"
" path: skills\n",
encoding="utf-8",
+1 -1
View File
@@ -36,7 +36,7 @@ class InstallScriptTests(unittest.TestCase):
link = home / relative
self.assertTrue(link.is_symlink(), relative)
self.assertEqual(link.resolve(), (REPO_ROOT / "skills" / "skiff").resolve())
self.assertIn("已安装项目 skill", result.stdout)
self.assertIn("已全局安装 builtin skiff skill", result.stdout)
if __name__ == "__main__":
+122 -58
View File
@@ -12,7 +12,7 @@ from unittest.mock import Mock, patch
from skiff import cli
from skiff import yaml_io
from skiff.registry import external_repo_path, external_skill_path
from skiff.catalog import catalog_repo_path, catalog_skill_path
from skiff.selector import SkillChoice, filter_choices, fit_to_width, select_skills
@@ -28,7 +28,7 @@ class SelectorTests(unittest.TestCase):
data = {
"skills": [
{
"name": "external-one",
"name": "catalog-one",
"source": "registry",
"targets": ["codex"],
}
@@ -39,24 +39,24 @@ class SelectorTests(unittest.TestCase):
def test_filter_matches_name_kind_and_description(self) -> None:
choices = [
SkillChoice("frontend-design", "external", "创建界面"),
SkillChoice("frontend-design", "catalog", "创建界面"),
SkillChoice(
"discussion-notes",
"owned",
"builtin",
"维护讨论笔记",
readonly_status="全局: codex",
),
]
self.assertEqual([c.name for c in filter_choices(choices, "front")], ["frontend-design"])
self.assertEqual([c.name for c in filter_choices(choices, "owned")], ["discussion-notes"])
self.assertEqual([c.name for c in filter_choices(choices, "builtin")], ["discussion-notes"])
self.assertEqual([c.name for c in filter_choices(choices, "界面")], ["frontend-design"])
self.assertEqual([c.name for c in filter_choices(choices, "codex")], ["discussion-notes"])
def test_selector_preserves_preselected_items(self) -> None:
choices = [
SkillChoice("already-there", "owned", installed=True),
SkillChoice("new-skill", "external"),
SkillChoice("already-there", "builtin", installed=True),
SkillChoice("new-skill", "catalog"),
]
screen = Mock()
screen.getmaxyx.return_value = (24, 100)
@@ -76,7 +76,7 @@ class SelectorTests(unittest.TestCase):
choices = [
SkillChoice(
"ack",
"owned",
"builtin",
installed=False,
readonly_status="全局: cursor,claude,codex",
)
@@ -108,7 +108,7 @@ class SelectorTests(unittest.TestCase):
choices = [
SkillChoice(
"ack",
"owned",
"builtin",
description="初始化、检查并运行 ACK",
readonly_status="全局: codex",
)
@@ -128,7 +128,7 @@ class SelectorTests(unittest.TestCase):
)
self.assertEqual(
"".join(call.args[2] for call in title_calls),
"[ ] ack owned 全局: codex",
"[ ] ack builtin 全局: codex",
)
self.assertEqual(description_call.args[2], " 初始化、检查并运行 ACK")
name_call = next(call for call in title_calls if call.args[2] == "ack")
@@ -142,8 +142,8 @@ class SelectorTests(unittest.TestCase):
"repository",
children=("waza/think", "waza/ui"),
),
SkillChoice("waza/think", "external:waza", indent=1),
SkillChoice("waza/ui", "external:waza", indent=1),
SkillChoice("waza/think", "catalog:waza", indent=1),
SkillChoice("waza/ui", "catalog:waza", indent=1),
]
screen = Mock()
screen.getmaxyx.return_value = (24, 100)
@@ -161,8 +161,8 @@ class SelectorTests(unittest.TestCase):
"repository",
children=("waza/think", "waza/ui"),
),
SkillChoice("waza/think", "external:waza", installed=True, indent=1),
SkillChoice("waza/ui", "external:waza", indent=1),
SkillChoice("waza/think", "catalog:waza", installed=True, indent=1),
SkillChoice("waza/ui", "catalog:waza", indent=1),
]
screen = Mock()
screen.getmaxyx.return_value = (24, 100)
@@ -182,17 +182,17 @@ class SelectorTests(unittest.TestCase):
first = {"repo": "https://example.test/skills.git", "ref": "main", "path": "a"}
second = {"repo": "https://example.test/skills.git", "ref": "main", "path": "b"}
self.assertEqual(external_repo_path(first), external_repo_path(second))
self.assertEqual(catalog_repo_path(first), catalog_repo_path(second))
def test_external_skill_path_rejects_checkout_escape(self) -> None:
def test_catalog_skill_path_rejects_checkout_escape(self) -> None:
entry = {
"repo": "https://example.test/skills.git",
"ref": "main",
"path": "../../outside",
}
with self.assertRaisesRegex(SystemExit, "超出外部仓库"):
external_skill_path("unsafe-skill", entry)
with self.assertRaisesRegex(SystemExit, "超出来源仓库"):
catalog_skill_path("unsafe-skill", entry)
def test_collection_discovery_ignores_symlinked_skill(self) -> None:
with tempfile.TemporaryDirectory() as temp:
@@ -210,12 +210,12 @@ class SelectorTests(unittest.TestCase):
"path": "skills",
}
with patch("skiff.registry.external_checkout_path", return_value=checkout):
from skiff.registry import discover_external_skills
with patch("skiff.catalog.catalog_checkout_path", return_value=checkout):
from skiff.catalog import discover_catalog_skills
self.assertEqual(discover_external_skills("unsafe", entry), {})
self.assertEqual(discover_catalog_skills("unsafe", entry), {})
def test_registry_clone_is_shallow(self) -> None:
def test_catalog_clone_is_shallow(self) -> None:
with tempfile.TemporaryDirectory() as temp:
root = Path(temp)
checkout = root / "checkout"
@@ -226,16 +226,16 @@ class SelectorTests(unittest.TestCase):
"path": "skills",
}
with (
patch.object(cli, "external_skill_path", return_value=skill_root),
patch.object(cli, "external_checkout_path", return_value=checkout),
patch.object(cli, "catalog_skill_path", return_value=skill_root),
patch.object(cli, "catalog_checkout_path", return_value=checkout),
patch.object(
cli,
"discover_external_skills",
"discover_catalog_skills",
side_effect=[{}, {"demo": skill_root / "demo"}],
),
patch.object(cli.subprocess, "run") as run,
):
cli._ensure_registry_fetched("demo-pack", entry)
cli._ensure_catalog_fetched("demo-pack", entry)
run.assert_called_once_with(
[
@@ -277,9 +277,9 @@ class SelectCommandTests(unittest.TestCase):
def test_project_selection_installs_new_and_records_all_selected(self) -> None:
with tempfile.TemporaryDirectory() as temp:
project = Path(temp)
external = project / "external-one"
external.mkdir()
external.joinpath("SKILL.md").write_text("---\n", encoding="utf-8")
catalog = project / "catalog-one"
catalog.mkdir()
catalog.joinpath("SKILL.md").write_text("---\n", encoding="utf-8")
args = argparse.Namespace(
agents=[["codex"]],
global_scope=False,
@@ -301,28 +301,28 @@ class SelectCommandTests(unittest.TestCase):
) -> set[str]:
selected_choices.extend(choices)
selected_scope.append(scope_label)
return {"owned-one", "external-one"}
return {"builtin-one", "catalog-one"}
with (
patch.object(cli.sys, "stdin", stdin),
patch.object(cli.sys, "stdout", stdout),
patch.object(cli, "ensure_skills_home"),
patch.object(cli, "list_owned_skills", return_value=["owned-one"]),
patch.object(cli, "skill_description", return_value="owned"),
patch.object(cli, "list_builtin_skills", return_value=["builtin-one"]),
patch.object(cli, "skill_description", return_value="builtin"),
patch.object(
cli,
"load_registry",
"load_catalog",
return_value={
"external-one": {
"catalog-one": {
"repo": "https://example.test/skills.git",
"ref": "main",
"path": "external-one",
"path": "catalog-one",
}
},
),
patch.object(cli, "_registry_skill_names", return_value=["external-one"]),
patch.object(cli, "external_skill_path", return_value=external),
patch.object(cli, "_list_fully_installed_names", return_value=["owned-one"]),
patch.object(cli, "_catalog_skill_names", return_value=["catalog-one"]),
patch.object(cli, "catalog_skill_path", return_value=catalog),
patch.object(cli, "_list_fully_installed_names", return_value=["builtin-one"]),
patch.object(
cli,
"_global_installation_note",
@@ -331,7 +331,7 @@ class SelectCommandTests(unittest.TestCase):
patch.object(
cli,
"_is_fully_installed",
side_effect=lambda name, expected, project_root, targets: name == "owned-one",
side_effect=lambda name, expected, project_root, targets: name == "builtin-one",
),
patch.object(
cli,
@@ -348,10 +348,11 @@ class SelectCommandTests(unittest.TestCase):
manifest = (project / ".skills.yaml").read_text(encoding="utf-8")
self.assertEqual(installed, ["external-one"])
self.assertIn("owned-one", manifest)
self.assertIn('name: "external-one"', manifest)
self.assertIn("source: registry", manifest)
self.assertEqual(installed, ["catalog-one"])
self.assertIn("builtin-one", manifest)
self.assertIn("source: builtin", manifest)
self.assertIn('name: "catalog-one"', manifest)
self.assertIn('source: "catalog:catalog-one"', manifest)
self.assertIn("targets:", manifest)
self.assertIn("codex", manifest)
self.assertEqual(
@@ -389,7 +390,7 @@ class SelectCommandTests(unittest.TestCase):
self.assertEqual(note, "全局: cursor;全局同名冲突: claude")
def test_select_rejects_invalid_registry_name_before_rendering(self) -> None:
def test_select_rejects_invalid_catalog_name_before_rendering(self) -> None:
args = argparse.Namespace(
agents=None,
global_scope=True,
@@ -405,10 +406,10 @@ class SelectCommandTests(unittest.TestCase):
patch.object(cli.sys, "stdin", stdin),
patch.object(cli.sys, "stdout", stdout),
patch.object(cli, "ensure_skills_home"),
patch.object(cli, "list_owned_skills", return_value=[]),
patch.object(cli, "list_builtin_skills", return_value=[]),
patch.object(
cli,
"load_registry",
"load_catalog",
return_value={"../../victim": {"repo": "https://example.test/repo.git"}},
),
patch.object(cli, "select_skills") as selector,
@@ -418,7 +419,7 @@ class SelectCommandTests(unittest.TestCase):
selector.assert_not_called()
def test_select_expands_registry_collection_choices(self) -> None:
def test_select_expands_catalog_collection_choices(self) -> None:
args = argparse.Namespace(
agents=[["codex"]],
global_scope=True,
@@ -440,10 +441,10 @@ class SelectCommandTests(unittest.TestCase):
patch.object(cli.sys, "stdin", stdin),
patch.object(cli.sys, "stdout", stdout),
patch.object(cli, "ensure_skills_home"),
patch.object(cli, "list_owned_skills", return_value=[]),
patch.object(cli, "list_builtin_skills", return_value=[]),
patch.object(
cli,
"load_registry",
"load_catalog",
return_value={
"waza": {
"repo": "https://example.test/waza.git",
@@ -454,7 +455,7 @@ class SelectCommandTests(unittest.TestCase):
),
patch.object(
cli,
"_registry_skill_names",
"_catalog_skill_names",
return_value=["think", "ui"],
),
patch.object(cli, "_list_fully_installed_names", return_value=[]),
@@ -476,9 +477,72 @@ class SelectCommandTests(unittest.TestCase):
)
self.assertEqual(selected_choices[0].children, ("waza/think", "waza/ui"))
self.assertEqual([choice.indent for choice in selected_choices[1:]], [1, 1])
self.assertEqual(installed, [("think", "registry:waza")])
self.assertEqual(installed, [("think", "catalog:waza")])
def test_select_deduplicates_local_registry_collection_from_owned(self) -> None:
def test_select_expands_custom_collection_choices(self) -> None:
args = argparse.Namespace(
agents=[["codex"]],
global_scope=True,
project=None,
yes=False,
)
stdin = Mock()
stdout = Mock()
stdin.isatty.return_value = True
stdout.isatty.return_value = True
selected_choices: list[SkillChoice] = []
installed: list[tuple[str, str | None]] = []
def choose(choices: list[SkillChoice], **_: object) -> set[str]:
selected_choices.extend(choices)
return {"company/review"}
with tempfile.TemporaryDirectory() as temp:
root = Path(temp)
review = root / "review"
release = root / "release"
review.mkdir()
release.mkdir()
with (
patch.object(cli.sys, "stdin", stdin),
patch.object(cli.sys, "stdout", stdout),
patch.object(cli, "ensure_skills_home"),
patch.object(cli, "list_builtin_skills", return_value=[]),
patch.object(cli, "load_catalog", return_value={}),
patch.object(
cli,
"load_sources",
return_value={"company": {"local_path": str(root)}},
),
patch.object(cli, "_ensure_source_fetched"),
patch.object(
cli,
"discover_source_skills",
return_value={"review": review, "release": release},
),
patch.object(cli, "_is_fully_installed", return_value=False),
patch.object(cli, "select_skills", side_effect=choose),
patch.object(
cli,
"_install_skill",
side_effect=lambda name, targets, project_root, **kwargs: installed.append(
(name, kwargs.get("source"))
),
),
):
cli.cmd_select(args)
self.assertEqual(
[choice.name for choice in selected_choices],
["company", "company/review", "company/release"],
)
self.assertEqual(
selected_choices[0].children,
("company/review", "company/release"),
)
self.assertEqual(installed, [("review", "company")])
def test_select_deduplicates_local_catalog_collection_from_builtin(self) -> None:
args = argparse.Namespace(
agents=[["codex"]],
global_scope=True,
@@ -504,11 +568,11 @@ class SelectCommandTests(unittest.TestCase):
patch.object(cli.sys, "stdout", stdout),
patch.object(cli, "SKILLS_DIR", skills_root),
patch.object(cli, "ensure_skills_home"),
patch.object(cli, "list_owned_skills", return_value=["ack"]),
patch.object(cli, "skill_description", return_value="owned ack"),
patch.object(cli, "list_builtin_skills", return_value=["ack"]),
patch.object(cli, "skill_description", return_value="builtin ack"),
patch.object(
cli,
"load_registry",
"load_catalog",
return_value={
"skills": {
"repo": "~/.skills",
@@ -517,8 +581,8 @@ class SelectCommandTests(unittest.TestCase):
}
},
),
patch.object(cli, "_registry_skill_names", return_value=["ack"]),
patch.object(cli, "external_skill_path", return_value=skills_root),
patch.object(cli, "_catalog_skill_names", return_value=["ack"]),
patch.object(cli, "catalog_skill_path", return_value=skills_root),
patch.object(cli, "_is_fully_installed", return_value=False),
patch.object(cli, "select_skills", side_effect=choose),
):
@@ -540,7 +604,7 @@ class SelectCommandTests(unittest.TestCase):
with (
patch.object(cli, "_ensure_source_fetched"),
patch.object(cli, "resolve_skill_source", return_value=(skill, "owned")),
patch.object(cli, "resolve_skill_source", return_value=(skill, "builtin")),
patch.object(
cli,
"agent_skill_dir",