fix: resolve command references per integration type (dot vs hyphen) (#2354)

* fix: resolve command references per integration type (dot vs hyphen) Replace hardcoded /speckit.<cmd> references in templates with __SPECKIT_COMMAND_<NAME>__ placeholders that are resolved at setup time based on the integration type: - Markdown/TOML/YAML agents: separator='.' → /speckit.plan - Skills agents: separator='-' → /speckit-plan Changes: - Add resolve_command_refs() static method to IntegrationBase - Add invoke_separator class attribute (. for base, - for skills) - Wire into process_template() as step 8 - Update _install_shared_infra() to process page templates - Replace /speckit.* in 5 command templates and 3 page templates - Add unit tests for resolve_command_refs (positive + negative) - Add integration tests verifying on-disk content for all agents - Add end-to-end CLI tests for Claude (skills) and Copilot (markdown) Fixes #2347 * review: use effective_invoke_separator() for Copilot skills mode Address PR review feedback: instead of bleeding _skills_mode knowledge into the CLI layer, add effective_invoke_separator() method to IntegrationBase that accepts parsed_options. CopilotIntegration overrides it to return "-" when skills mode is requested. The CLI layer simply asks the integration for its separator — no hasattr or _skills_mode coupling. Also adds tests for the new method on both base and Copilot, plus an end-to-end test for 'specify init --integration copilot --integration-options --skills' verifying page templates get hyphen refs. * fix: build_command_invocation preserves full suffix for extension commands Previously rsplit('.', 1)[-1] on 'speckit.git.commit' yielded just 'commit', producing /speckit.commit instead of /speckit.git.commit (or /speckit-git-commit for skills). Fix: strip only the 'speckit.' prefix when present, then join remaining segments with the appropriate separator. Updated in IntegrationBase, SkillsIntegration, and CopilotIntegration. Added tests for extension commands in build_command_invocation across all three. * fix: Copilot dispatch_command() preserves full extension command suffix dispatch_command() had the same rsplit('.', 1)[-1] bug as build_command_invocation() — speckit.git.commit would dispatch as /speckit-commit instead of /speckit-git-commit in skills mode, or --agent speckit.commit instead of speckit.git.commit in default mode.
2026-07-03 12:28:06 +08:00 · 2026-04-24 10:04:14 -05:00
parent 6413414907
commit 52c0a5f88f
21 changed files with 434 additions and 55 deletions
--- a/tests/integrations/test_base.py
+++ b/tests/integrations/test_base.py
@@ -6,6 +6,7 @@ from specify_cli.integrations.base import (
    IntegrationBase,
    IntegrationOption,
    MarkdownIntegration,
+    SkillsIntegration,
 )
 from specify_cli.integrations.manifest import IntegrationManifest
 from .conftest import StubIntegration
@@ -167,3 +168,130 @@ class TestBasePrimitives:
            assert f.parent.name == "commands"
            assert f.name.startswith("speckit.")
            assert f.name.endswith(".md")
+
+
+class TestBuildCommandInvocation:
+    """Tests for build_command_invocation across integration types."""
+
+    def test_base_core_command_dotted(self):
+        i = StubIntegration()
+        assert i.build_command_invocation("speckit.plan") == "/speckit.plan"
+
+    def test_base_core_command_bare(self):
+        i = StubIntegration()
+        assert i.build_command_invocation("plan") == "/speckit.plan"
+
+    def test_base_core_command_with_args(self):
+        i = StubIntegration()
+        assert i.build_command_invocation("plan", "my feature") == "/speckit.plan my feature"
+
+    def test_base_extension_command(self):
+        i = StubIntegration()
+        assert i.build_command_invocation("speckit.git.commit") == "/speckit.git.commit"
+
+    def test_base_extension_command_bare(self):
+        i = StubIntegration()
+        assert i.build_command_invocation("git.commit") == "/speckit.git.commit"
+
+    def test_skills_core_command(self):
+        from specify_cli.integrations import get_integration
+        i = get_integration("codex")
+        assert i.build_command_invocation("speckit.plan") == "/speckit-plan"
+        assert i.build_command_invocation("plan") == "/speckit-plan"
+
+    def test_skills_extension_command(self):
+        from specify_cli.integrations import get_integration
+        i = get_integration("codex")
+        assert i.build_command_invocation("speckit.git.commit") == "/speckit-git-commit"
+        assert i.build_command_invocation("git.commit") == "/speckit-git-commit"
+
+    def test_skills_extension_command_with_args(self):
+        from specify_cli.integrations import get_integration
+        i = get_integration("codex")
+        assert i.build_command_invocation("speckit.git.commit", "fix typo") == "/speckit-git-commit fix typo"
+
+
+class TestResolveCommandRefs:
+    """Tests for __SPECKIT_COMMAND_<NAME>__ placeholder resolution."""
+
+    def test_dot_separator_core_command(self):
+        text = "Run `__SPECKIT_COMMAND_PLAN__` to plan."
+        result = IntegrationBase.resolve_command_refs(text, ".")
+        assert result == "Run `/speckit.plan` to plan."
+
+    def test_hyphen_separator_core_command(self):
+        text = "Run `__SPECKIT_COMMAND_PLAN__` to plan."
+        result = IntegrationBase.resolve_command_refs(text, "-")
+        assert result == "Run `/speckit-plan` to plan."
+
+    def test_multiple_placeholders(self):
+        text = "__SPECKIT_COMMAND_SPECIFY__ then __SPECKIT_COMMAND_PLAN__ then __SPECKIT_COMMAND_TASKS__"
+        result = IntegrationBase.resolve_command_refs(text, ".")
+        assert result == "/speckit.specify then /speckit.plan then /speckit.tasks"
+
+    def test_extension_command_dot(self):
+        text = "Run __SPECKIT_COMMAND_GIT_COMMIT__ to commit."
+        result = IntegrationBase.resolve_command_refs(text, ".")
+        assert result == "Run /speckit.git.commit to commit."
+
+    def test_extension_command_hyphen(self):
+        text = "Run __SPECKIT_COMMAND_GIT_COMMIT__ to commit."
+        result = IntegrationBase.resolve_command_refs(text, "-")
+        assert result == "Run /speckit-git-commit to commit."
+
+    def test_no_placeholders_unchanged(self):
+        text = "No placeholders here."
+        assert IntegrationBase.resolve_command_refs(text, ".") == text
+
+    def test_default_separator_is_dot(self):
+        text = "__SPECKIT_COMMAND_PLAN__"
+        assert IntegrationBase.resolve_command_refs(text) == "/speckit.plan"
+
+    def test_invoke_separator_class_attribute(self):
+        assert IntegrationBase.invoke_separator == "."
+        assert SkillsIntegration.invoke_separator == "-"
+
+    def test_effective_invoke_separator_default(self):
+        """Base classes return invoke_separator regardless of parsed_options."""
+        from .conftest import StubIntegration
+        stub = StubIntegration()
+        assert stub.effective_invoke_separator() == "."
+        assert stub.effective_invoke_separator({"skills": True}) == "."
+
+    def test_process_template_resolves_placeholders(self):
+        content = "---\ndescription: test\n---\nRun __SPECKIT_COMMAND_PLAN__ now."
+        result = IntegrationBase.process_template(
+            content, "test-agent", "sh", invoke_separator="."
+        )
+        assert "/speckit.plan" in result
+        assert "__SPECKIT_COMMAND_" not in result
+
+    def test_process_template_skills_separator(self):
+        content = "---\ndescription: test\n---\nRun __SPECKIT_COMMAND_PLAN__ now."
+        result = IntegrationBase.process_template(
+            content, "test-agent", "sh", invoke_separator="-"
+        )
+        assert "/speckit-plan" in result
+        assert "__SPECKIT_COMMAND_" not in result
+
+    def test_unclosed_placeholder_unchanged(self):
+        text = "Run __SPECKIT_COMMAND_PLAN to plan."
+        assert IntegrationBase.resolve_command_refs(text, ".") == text
+
+    def test_empty_name_not_matched(self):
+        text = "Run __SPECKIT_COMMAND___ to plan."
+        assert IntegrationBase.resolve_command_refs(text, ".") == text
+
+    def test_lowercase_placeholder_not_matched(self):
+        text = "Run __SPECKIT_COMMAND_plan__ to plan."
+        assert IntegrationBase.resolve_command_refs(text, ".") == text
+
+    def test_placeholder_adjacent_to_text(self):
+        text = "foo__SPECKIT_COMMAND_PLAN__bar"
+        result = IntegrationBase.resolve_command_refs(text, ".")
+        assert result == "foo/speckit.planbar"
+
+    def test_placeholder_with_digits(self):
+        text = "__SPECKIT_COMMAND_V2_PLAN__"
+        result = IntegrationBase.resolve_command_refs(text, ".")
+        assert result == "/speckit.v2.plan"
--- a/tests/integrations/test_cli.py
+++ b/tests/integrations/test_cli.py
@@ -471,3 +471,133 @@ class TestGitExtensionAutoInstall:
        assert claude_skills.exists(), "Claude skills directory was not created"
        git_skills = [f for f in claude_skills.iterdir() if f.name.startswith("speckit-git-")]
        assert len(git_skills) > 0, "no git extension commands registered"
+
+
+class TestSharedInfraCommandRefs:
+    """Verify _install_shared_infra resolves __SPECKIT_COMMAND_*__ in page templates."""
+
+    def test_dot_separator_in_page_templates(self, tmp_path):
+        """Markdown agents get /speckit.<name> in page templates."""
+        from specify_cli import _install_shared_infra
+
+        project = tmp_path / "dot-test"
+        project.mkdir()
+        (project / ".specify").mkdir()
+
+        _install_shared_infra(project, "sh", invoke_separator=".")
+
+        plan = project / ".specify" / "templates" / "plan-template.md"
+        assert plan.exists()
+        content = plan.read_text(encoding="utf-8")
+        assert "__SPECKIT_COMMAND_" not in content, "unresolved placeholder in plan-template.md"
+        assert "/speckit.plan" in content
+
+        checklist = project / ".specify" / "templates" / "checklist-template.md"
+        content = checklist.read_text(encoding="utf-8")
+        assert "__SPECKIT_COMMAND_" not in content
+        assert "/speckit.checklist" in content
+
+    def test_hyphen_separator_in_page_templates(self, tmp_path):
+        """Skills agents get /speckit-<name> in page templates."""
+        from specify_cli import _install_shared_infra
+
+        project = tmp_path / "hyphen-test"
+        project.mkdir()
+        (project / ".specify").mkdir()
+
+        _install_shared_infra(project, "sh", invoke_separator="-")
+
+        plan = project / ".specify" / "templates" / "plan-template.md"
+        assert plan.exists()
+        content = plan.read_text(encoding="utf-8")
+        assert "__SPECKIT_COMMAND_" not in content, "unresolved placeholder in plan-template.md"
+        assert "/speckit-plan" in content
+        assert "/speckit.plan" not in content, "dot-notation leaked into skills page template"
+
+        tasks = project / ".specify" / "templates" / "tasks-template.md"
+        content = tasks.read_text(encoding="utf-8")
+        assert "__SPECKIT_COMMAND_" not in content
+        assert "/speckit-tasks" in content
+
+    def test_full_init_claude_resolves_page_templates(self, tmp_path):
+        """Full CLI init with Claude (skills agent) produces hyphen refs in page templates."""
+        from typer.testing import CliRunner
+        from specify_cli import app
+
+        runner = CliRunner()
+        project = tmp_path / "init-claude"
+        old_cwd = os.getcwd()
+        try:
+            os.chdir(tmp_path)
+            result = runner.invoke(app, [
+                "init", str(project),
+                "--integration", "claude",
+                "--script", "sh",
+                "--no-git",
+                "--ignore-agent-tools",
+            ], catch_exceptions=False)
+        finally:
+            os.chdir(old_cwd)
+
+        assert result.exit_code == 0, f"init failed: {result.output}"
+
+        plan = project / ".specify" / "templates" / "plan-template.md"
+        content = plan.read_text(encoding="utf-8")
+        assert "/speckit-plan" in content, "Claude (skills) should use /speckit-plan"
+        assert "__SPECKIT_COMMAND_" not in content
+
+    def test_full_init_copilot_resolves_page_templates(self, tmp_path):
+        """Full CLI init with Copilot (markdown agent) produces dot refs in page templates."""
+        from typer.testing import CliRunner
+        from specify_cli import app
+
+        runner = CliRunner()
+        project = tmp_path / "init-copilot"
+        old_cwd = os.getcwd()
+        try:
+            os.chdir(tmp_path)
+            result = runner.invoke(app, [
+                "init", str(project),
+                "--integration", "copilot",
+                "--script", "sh",
+                "--no-git",
+                "--ignore-agent-tools",
+            ], catch_exceptions=False)
+        finally:
+            os.chdir(old_cwd)
+
+        assert result.exit_code == 0, f"init failed: {result.output}"
+
+        plan = project / ".specify" / "templates" / "plan-template.md"
+        content = plan.read_text(encoding="utf-8")
+        assert "/speckit.plan" in content, "Copilot (markdown) should use /speckit.plan"
+        assert "__SPECKIT_COMMAND_" not in content
+
+    def test_full_init_copilot_skills_resolves_page_templates(self, tmp_path):
+        """Full CLI init with Copilot --skills produces hyphen refs in page templates."""
+        from typer.testing import CliRunner
+        from specify_cli import app
+
+        runner = CliRunner()
+        project = tmp_path / "init-copilot-skills"
+        old_cwd = os.getcwd()
+        try:
+            os.chdir(tmp_path)
+            result = runner.invoke(app, [
+                "init", str(project),
+                "--integration", "copilot",
+                "--integration-options", "--skills",
+                "--script", "sh",
+                "--no-git",
+                "--ignore-agent-tools",
+            ], catch_exceptions=False)
+        finally:
+            os.chdir(old_cwd)
+
+        assert result.exit_code == 0, f"init failed: {result.output}"
+
+        plan = project / ".specify" / "templates" / "plan-template.md"
+        content = plan.read_text(encoding="utf-8")
+        assert "/speckit-plan" in content, "Copilot --skills should use /speckit-plan"
+        assert "/speckit.plan" not in content, "dot-notation leaked into Copilot skills page template"
+        assert "__SPECKIT_COMMAND_" not in content
--- a/tests/integrations/test_integration_base_markdown.py
+++ b/tests/integrations/test_integration_base_markdown.py
@@ -98,6 +98,7 @@ class MarkdownIntegrationTests:
            assert "{SCRIPT}" not in content, f"{f.name} has unprocessed {{SCRIPT}}"
            assert "__AGENT__" not in content, f"{f.name} has unprocessed __AGENT__"
            assert "{ARGS}" not in content, f"{f.name} has unprocessed {{ARGS}}"
+            assert "__SPECKIT_COMMAND_" not in content, f"{f.name} has unprocessed __SPECKIT_COMMAND_*__"
            assert "\nscripts:\n" not in content, f"{f.name} has unstripped scripts: block"

    def test_plan_references_correct_context_file(self, tmp_path):
--- a/tests/integrations/test_integration_base_skills.py
+++ b/tests/integrations/test_integration_base_skills.py
@@ -159,6 +159,22 @@ class SkillsIntegrationTests:
            assert "{SCRIPT}" not in content, f"{f.name} has unprocessed {{SCRIPT}}"
            assert "__AGENT__" not in content, f"{f.name} has unprocessed __AGENT__"
            assert "{ARGS}" not in content, f"{f.name} has unprocessed {{ARGS}}"
+            assert "__SPECKIT_COMMAND_" not in content, f"{f.name} has unprocessed __SPECKIT_COMMAND_*__"
+
+    def test_command_refs_use_hyphen_separator(self, tmp_path):
+        """Skills agents must resolve command refs with hyphen separator."""
+        i = get_integration(self.KEY)
+        m = IntegrationManifest(self.KEY, tmp_path)
+        created = i.setup(tmp_path, m)
+        skill_files = [f for f in created if "scripts" not in f.parts]
+        assert len(skill_files) > 0
+        for f in skill_files:
+            content = f.read_text(encoding="utf-8")
+            # Skills agents must use /speckit-<name>, not /speckit.<name>
+            assert "/speckit." not in content, (
+                f"{f.name} contains dot-notation /speckit. reference; "
+                f"skills agents must use /speckit-<name>"
+            )

    def test_skill_body_has_content(self, tmp_path):
        """Each SKILL.md body should contain template content after the frontmatter."""
--- a/tests/integrations/test_integration_base_toml.py
+++ b/tests/integrations/test_integration_base_toml.py
@@ -106,6 +106,7 @@ class TomlIntegrationTests:
            assert "{SCRIPT}" not in content, f"{f.name} has unprocessed {{SCRIPT}}"
            assert "__AGENT__" not in content, f"{f.name} has unprocessed __AGENT__"
            assert "{ARGS}" not in content, f"{f.name} has unprocessed {{ARGS}}"
+            assert "__SPECKIT_COMMAND_" not in content, f"{f.name} has unprocessed __SPECKIT_COMMAND_*__"

    def test_toml_has_description(self, tmp_path):
        """Every TOML command file should have a description key."""
--- a/tests/integrations/test_integration_base_yaml.py
+++ b/tests/integrations/test_integration_base_yaml.py
@@ -105,6 +105,7 @@ class YamlIntegrationTests:
            assert "{SCRIPT}" not in content, f"{f.name} has unprocessed {{SCRIPT}}"
            assert "__AGENT__" not in content, f"{f.name} has unprocessed __AGENT__"
            assert "{ARGS}" not in content, f"{f.name} has unprocessed {{ARGS}}"
+            assert "__SPECKIT_COMMAND_" not in content, f"{f.name} has unprocessed __SPECKIT_COMMAND_*__"

    def test_yaml_has_title(self, tmp_path):
        """Every YAML recipe should have a title field."""
--- a/tests/integrations/test_integration_claude.py
+++ b/tests/integrations/test_integration_claude.py
@@ -55,6 +55,8 @@ class TestClaudeIntegration:
        assert "{SCRIPT}" not in content
        assert "{ARGS}" not in content
        assert "__AGENT__" not in content
+        assert "__SPECKIT_COMMAND_" not in content, "unprocessed __SPECKIT_COMMAND_*__"
+        assert "/speckit." not in content, "skills agent must use /speckit-<name> not /speckit.<name>"

        parts = content.split("---", 2)
        parsed = yaml.safe_load(parts[1])
--- a/tests/integrations/test_integration_copilot.py
+++ b/tests/integrations/test_integration_copilot.py
@@ -144,6 +144,7 @@ class TestCopilotIntegration:
            assert "{SCRIPT}" not in content, f"{agent_file.name} has unprocessed {{SCRIPT}}"
            assert "__AGENT__" not in content, f"{agent_file.name} has unprocessed __AGENT__"
            assert "{ARGS}" not in content, f"{agent_file.name} has unprocessed {{ARGS}}"
+            assert "__SPECKIT_COMMAND_" not in content, f"{agent_file.name} has unprocessed __SPECKIT_COMMAND_*__"
            assert "\nscripts:\n" not in content

    def test_plan_references_correct_context_file(self, tmp_path):
@@ -444,6 +445,27 @@ class TestCopilotSkillsMode:
            assert "{SCRIPT}" not in content, f"{f.name} has unprocessed {{SCRIPT}}"
            assert "__AGENT__" not in content, f"{f.name} has unprocessed __AGENT__"
            assert "{ARGS}" not in content, f"{f.name} has unprocessed {{ARGS}}"
+            assert "__SPECKIT_COMMAND_" not in content, f"{f.name} has unprocessed __SPECKIT_COMMAND_*__"
+
+    def test_skills_command_refs_use_hyphen(self, tmp_path):
+        """Copilot skills mode must use /speckit-<name> not /speckit.<name>."""
+        copilot = self._make_copilot()
+        created, _ = self._setup_skills(copilot, tmp_path)
+        skill_files = [f for f in created if f.name == "SKILL.md"]
+        assert len(skill_files) > 0
+        for f in skill_files:
+            content = f.read_text(encoding="utf-8")
+            assert "/speckit." not in content, (
+                f"{f.name} contains dot-notation /speckit. reference; "
+                f"skills mode must use /speckit-<name>"
+            )
+
+    def test_skills_mode_invoke_separator(self):
+        """Copilot effective_invoke_separator should reflect skills mode."""
+        copilot = self._make_copilot()
+        assert copilot.effective_invoke_separator() == "."
+        assert copilot.effective_invoke_separator({"skills": True}) == "-"
+        assert copilot.effective_invoke_separator({"skills": False}) == "."

    def test_skill_body_has_content(self, tmp_path):
        """Each SKILL.md body should contain template content."""
@@ -509,6 +531,12 @@ class TestCopilotSkillsMode:
        assert copilot.build_command_invocation("plan") == "/speckit-plan"
        assert copilot.build_command_invocation("plan", "my args") == "/speckit-plan my args"

+    def test_build_command_invocation_skills_extension_command(self):
+        copilot = self._make_copilot()
+        copilot._skills_mode = True
+        assert copilot.build_command_invocation("speckit.git.commit") == "/speckit-git-commit"
+        assert copilot.build_command_invocation("git.commit") == "/speckit-git-commit"
+
    def test_build_command_invocation_default_mode(self):
        copilot = self._make_copilot()
        assert copilot.build_command_invocation("plan", "my args") == "my args"
--- a/tests/integrations/test_integration_forge.py
+++ b/tests/integrations/test_integration_forge.py
@@ -152,6 +152,7 @@ class TestForgeIntegration:
            assert "{SCRIPT}" not in content, f"{cmd_file.name} has unprocessed {{SCRIPT}}"
            assert "__AGENT__" not in content, f"{cmd_file.name} has unprocessed __AGENT__"
            assert "{ARGS}" not in content, f"{cmd_file.name} has unprocessed {{ARGS}}"
+            assert "__SPECKIT_COMMAND_" not in content, f"{cmd_file.name} has unprocessed __SPECKIT_COMMAND_*__"
            # Check Forge-specific: $ARGUMENTS should be replaced with {{parameters}}
            assert "$ARGUMENTS" not in content, f"{cmd_file.name} has unprocessed $ARGUMENTS"
            # Frontmatter sections should be stripped
--- a/tests/integrations/test_integration_generic.py
+++ b/tests/integrations/test_integration_generic.py
@@ -101,6 +101,7 @@ class TestGenericIntegration:
            assert "{SCRIPT}" not in content, f"{f.name} has unprocessed {{SCRIPT}}"
            assert "__AGENT__" not in content, f"{f.name} has unprocessed __AGENT__"
            assert "{ARGS}" not in content, f"{f.name} has unprocessed {{ARGS}}"
+            assert "__SPECKIT_COMMAND_" not in content, f"{f.name} has unprocessed __SPECKIT_COMMAND_*__"

    def test_all_files_tracked_in_manifest(self, tmp_path):
        i = get_integration("generic")