Skip to content

Commit fa56921

Browse files
committed
fix: harden kimi migration and cache hook init options
1 parent d74385e commit fa56921

File tree

4 files changed

+65
-36
lines changed

4 files changed

+65
-36
lines changed

src/specify_cli/__init__.py

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1490,10 +1490,6 @@ def load_init_options(project_path: Path) -> dict[str, Any]:
14901490
return {}
14911491

14921492

1493-
# Agent-specific skill directory overrides for agents whose skills directory
1494-
# doesn't follow the standard <agent_folder>/skills/ pattern
1495-
AGENT_SKILLS_DIR_OVERRIDES = {}
1496-
14971493
# Default skills directory for agents not in AGENT_CONFIG
14981494
DEFAULT_SKILLS_DIR = ".agents/skills"
14991495

@@ -1526,13 +1522,9 @@ def load_init_options(project_path: Path) -> dict[str, Any]:
15261522
def _get_skills_dir(project_path: Path, selected_ai: str) -> Path:
15271523
"""Resolve the agent-specific skills directory for the given AI assistant.
15281524
1529-
Uses ``AGENT_SKILLS_DIR_OVERRIDES`` first, then falls back to
1530-
``AGENT_CONFIG[agent]["folder"] + "skills"``, and finally to
1531-
``DEFAULT_SKILLS_DIR``.
1525+
Uses ``AGENT_CONFIG[agent]["folder"] + "skills"`` and falls back to
1526+
``DEFAULT_SKILLS_DIR`` for unknown agents.
15321527
"""
1533-
if selected_ai in AGENT_SKILLS_DIR_OVERRIDES:
1534-
return project_path / AGENT_SKILLS_DIR_OVERRIDES[selected_ai]
1535-
15361528
agent_config = AGENT_CONFIG.get(selected_ai, {})
15371529
agent_folder = agent_config.get("folder", "")
15381530
if agent_folder:
@@ -1646,7 +1638,7 @@ def install_ai_skills(
16461638
command_name = command_name[len("speckit."):]
16471639
if command_name.endswith(".agent"):
16481640
command_name = command_name[:-len(".agent")]
1649-
skill_name = f"speckit-{command_name}"
1641+
skill_name = f"speckit-{command_name.replace('.', '-')}"
16501642

16511643
# Create skill directory (additive — never removes existing content)
16521644
skill_dir = skills_dir / skill_name
@@ -1739,7 +1731,7 @@ def _migrate_legacy_kimi_dotted_skills(skills_dir: Path) -> tuple[int, int]:
17391731
Returns:
17401732
Tuple[migrated_count, removed_count]
17411733
- migrated_count: old dotted dir renamed to hyphenated dir
1742-
- removed_count: old dotted dir deleted because hyphenated dir already existed
1734+
- removed_count: old dotted dir deleted when equivalent hyphenated dir existed
17431735
"""
17441736
if not skills_dir.is_dir():
17451737
return (0, 0)
@@ -1759,12 +1751,23 @@ def _migrate_legacy_kimi_dotted_skills(skills_dir: Path) -> tuple[int, int]:
17591751

17601752
target_dir = skills_dir / f"speckit-{suffix.replace('.', '-')}"
17611753

1762-
if target_dir.exists():
1763-
shutil.rmtree(legacy_dir)
1764-
removed_count += 1
1765-
else:
1754+
if not target_dir.exists():
17661755
shutil.move(str(legacy_dir), str(target_dir))
17671756
migrated_count += 1
1757+
continue
1758+
1759+
# If the new target already exists, avoid destructive cleanup unless
1760+
# both SKILL.md files are byte-identical.
1761+
target_skill = target_dir / "SKILL.md"
1762+
legacy_skill = legacy_dir / "SKILL.md"
1763+
if target_skill.is_file():
1764+
try:
1765+
if target_skill.read_bytes() == legacy_skill.read_bytes():
1766+
shutil.rmtree(legacy_dir)
1767+
removed_count += 1
1768+
except OSError:
1769+
# Best-effort migration: preserve legacy dir on read failures.
1770+
pass
17681771

17691772
return (migrated_count, removed_count)
17701773

src/specify_cli/extensions.py

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1646,17 +1646,20 @@ def __init__(self, project_root: Path):
16461646
self.project_root = project_root
16471647
self.extensions_dir = project_root / ".specify" / "extensions"
16481648
self.config_file = project_root / ".specify" / "extensions.yml"
1649+
self._init_options_cache: Optional[Dict[str, Any]] = None
16491650

16501651
def _load_init_options(self) -> Dict[str, Any]:
1651-
"""Load persisted init options used to determine invocation style."""
1652-
options_file = self.project_root / self.INIT_OPTIONS_FILE
1653-
if not options_file.exists():
1654-
return {}
1655-
try:
1656-
payload = json.loads(options_file.read_text(encoding="utf-8"))
1657-
return payload if isinstance(payload, dict) else {}
1658-
except (json.JSONDecodeError, OSError, UnicodeError):
1659-
return {}
1652+
"""Load persisted init options used to determine invocation style.
1653+
1654+
Uses the shared helper from specify_cli and caches values per executor
1655+
instance to avoid repeated filesystem reads during hook rendering.
1656+
"""
1657+
if self._init_options_cache is None:
1658+
from . import load_init_options
1659+
1660+
payload = load_init_options(self.project_root)
1661+
self._init_options_cache = payload if isinstance(payload, dict) else {}
1662+
return self._init_options_cache
16601663

16611664
@staticmethod
16621665
def _skill_name_from_command(command: str) -> str:

tests/test_ai_skills.py

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
_get_skills_dir,
2727
_migrate_legacy_kimi_dotted_skills,
2828
install_ai_skills,
29-
AGENT_SKILLS_DIR_OVERRIDES,
3029
DEFAULT_SKILLS_DIR,
3130
SKILL_DESCRIPTIONS,
3231
AGENT_CONFIG,
@@ -204,14 +203,6 @@ def test_all_configured_agents_resolve(self, project_dir):
204203
# Should always end with "skills"
205204
assert result.name == "skills"
206205

207-
def test_override_takes_precedence_over_config(self, project_dir):
208-
"""AGENT_SKILLS_DIR_OVERRIDES should take precedence over AGENT_CONFIG."""
209-
for agent_key in AGENT_SKILLS_DIR_OVERRIDES:
210-
result = _get_skills_dir(project_dir, agent_key)
211-
expected = project_dir / AGENT_SKILLS_DIR_OVERRIDES[agent_key]
212-
assert result == expected
213-
214-
215206
class TestKimiLegacySkillMigration:
216207
"""Test temporary migration from Kimi dotted skill names to hyphenated names."""
217208

@@ -228,20 +219,37 @@ def test_migrates_legacy_dotted_skill_directory(self, project_dir):
228219
assert not legacy_dir.exists()
229220
assert (skills_dir / "speckit-plan" / "SKILL.md").exists()
230221

231-
def test_removes_legacy_dir_when_hyphenated_target_exists(self, project_dir):
222+
def test_removes_legacy_dir_when_hyphenated_target_exists_with_same_content(self, project_dir):
232223
skills_dir = project_dir / ".kimi" / "skills"
233224
legacy_dir = skills_dir / "speckit.plan"
234225
legacy_dir.mkdir(parents=True)
235226
(legacy_dir / "SKILL.md").write_text("legacy")
236227
target_dir = skills_dir / "speckit-plan"
237228
target_dir.mkdir(parents=True)
238-
(target_dir / "SKILL.md").write_text("new")
229+
(target_dir / "SKILL.md").write_text("legacy")
239230

240231
migrated, removed = _migrate_legacy_kimi_dotted_skills(skills_dir)
241232

242233
assert migrated == 0
243234
assert removed == 1
244235
assert not legacy_dir.exists()
236+
assert (target_dir / "SKILL.md").read_text() == "legacy"
237+
238+
def test_keeps_legacy_dir_when_hyphenated_target_differs(self, project_dir):
239+
skills_dir = project_dir / ".kimi" / "skills"
240+
legacy_dir = skills_dir / "speckit.plan"
241+
legacy_dir.mkdir(parents=True)
242+
(legacy_dir / "SKILL.md").write_text("legacy")
243+
target_dir = skills_dir / "speckit-plan"
244+
target_dir.mkdir(parents=True)
245+
(target_dir / "SKILL.md").write_text("new")
246+
247+
migrated, removed = _migrate_legacy_kimi_dotted_skills(skills_dir)
248+
249+
assert migrated == 0
250+
assert removed == 0
251+
assert legacy_dir.exists()
252+
assert (legacy_dir / "SKILL.md").read_text() == "legacy"
245253
assert (target_dir / "SKILL.md").read_text() == "new"
246254

247255

tests/test_extensions.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3320,3 +3320,18 @@ def test_extension_command_uses_hyphenated_skill_invocation(self, project_dir):
33203320
assert "Executing: `/skill:speckit-test-hello`" in message
33213321
assert "EXECUTE_COMMAND: speckit.test.hello" in message
33223322
assert "EXECUTE_COMMAND_INVOCATION: /skill:speckit-test-hello" in message
3323+
3324+
def test_hook_executor_caches_init_options_lookup(self, project_dir, monkeypatch):
3325+
"""Init options should be loaded once per executor instance."""
3326+
calls = {"count": 0}
3327+
3328+
def fake_load_init_options(_project_root):
3329+
calls["count"] += 1
3330+
return {"ai": "kimi", "ai_skills": False}
3331+
3332+
monkeypatch.setattr("specify_cli.load_init_options", fake_load_init_options)
3333+
3334+
hook_executor = HookExecutor(project_dir)
3335+
assert hook_executor._render_hook_invocation("speckit.plan") == "/skill:speckit-plan"
3336+
assert hook_executor._render_hook_invocation("speckit.tasks") == "/skill:speckit-tasks"
3337+
assert calls["count"] == 1

0 commit comments

Comments
 (0)