Skip to content

Commit 3e750e8

Browse files
committed
fix: address Copilot review comments on hook invocation consistency
- Add is_slash_skills_agent() helper to extensions.py to centralize the agent-to-invocation-format mapping, reducing drift risk between HookExecutor._render_hook_invocation() and init.py _display_cmd() - Use the shared helper in both locations; init.py now imports and delegates to is_slash_skills_agent() instead of maintaining its own per-agent boolean matrix - Fix test_hooks_render_skill_invocation to use ai_skills=False, proving Zed renders /speckit-<name> unconditionally - Add parameterized TestSlashSkillsSets covering all agents in ALWAYS_SLASH_AGENTS and CONDITIONAL_SLASH_AGENTS with ai_skills both true and false
1 parent b0a716c commit 3e750e8

3 files changed

Lines changed: 67 additions & 19 deletions

File tree

src/specify_cli/commands/init.py

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -738,20 +738,15 @@ def init(
738738
step_num += 1
739739
usage_label = "skills" if native_skill_mode else "slash commands"
740740

741+
from ..extensions import is_slash_skills_agent as _is_slash_skills_agent
742+
741743
def _display_cmd(name: str) -> str:
742744
if codex_skill_mode:
743745
return f"$speckit-{name}"
744-
if claude_skill_mode:
745-
return f"/speckit-{name}"
746746
if kimi_skill_mode:
747747
return f"/skill:speckit-{name}"
748748
if (
749-
agy_skill_mode
750-
or trae_skill_mode
751-
or cursor_agent_skill_mode
752-
or copilot_skill_mode
753-
or devin_skill_mode
754-
or zed_skill_mode
749+
_is_slash_skills_agent(selected_ai, _is_skills_integration)
755750
or cline_skill_mode
756751
):
757752
return f"/speckit-{name}"

src/specify_cli/extensions.py

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,22 @@
5151
)
5252
EXTENSION_COMMAND_NAME_PATTERN = re.compile(r"^speckit\.([a-z0-9-]+)\.([a-z0-9-]+)$")
5353

54+
55+
def is_slash_skills_agent(selected_ai: str, ai_skills_enabled: bool) -> bool:
56+
"""Return True if *selected_ai* uses ``/speckit-<name>`` hook invocations.
57+
58+
The decision is based on the agent sets defined above:
59+
60+
* Agents in `ALWAYS_SLASH_AGENTS` always use slash invocations.
61+
* Agents in `CONDITIONAL_SLASH_AGENTS` only use them when
62+
*ai_skills_enabled* is ``True``.
63+
* All other agents return ``False``.
64+
"""
65+
return selected_ai in ALWAYS_SLASH_AGENTS or (
66+
selected_ai in CONDITIONAL_SLASH_AGENTS and ai_skills_enabled
67+
)
68+
69+
5470
DEFAULT_HOOK_PRIORITY = 10
5571

5672
REINSTALL_COMMAND = "uv tool install specify-cli --force --from git+https://github.com/github/spec-kit.git"
@@ -2807,12 +2823,7 @@ def _render_hook_invocation(self, command: Any) -> str:
28072823

28082824
return f"/{format_cline_command_name(command_id)}"
28092825

2810-
# Agents that use /speckit-<name> (slash-skills invocation):
2811-
# - Always skills-based: devin, trae, zed
2812-
# - Conditional on ai_skills: agy, claude, copilot, cursor-agent
2813-
use_slash = selected_ai in ALWAYS_SLASH_AGENTS or (
2814-
selected_ai in CONDITIONAL_SLASH_AGENTS and ai_skills_enabled
2815-
)
2826+
use_slash = is_slash_skills_agent(selected_ai, ai_skills_enabled)
28162827

28172828
if skill_name and use_slash:
28182829
return f"/{skill_name}"

tests/integrations/test_integration_zed.py

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
import json
44

5+
import pytest
6+
57
from specify_cli.integrations import get_integration
68

