diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index b67100b27..9728bc909 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -1667,46 +1667,55 @@ class PresetManager: 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. + *different* agent — e.g. a legacy Copilot override (written while + Copilot was active with ``ai_skills`` enabled) followed directly by + ``integration use claude``, with no intervening rescaffold for + Copilot — permanently orphaning Copilot'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. + Every *configured* integration's skills directory is probed (via + the same safe, symlink-validated helpers used for + restore/removal), not only agents whose registrar config is + statically ``/SKILL.md``-only: a command-backed agent (e.g. + Copilot, whose command extension is ``.agent.md``) renders its + preset overrides as ``SKILL.md`` files exactly like a native + skill-only agent whenever it was the active agent with + ``ai_skills`` enabled, so excluding it would miss real, + preset-owned provenance and misattribute it to whichever agent + happens to be processed first. Each directory is probed for a + ``SKILL.md`` whose frontmatter records this exact preset as the + owner (``metadata.source == "preset:"``, the same marker + :meth:`_register_skills` writes) — this marker check is what keeps + the broadened probe from falsely attributing ownership to an + agent's directory that never actually held this preset's override + (e.g. a command-mode agent that never rendered skills, or an + unrelated skill of the same name). 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 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" - ) + candidate_agents = sorted(registrar.AGENT_CONFIGS) # 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 + # (e.g. agy/amp/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: + for agent_name in candidate_agents: skills_dir = self._safe_skills_dir_for_agent(agent_name) if skills_dir is None: continue diff --git a/tests/test_presets.py b/tests/test_presets.py index a87606829..18a1b7318 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4756,6 +4756,157 @@ class TestPresetSkills: ) assert "Core specify body" in content + def test_rescaffold_legacy_flat_list_infers_command_backed_skills_owner( + self, project_dir, temp_dir + ): + """Legacy provenance inference must also probe command-backed agents + that were running in skills mode, not only agents whose command + registrar config is statically ``/SKILL.md``-only. + + Copilot is command-backed (``extension: ".agent.md"``), but with + ``ai_skills`` enabled its preset overrides render as ``SKILL.md`` + files under ``.github/skills`` exactly like a native skill-only + agent (claude, codex, ...). Before the fix, + ``_infer_legacy_skill_provenance`` only probed agents whose + registrar config has a static ``extension == "/SKILL.md"``, so a + real preset-owned ``.github/skills/.../SKILL.md`` written while + Copilot was the active, skills-mode agent was never found — the + legacy flat list was misattributed entirely to whichever agent the + first post-upgrade switch happened to activate, permanently + orphaning Copilot's override on later removal (#2948). + """ + self._write_init_options(project_dir, ai="copilot", 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", + ) + + copilot_skills_dir = project_dir / ".github" / "skills" + self._create_skill(copilot_skills_dir, "speckit-specify") + claude_skills_dir = project_dir / ".claude" / "skills" + self._create_skill(claude_skills_dir, "speckit-specify") + + preset_dir = self._create_command_preset( + temp_dir, "legacy-copilot-skills-preset", "speckit.specify", + "Legacy copilot skills test", "preset body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + copilot_skill = copilot_skills_dir / "speckit-specify" / "SKILL.md" + assert "preset:legacy-copilot-skills-preset" in copilot_skill.read_text(), ( + "sanity: install should have written the override under " + "copilot's skills directory while copilot was active in " + "skills mode" + ) + # Sanity: no command-mode artifact was written either — copilot's + # command file and skills file are mutually exclusive. + assert not list((project_dir / ".github" / "agents").glob("*specify*")), ( + "sanity: copilot in skills mode must not also write a command " + "file that could be falsely attributed instead" + ) + + # Simulate a pre-#2948 registry: a flat list with no per-agent + # provenance, even though the file on disk was actually written + # under copilot's skills directory. + manager.registry.update( + "legacy-copilot-skills-preset", + {"registered_skills": ["speckit-specify"]}, + ) + + # Directly switch to claude — no intervening rescaffold for + # copilot — mirroring `integration use claude` run right after + # upgrading spec-kit versions. + self._write_init_options(project_dir, ai="claude", ai_skills=True) + manager.register_enabled_presets_for_agent("claude") + + metadata = manager.registry.get("legacy-copilot-skills-preset") + registered_skills = metadata.get("registered_skills") + assert isinstance(registered_skills, dict) + assert set(registered_skills) == {"copilot", "claude"}, ( + "migrating a legacy flat-list entry on a direct switch must " + "infer the actual writer (copilot, running in skills mode) " + "even though copilot's registrar config is command-backed, " + "not just agents with a static /SKILL.md extension (#2948)" + ) + + assert manager.remove("legacy-copilot-skills-preset") is True + + claude_skill = claude_skills_dir / "speckit-specify" / "SKILL.md" + for skill_file, label in ((copilot_skill, "copilot"), (claude_skill, "claude")): + assert skill_file.exists(), f"{label} skill file should still exist after removal" + content = skill_file.read_text() + assert "preset:legacy-copilot-skills-preset" not in content, ( + f"{label}'s preset override must be restored on removal, " + "not permanently orphaned by a legacy migration that " + "failed to probe command-backed skills-mode agents (#2948)" + ) + assert "Core specify body" in content + + def test_infer_legacy_skill_provenance_does_not_falsely_attribute_command_mode_copilot( + self, project_dir, temp_dir + ): + """Broadening provenance inference to command-backed agents must not + falsely attribute ownership to an agent's directory that has no + preset-owned marker. + + Copilot stays in plain command mode throughout (no skills ever + rendered there), so ``.github/skills`` never receives this + preset's ``SKILL.md``. Probing copilot's skills directory anyway + (now that inference isn't restricted to static ``/SKILL.md`` + agents) must find nothing there and must not invent a false + ``"copilot"`` entry (#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") + # Copilot has never been active; its command directory holds an + # unrelated file so the directory exists, but no skills directory + # or SKILL.md was ever written for it. + copilot_commands_dir = project_dir / ".github" / "agents" + copilot_commands_dir.mkdir(parents=True) + + preset_dir = self._create_command_preset( + temp_dir, "no-false-attribution-preset", "speckit.specify", + "No false attribution test", "preset body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + manager.registry.update( + "no-false-attribution-preset", + {"registered_skills": ["speckit-specify"]}, + ) + + # Rescaffold again for the same agent (claude) with unchanged + # names, triggering the legacy migration path. + manager.register_enabled_presets_for_agent("claude") + + metadata = manager.registry.get("no-false-attribution-preset") + registered_skills = metadata.get("registered_skills") + assert isinstance(registered_skills, dict) + assert set(registered_skills) == {"claude"}, ( + "copilot must not appear in the migrated registry when it has " + "never actually rendered this preset's skill — probing its " + "directory for a marker match must not create a false " + "attribution (#2948)" + ) + assert not (project_dir / ".github" / "skills").exists(), ( + "no .github/skills directory should have been created as a " + "side effect of probing for provenance (#2948)" + ) + def test_symlinked_skills_dir_rejected_on_removal(self, project_dir, temp_dir): """Removal must validate a recorded skill directory before touching it.