diff --git a/docs/integrations/copilot.md b/docs/integrations/copilot.md index c0d5a53833..29a38f26b4 100644 --- a/docs/integrations/copilot.md +++ b/docs/integrations/copilot.md @@ -22,7 +22,49 @@ See [integration management](cli.md) for activation, upgrades, and removal. Skills are installed as `.github/skills/speckit-/SKILL.md`. Invoke them in the agent's chat as `/speckit-specify` or `/speckit-bug-assess` for the installed bug extension. These are agent skills, -not terminal commands. +not terminal commands. Generated skills are both user- and model-invocable by +default. + +## User-Invocable-Only Skills + +To stop Copilot loading the core Spec Kit command skills automatically in the +skills layout: + +```bash +specify init my-project --integration copilot --integration-options="--no-model-invocation" +``` + +This writes `disable-model-invocation: true` into the core command skills. +VS Code's [agent skills documentation](https://code.visualstudio.com/docs/agent-customization/agent-skills) +says this requires manual invocation through the `/` slash command only. +GitHub's Copilot CLI skills documentation does not list the field, but in a +manual check with Copilot CLI 1.0.70 the model called its `skill` tool for +`speckit-specify` without the flag and did not with it, and an explicit +`/speckit-specify` still ran. With the flag the model could still read the +skill's `SKILL.md` as an ordinary file, so it is not a hard block. Neither +source says whether a skill with this flag keeps its description out of the +model's context. + +- The option has no effect with `--commands`: the `.agent.md` and `.prompt.md` + files are not skills and no flag is written. +- Extension skills stay model-invocable, because mandatory extension hooks rely + on the model invoking them. If an extension's mandatory hook runs a core + command (its `command:` is a core `speckit.`), leave this option off: + the model cannot run that hook while the core skill is user-invocable only. +- The option is stored with the integration's other options and reapplied by + `specify integration upgrade copilot`. Passing `--integration-options` to + `upgrade` replaces all stored options; + `--integration-options=""` clears them. Re-running `specify init` likewise + stores only the options passed to it. +- Core command skills that a preset overrides, or stops overriding, are reverted + to the default by commands that re-register preset skills without reinstalling + Copilot: commands that install, remove or update presets (including `specify + bundle` commands that carry presets), `specify integration use copilot`, and + `specify integration switch copilot` when Copilot is installed but not the + default. To reapply the option, run `specify integration upgrade copilot + --force` while Copilot is the default integration (`--force` is needed because + those commands modify installed skill files, and it overwrites local edits to + Spec Kit-managed files). ## Commands Layout diff --git a/src/specify_cli/integrations/copilot/__init__.py b/src/specify_cli/integrations/copilot/__init__.py index a283dc3df2..5b20e46965 100644 --- a/src/specify_cli/integrations/copilot/__init__.py +++ b/src/specify_cli/integrations/copilot/__init__.py @@ -178,6 +178,14 @@ class CopilotIntegration(IntegrationBase): # Mutable flag set by setup() — indicates the active scaffolding mode. _skills_mode: bool = True + # Set by ``setup()`` from ``--no-model-invocation`` (skills layout only); + # applies to core command skills only, since mandatory extension hooks rely + # on the model invoking the hook skill. Integrations are process-wide + # singletons, so preset overrides re-registered later in the same run see + # the same value. Commands that rewrite skills without calling ``setup()`` + # (e.g. ``preset add``, ``integration use``) get the default. + _disable_model_invocation: bool = False + def effective_invoke_separator( self, parsed_options: dict[str, Any] | None = None, @@ -299,6 +307,17 @@ def options(cls) -> list[IntegrationOption]: ), ), ) + opts.append( + IntegrationOption( + "--no-model-invocation", + is_flag=True, + default=False, + help=( + "Install core command skills as user-invocable only " + "(disable-model-invocation: true); skills layout only, documented for VS Code" + ), + ), + ) return opts def _resolve_executable(self) -> str: @@ -482,14 +501,78 @@ def stale_cleanup_exclusions(self) -> set[str]: exclusions.add(".vscode/settings.json") return exclusions + @staticmethod + def _inject_frontmatter_flag(content: str, key: str, value: str = "true") -> str: + """Insert ``key: value`` before the closing ``---`` if not already present.""" + lines = content.splitlines(keepends=True) + dash_count = 0 + for line in lines: + stripped = line.rstrip("\n\r") + if stripped == "---": + dash_count += 1 + if dash_count == 2: + break + continue + if dash_count == 1 and stripped.startswith(f"{key}:"): + return content + + out: list[str] = [] + dash_count = 0 + injected = False + for line in lines: + if line.rstrip("\n\r") == "---": + dash_count += 1 + if dash_count == 2 and not injected: + eol = "\r\n" if line.endswith("\r\n") else "\n" + out.append(f"{key}: {value}{eol}") + injected = True + out.append(line) + return "".join(out) + + @staticmethod + def _skill_name_from_content(content: str) -> str | None: + """Return the ``name:`` field of a skill's first frontmatter block.""" + dash_count = 0 + for line in content.splitlines(): + stripped = line.rstrip("\r\n") + if stripped == "---": + dash_count += 1 + if dash_count == 2: + break + continue + if dash_count == 1 and stripped.startswith("name:"): + name = stripped[len("name:"):].strip().strip('"').strip("'") + return name or None + return None + + @staticmethod + def _is_core_command_skill(name: str | None) -> bool: + from specify_cli.extensions import CORE_COMMAND_NAMES # lazy: avoids circular import + + return bool(name) and name.startswith("speckit-") and ( + name.removeprefix("speckit-") in CORE_COMMAND_NAMES + ) + def post_process_skill_content(self, content: str) -> str: """Inject shared hook guidance into Copilot skill content. Delegates to :class:`_CopilotSkillsHelper` for shared post-processing. The ``mode:`` frontmatter field is intentionally omitted: VS Code Copilot Agent Skills do not support it (see issue #2799). + + ``disable-model-invocation: true`` is added to core command skills + when the last ``setup()`` in this process was given + ``--no-model-invocation`` in the skills layout. Otherwise the key is + omitted, which VS Code treats as ``false``. """ - return _CopilotSkillsHelper().post_process_skill_content(content) + updated = _CopilotSkillsHelper().post_process_skill_content(content) + if self._disable_model_invocation and self._is_core_command_skill( + self._skill_name_from_content(updated) + ): + updated = self._inject_frontmatter_flag( + updated, "disable-model-invocation" + ) + return updated def setup( self, @@ -506,10 +589,17 @@ def setup( """ parsed_options = parsed_options or {} self._skills_mode = self.is_skills_mode(parsed_options, project_root) - if self._skills_mode: - created = self._setup_skills(project_root, manifest, parsed_options, **opts) - else: - created = self._setup_commands(project_root, manifest, parsed_options, **opts) + self._disable_model_invocation = ( + self._skills_mode and parsed_options.get("no_model_invocation") is True + ) + try: + if self._skills_mode: + created = self._setup_skills(project_root, manifest, parsed_options, **opts) + else: + created = self._setup_commands(project_root, manifest, parsed_options, **opts) + except BaseException: + self._disable_model_invocation = False + raise # Install agent runtime events event_files = self.emit_events( diff --git a/tests/integrations/test_integration_copilot.py b/tests/integrations/test_integration_copilot.py index 921b023be6..2f4621b2ba 100644 --- a/tests/integrations/test_integration_copilot.py +++ b/tests/integrations/test_integration_copilot.py @@ -2,14 +2,34 @@ import json import os +from pathlib import Path +from unittest.mock import patch import pytest import typer import yaml from specify_cli.integrations import get_integration +from specify_cli.integrations.base import SkillsIntegration from specify_cli.integrations.manifest import IntegrationManifest +REPO_ROOT = Path(__file__).resolve().parents[2] + + +@pytest.fixture(autouse=True) +def _reset_copilot_model_invocation(): + """``setup()`` stores ``--no-model-invocation`` on the process-wide Copilot + singleton; keep it from leaking between tests.""" + copilot = get_integration("copilot") + copilot._disable_model_invocation = False + yield + copilot._disable_model_invocation = False + + +def _skill_frontmatter(skill_file: Path) -> dict: + content = skill_file.read_text(encoding="utf-8") + return yaml.safe_load(content.split("---", 2)[1]) + class TestCopilotCommandsMode: def test_copilot_key_and_config(self): @@ -981,3 +1001,258 @@ def test_dispatch_detects_speckit_skills_layout(self, tmp_path): assert "my args" in prompt, ( f"Skills mode prompt should preserve user args, got: {prompt}" ) + + +class TestCopilotNoModelInvocationOption: + """``--integration-options="--no-model-invocation"`` opts out of model invocation (#2654).""" + + def test_options_include_no_model_invocation_flag(self): + opts = get_integration("copilot").options() + matching = [o for o in opts if o.name == "--no-model-invocation"] + assert len(matching) == 1 + assert matching[0].is_flag is True + # Opt-in: skills stay model-invocable unless the flag is passed. + assert matching[0].default is False + + def test_default_skills_omit_disable_model_invocation(self, tmp_path): + i = get_integration("copilot") + created = i.setup(tmp_path, IntegrationManifest("copilot", tmp_path), script_type="sh") + skill_files = [f for f in created if f.name == "SKILL.md"] + assert len(skill_files) > 0 + for f in skill_files: + assert "disable-model-invocation" not in f.read_text(encoding="utf-8") + + def test_setup_with_option_sets_disable_model_invocation_true(self, tmp_path): + i = get_integration("copilot") + m = IntegrationManifest("copilot", tmp_path) + created = i.setup( + tmp_path, m, parsed_options={"no_model_invocation": True}, script_type="sh" + ) + skill_files = [f for f in created if f.name == "SKILL.md"] + assert len(skill_files) > 0 + for f in skill_files: + fm = _skill_frontmatter(f) + assert fm["disable-model-invocation"] is True, f.parent.name + assert f.read_text(encoding="utf-8").count("disable-model-invocation:") == 1 + + def test_post_process_follows_option_from_setup(self, tmp_path): + """Preset overrides of core commands re-registered in the same run as + ``setup()`` (init, upgrade) get the same value.""" + i = get_integration("copilot") + i.setup( + tmp_path, + IntegrationManifest("copilot", tmp_path), + parsed_options={"no_model_invocation": True}, + script_type="sh", + ) + result = i.post_process_skill_content("---\nname: speckit-plan\n---\n\nBody\n") + assert "disable-model-invocation: true" in result + + def test_extension_skills_stay_model_invocable(self, tmp_path): + """Mandatory extension hooks rely on the model invoking the hook skill, + so the option never applies to extension commands.""" + i = get_integration("copilot") + i.setup( + tmp_path, + IntegrationManifest("copilot", tmp_path), + parsed_options={"no_model_invocation": True}, + script_type="sh", + ) + result = i.post_process_skill_content( + "---\nname: speckit-git-feature\n---\n\nBody\n" + ) + assert "disable-model-invocation" not in result + + def test_option_requires_speckit_prefixed_core_name(self, tmp_path): + """A skill merely named like a core stem is not a core command skill.""" + i = get_integration("copilot") + i.setup( + tmp_path, + IntegrationManifest("copilot", tmp_path), + parsed_options={"no_model_invocation": True}, + script_type="sh", + ) + result = i.post_process_skill_content("---\nname: tasks\n---\n\nBody\n") + assert "disable-model-invocation" not in result + + def test_injection_preserves_crlf(self, tmp_path): + i = get_integration("copilot") + i.setup( + tmp_path, + IntegrationManifest("copilot", tmp_path), + parsed_options={"no_model_invocation": True}, + script_type="sh", + ) + result = i.post_process_skill_content("---\r\nname: speckit-plan\r\n---\r\nBody\r\n") + assert "disable-model-invocation: true\r\n---" in result + assert "true\n---" not in result.replace("\r\n", "") + + def test_commands_layout_ignores_option(self, tmp_path): + i = get_integration("copilot") + created = i.setup( + tmp_path, + IntegrationManifest("copilot", tmp_path), + parsed_options={"commands": True, "no_model_invocation": True}, + script_type="sh", + ) + assert not (tmp_path / ".github" / "skills").exists() + for f in created: + if f.suffix == ".md": + assert "disable-model-invocation" not in f.read_text(encoding="utf-8") + result = i.post_process_skill_content("---\nname: speckit-plan\n---\n\nBody\n") + assert "disable-model-invocation" not in result + + def test_failed_setup_does_not_leave_option_set(self, tmp_path): + i = get_integration("copilot") + with patch.object(SkillsIntegration, "setup", side_effect=RuntimeError("boom")): + with pytest.raises(RuntimeError): + i.setup( + tmp_path, + IntegrationManifest("copilot", tmp_path), + parsed_options={"no_model_invocation": True}, + script_type="sh", + ) + result = i.post_process_skill_content("---\nname: speckit-plan\n---\n\nBody\n") + assert "disable-model-invocation" not in result + + def test_setup_without_option_resets(self, tmp_path): + i = get_integration("copilot") + first = tmp_path / "first" + first.mkdir() + i.setup( + first, + IntegrationManifest("copilot", first), + parsed_options={"no_model_invocation": True}, + script_type="sh", + ) + second = tmp_path / "second" + second.mkdir() + created = i.setup(second, IntegrationManifest("copilot", second), script_type="sh") + for f in (f for f in created if f.name == "SKILL.md"): + assert "disable-model-invocation" not in f.read_text(encoding="utf-8") + result = i.post_process_skill_content("---\nname: speckit-plan\n---\n\nBody\n") + assert "disable-model-invocation" not in result + + def test_cli_rejects_value_for_flag(self, tmp_path): + from typer.testing import CliRunner + from specify_cli import app + + project = tmp_path / "copilot-bad-option" + project.mkdir() + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke( + app, + [ + "init", + "--here", + "--integration", + "copilot", + "--integration-options", + "--no-model-invocation=yes", + "--script", + "sh", + "--ignore-agent-tools", + ], + catch_exceptions=False, + ) + finally: + os.chdir(old_cwd) + + assert result.exit_code != 0 + assert "does not accept a value" in result.output + assert not (project / ".github" / "skills").exists() + + def test_option_persists_across_upgrade_and_can_be_cleared(self, tmp_path): + from tests.specify_cli.integrations._helpers import _init_project, _run_in_project + + project = _init_project(tmp_path, "copilot", "--no-model-invocation") + skills_dir = project / ".github" / "skills" + skill_files = sorted(skills_dir.glob("speckit-*/SKILL.md")) + assert len(skill_files) > 0 + for f in skill_files: + assert _skill_frontmatter(f)["disable-model-invocation"] is True, f.parent.name + + state = json.loads((project / ".specify" / "integration.json").read_text(encoding="utf-8")) + settings = state["integration_settings"]["copilot"] + assert settings["raw_options"] == "--no-model-invocation" + assert settings["parsed_options"] == {"no_model_invocation": True} + + # A later ``specify integration upgrade`` is a new process: drop the + # in-memory value so only the persisted options can supply it. + get_integration("copilot")._disable_model_invocation = False + result = _run_in_project(project, ["integration", "upgrade", "copilot", "--force"]) + assert result.exit_code == 0, result.output + for f in skill_files: + assert _skill_frontmatter(f)["disable-model-invocation"] is True, f.parent.name + + # Passing replacement options (here: none) clears it. + result = _run_in_project( + project, + ["integration", "upgrade", "copilot", "--force", "--integration-options", ""], + ) + assert result.exit_code == 0, result.output + for f in skill_files: + assert "disable-model-invocation" not in f.read_text(encoding="utf-8"), f.parent.name + state = json.loads((project / ".specify" / "integration.json").read_text(encoding="utf-8")) + assert "parsed_options" not in state["integration_settings"]["copilot"] + + def test_upgrade_reapplies_option_to_preset_overrides(self, tmp_path): + """``preset add`` runs without ``setup()`` (documented limitation); + ``integration upgrade copilot --force`` reapplies the option to the + core-command skills a preset overrides.""" + from tests.specify_cli.integrations._helpers import _init_project, _run_in_project + + project = _init_project(tmp_path, "copilot", "--no-model-invocation") + skill_file = project / ".github" / "skills" / "speckit-plan" / "SKILL.md" + assert _skill_frontmatter(skill_file)["disable-model-invocation"] is True + + get_integration("copilot")._disable_model_invocation = False + result = _run_in_project( + project, ["preset", "add", "--dev", str(REPO_ROOT / "presets" / "lean")] + ) + assert result.exit_code == 0, result.output + fm = _skill_frontmatter(skill_file) + assert fm["metadata"]["source"] == "preset:lean" + # Documented in docs/integrations/copilot.md; update the docs if + # preset commands start honouring the option. + assert "disable-model-invocation" not in fm + + # The docs say ``--force`` is needed here: preset add modified + # installed skill files, so a plain upgrade refuses. + result = _run_in_project(project, ["integration", "upgrade", "copilot"]) + assert result.exit_code != 0 + assert "modified since installation" in result.output + assert "disable-model-invocation" not in _skill_frontmatter(skill_file) + + get_integration("copilot")._disable_model_invocation = False + result = _run_in_project(project, ["integration", "upgrade", "copilot", "--force"]) + assert result.exit_code == 0, result.output + fm = _skill_frontmatter(skill_file) + assert fm["metadata"]["source"] == "preset:lean" + assert fm["disable-model-invocation"] is True + + def test_extension_hook_skills_stay_model_invocable_after_upgrade(self, tmp_path): + """``integration upgrade`` re-registers extension skills in the same + process as ``setup()``; they must stay model-invocable so mandatory + hooks (e.g. ``speckit.git.feature``) can still be invoked by the model.""" + from tests.specify_cli.integrations._helpers import _init_project, _run_in_project + + project = _init_project(tmp_path, "copilot", "--no-model-invocation") + result = _run_in_project( + project, ["extension", "add", "--dev", str(REPO_ROOT / "extensions" / "git")] + ) + assert result.exit_code == 0, result.output + get_integration("copilot")._disable_model_invocation = False + result = _run_in_project(project, ["integration", "upgrade", "copilot", "--force"]) + assert result.exit_code == 0, result.output + + skills_dir = project / ".github" / "skills" + git_skills = sorted(skills_dir.glob("speckit-git-*/SKILL.md")) + assert git_skills, "git extension skills were not generated" + for f in git_skills: + assert "disable-model-invocation" not in f.read_text(encoding="utf-8"), f.parent.name + assert _skill_frontmatter(skills_dir / "speckit-specify" / "SKILL.md")[ + "disable-model-invocation" + ] is True