mirror of
https://github.com/github/spec-kit.git
synced 2026-03-28 08:13:09 +00:00
Unify Kimi/Codex skill naming and migrate legacy dotted Kimi dirs (#1971)
* fix: unify hyphenated skills and migrate legacy kimi dotted dirs * fix: preserve legacy kimi dotted preset skill overrides * fix: migrate kimi legacy dotted skills without ai-skills flag * fix: harden kimi migration and cache hook init options * fix: apply kimi preset skill overrides without ai-skills flag * fix: keep sequential branch numbering beyond 999 * test: align kimi scaffold skill path with hyphen naming * chore: align hook typing and preset skill comment * fix: restore AGENT_SKILLS_DIR_OVERRIDES compatibility export * refactor: remove AGENT_SKILLS_DIR_OVERRIDES and update callers * fix(ps1): support sequential branch numbers above 999 * fix: resolve preset skill placeholders for skills agents * Fix legacy kimi migration safety and preset skill dir checks * Harden TOML rendering and consolidate preset skill restore parsing * Fix PowerShell overflow and hook message fallback for empty invocations * Restore preset skills from extensions * Refine preset skill restore helpers * Harden skill path and preset checks * Guard non-dict init options * Avoid deleting unmanaged preset skill dirs * Unify extension skill naming with hooks * Harden extension native skill registration * Normalize preset skill titles
This commit is contained in:
Hamilton Snow
@@ -41,17 +41,14 @@ def _create_init_options(project_root: Path, ai: str = "claude", ai_skills: bool
|
||||
def _create_skills_dir(project_root: Path, ai: str = "claude") -> Path:
|
||||
"""Create and return the expected skills directory for the given agent."""
|
||||
# Match the logic in _get_skills_dir() from specify_cli
|
||||
from specify_cli import AGENT_CONFIG, AGENT_SKILLS_DIR_OVERRIDES, DEFAULT_SKILLS_DIR
|
||||
from specify_cli import AGENT_CONFIG, DEFAULT_SKILLS_DIR
|
||||
|
||||
if ai in AGENT_SKILLS_DIR_OVERRIDES:
|
||||
skills_dir = project_root / AGENT_SKILLS_DIR_OVERRIDES[ai]
|
||||
agent_config = AGENT_CONFIG.get(ai, {})
|
||||
agent_folder = agent_config.get("folder", "")
|
||||
if agent_folder:
|
||||
skills_dir = project_root / agent_folder.rstrip("/") / "skills"
|
||||
else:
|
||||
agent_config = AGENT_CONFIG.get(ai, {})
|
||||
agent_folder = agent_config.get("folder", "")
|
||||
if agent_folder:
|
||||
skills_dir = project_root / agent_folder.rstrip("/") / "skills"
|
||||
else:
|
||||
skills_dir = project_root / DEFAULT_SKILLS_DIR
|
||||
skills_dir = project_root / DEFAULT_SKILLS_DIR
|
||||
|
||||
skills_dir.mkdir(parents=True, exist_ok=True)
|
||||
return skills_dir
|
||||
@@ -195,6 +192,24 @@ class TestExtensionManagerGetSkillsDir:
|
||||
result = manager._get_skills_dir()
|
||||
assert result is None
|
||||
|
||||
def test_returns_kimi_skills_dir_when_ai_skills_disabled(self, project_dir):
|
||||
"""Kimi should still use its native skills dir when ai_skills is false."""
|
||||
_create_init_options(project_dir, ai="kimi", ai_skills=False)
|
||||
skills_dir = _create_skills_dir(project_dir, ai="kimi")
|
||||
manager = ExtensionManager(project_dir)
|
||||
result = manager._get_skills_dir()
|
||||
assert result == skills_dir
|
||||
|
||||
def test_returns_none_for_non_dict_init_options(self, project_dir):
|
||||
"""Corrupted-but-parseable init-options should not crash skill-dir lookup."""
|
||||
opts_file = project_dir / ".specify" / "init-options.json"
|
||||
opts_file.parent.mkdir(parents=True, exist_ok=True)
|
||||
opts_file.write_text("[]")
|
||||
_create_skills_dir(project_dir, ai="claude")
|
||||
manager = ExtensionManager(project_dir)
|
||||
result = manager._get_skills_dir()
|
||||
assert result is None
|
||||
|
||||
|
||||
# ===== Extension Skill Registration Tests =====
|
||||
|
||||
@@ -211,8 +226,8 @@ class TestExtensionSkillRegistration:
|
||||
|
||||
# Check that skill directories were created
|
||||
skill_dirs = sorted([d.name for d in skills_dir.iterdir() if d.is_dir()])
|
||||
assert "speckit-test-ext.hello" in skill_dirs
|
||||
assert "speckit-test-ext.world" in skill_dirs
|
||||
assert "speckit-test-ext-hello" in skill_dirs
|
||||
assert "speckit-test-ext-world" in skill_dirs
|
||||
|
||||
def test_skill_md_content_correct(self, skills_project, extension_dir):
|
||||
"""SKILL.md should have correct agentskills.io structure."""
|
||||
@@ -222,13 +237,13 @@ class TestExtensionSkillRegistration:
|
||||
extension_dir, "0.1.0", register_commands=False
|
||||
)
|
||||
|
||||
skill_file = skills_dir / "speckit-test-ext.hello" / "SKILL.md"
|
||||
skill_file = skills_dir / "speckit-test-ext-hello" / "SKILL.md"
|
||||
assert skill_file.exists()
|
||||
content = skill_file.read_text()
|
||||
|
||||
# Check structure
|
||||
assert content.startswith("---\n")
|
||||
assert "name: speckit-test-ext.hello" in content
|
||||
assert "name: speckit-test-ext-hello" in content
|
||||
assert "description:" in content
|
||||
assert "Test hello command" in content
|
||||
assert "source: extension:test-ext" in content
|
||||
@@ -244,7 +259,7 @@ class TestExtensionSkillRegistration:
|
||||
extension_dir, "0.1.0", register_commands=False
|
||||
)
|
||||
|
||||
skill_file = skills_dir / "speckit-test-ext.hello" / "SKILL.md"
|
||||
skill_file = skills_dir / "speckit-test-ext-hello" / "SKILL.md"
|
||||
content = skill_file.read_text()
|
||||
|
||||
assert content.startswith("---\n")
|
||||
@@ -252,7 +267,7 @@ class TestExtensionSkillRegistration:
|
||||
assert len(parts) >= 3
|
||||
parsed = yaml.safe_load(parts[1])
|
||||
assert isinstance(parsed, dict)
|
||||
assert parsed["name"] == "speckit-test-ext.hello"
|
||||
assert parsed["name"] == "speckit-test-ext-hello"
|
||||
assert "description" in parsed
|
||||
|
||||
def test_no_skills_when_ai_skills_disabled(self, no_skills_project, extension_dir):
|
||||
@@ -281,7 +296,7 @@ class TestExtensionSkillRegistration:
|
||||
project_dir, skills_dir = skills_project
|
||||
|
||||
# Pre-create a custom skill
|
||||
custom_dir = skills_dir / "speckit-test-ext.hello"
|
||||
custom_dir = skills_dir / "speckit-test-ext-hello"
|
||||
custom_dir.mkdir(parents=True)
|
||||
custom_content = "# My Custom Hello Skill\nUser-modified content\n"
|
||||
(custom_dir / "SKILL.md").write_text(custom_content)
|
||||
@@ -296,9 +311,9 @@ class TestExtensionSkillRegistration:
|
||||
|
||||
# But the other skill should still be created
|
||||
metadata = manager.registry.get(manifest.id)
|
||||
assert "speckit-test-ext.world" in metadata["registered_skills"]
|
||||
assert "speckit-test-ext-world" in metadata["registered_skills"]
|
||||
# The pre-existing one should NOT be in registered_skills (it was skipped)
|
||||
assert "speckit-test-ext.hello" not in metadata["registered_skills"]
|
||||
assert "speckit-test-ext-hello" not in metadata["registered_skills"]
|
||||
|
||||
def test_registered_skills_in_registry(self, skills_project, extension_dir):
|
||||
"""Registry should contain registered_skills list."""
|
||||
@@ -311,11 +326,11 @@ class TestExtensionSkillRegistration:
|
||||
metadata = manager.registry.get(manifest.id)
|
||||
assert "registered_skills" in metadata
|
||||
assert len(metadata["registered_skills"]) == 2
|
||||
assert "speckit-test-ext.hello" in metadata["registered_skills"]
|
||||
assert "speckit-test-ext.world" in metadata["registered_skills"]
|
||||
assert "speckit-test-ext-hello" in metadata["registered_skills"]
|
||||
assert "speckit-test-ext-world" in metadata["registered_skills"]
|
||||
|
||||
def test_kimi_uses_dot_notation(self, project_dir, temp_dir):
|
||||
"""Kimi agent should use dot notation for skill names."""
|
||||
def test_kimi_uses_hyphenated_skill_names(self, project_dir, temp_dir):
|
||||
"""Kimi agent should use the same hyphenated skill names as hooks."""
|
||||
_create_init_options(project_dir, ai="kimi", ai_skills=True)
|
||||
_create_skills_dir(project_dir, ai="kimi")
|
||||
ext_dir = _create_extension_dir(temp_dir, ext_id="test-ext")
|
||||
@@ -326,9 +341,80 @@ class TestExtensionSkillRegistration:
|
||||
)
|
||||
|
||||
metadata = manager.registry.get(manifest.id)
|
||||
# Kimi should use dots, not hyphens
|
||||
assert "speckit.test-ext.hello" in metadata["registered_skills"]
|
||||
assert "speckit.test-ext.world" in metadata["registered_skills"]
|
||||
assert "speckit-test-ext-hello" in metadata["registered_skills"]
|
||||
assert "speckit-test-ext-world" in metadata["registered_skills"]
|
||||
|
||||
def test_kimi_creates_skills_when_ai_skills_disabled(self, project_dir, temp_dir):
|
||||
"""Kimi should still auto-register extension skills in native-skills mode."""
|
||||
_create_init_options(project_dir, ai="kimi", ai_skills=False)
|
||||
skills_dir = _create_skills_dir(project_dir, ai="kimi")
|
||||
ext_dir = _create_extension_dir(temp_dir, ext_id="test-ext")
|
||||
|
||||
manager = ExtensionManager(project_dir)
|
||||
manifest = manager.install_from_directory(
|
||||
ext_dir, "0.1.0", register_commands=False
|
||||
)
|
||||
|
||||
metadata = manager.registry.get(manifest.id)
|
||||
assert "speckit-test-ext-hello" in metadata["registered_skills"]
|
||||
assert "speckit-test-ext-world" in metadata["registered_skills"]
|
||||
assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists()
|
||||
|
||||
def test_skill_registration_resolves_script_placeholders(self, project_dir, temp_dir):
|
||||
"""Auto-registered extension skills should resolve script placeholders."""
|
||||
_create_init_options(project_dir, ai="claude", ai_skills=True)
|
||||
skills_dir = _create_skills_dir(project_dir, ai="claude")
|
||||
|
||||
ext_dir = temp_dir / "scripted-ext"
|
||||
ext_dir.mkdir()
|
||||
manifest_data = {
|
||||
"schema_version": "1.0",
|
||||
"extension": {
|
||||
"id": "scripted-ext",
|
||||
"name": "Scripted Extension",
|
||||
"version": "1.0.0",
|
||||
"description": "Test",
|
||||
},
|
||||
"requires": {"speckit_version": ">=0.1.0"},
|
||||
"provides": {
|
||||
"commands": [
|
||||
{
|
||||
"name": "speckit.scripted-ext.plan",
|
||||
"file": "commands/plan.md",
|
||||
"description": "Scripted plan command",
|
||||
}
|
||||
]
|
||||
},
|
||||
}
|
||||
with open(ext_dir / "extension.yml", "w") as f:
|
||||
yaml.dump(manifest_data, f)
|
||||
|
||||
(ext_dir / "commands").mkdir()
|
||||
(ext_dir / "commands" / "plan.md").write_text(
|
||||
"---\n"
|
||||
"description: Scripted plan command\n"
|
||||
"scripts:\n"
|
||||
" sh: ../../scripts/bash/setup-plan.sh --json \"{ARGS}\"\n"
|
||||
"agent_scripts:\n"
|
||||
" sh: ../../scripts/bash/update-agent-context.sh __AGENT__\n"
|
||||
"---\n\n"
|
||||
"Run {SCRIPT}\n"
|
||||
"Then {AGENT_SCRIPT}\n"
|
||||
"Review templates/checklist.md and memory/constitution.md for __AGENT__.\n"
|
||||
)
|
||||
|
||||
manager = ExtensionManager(project_dir)
|
||||
manager.install_from_directory(ext_dir, "0.1.0", register_commands=False)
|
||||
|
||||
content = (skills_dir / "speckit-scripted-ext-plan" / "SKILL.md").read_text()
|
||||
assert "{SCRIPT}" not in content
|
||||
assert "{AGENT_SCRIPT}" not in content
|
||||
assert "{ARGS}" not in content
|
||||
assert "__AGENT__" not in content
|
||||
assert '.specify/scripts/bash/setup-plan.sh --json "$ARGUMENTS"' in content
|
||||
assert ".specify/scripts/bash/update-agent-context.sh claude" in content
|
||||
assert ".specify/templates/checklist.md" in content
|
||||
assert ".specify/memory/constitution.md" in content
|
||||
|
||||
def test_missing_command_file_skipped(self, skills_project, temp_dir):
|
||||
"""Commands with missing source files should be skipped gracefully."""
|
||||
@@ -375,8 +461,8 @@ class TestExtensionSkillRegistration:
|
||||
)
|
||||
|
||||
metadata = manager.registry.get(manifest.id)
|
||||
assert "speckit-missing-cmd-ext.exists" in metadata["registered_skills"]
|
||||
assert "speckit-missing-cmd-ext.ghost" not in metadata["registered_skills"]
|
||||
assert "speckit-missing-cmd-ext-exists" in metadata["registered_skills"]
|
||||
assert "speckit-missing-cmd-ext-ghost" not in metadata["registered_skills"]
|
||||
|
||||
|
||||
# ===== Extension Skill Unregistration Tests =====
|
||||
@@ -393,16 +479,16 @@ class TestExtensionSkillUnregistration:
|
||||
)
|
||||
|
||||
# Verify skills exist
|
||||
assert (skills_dir / "speckit-test-ext.hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-test-ext.world" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-test-ext-world" / "SKILL.md").exists()
|
||||
|
||||
# Remove extension
|
||||
result = manager.remove(manifest.id, keep_config=False)
|
||||
assert result is True
|
||||
|
||||
# Skills should be gone
|
||||
assert not (skills_dir / "speckit-test-ext.hello").exists()
|
||||
assert not (skills_dir / "speckit-test-ext.world").exists()
|
||||
assert not (skills_dir / "speckit-test-ext-hello").exists()
|
||||
assert not (skills_dir / "speckit-test-ext-world").exists()
|
||||
|
||||
def test_other_skills_preserved_on_remove(self, skills_project, extension_dir):
|
||||
"""Non-extension skills should not be affected by extension removal."""
|
||||
@@ -433,8 +519,8 @@ class TestExtensionSkillUnregistration:
|
||||
)
|
||||
|
||||
# Manually delete skill dirs before calling remove
|
||||
shutil.rmtree(skills_dir / "speckit-test-ext.hello")
|
||||
shutil.rmtree(skills_dir / "speckit-test-ext.world")
|
||||
shutil.rmtree(skills_dir / "speckit-test-ext-hello")
|
||||
shutil.rmtree(skills_dir / "speckit-test-ext-world")
|
||||
|
||||
# Should not raise
|
||||
result = manager.remove(manifest.id, keep_config=False)
|
||||
@@ -457,6 +543,21 @@ class TestExtensionSkillUnregistration:
|
||||
class TestExtensionSkillEdgeCases:
|
||||
"""Test edge cases in extension skill registration."""
|
||||
|
||||
def test_install_with_non_dict_init_options_does_not_crash(self, project_dir, extension_dir):
|
||||
"""Corrupted init-options payloads should disable skill registration, not crash install."""
|
||||
opts_file = project_dir / ".specify" / "init-options.json"
|
||||
opts_file.parent.mkdir(parents=True, exist_ok=True)
|
||||
opts_file.write_text("[]")
|
||||
_create_skills_dir(project_dir, ai="claude")
|
||||
|
||||
manager = ExtensionManager(project_dir)
|
||||
manifest = manager.install_from_directory(
|
||||
extension_dir, "0.1.0", register_commands=False
|
||||
)
|
||||
|
||||
metadata = manager.registry.get(manifest.id)
|
||||
assert metadata["registered_skills"] == []
|
||||
|
||||
def test_command_without_frontmatter(self, skills_project, temp_dir):
|
||||
"""Commands without YAML frontmatter should still produce valid skills."""
|
||||
project_dir, skills_dir = skills_project
|
||||
@@ -495,10 +596,10 @@ class TestExtensionSkillEdgeCases:
|
||||
ext_dir, "0.1.0", register_commands=False
|
||||
)
|
||||
|
||||
skill_file = skills_dir / "speckit-nofm-ext.plain" / "SKILL.md"
|
||||
skill_file = skills_dir / "speckit-nofm-ext-plain" / "SKILL.md"
|
||||
assert skill_file.exists()
|
||||
content = skill_file.read_text()
|
||||
assert "name: speckit-nofm-ext.plain" in content
|
||||
assert "name: speckit-nofm-ext-plain" in content
|
||||
# Fallback description when no frontmatter description
|
||||
assert "Extension command: speckit.nofm-ext.plain" in content
|
||||
assert "Body without frontmatter." in content
|
||||
@@ -515,8 +616,8 @@ class TestExtensionSkillEdgeCases:
|
||||
)
|
||||
|
||||
skills_dir = project_dir / ".gemini" / "skills"
|
||||
assert (skills_dir / "speckit-test-ext.hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-test-ext.world" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-test-ext-world" / "SKILL.md").exists()
|
||||
|
||||
def test_multiple_extensions_independent_skills(self, skills_project, temp_dir):
|
||||
"""Installing and removing different extensions should be independent."""
|
||||
@@ -534,15 +635,15 @@ class TestExtensionSkillEdgeCases:
|
||||
)
|
||||
|
||||
# Both should have skills
|
||||
assert (skills_dir / "speckit-ext-a.hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-ext-b.hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-ext-a-hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-ext-b-hello" / "SKILL.md").exists()
|
||||
|
||||
# Remove ext-a
|
||||
manager.remove("ext-a", keep_config=False)
|
||||
|
||||
# ext-a skills gone, ext-b skills preserved
|
||||
assert not (skills_dir / "speckit-ext-a.hello").exists()
|
||||
assert (skills_dir / "speckit-ext-b.hello" / "SKILL.md").exists()
|
||||
assert not (skills_dir / "speckit-ext-a-hello").exists()
|
||||
assert (skills_dir / "speckit-ext-b-hello" / "SKILL.md").exists()
|
||||
|
||||
def test_malformed_frontmatter_handled(self, skills_project, temp_dir):
|
||||
"""Commands with invalid YAML frontmatter should still produce valid skills."""
|
||||
@@ -591,7 +692,7 @@ class TestExtensionSkillEdgeCases:
|
||||
ext_dir, "0.1.0", register_commands=False
|
||||
)
|
||||
|
||||
skill_file = skills_dir / "speckit-badfm-ext.broken" / "SKILL.md"
|
||||
skill_file = skills_dir / "speckit-badfm-ext-broken" / "SKILL.md"
|
||||
assert skill_file.exists()
|
||||
content = skill_file.read_text()
|
||||
# Fallback description since frontmatter was invalid
|
||||
@@ -607,7 +708,7 @@ class TestExtensionSkillEdgeCases:
|
||||
)
|
||||
|
||||
# Verify skills exist
|
||||
assert (skills_dir / "speckit-test-ext.hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists()
|
||||
|
||||
# Delete init-options.json to simulate user change
|
||||
init_opts = project_dir / ".specify" / "init-options.json"
|
||||
@@ -616,8 +717,8 @@ class TestExtensionSkillEdgeCases:
|
||||
# Remove should still clean up via fallback scan
|
||||
result = manager.remove(manifest.id, keep_config=False)
|
||||
assert result is True
|
||||
assert not (skills_dir / "speckit-test-ext.hello").exists()
|
||||
assert not (skills_dir / "speckit-test-ext.world").exists()
|
||||
assert not (skills_dir / "speckit-test-ext-hello").exists()
|
||||
assert not (skills_dir / "speckit-test-ext-world").exists()
|
||||
|
||||
def test_remove_cleans_up_when_ai_skills_toggled(self, skills_project, extension_dir):
|
||||
"""Skills should be cleaned up even if ai_skills is toggled to false after install."""
|
||||
@@ -628,7 +729,7 @@ class TestExtensionSkillEdgeCases:
|
||||
)
|
||||
|
||||
# Verify skills exist
|
||||
assert (skills_dir / "speckit-test-ext.hello" / "SKILL.md").exists()
|
||||
assert (skills_dir / "speckit-test-ext-hello" / "SKILL.md").exists()
|
||||
|
||||
# Toggle ai_skills to false
|
||||
_create_init_options(project_dir, ai="claude", ai_skills=False)
|
||||
@@ -636,5 +737,5 @@ class TestExtensionSkillEdgeCases:
|
||||
# Remove should still clean up via fallback scan
|
||||
result = manager.remove(manifest.id, keep_config=False)
|
||||
assert result is True
|
||||
assert not (skills_dir / "speckit-test-ext.hello").exists()
|
||||
assert not (skills_dir / "speckit-test-ext.world").exists()
|
||||
assert not (skills_dir / "speckit-test-ext-hello").exists()
|
||||
assert not (skills_dir / "speckit-test-ext-world").exists()
|
||||
|
||||
Reference in New Issue
Block a user