79
from .test_integration_base_skills import SkillsIntegrationTests
@@ -34,13 +36,14 @@ class TestZedHookInvocations:
3436
"""Zed hook messages should reference slash-invokable skills."""
3537

3638
def test_hooks_render_skill_invocation(self, tmp_path):
39+
"""Zed is always skills-based: renders /speckit-plan even with ai_skills=False."""
3740
from specify_cli.extensions import HookExecutor
3841

3942
project = tmp_path / "zed-hooks"
4043
project.mkdir()
4144
init_options = project / ".specify" / "init-options.json"
4245
init_options.parent.mkdir(parents=True, exist_ok=True)
43-
init_options.write_text(json.dumps({"ai": "zed", "ai_skills": True}))
46+
init_options.write_text(json.dumps({"ai": "zed", "ai_skills": False}))
4447

4548
hook_executor = HookExecutor(project)
4649
message = hook_executor.format_hook_message(
@@ -54,8 +57,6 @@ def test_hooks_render_skill_invocation(self, tmp_path):
5457
],
5558
)
5659

57-
assert "Executing: `/speckit-plan`" in message
58-
assert "EXECUTE_COMMAND: speckit.plan" in message
5960
assert "EXECUTE_COMMAND_INVOCATION: /speckit-plan" in message
6061

6162
def test_init_persists_ai_skills_for_zed(self, tmp_path, monkeypatch):
@@ -107,7 +108,48 @@ def test_init_persists_ai_skills_for_zed(self, tmp_path, monkeypatch):
107108
],
108109
)
109110
assert "Executing: `/speckit-plan`" in message, (
110-
"Hook rendering must produce /speckit-plan for Zed without hint injection\n"
111-
f"Got message: {message}"
111+
"Hook rendering must produce /speckit-plan for Zed without hint injection"
112112
)
113113
assert "EXECUTE_COMMAND_INVOCATION: /speckit-plan" in message
114+
115+
116+
class TestSlashSkillsSets:
117+
"""Parameterized coverage for ALWAYS_SLASH_AGENTS / CONDITIONAL_SLASH_AGENTS."""
118+
119+
@staticmethod
120+
def _render_invocation(project_path, ai: str, ai_skills: bool) -> str:
121+
"""Return the rendered invocation for ``speckit.plan`` via HookExecutor."""
122+
from specify_cli.extensions import HookExecutor
123+
124+
init_options = project_path / ".specify" / "init-options.json"
125+
init_options.parent.mkdir(parents=True, exist_ok=True)
126+
init_options.write_text(json.dumps({"ai": ai, "ai_skills": ai_skills}))
127+
hook_executor = HookExecutor(project_path)
128+
return hook_executor._render_hook_invocation("speckit.plan")
129+
130+
@pytest.mark.parametrize(
131+
("ai", "ai_skills", "expected"),
132+
[
133+
# ALWAYS_SLASH_AGENTS — unconditional on ai_skills
134+
("devin", True, "/speckit-plan"),
135+
("devin", False, "/speckit-plan"),
136+
("trae", True, "/speckit-plan"),
137+
("trae", False, "/speckit-plan"),
138+
("zed", True, "/speckit-plan"),
139+
("zed", False, "/speckit-plan"),
140+
# CONDITIONAL_SLASH_AGENTS — only when ai_skills is enabled
141+
("agy", True, "/speckit-plan"),
142+
("agy", False, "/speckit.plan"),
143+
("claude", True, "/speckit-plan"),
144+
("claude", False, "/speckit.plan"),
145+
("copilot", True, "/speckit-plan"),
146+
("copilot", False, "/speckit.plan"),
147+
("cursor-agent", True, "/speckit-plan"),
148+
("cursor-agent", False, "/speckit.plan"),
149+
],
150+
)
151+
def test_hook_invocation_format(self, tmp_path, ai, ai_skills, expected):
152+
result = self._render_invocation(tmp_path, ai, ai_skills)
153+
assert result == expected, (
154+
f"{ai} (ai_skills={ai_skills}): expected {expected!r}, got {result!r}"
155+
)

0 commit comments

Comments
 (0)