From 12a3d67731ef6f5e302a9d9ca5bf4bbddfed0088 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:44:15 +0200 Subject: [PATCH] fix: address third round of review feedback (multi-integration semantics) Fixes five deeper active-only registration bugs surfaced by Copilot review after 2486c08, all in the presets/extensions single-active integration rule (#2948): 1. presets: _reconcile_composed_commands (run after install/remove) bypassed the active-only filter entirely, writing composition-winner command files for every detected non-skill agent via register_commands_for_non_skill_agents. Added an only_agent param to that registrar method (mirroring register_commands_for_all_agents) and threaded it through all 5 reconciliation call sites. 2. presets: `integration use copilot` with --skills (ai_skills: true) wrote both the static .agent.md command file AND the SKILL.md mirror for the same override. Mirrored the extension path's ai_skills guard in both _register_commands and the reconciliation pass: a command-backed active agent running in skills mode is excluded from non-skill command registration. 3. presets: registered_skills was a flat list, so switching between two skill-mode agents (e.g. Claude -> Codex) and then removing the preset only restored the currently active agent's directory, permanently orphaning the other. _unregister_skills now restores every existing skill-mode agent directory instead of only the active one. 4. extensions: load_init_options() collapses "no file" and "corrupted file" into the same {}, so the round-2 fail-closed fix didn't actually distinguish them. Added a shared resolve_active_agent_for_registration() helper in _init_options.py that checks file existence separately from parse success, returning a distinct sentinel for "file absent" vs None for "corrupted or invalid". extensions/__init__.py now uses this helper. 5. presets: same corruption-collapsing bug in _register_commands's active_agent resolution. Now uses the same shared helper as (4). Adds regression tests for all five: reconciliation active-only filtering, copilot --skills dual-write prevention, multi-skill-agent switch+remove, and corrupted init-options fail-closed behavior for both extension add and preset add. Each test was verified to fail against the pre-fix code and pass with the fix. Targeted (883) and full (3923 passed, 109 skipped) suites pass; ruff check clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/_init_options.py | 44 +++- src/specify_cli/agents.py | 6 + src/specify_cli/extensions/__init__.py | 45 ++-- src/specify_cli/presets/__init__.py | 164 ++++++++++-- .../test_integration_subcommand.py | 35 +++ tests/test_presets.py | 243 ++++++++++++++++++ 6 files changed, 501 insertions(+), 36 deletions(-) diff --git a/src/specify_cli/_init_options.py b/src/specify_cli/_init_options.py index 6acf4a000..7aa594b0c 100644 --- a/src/specify_cli/_init_options.py +++ b/src/specify_cli/_init_options.py @@ -3,12 +3,22 @@ import json from collections.abc import Mapping from pathlib import Path -from typing import Any +from typing import Any, Union INIT_OPTIONS_FILE = ".specify/init-options.json" +class _MissingInitOptionsFile: + """Sentinel: init-options.json does not exist at all (legacy layout).""" + + def __repr__(self) -> str: # pragma: no cover - debug aid only + return "MISSING_INIT_OPTIONS_FILE" + + +MISSING_INIT_OPTIONS_FILE = _MissingInitOptionsFile() + + def save_init_options(project_path: Path, options: dict[str, Any]) -> None: """Persist the CLI options used during ``specify init``.""" dest = project_path / INIT_OPTIONS_FILE @@ -34,3 +44,35 @@ def load_init_options(project_path: Path) -> dict[str, Any]: def is_ai_skills_enabled(opts: Mapping[str, Any] | None) -> bool: """Return True only when init options explicitly enable AI skills.""" return isinstance(opts, Mapping) and opts.get("ai_skills") is True + + +def resolve_active_agent_for_registration( + project_path: Path, +) -> Union[str, None, _MissingInitOptionsFile]: + """Resolve the active integration key for active-only registration (#2948). + + ``load_init_options`` collapses "no file", "unreadable/malformed file", + and "valid file with no recorded active agent" into the same ``{}`` + result, which previously made corrupted-but-present init-options behave + like a legacy pre-init-options project and fall back to registering + every detected agent. This helper distinguishes those cases explicitly: + + - Returns :data:`MISSING_INIT_OPTIONS_FILE` when init-options.json does + not exist at all (pre-init-options layout or direct library use). + Callers should fall back to detection-based registration for all + agents, matching the original pre-#2948 behavior for such projects. + - Returns ``None`` when init-options.json exists but could not provide a + valid non-empty string active agent (malformed/unreadable JSON, + non-object payload, or a non-string/empty ``ai`` value). Callers must + fail closed (register nothing) rather than treat this like "no file" + or pass a non-string key into agent-config lookups. + - Returns the active agent key (a non-empty string) otherwise. + """ + path = project_path / INIT_OPTIONS_FILE + if not path.exists(): + return MISSING_INIT_OPTIONS_FILE + + active_agent = load_init_options(project_path).get("ai") + if isinstance(active_agent, str) and active_agent: + return active_agent + return None diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 26add257f..735032039 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -1082,6 +1082,7 @@ class CommandRegistrar: context_note: Optional[str] = None, link_outputs: bool = False, extension_id: Optional[str] = None, + only_agent: Optional[str] = None, ) -> Dict[str, List[str]]: """Register commands for all non-skill agents in the project. @@ -1098,6 +1099,9 @@ class CommandRegistrar: link_outputs: If True, create dev-mode symlinks for rendered command files when supported by the OS. extension_id: Extension id when rendering extension-owned commands. + only_agent: If set, restrict registration to this single agent + (#2948). An agent name that matches no configured agent + (e.g. an empty string) yields no registrations at all. Returns: Dictionary mapping agent names to list of registered commands @@ -1105,6 +1109,8 @@ class CommandRegistrar: results = {} self._ensure_configs() for agent_name, agent_config in self.AGENT_CONFIGS.items(): + if only_agent is not None and agent_name != only_agent: + continue if agent_config.get("extension") == "/SKILL.md": continue detect_dir_str = agent_config.get("detect_dir") diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 1375dd60d..2891ad694 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -989,29 +989,34 @@ class ExtensionManager: when selected via ``integration use`` / ``switch`` (rescaffold). Projects without a recorded active integration at all (pre-init-options - layouts or direct library use) fall back to detection-based - registration for all agents. A *recorded* active key that has no - registrar config (e.g. ``generic``, which is deliberately excluded - from ``AGENT_CONFIGS``) is not treated as "no active integration" — - it must not cause registration to target other detected agents. + layouts or direct library use, i.e. init-options.json does not + exist) fall back to detection-based registration for all agents. A + *recorded* active key that has no registrar config (e.g. ``generic``, + which is deliberately excluded from ``AGENT_CONFIGS``) is not treated + as "no active integration" — it must not cause registration to + target other detected agents. - A recorded but malformed ``ai`` value (non-string, e.g. ``[]`` or - ``null``) is also not "no active integration" — corrupted - init-options must fail closed (register nothing) rather than - fall back to registering every detected agent. + An init-options.json that exists but is corrupted, unreadable, or + has a malformed/empty ``ai`` value (e.g. ``[]`` or ``null``) is also + not "no active integration" — fail closed (register nothing) rather + than fall back to registering every detected agent, which would + otherwise happen because a corrupted file loads the same as an + absent one. Returns: Mapping of agent name to registered command names, matching the ``registered_commands`` registry shape. """ from .. import load_init_options + from .._init_options import ( + MISSING_INIT_OPTIONS_FILE, + resolve_active_agent_for_registration, + ) registrar = CommandRegistrar() - init_options = load_init_options(self.project_root) - if not isinstance(init_options, dict): - init_options = {} + active_agent = resolve_active_agent_for_registration(self.project_root) - if "ai" not in init_options: + if active_agent is MISSING_INIT_OPTIONS_FILE: return registrar.register_commands_for_all_agents( manifest, extension_dir, @@ -1020,14 +1025,16 @@ class ExtensionManager: create_missing_active_skills_dir=True, ) - active_agent = init_options.get("ai") - if not isinstance(active_agent, str) or not active_agent: - # A recorded key was found but it is malformed (not a non-empty - # string). Fail closed instead of falling back to all agents or - # passing a non-string key into AGENT_CONFIGS.get() below, which - # would raise TypeError for unhashable values like a list. + if active_agent is None: + # init-options.json exists but could not provide a valid active + # agent (corrupted/unreadable/non-object JSON, or a malformed + # "ai" value). Fail closed instead of falling back to all agents + # or passing a non-string key into AGENT_CONFIGS.get() below, + # which would raise TypeError for unhashable values like a list. return {} + init_options = load_init_options(self.project_root) + # A recorded active key with no registrar config (e.g. "generic", # deliberately excluded from AGENT_CONFIGS) has nothing to register # through this path, but it is still an active integration. Passing diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 516083136..df76828cb 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -28,7 +28,12 @@ from packaging import version as pkg_version from packaging.specifiers import SpecifierSet, InvalidSpecifier from ..extensions import REINSTALL_COMMAND, ExtensionRegistry, normalize_priority -from .._init_options import is_ai_skills_enabled, load_init_options +from .._init_options import ( + MISSING_INIT_OPTIONS_FILE, + is_ai_skills_enabled, + load_init_options, + resolve_active_agent_for_registration, +) from ..integrations.base import IntegrationBase from .._utils import dump_frontmatter, version_satisfies from ..shared_infra import verify_archive_sha256 @@ -683,10 +688,39 @@ class PresetManager: # Single-active rule (#2948): preset command overrides register for # the active integration only. A project without a recorded active - # integration falls back to detection-based registration for all - # agents; a recorded key with no registrar config (e.g. "generic") - # naturally yields no matches via only_agent instead of falling back. - active_agent = load_init_options(self.project_root).get("ai") + # integration (init-options.json does not exist at all — a legacy + # pre-init-options layout or direct library use) falls back to + # detection-based registration for all agents. A recorded key with + # no registrar config (e.g. "generic") naturally yields no matches + # via only_agent instead of falling back. + # + # An init-options.json that exists but is corrupted, unreadable, or + # has a malformed/empty "ai" value must not be treated the same as + # "no file" — that would silently reintroduce all-agent + # registration. Fail closed (register nothing) instead. + resolved_agent = resolve_active_agent_for_registration(self.project_root) + if resolved_agent is MISSING_INIT_OPTIONS_FILE: + active_agent = None + elif resolved_agent is None: + return {} + else: + active_agent = resolved_agent + # Mirror the extension path's ai_skills guard: when the active + # agent is a command-backed integration (extension != "/SKILL.md") + # running in skills mode, its preset command overrides render as + # skills via _register_skills, not as command files. Command-mode + # and skills-mode artifacts are mutually exclusive — writing both + # (e.g. `integration use copilot` with `--skills`) leaves a stale + # command file alongside the SKILL.md that is actually active. + init_options = load_init_options(self.project_root) + agent_config = registrar.AGENT_CONFIGS.get(active_agent) + if ( + agent_config + and is_ai_skills_enabled(init_options) + and agent_config.get("extension") != "/SKILL.md" + ): + return {} + return registrar.register_commands_for_all_agents( commands_to_register, manifest.id, @@ -787,6 +821,15 @@ class PresetManager: reflect the current priority stack rather than depending on install/remove order. + Single-active rule (#2948): non-skill command-file registration + performed by this pass is restricted to the active integration, the + same as ``_register_commands``. Without this, reconciliation after + install/remove would write command files for every detected + non-skill agent even though registration itself is active-only, + leaving inactive integrations with artifacts that are never + recorded in ``registered_commands`` (and therefore never cleaned up + on removal). + Args: command_names: List of command names to reconcile """ @@ -801,6 +844,30 @@ class PresetManager: resolver = PresetResolver(self.project_root) registrar = CommandRegistrar() + # Resolve the active-only restriction once. MISSING_INIT_OPTIONS_FILE + # (legacy pre-init-options project) keeps the pre-#2948 fallback of + # registering every detected non-skill agent; a corrupted/malformed + # init-options.json fails closed via a sentinel that matches no real + # agent name instead of silently falling back to "no restriction". + resolved_agent = resolve_active_agent_for_registration(self.project_root) + if resolved_agent is MISSING_INIT_OPTIONS_FILE: + only_agent: Optional[str] = None + elif resolved_agent is None: + only_agent = "" + else: + only_agent = resolved_agent + # Mirror _register_commands's ai_skills guard: a command-backed + # active agent running in skills mode renders preset/extension + # overrides as skills, not command files, so this non-skill + # command reconciliation pass must not target it either. + agent_config = registrar.AGENT_CONFIGS.get(only_agent) + if ( + agent_config + and is_ai_skills_enabled(load_init_options(self.project_root)) + and agent_config.get("extension") != "/SKILL.md" + ): + only_agent = "" + # Cache registry and manifests outside the loop to avoid # repeated filesystem reads for each command name. presets_by_priority = list(self.registry.list_by_priority()) @@ -830,7 +897,8 @@ class PresetManager: for tmpl in manifest.templates: if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": self._register_for_non_skill_agents( - registrar, [tmpl], manifest.id, pack_dir + registrar, [tmpl], manifest.id, pack_dir, + only_agent=only_agent, ) registered = True break @@ -858,6 +926,7 @@ class PresetManager: matching_cmds, ext_id, ext_dir, self.project_root, context_note=f"\n\n\n", + only_agent=only_agent, ) registered = True except Exception: @@ -869,6 +938,7 @@ class PresetManager: self._register_command_from_path( registrar, cmd_name, top_path, source_id=source_id, + only_agent=only_agent, ) else: # Composed command — resolve from full stack @@ -919,6 +989,7 @@ class PresetManager: registrar, [{**tmpl, "file": f".composed/{cmd_name}.md"}], manifest.id, pack_dir, + only_agent=only_agent, ) registered = True break @@ -940,6 +1011,7 @@ class PresetManager: self._register_command_from_path( registrar, cmd_name, composed_file, source_id=source_id, + only_agent=only_agent, ) def _register_command_from_path( @@ -948,6 +1020,7 @@ class PresetManager: cmd_name: str, cmd_path: Path, source_id: str = "reconciled", + only_agent: Optional[str] = None, ) -> None: """Register a single command from a file path (non-preset source). @@ -959,6 +1032,7 @@ class PresetManager: cmd_name: Command name cmd_path: Path to the command file source_id: Source attribution for rendered output + only_agent: If set, restrict registration to this single agent (#2948). """ if not cmd_path.exists(): return @@ -988,7 +1062,8 @@ class PresetManager: except Exception: pass # best-effort alias loading self._register_for_non_skill_agents( - registrar, [cmd_tmpl], source_id, cmd_path.parent + registrar, [cmd_tmpl], source_id, cmd_path.parent, + only_agent=only_agent, ) def _register_for_non_skill_agents( @@ -997,6 +1072,7 @@ class PresetManager: commands: List[Dict[str, Any]], source_id: str, source_dir: Path, + only_agent: Optional[str] = None, ) -> None: """Register commands for non-skill agents during reconciliation. @@ -1010,9 +1086,15 @@ class PresetManager: Writing raw command content to skill agents would produce invalid SKILL.md files (missing skill frontmatter, descriptions, etc.). + + Args: + only_agent: If set, restrict registration to this single agent, + matching the active-only rule applied by ``_register_commands`` + (#2948). """ registrar.register_commands_for_non_skill_agents( - commands, source_id, source_dir, self.project_root + commands, source_id, source_dir, self.project_root, + only_agent=only_agent, ) class _FilteredManifest: @@ -1445,6 +1527,45 @@ class PresetManager: return written + def _tracked_skill_agent_dirs(self) -> List[tuple]: + """Return (skills_dir, agent_name) pairs for every skill-mode + integration directory that currently exists under the project root. + + ``registered_skills`` only tracks skill *names*, not which agent + directories they were written under, so a preset used first under + one skill-mode agent and later switched to another can have live + overrides in both directories at removal time. Restoring every + existing skill-mode directory (instead of only the currently active + one) ensures none of them are left permanently orphaned. + + Multiple integration keys can share the same physical directory + (e.g. ``agy``/``codex``/``zed`` all use ``.agents/skills``); only one + representative agent name is kept per unique resolved directory so + each physical directory is processed exactly once. + """ + from .. import _get_skills_dir as _resolve_skills_dir + from ..integrations import INTEGRATION_REGISTRY + from ..integrations.base import SkillsIntegration + + seen: Dict[Path, str] = {} + for key in sorted(INTEGRATION_REGISTRY): + integration = INTEGRATION_REGISTRY[key] + if not ( + isinstance(integration, SkillsIntegration) + or getattr(integration, "_skills_mode", False) + ): + continue + skills_dir = _resolve_skills_dir(self.project_root, key) + if not skills_dir.is_dir(): + continue + try: + resolved = skills_dir.resolve() + except OSError: + continue + seen.setdefault(resolved, key) + + return [(path, agent) for path, agent in seen.items()] + def _unregister_skills(self, skill_names: List[str], preset_dir: Path) -> None: """Restore original SKILL.md files after a preset is removed. @@ -1452,6 +1573,11 @@ class PresetManager: regenerate the skill from the core command template. If no core template exists, the skill directory is removed. + Restores across every existing skill-mode agent directory (see + :meth:`_tracked_skill_agent_dirs`), not just the currently active + integration, so switching integrations before removal can't leave a + preset override behind permanently. + Args: skill_names: List of skill names written by the preset. preset_dir: The preset's installed directory (may already be deleted). @@ -1459,20 +1585,26 @@ class PresetManager: if not skill_names: return - skills_dir = self._get_skills_dir() - if not skills_dir: - return + for skills_dir, agent_name in self._tracked_skill_agent_dirs(): + self._unregister_skills_in_dir(skill_names, skills_dir, agent_name) - from .. import SKILL_DESCRIPTIONS, load_init_options + def _unregister_skills_in_dir( + self, skill_names: List[str], skills_dir: Path, selected_ai: Optional[str] + ) -> None: + """Restore original SKILL.md files within a single skills directory. + + Args: + skill_names: List of skill names written by the preset. + skills_dir: The skills directory to restore within. + selected_ai: The agent name that owns ``skills_dir``, used for + placeholder resolution and argument-hint formatting. + """ + from .. import SKILL_DESCRIPTIONS from ..agents import CommandRegistrar from ..integrations import get_integration # Locate core command templates from the project's installed templates core_templates_dir = self.project_root / ".specify" / "templates" / "commands" - init_opts = load_init_options(self.project_root) - if not isinstance(init_opts, dict): - init_opts = {} - selected_ai = init_opts.get("ai") registrar = CommandRegistrar() integration = get_integration(selected_ai) if isinstance(selected_ai, str) else None extension_restore_index = self._build_extension_skill_restore_index() diff --git a/tests/integrations/test_integration_subcommand.py b/tests/integrations/test_integration_subcommand.py index 32c51494b..8720e9ea7 100644 --- a/tests/integrations/test_integration_subcommand.py +++ b/tests/integrations/test_integration_subcommand.py @@ -1490,6 +1490,41 @@ class TestIntegrationInstall: "back-fill every detected agent (#2948)" ) + def test_extension_add_corrupted_init_options_file_fails_closed(self, tmp_path): + """A present-but-unparseable init-options.json must fail closed too, + not be treated the same as "no file at all". + + ``load_init_options`` returns ``{}`` for a corrupted/unreadable + file just like it does for a missing file, so a naive "no active + agent recorded" check based on ``load_init_options`` alone can't + tell a legacy pre-init-options project (legitimate all-agent + fallback) apart from a corrupted-but-present file for a #2948 + project (must fail closed). Corrupting the file after a normal + init must not reintroduce the all-agent fallback. + """ + project = _init_project(tmp_path, "claude") + + result = _run_in_project(project, [ + "integration", "install", "codex", + "--script", "sh", + ]) + assert result.exit_code == 0, result.output + + init_options_path = project / ".specify" / "init-options.json" + init_options_path.write_text("{not valid json", encoding="utf-8") + + result = _run_in_project(project, ["extension", "add", "git"]) + assert result.exit_code == 0, f"extension add failed: {result.output}" + + registry_path = project / ".specify" / "extensions" / ".registry" + registered = json.loads(registry_path.read_text(encoding="utf-8"))[ + "extensions" + ]["git"]["registered_commands"] + assert registered == {}, ( + "a corrupted init-options.json must fail closed, not be " + "treated like a legacy project missing the file entirely (#2948)" + ) + # ── uninstall ──────────────────────────────────────────────────────── diff --git a/tests/test_presets.py b/tests/test_presets.py index 54cb9dd5b..6c468e444 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3063,6 +3063,57 @@ class TestInitOptions: assert is_ai_skills_enabled({"ai_skills": value}) is expected +class TestResolveActiveAgentForRegistration: + """Tests for the shared #2948 active-agent resolution helper. + + ``load_init_options`` collapses "no file", "corrupted file", and + "valid file with no active agent" into the same ``{}``. Extensions and + presets both need to tell those apart: no file means "legacy project, + fall back to all detected agents"; a corrupted or malformed file means + "fail closed, register nothing" so a corrupted init-options.json can't + silently reintroduce all-agent registration. + """ + + def test_missing_file_returns_sentinel(self, project_dir): + from specify_cli._init_options import ( + MISSING_INIT_OPTIONS_FILE, + resolve_active_agent_for_registration, + ) + + assert ( + resolve_active_agent_for_registration(project_dir) + is MISSING_INIT_OPTIONS_FILE + ) + + def test_valid_active_agent_returns_string(self, project_dir): + from specify_cli import save_init_options + from specify_cli._init_options import resolve_active_agent_for_registration + + save_init_options(project_dir, {"ai": "claude"}) + + assert resolve_active_agent_for_registration(project_dir) == "claude" + + def test_corrupted_json_fails_closed(self, project_dir): + """A present-but-unparseable file must not behave like "no file".""" + from specify_cli._init_options import resolve_active_agent_for_registration + + opts_file = project_dir / ".specify" / "init-options.json" + opts_file.parent.mkdir(parents=True, exist_ok=True) + opts_file.write_text("{bad json", encoding="utf-8") + + assert resolve_active_agent_for_registration(project_dir) is None + + @pytest.mark.parametrize("value", [[], {}, "", 0, None, ["claude"]]) + def test_malformed_ai_value_fails_closed(self, project_dir, value): + """A recorded but non-string/empty ``ai`` value fails closed too.""" + from specify_cli import save_init_options + from specify_cli._init_options import resolve_active_agent_for_registration + + save_init_options(project_dir, {"ai": value}) + + assert resolve_active_agent_for_registration(project_dir) is None + + class TestPresetSkills: """Tests for preset skill registration and unregistration. @@ -4011,6 +4062,198 @@ class TestPresetSkills: skill_content = (skills_dir / "speckit-specify" / "SKILL.md").read_text() assert "untouched" in skill_content + def test_preset_add_corrupted_init_options_fails_closed(self, project_dir, temp_dir): + """Corrupted (but present) init-options.json must not back-fill every + detected agent for preset command registration. + + Before the shared ``resolve_active_agent_for_registration`` fix, + ``load_init_options`` returning ``{}`` for a corrupted file was + indistinguishable from "no file at all", so ``_register_commands`` + treated it like a legacy pre-init-options project and registered + the preset's command override for every detected agent (#2948). + """ + init_options = project_dir / ".specify" / "init-options.json" + init_options.parent.mkdir(parents=True, exist_ok=True) + init_options.write_text("{not valid json", encoding="utf-8") + + gemini_dir = project_dir / ".gemini" / "commands" + gemini_dir.mkdir(parents=True) + + preset_dir = self._create_command_preset( + temp_dir, "corrupt-init-preset", "speckit.specify", + "Corrupt init test", "preset body", + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + metadata = manager.registry.get("corrupt-init-preset") + assert metadata.get("registered_commands") == {}, ( + "a corrupted init-options.json must fail closed, not " + "back-fill every detected agent (#2948)" + ) + assert not list(gemini_dir.glob("*specify*")), ( + "no command file should be written for any agent when " + "init-options.json is corrupted" + ) + + def test_reconciliation_restricted_to_active_agent(self, project_dir, temp_dir): + """Reconciliation after install/remove must also respect the + single-active rule, not just the initial registration. + + ``_reconcile_composed_commands`` (invoked after + ``install_from_directory``/``remove``) resolves composition winners + via ``register_commands_for_non_skill_agents``, a separate code + path from ``_register_commands``'s initial registration. Before the + fix it ignored the active-agent restriction entirely and wrote the + winning content for every detected non-skill agent, leaving + untracked orphaned artifacts in inactive integrations (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + gemini_dir = project_dir / ".gemini" / "commands" + gemini_dir.mkdir(parents=True) + + # A non-replace (append) strategy command forces reconciliation to + # run register_commands_for_non_skill_agents for every non-skill + # agent directory it detects. + preset_dir = temp_dir / "reconcile-active-only" + preset_dir.mkdir() + (preset_dir / "commands").mkdir() + (preset_dir / "commands" / "speckit.specify.md").write_text( + "---\ndescription: Appended\nstrategy: append\n---\n\nAppended body\n", + encoding="utf-8", + ) + manifest_data = { + "schema_version": "1.0", + "preset": { + "id": "reconcile-active-only", + "name": "Reconcile Active Only", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [{ + "type": "command", + "name": "speckit.specify", + "file": "commands/speckit.specify.md", + "strategy": "append", + }] + }, + } + with open(preset_dir / "preset.yml", "w") as f: + yaml.dump(manifest_data, f) + + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + assert not list(gemini_dir.glob("*specify*")), ( + "reconciliation must not write command files for a detected " + "but inactive non-skill agent (#2948)" + ) + + def test_copilot_skills_mode_skips_command_registration(self, project_dir, temp_dir): + """``integration use copilot`` with skills mode enabled must only + write the SKILL.md mirror, not also copilot's static command file. + + Copilot is command-backed (``extension: ".agent.md"``), but when + ``ai_skills`` is enabled its preset overrides are meant to render + exclusively as skills via ``_register_skills``. Before the fix, + ``_register_commands`` had no ``ai_skills`` guard (unlike the + extensions path), so both a stale ``.agent.md`` command file and + the ``SKILL.md`` mirror were written for the same override (#2948). + """ + self._write_init_options(project_dir, ai="copilot", ai_skills=True) + copilot_commands_dir = project_dir / ".github" / "agents" + copilot_commands_dir.mkdir(parents=True) + skills_dir = project_dir / ".github" / "skills" + self._create_skill(skills_dir, "speckit-specify") + + preset_dir = self._create_command_preset( + temp_dir, "copilot-skills-preset", "speckit.specify", + "Copilot skills test", "preset body", + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + assert not list(copilot_commands_dir.glob("*specify*")), ( + "command-mode and skills-mode artifacts are mutually exclusive: " + "no .agent.md command file should be written when copilot is " + "running in skills mode (#2948)" + ) + skill_file = skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:copilot-skills-preset" in skill_file.read_text() + + def test_skill_switch_then_remove_restores_every_skill_agent_dir( + self, project_dir, temp_dir + ): + """Switching between two skill-mode agents before removing a preset + must restore both agents' directories, not just the currently + active one. + + ``registered_skills`` is a flat list of skill names shared across + agents, so it can't tell which agent directories a preset actually + touched. Before the fix, ``_unregister_skills`` only restored the + currently active agent's skills directory; a preset used first + under Claude and later switched to Codex would have its Claude + override left behind permanently on removal (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + + core_cmds = project_dir / ".specify" / "templates" / "commands" + core_cmds.mkdir(parents=True, exist_ok=True) + (core_cmds / "specify.md").write_text( + "---\ndescription: Core specify command\n---\n\nCore specify body\n", + encoding="utf-8", + ) + + # Native skill agents only materialize a *brand-new* preset skill + # when their skills directory already exists (mirrors every other + # skill test in this class); pre-create both agents' directories so + # install and the later switch both find an existing skill to + # overwrite via _register_commands/_register_skills. + claude_skills_dir = project_dir / ".claude" / "skills" + self._create_skill(claude_skills_dir, "speckit-specify") + codex_skills_dir = project_dir / ".agents" / "skills" + self._create_skill(codex_skills_dir, "speckit-specify") + + preset_dir = self._create_command_preset( + temp_dir, "multi-skill-agent-preset", "speckit.specify", + "Multi skill agent test", "preset body", + ) + + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + claude_skill = claude_skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:multi-skill-agent-preset" in claude_skill.read_text() + + # Switch the active agent to codex (a different skill-mode agent) + # and re-register enabled presets for it, mirroring what + # `integration use codex` does. + self._write_init_options(project_dir, ai="codex", ai_skills=True) + manager.register_enabled_presets_for_agent("codex") + + codex_skill = codex_skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:multi-skill-agent-preset" in codex_skill.read_text(), ( + "sanity: switching to codex should rescaffold the preset there" + ) + assert "preset:multi-skill-agent-preset" in claude_skill.read_text(), ( + "sanity: the previous agent's registration is preserved on switch" + ) + + assert manager.remove("multi-skill-agent-preset") is True + + for skill_file, label in ((claude_skill, "claude"), (codex_skill, "codex")): + assert skill_file.exists(), f"{label} skill file should still exist after removal" + content = skill_file.read_text() + assert "preset:multi-skill-agent-preset" not in content, ( + f"{label}'s preset override must be restored on removal, " + "not orphaned permanently (#2948)" + ) + assert "Core specify body" in content + class TestPresetSetPriority: """Test preset set-priority CLI command."""