diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 2891ad694..669de120c 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1805,6 +1805,22 @@ class ExtensionManager: and bool(agent_config) and agent_config.get("extension") != "/SKILL.md" ) + # Mirror image of skills_mode_active: this agent is command-backed, + # active, and currently in command mode. Used to detect a + # skills -> command toggle for this same agent, where the skills + # phase below returns empty (its directory no longer resolves) but + # a previously-written extension SKILL.md is now stale (#2948). + command_mode_active = ( + active_agent == agent_name + and not ai_skills_enabled + and bool(agent_config) + and agent_config.get("extension") != "/SKILL.md" + ) + agent_skills_dir = None + if agent_config and agent_config.get("extension") != "/SKILL.md": + from .. import _get_skills_dir as _resolve_agent_skills_dir + + agent_skills_dir = _resolve_agent_skills_dir(self.project_root, agent_name) for ext_id, metadata in self.registry.list().items(): if not metadata.get("enabled", True): @@ -1840,6 +1856,30 @@ class ExtensionManager: new_registered.pop(agent_name, None) if new_registered != registered_commands: updates["registered_commands"] = new_registered + elif agent_config and skills_mode_active: + # Toggled command -> skills for this same agent: the + # commands phase above is skipped, but a command file + # this extension previously wrote for this agent while + # command mode was active is still on disk and still + # tracked. Remove it narrowly for this agent so + # command-mode and skills-mode artifacts stay mutually + # exclusive, matching unregister_agent_artifacts's + # per-agent command cleanup (#2948). + registered_commands = metadata.get("registered_commands", {}) + if isinstance(registered_commands, dict) and registered_commands.get( + agent_name + ): + stale_commands = self._valid_name_list( + registered_commands.get(agent_name) + ) + if stale_commands: + registrar.unregister_commands( + {agent_name: stale_commands}, self.project_root + ) + new_registered = copy.deepcopy(registered_commands) + new_registered.pop(agent_name, None) + if new_registered != registered_commands: + updates["registered_commands"] = new_registered # Extension *skills* are only ever rendered for the active agent: # `_register_extension_skills` resolves the skills dir and @@ -1879,6 +1919,34 @@ class ExtensionManager: dict.fromkeys(existing_skills + registered_skills) ) updates["registered_skills"] = merged_skills + elif command_mode_active and agent_skills_dir is not None: + # Mirror image: toggled skills -> command for + # this same agent. _register_extension_skills + # returned empty because this agent's skills + # directory no longer resolves once ai_skills is + # off, but a SKILL.md this extension wrote while + # skills mode was active may still be tracked + # and still on disk. Remove it narrowly for this + # agent's directory only (#2948). + existing_skills = self._valid_name_list( + metadata.get("registered_skills", []) + ) + owned_here = [ + name + for name in existing_skills + if (agent_skills_dir / name).is_dir() + ] + if owned_here: + self._unregister_extension_skills( + owned_here, ext_id, skills_dir=agent_skills_dir + ) + remaining = [ + name + for name in existing_skills + if (agent_skills_dir / name).is_dir() + ] + if remaining != existing_skills: + updates["registered_skills"] = remaining if updates: self.registry.update(ext_id, updates) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 1941da3d4..b67100b27 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -804,11 +804,36 @@ class PresetManager: if merged_commands != existing_commands: self.registry.update(pack_id, {"registered_commands": merged_commands}) + # Record this preset's command names for reconciliation now, + # right after the commands phase succeeds and before calling + # the independently fallible _register_skills(). Both used to + # be recorded together after skills also succeeded; if skills + # raised, the exception (caught below) skipped this entirely, + # so a preset whose commands phase wrote real content never + # got reconciled against the full priority stack and its raw + # content could be left in place instead of a project + # override or higher-precedence preset (#2948). + for tmpl in manifest.templates: + if tmpl.get("type") == "command": + affected_cmd_names.add(tmpl["name"]) + registered_skills = self._register_skills(manifest, pack_dir) raw_existing_skills = metadata.get("registered_skills") - existing_skills = self._normalize_registered_skills( - raw_existing_skills, fallback_agent=agent_name - ) + if isinstance(raw_existing_skills, list) and raw_existing_skills: + # Legacy flat-list value: don't assume agent_name wrote + # every name (the first post-upgrade operation may be a + # direct switch to a different skill-mode agent) — + # infer real ownership from on-disk provenance instead + # (#2948). + existing_skills = self._infer_legacy_skill_provenance( + [n for n in raw_existing_skills if isinstance(n, str)], + pack_id, + fallback_agent=agent_name, + ) + else: + existing_skills = self._normalize_registered_skills( + raw_existing_skills, fallback_agent=agent_name + ) merged_skills = copy.deepcopy(existing_skills) if registered_skills.get(agent_name): merged_skills[agent_name] = registered_skills[agent_name] @@ -836,10 +861,6 @@ class PresetManager: ) if merged_skills != existing_skills or needs_migration: self.registry.update(pack_id, {"registered_skills": merged_skills}) - - for tmpl in manifest.templates: - if tmpl.get("type") == "command": - affected_cmd_names.add(tmpl["name"]) except Exception as pack_err: from .. import _print_cli_warning @@ -1636,6 +1657,97 @@ class PresetManager: return {selected_ai: written} if written else {} + def _infer_legacy_skill_provenance( + self, skill_names: List[str], pack_id: str, fallback_agent: str + ) -> Dict[str, List[str]]: + """Infer per-agent ownership of a legacy flat-list ``registered_skills`` value. + + Pre-#2948 registries recorded ``registered_skills`` as a flat list + with no record of which agent directory each name was actually + written under. Blindly attributing every name to ``fallback_agent`` + (the agent currently being processed) loses the real writer whenever + the *first* operation after upgrading is a direct switch to a + *different* skill-mode agent — e.g. a legacy Claude override + followed directly by ``integration use codex``, with no + intervening rescaffold for Claude — permanently orphaning Claude's + override on later removal. + + Instead, every configured skill-mode agent's directory is probed + (via the same safe, symlink-validated helpers used for + restore/removal) for a ``SKILL.md`` whose frontmatter records this + exact preset as the owner (``metadata.source == "preset:"``, + the same marker :meth:`_register_skills` writes). A name can + legitimately be found under more than one agent's directory — the + preset may have been active while the user switched between several + skill-mode agents before provenance tracking existed — so every + matching agent is recorded, not just the first. Names that can't be + matched to any directory (e.g. the file was deleted out of band) + fall back to ``fallback_agent``, preserving the previous + best-effort behaviour for the unrecoverable case. + """ + from ..agents import CommandRegistrar + + registrar = CommandRegistrar() + skill_mode_agents = sorted( + name + for name, cfg in registrar.AGENT_CONFIGS.items() + if cfg.get("extension") == "/SKILL.md" + ) + + # Multiple agent names can resolve to the same physical directory + # (e.g. agy/codex/zed all use .agents/skills); group by directory so + # each is probed once and attributed to a single deterministic + # canonical agent name, matching the tie-break already used by + # _unregister_skills's directory grouping. Deliberately keep the + # unresolved path (matching what _safe_skills_dir_for_agent already + # validated) rather than calling .resolve() here: on macOS /var is + # itself a symlink to /private/var, so resolving would make this + # path diverge from self.project_root's own resolution state and + # make every subsequent containment check in + # _validate_skill_subdir() spuriously fail. + dir_to_agents: Dict[Path, List[str]] = {} + for agent_name in skill_mode_agents: + skills_dir = self._safe_skills_dir_for_agent(agent_name) + if skills_dir is None: + continue + dir_to_agents.setdefault(skills_dir, []).append(agent_name) + + marker = f"preset:{pack_id}" + inferred: Dict[str, List[str]] = {} + matched_names: set = set() + for resolved_dir, agents in dir_to_agents.items(): + canonical_agent = fallback_agent if fallback_agent in agents else sorted(agents)[0] + for name in skill_names: + skill_subdir = resolved_dir / name + if not self._validate_skill_subdir(skill_subdir, create=False): + continue + skill_file = skill_subdir / "SKILL.md" + if not skill_file.is_file(): + continue + try: + content = skill_file.read_text(encoding="utf-8") + except OSError: + continue + frontmatter, _ = registrar.parse_frontmatter(content) + skill_metadata = frontmatter.get("metadata") + source = ( + skill_metadata.get("source") + if isinstance(skill_metadata, dict) + else None + ) + if source == marker: + inferred.setdefault(canonical_agent, []).append(name) + matched_names.add(name) + + unmatched = [name for name in skill_names if name not in matched_names] + if unmatched and fallback_agent: + fallback_names = inferred.setdefault(fallback_agent, []) + for name in unmatched: + if name not in fallback_names: + fallback_names.append(name) + + return inferred + @staticmethod def _normalize_registered_skills( value: Any, fallback_agent: Optional[str] = None @@ -1651,6 +1763,11 @@ class PresetManager: list to the agent currently being processed so the format self-migrates on the next write. Without a fallback agent, legacy lists are dropped rather than guessed at. + + Callers that can identify the owning preset (i.e. have a + ``pack_id``) should prefer :meth:`_infer_legacy_skill_provenance` + for a legacy flat-list value instead, which probes on-disk + provenance rather than assuming ``fallback_agent`` wrote every name. """ if isinstance(value, dict): return { diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index c2d9bb439..8a7af933f 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -1262,6 +1262,103 @@ class TestExtensionSkillRegistration: assert "register extension skills for extension 'skill-fail'" in captured.out assert "Continuing with available registration results" in captured.out + def test_rescaffold_toggle_command_to_skills_removes_stale_extension_command_file( + self, project_dir, temp_dir + ): + """Toggling the *same* active agent from command mode to skills mode + must remove the stale extension command-mode artifact, not just add + the skills-mode one. + + Copilot stays the active agent throughout (mirroring ``integration + upgrade copilot --skills``, not a switch to a different agent). + ``register_enabled_extensions_for_agent`` skips the commands phase + once ``skills_mode_active`` is true, but before this fix it never + removed the ``.agent.md`` file (and ``registered_commands["copilot"]`` + entry) the commands phase previously wrote while command mode was + active — leaving both artifacts on disk at once and violating the + command/skill mutual-exclusion the PR description claims (#2948). + """ + _create_init_options(project_dir, ai="copilot", ai_skills=False) + manager = ExtensionManager(project_dir) + manager.install_from_directory( + _create_extension_dir(temp_dir, ext_id="toggle-ext"), "0.1.0", + register_commands=False, + ) + manager.register_enabled_extensions_for_agent("copilot") + + agents_dir = project_dir / ".github" / "agents" + cmd_file = agents_dir / "speckit.toggle-ext.hello.agent.md" + assert cmd_file.exists(), "sanity: command mode should write .agent.md" + + # Toggle ai_skills on for the same active agent (copilot) and + # rescaffold, mirroring `integration upgrade copilot --skills`. + _create_init_options(project_dir, ai="copilot", ai_skills=True) + manager.register_enabled_extensions_for_agent("copilot") + + assert not cmd_file.exists(), ( + "the stale command-mode .agent.md file must be removed once " + "this agent toggles to skills mode, not left alongside the " + "new SKILL.md (#2948)" + ) + metadata = manager.registry.get("toggle-ext") + registered_commands = metadata.get("registered_commands", {}) + assert not registered_commands.get("copilot"), ( + "registered_commands tracking for copilot must be cleared " + "once its command file is removed on toggle (#2948)" + ) + skills_dir = project_dir / ".github" / "skills" + skill_file = skills_dir / "speckit-toggle-ext-hello" / "SKILL.md" + assert skill_file.exists(), "sanity: skills mode should write SKILL.md" + + def test_rescaffold_toggle_skills_to_command_removes_stale_extension_skill_file( + self, project_dir, temp_dir + ): + """Toggling the *same* active agent from skills mode to command mode + must remove the stale extension skills-mode artifact, not just add + the command-mode one. + + Mirror image of the command->skills toggle: ``_register_extension_skills`` + returns an empty list once skills mode is off for this agent (its + skills directory no longer resolves), but before this fix an empty + result was silently treated as "nothing to register" rather than + "this agent's skill was rendered here previously and is now stale", + so the ``SKILL.md`` this extension wrote while skills mode was + active stayed on disk even though a fresh ``.agent.md`` was written + right alongside it (#2948). + """ + _create_init_options(project_dir, ai="copilot", ai_skills=True) + manager = ExtensionManager(project_dir) + manager.install_from_directory( + _create_extension_dir(temp_dir, ext_id="toggle-ext2"), "0.1.0", + register_commands=False, + ) + manager.register_enabled_extensions_for_agent("copilot") + + skills_dir = project_dir / ".github" / "skills" + skill_file = skills_dir / "speckit-toggle-ext2-hello" / "SKILL.md" + assert skill_file.exists(), "sanity: skills mode should write SKILL.md" + + # Toggle ai_skills off for the same active agent (copilot) and + # rescaffold, mirroring `integration upgrade copilot` (no --skills). + _create_init_options(project_dir, ai="copilot", ai_skills=False) + manager.register_enabled_extensions_for_agent("copilot") + + agents_dir = project_dir / ".github" / "agents" + cmd_file = agents_dir / "speckit.toggle-ext2.hello.agent.md" + assert cmd_file.exists(), "sanity: command mode should write .agent.md" + + assert not skill_file.exists(), ( + "the stale skills-mode SKILL.md file must be removed once this " + "agent toggles to command mode, not left alongside the new " + ".agent.md (#2948)" + ) + metadata = manager.registry.get("toggle-ext2") + registered_skills = metadata.get("registered_skills", []) + assert "speckit-toggle-ext2-hello" not in registered_skills, ( + "registered_skills tracking must be cleared for the removed " + "skill file, not left dangling once it's orphaned (#2948)" + ) + def test_existing_agent_command_path_file_is_not_detected( self, project_dir, temp_dir ): diff --git a/tests/test_presets.py b/tests/test_presets.py index b50023cbf..a87606829 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4274,6 +4274,68 @@ class TestPresetSkills: "and preset removal can't clean it up (#2948)" ) + def test_rescaffold_reconciles_override_even_when_skills_phase_fails( + self, project_dir, temp_dir + ): + """A project override must still win after rescaffold even if the + independently-fallible skills phase raises for that preset. + + ``register_enabled_presets_for_agent`` only records a preset's + command names into ``affected_cmd_names`` — the set later passed to + ``_reconcile_composed_commands``/``_reconcile_skills`` — in the + ``for tmpl in manifest.templates`` loop that runs *after* + ``_register_skills`` inside the per-preset ``try`` block. If + ``_register_skills`` raises, the per-preset ``except`` catches it + and ``continue``s before that loop ever runs, so this preset's + command names never make it into ``affected_cmd_names`` even though + ``_register_commands`` already wrote its raw content to disk. The + final reconciliation call is skipped for this preset entirely, + leaving the raw preset content in place instead of the project + override that should win (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + + overrides_dir = project_dir / ".specify" / "templates" / "overrides" + overrides_dir.mkdir(parents=True, exist_ok=True) + (overrides_dir / "speckit.specify.md").write_text( + "---\ndescription: Override specify\n---\n\nOverride body\n", + encoding="utf-8", + ) + + gemini_dir = project_dir / ".gemini" / "commands" + gemini_dir.mkdir(parents=True) + + preset_dir = self._create_command_preset( + temp_dir, "reconcile-despite-skills-failure", "speckit.specify", + "Preset specify", "Preset body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + # Simulate `integration use gemini` with the skills phase failing + # for this preset (e.g. a symlink/permission error unrelated to the + # commands phase, which already succeeded). + self._write_init_options(project_dir, ai="gemini", ai_skills=False) + + from unittest.mock import patch + + with patch.object( + PresetManager, "_register_skills", + side_effect=RuntimeError("simulated skills failure"), + ): + manager.register_enabled_presets_for_agent("gemini") + + cmd_file = gemini_dir / "speckit.specify.toml" + assert cmd_file.exists(), "sanity: gemini should get a command file at all" + content = cmd_file.read_text() + assert "Override body" in content, ( + "the project override must still win after rescaffold, even " + "though this preset's skills phase raised — a fallible skills " + "phase must not skip reconciliation for command writes that " + "already succeeded (#2948)" + ) + assert "Preset body" not in content + 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. @@ -4610,6 +4672,90 @@ class TestPresetSkills: ) assert "Core specify body" in content + def test_rescaffold_legacy_flat_list_direct_switch_preserves_original_agent( + self, project_dir, temp_dir + ): + """A legacy flat-list ``registered_skills`` entry must not be + misattributed to the wrong agent when the *first* post-upgrade + operation is a direct switch to a different skill-mode agent. + + Blindly attributing every legacy flat-list name to ``agent_name`` — + the agent currently being (re)activated — loses the actual writer + whenever that first operation is ``integration use codex`` (or + ``switch``) run directly against a legacy Claude override, without + an intervening same-agent rescaffold for Claude first. The + migrated dict then only records ``{"codex": [...]}``, so a later + ``remove()`` restores Codex but permanently orphans the Claude + override that was never in the registry to begin with (#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", + ) + + 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, "legacy-direct-switch-preset", "speckit.specify", + "Legacy direct switch test", "preset body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + # install_from_directory wrote the preset's override to Claude's + # skill directory (the active agent at install time) — sanity-check + # that the marker is actually there before simulating the legacy + # registry format. + claude_skill = claude_skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:legacy-direct-switch-preset" in claude_skill.read_text(), ( + "sanity: install should have written the override under claude" + ) + + # Simulate a pre-#2948 registry: a flat list with no per-agent + # provenance, even though the file on disk was actually written + # under claude's directory. + manager.registry.update( + "legacy-direct-switch-preset", + {"registered_skills": ["speckit-specify"]}, + ) + + # Directly switch to codex — no intervening rescaffold for claude — + # mirroring `integration use codex` / `switch codex` run right after + # upgrading spec-kit versions. + self._write_init_options(project_dir, ai="codex", ai_skills=True) + manager.register_enabled_presets_for_agent("codex") + + metadata = manager.registry.get("legacy-direct-switch-preset") + registered_skills = metadata.get("registered_skills") + assert isinstance(registered_skills, dict) + assert set(registered_skills) == {"claude", "codex"}, ( + "migrating a legacy flat-list entry on a direct switch must " + "infer the actual writer (claude) from the existing on-disk " + "SKILL.md provenance, not attribute every name to whichever " + "agent happens to be activated first after the upgrade " + "(#2948)" + ) + + assert manager.remove("legacy-direct-switch-preset") is True + + codex_skill = codex_skills_dir / "speckit-specify" / "SKILL.md" + 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:legacy-direct-switch-preset" not in content, ( + f"{label}'s preset override must be restored on removal, " + "not permanently orphaned by a misattributed legacy " + "migration (#2948)" + ) + assert "Core specify body" in content + def test_symlinked_skills_dir_rejected_on_removal(self, project_dir, temp_dir): """Removal must validate a recorded skill directory before touching it.