diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 96f96f6c4..b8ec4f00f 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2001,6 +2001,10 @@ class ExtensionManager: # registration of the remaining enabled extensions for this agent. try: updates: Dict[str, Any] = {} + # Set when a command -> skills toggle for this same agent + # defers stale command-mode cleanup until the skills + # replacement below confirms success (#2948). + deferred_stale_commands: Optional[List[str]] = None if agent_config and not skills_mode_active: registered = registrar.register_commands_for_agent( @@ -2022,28 +2026,27 @@ class ExtensionManager: 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 + # commands phase above is skipped. 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). + # tracked, but it must NOT be removed yet — the skills + # phase below is an independently fallible replacement + # step, and deleting the old artifact before it + # succeeds would leave neither the old command file nor + # a new skill file if skills registration raises. The + # actual removal is deferred until after the skills + # phase below completes without raising (#2948). registered_commands = metadata.get("registered_commands", {}) if isinstance(registered_commands, dict) and registered_commands.get( agent_name ): - stale_commands = self._valid_name_list( + deferred_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 + else: + deferred_stale_commands = None + else: + deferred_stale_commands = None # Extension *skills* are only ever rendered for the active agent: # `_register_extension_skills` resolves the skills dir and @@ -2122,6 +2125,25 @@ class ExtensionManager: if remaining != existing_skills: updates["registered_skills"] = remaining + # The skills phase above completed without raising + # (this ``else:`` is only reached on success), so a + # deferred command -> skills toggle cleanup queued + # above is now safe to apply: the replacement skill + # registration is confirmed, so the stale + # command-mode artifact can finally be removed + # without risking a transient state where neither + # artifact exists (#2948). + if deferred_stale_commands: + registrar.unregister_commands( + {agent_name: deferred_stale_commands}, self.project_root + ) + registered_commands = metadata.get("registered_commands", {}) + if isinstance(registered_commands, dict): + new_registered = copy.deepcopy(registered_commands) + new_registered.pop(agent_name, None) + if new_registered != registered_commands: + updates["registered_commands"] = new_registered + if updates: self.registry.update(ext_id, updates) except Exception as ext_err: diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 69021dcd3..dc260935c 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -785,17 +785,24 @@ class PresetManager: if not isinstance(existing_commands, dict): existing_commands = {} merged_commands = copy.deepcopy(existing_commands) + # Toggled command -> skills for this same agent: + # _register_commands's ai_skills guard just made this a + # no-op, but the command file this preset wrote while + # command mode was active is still on disk and still + # tracked. Do NOT unregister it yet — _register_skills() + # below is an independently fallible replacement step, and + # deleting the old artifact before it succeeds would leave + # neither the old command file nor a new skill file if + # skills registration raises. The old artifact is only + # removed after the skills phase below completes without + # raising, preserving command/skill mutual exclusion while + # never leaving a transient failure with nothing in place + # (#2948). + stale_command_names: Optional[List[str]] = None if registered_commands.get(agent_name): merged_commands[agent_name] = registered_commands[agent_name] elif ai_skills_now and merged_commands.get(agent_name): - # Toggled command -> skills for this same agent: - # _register_commands's ai_skills guard just made this a - # no-op, but the command file this preset wrote while - # command mode was active is still on disk and still - # tracked. Unregister it narrowly for this agent so - # command-mode and skills-mode artifacts stay mutually - # exclusive (#2948). - self._unregister_commands({agent_name: merged_commands.pop(agent_name)}) + stale_command_names = merged_commands[agent_name] # Persist the commands phase immediately, mirroring # install_from_directory(): _register_skills is an # independently fallible phase, and if it raises, the files @@ -843,7 +850,12 @@ class PresetManager: # directory once ai_skills is off, so _register_skills # is a no-op — but the SKILL.md this preset wrote while # skills mode was active is still tracked and still on - # disk. Restore/remove it narrowly for this agent (#2948). + # disk. Restore/remove it narrowly for this agent. This + # direction is already register-new-then-remove-old: + # _register_commands (the replacement) ran unconditionally + # above and only reaches here once it has already + # succeeded, so this cleanup happens only after the new + # command artifact is confirmed in place (#2948). self._unregister_skills( {agent_name: merged_skills.pop(agent_name)}, pack_dir ) @@ -861,6 +873,15 @@ class PresetManager: ) if merged_skills != existing_skills or needs_migration: self.registry.update(pack_id, {"registered_skills": merged_skills}) + + # The skills phase above completed without raising, so the + # replacement artifact is confirmed — now it's safe to + # remove the stale command-mode artifact deferred earlier + # (#2948). + if stale_command_names: + self._unregister_commands({agent_name: stale_command_names}) + merged_commands.pop(agent_name, None) + self.registry.update(pack_id, {"registered_commands": merged_commands}) except Exception as pack_err: from .. import _print_cli_warning @@ -910,6 +931,50 @@ class PresetManager: registrar = CommandRegistrar() registrar.unregister_commands(registered_commands, self.project_root) + def _merge_pack_registered_commands( + self, pack_id: str, written: Optional[Dict[str, List[str]]] + ) -> None: + """Merge actually-written agent command registrations into a preset's metadata. + + Reconciliation (``_reconcile_composed_commands``) can write a + preset's content into an agent directory the preset never wrote to + before — most notably a historical (currently inactive) agent + supplied via ``extra_agents`` when a higher-priority preset is + removed. If that write isn't reflected back into the winning + preset's own ``registered_commands``, the registry silently lies + about which directories the preset owns: a later removal of this + same preset only cleans up the agents it already knew about, + orphaning the directory reconciliation just wrote to on its behalf + (#2948). + + Args: + pack_id: The preset whose metadata should be updated. + written: ``{agent_name: [cmd_name, ...]}`` actually written by + the reconciliation call just made, exactly mirroring + ``CommandRegistrar.register_commands_for_non_skill_agents``'s + return value. A falsy value is a no-op. + """ + if not written: + return + metadata = self.registry.get(pack_id) + if metadata is None: + return # pack_id no longer installed (e.g. removed mid-loop) + existing_commands = metadata.get("registered_commands", {}) + if not isinstance(existing_commands, dict): + existing_commands = {} + merged_commands = copy.deepcopy(existing_commands) + changed = False + for agent_name, cmd_names in written.items(): + if not cmd_names: + continue + existing_names = merged_commands.get(agent_name, []) + new_names = [n for n in cmd_names if n not in existing_names] + if new_names: + merged_commands[agent_name] = existing_names + new_names + changed = True + if changed: + self.registry.update(pack_id, {"registered_commands": merged_commands}) + def _reconcile_composed_commands( self, command_names: List[str], extra_agents: Optional[Set[str]] = None ) -> None: @@ -1003,10 +1068,11 @@ class PresetManager: if manifest: for tmpl in manifest.templates: if tmpl.get("name") == cmd_name and tmpl.get("type") == "command": - self._register_for_non_skill_agents( + written = self._register_for_non_skill_agents( registrar, [tmpl], manifest.id, pack_dir, only_agent=only_agent, extra_agents=extra_agents, ) + self._merge_pack_registered_commands(manifest.id, written) registered = True break break @@ -1105,12 +1171,13 @@ class PresetManager: composed_dir.mkdir(parents=True, exist_ok=True) composed_file = composed_dir / f"{cmd_name}.md" composed_file.write_text(composed, encoding="utf-8") - self._register_for_non_skill_agents( + written = self._register_for_non_skill_agents( registrar, [{**tmpl, "file": f".composed/{cmd_name}.md"}], manifest.id, pack_dir, only_agent=only_agent, extra_agents=extra_agents, ) + self._merge_pack_registered_commands(manifest.id, written) registered = True break else: @@ -1142,7 +1209,7 @@ class PresetManager: source_id: str = "reconciled", only_agent: Optional[str] = None, extra_agents: Optional[Set[str]] = None, - ) -> None: + ) -> Dict[str, List[str]]: """Register a single command from a file path (non-preset source). Used by reconciliation when the winning layer is an extension, @@ -1156,9 +1223,14 @@ class PresetManager: only_agent: If set, restrict registration to this single agent (#2948). extra_agents: Additional agent names to register for besides ``only_agent`` (post-removal reconciliation only, #2948). + + Returns: + ``{agent_name: [cmd_name, ...]}`` for every agent this call + actually registered the command for (empty if the source path + doesn't exist or nothing was written). """ if not cmd_path.exists(): - return + return {} cmd_tmpl: Dict[str, Any] = { "name": cmd_name, "type": "command", @@ -1184,7 +1256,7 @@ class PresetManager: break except Exception: pass # best-effort alias loading - self._register_for_non_skill_agents( + return self._register_for_non_skill_agents( registrar, [cmd_tmpl], source_id, cmd_path.parent, only_agent=only_agent, extra_agents=extra_agents, ) @@ -1197,7 +1269,7 @@ class PresetManager: source_dir: Path, only_agent: Optional[str] = None, extra_agents: Optional[Set[str]] = None, - ) -> None: + ) -> Dict[str, List[str]]: """Register commands for non-skill agents during reconciliation. Skill-based agents (``/SKILL.md`` layout) are handled separately: @@ -1219,8 +1291,15 @@ class PresetManager: ``only_agent``. Used by post-removal reconciliation to also restore surviving content into historical agent directories a just-removed preset actually wrote to (#2948). + + Returns: + ``{agent_name: [cmd_name, ...]}`` for every agent this call + actually registered a command for, mirroring + ``CommandRegistrar.register_commands_for_non_skill_agents``'s + return value so callers can merge it into a preset's own + ``registered_commands`` tracking (#2948). """ - registrar.register_commands_for_non_skill_agents( + return registrar.register_commands_for_non_skill_agents( commands, source_id, source_dir, self.project_root, only_agent=only_agent, extra_agents=extra_agents, ) @@ -1246,6 +1325,59 @@ class PresetManager: if t.get("name") in self._cmd_names ] + def _merge_pack_registered_skills( + self, pack_id: str, written: Optional[Dict[str, List[str]]] + ) -> None: + """Merge actually-written agent skill registrations into a preset's metadata. + + Mirrors :meth:`_merge_pack_registered_commands` for the skills + side: ``_reconcile_skills`` can render a preset's SKILL.md content + into an agent directory the preset never wrote to before — most + notably a historical (currently inactive) agent restored via + ``extra_skills_dirs`` when a higher-priority preset is removed. If + that write isn't reflected back into the winning preset's own + ``registered_skills``, a later removal of this same preset only + cleans up the agents it already knew about, orphaning the skill + directory reconciliation just wrote to on its behalf (#2948). + + Args: + pack_id: The preset whose metadata should be updated. + written: ``{agent_name: [skill_name, ...]}`` actually written + by the ``_register_skills`` call just made. A falsy value + is a no-op. + """ + if not written: + return + metadata = self.registry.get(pack_id) + if metadata is None: + return # pack_id no longer installed (e.g. removed mid-loop) + raw_existing_skills = metadata.get("registered_skills") + if isinstance(raw_existing_skills, list) and raw_existing_skills: + # Legacy flat-list value: infer real per-agent ownership from + # on-disk provenance rather than guessing (#2948). + fallback_agent = next(iter(written)) if written else None + existing_skills = self._infer_legacy_skill_provenance( + [n for n in raw_existing_skills if isinstance(n, str)], + pack_id, + fallback_agent=fallback_agent, + ) + else: + existing_skills = self._normalize_registered_skills(raw_existing_skills) + merged_skills = copy.deepcopy(existing_skills) + changed = ( + isinstance(raw_existing_skills, list) and bool(raw_existing_skills) + ) + for agent_name, skill_names in written.items(): + if not skill_names: + continue + existing_names = merged_skills.get(agent_name, []) + new_names = [n for n in skill_names if n not in existing_names] + if new_names: + merged_skills[agent_name] = existing_names + new_names + changed = True + if changed: + self.registry.update(pack_id, {"registered_skills": merged_skills}) + def _reconcile_skills( self, command_names: List[str], @@ -1426,12 +1558,21 @@ class PresetManager: # Preserve exact prior behaviour for the currently # active directory (including the ability to create # brand-new skill subdirectories when ai_skills is on). - self._register_skills(filtered_manifest, pack_dir) + written = self._register_skills(filtered_manifest, pack_dir) else: - self._register_skills( + written = self._register_skills( filtered_manifest, pack_dir, target_dir=skills_dir, target_agent=dir_agent or "", ) + # The winning preset may not have previously written to + # this directory's agent (most notably a historical agent + # reconciliation just restored content into via + # extra_skills_dirs). If that write isn't merged back into + # the preset's own registered_skills, its registry entry + # silently lies about which directories it owns and a + # later removal of this same preset orphans the directory + # reconciliation just wrote to on its behalf (#2948). + self._merge_pack_registered_skills(pack_id, written) if active_skills_dir: apply_to_dir(active_skills_dir, active_ai, is_active=True) @@ -1802,11 +1943,18 @@ class PresetManager: dir_to_agents.setdefault(skills_dir, []).append(agent_name) marker = f"preset:{pack_id}" + # Filter unsafe names once, up front, rather than only inside the + # matching loop: any name skipped there would otherwise still + # land in "unmatched" below and get blindly attributed to + # fallback_agent anyway, defeating the guard entirely (#2948). + safe_skill_names = [ + name for name in skill_names if self._is_safe_registry_skill_name(name) + ] 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: + for name in safe_skill_names: skill_subdir = resolved_dir / name if not self._validate_skill_subdir(skill_subdir, create=False): continue @@ -1828,7 +1976,7 @@ class PresetManager: inferred.setdefault(canonical_agent, []).append(name) matched_names.add(name) - unmatched = [name for name in skill_names if name not in matched_names] + unmatched = [name for name in safe_skill_names if name not in matched_names] if unmatched and fallback_agent: fallback_names = inferred.setdefault(fallback_agent, []) for name in unmatched: @@ -1893,6 +2041,36 @@ class PresetManager: return None return skills_dir + @staticmethod + def _is_safe_registry_skill_name(name: Any) -> bool: + """Validate a registry-provided skill name is a single safe path component. + + ``registered_skills`` entries are persisted registry data, not + derived from the current preset manifest, so a corrupted or + maliciously edited registry could contain an absolute path, a + multi-segment path (containing ``/`` or ``\\``), or a traversal + component (``"."``/``".."``) instead of a plain skill directory + name. Any of these — if joined directly onto a skills directory — + can escape the intended skill subtree while still resolving to a + location inside the project root, which is enough to pass the + parent-directory containment/symlink check alone (#2948). This + centralizes the single boundary check every preset cleanup and + provenance loop that consumes registry-provided skill names must + apply before ever constructing a path from one. + """ + if not isinstance(name, str) or not name: + return False + if name in (".", ".."): + return False + candidate = Path(name) + if candidate.is_absolute(): + return False + if len(candidate.parts) != 1: + return False + if candidate.name != name: + return False + return True + def _validate_skill_subdir(self, skill_subdir: Path, *, create: bool) -> bool: """Validate a single skill's subdirectory is symlink-free. @@ -2046,6 +2224,20 @@ class PresetManager: extension_restore_index = self._build_extension_skill_restore_index() for skill_name in skill_names: + # Guard against a corrupted/malicious registry entry: a + # registered_skills name is persisted data, not derived from + # the current manifest, so it must be validated as a single, + # relative, non-"."/".." path component before ever being + # joined onto skills_dir. Without this, an absolute name + # discards skills_dir entirely (Path's "/" operator drops the + # left side for an absolute right side) or a multi-component + # name containing ".." can resolve to a different, unrelated + # directory that still happens to be inside the project root + # — passing the containment-only symlink guard below and + # letting removal overwrite/delete it (#2948). + if not self._is_safe_registry_skill_name(skill_name): + continue + # Derive command name from skill name (speckit-specify -> specify) short_name = skill_name if short_name.startswith("speckit-"): diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 33e8b175e..c4cee6826 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -1310,6 +1310,62 @@ class TestExtensionSkillRegistration: skill_file = skills_dir / "speckit-toggle-ext-hello" / "SKILL.md" assert skill_file.exists(), "sanity: skills mode should write SKILL.md" + def test_toggle_command_to_skills_preserves_old_extension_command_on_skills_failure( + self, project_dir, temp_dir, monkeypatch + ): + """An extension command->skills toggle must not destroy the old + command artifact before the new skill registration has actually + succeeded. + + Mirrors the analogous preset-side fix + (``test_toggle_command_to_skills_preserves_old_command_on_skills_failure`` + in ``tests/test_presets.py``): before the fix, the stale + command-mode file/tracking was unregistered unconditionally as soon + as the commands phase was skipped for ``skills_mode_active``, + regardless of whether the subsequent, independently-fallible + ``_register_extension_skills()`` call actually succeeded. If skills + raises, the exception handler just warns and continues, leaving + neither the old command file nor a new skill file (#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-fail-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-fail-ext.hello.agent.md" + assert cmd_file.exists(), "sanity: command mode should write .agent.md" + metadata = manager.registry.get("toggle-fail-ext") + assert metadata.get("registered_commands", {}).get("copilot"), ( + "sanity: the command-mode write should be tracked for copilot" + ) + + # Toggle ai_skills on for the same active agent (copilot), but with + # skills registration injected to fail. + _create_init_options(project_dir, ai="copilot", ai_skills=True) + + def _raise_register_extension_skills(*args, **kwargs): + raise OSError("simulated extension skills-phase failure") + + monkeypatch.setattr( + manager, "_register_extension_skills", _raise_register_extension_skills + ) + manager.register_enabled_extensions_for_agent("copilot") + + assert cmd_file.exists(), ( + "the old command-mode artifact must survive when the " + "replacement skills registration fails — deleting it before " + "the new artifact is confirmed leaves neither in place (#2948)" + ) + metadata = manager.registry.get("toggle-fail-ext") + assert metadata.get("registered_commands", {}).get("copilot"), ( + "registered_commands must keep tracking copilot's still-live " + "command file when the skills replacement failed (#2948)" + ) + def test_rescaffold_toggle_skills_to_command_removes_stale_extension_skill_file( self, project_dir, temp_dir ): diff --git a/tests/test_presets.py b/tests/test_presets.py index 4a7bc8b16..0410f0a47 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -4425,6 +4425,67 @@ class TestPresetSkills: "sanity: the new skills-mode artifact should still be written" ) + def test_toggle_command_to_skills_preserves_old_command_on_skills_failure( + self, project_dir, temp_dir, monkeypatch + ): + """A command->skills toggle must not destroy the old command + artifact before the new skill registration has actually succeeded. + + Before the fix, the stale command-mode file/tracking was + unregistered unconditionally as soon as ``_register_commands``'s + ``ai_skills`` guard made the commands phase a no-op — regardless + of whether the subsequent, independently-fallible + ``_register_skills()`` call actually succeeded. If skills raises + (e.g. a transient I/O error), the per-preset exception handler + just logs and continues, leaving neither the old command file + nor a new skill file — the preset's command override vanishes + entirely from copilot until the next successful rescaffold + (#2948). + """ + self._write_init_options(project_dir, ai="copilot", ai_skills=False) + copilot_commands_dir = project_dir / ".github" / "agents" + copilot_commands_dir.mkdir(parents=True) + + preset_dir = self._create_command_preset( + temp_dir, "toggle-failure-preset", "speckit.specify", + "Toggle failure test", "preset body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_dir, "0.1.5") + + cmd_file = copilot_commands_dir / "speckit.specify.agent.md" + assert cmd_file.exists(), ( + "sanity: command mode should have written copilot's command file" + ) + metadata = manager.registry.get("toggle-failure-preset") + assert metadata["registered_commands"].get("copilot"), ( + "sanity: the command-mode write should be tracked for copilot" + ) + + # Flip ai_skills on for the *same* active agent and rescaffold, as + # `integration upgrade copilot` would, but with skills registration + # injected to fail. + self._write_init_options(project_dir, ai="copilot", ai_skills=True) + + def _raise_register_skills(*args, **kwargs): + raise OSError("simulated skills-phase failure") + + monkeypatch.setattr(manager, "_register_skills", _raise_register_skills) + manager.register_enabled_presets_for_agent("copilot") + + assert cmd_file.exists(), ( + "the old command-mode artifact must survive when the " + "replacement skills registration fails — deleting it before " + "the new artifact is confirmed leaves neither in place (#2948)" + ) + metadata = manager.registry.get("toggle-failure-preset") + assert metadata["registered_commands"].get("copilot"), ( + "registered_commands must keep tracking copilot's still-live " + "command file when the skills replacement failed, or a later " + "removal/rescaffold will believe there is nothing to clean up " + "even though the file is still on disk (#2948)" + ) + def test_rescaffold_toggle_skills_to_command_removes_stale_skill_file( self, project_dir, temp_dir ): @@ -5368,6 +5429,95 @@ class TestPresetSkills: "currently active agent" ) + def test_remove_reconciliation_tracks_new_historical_agent_for_survivor( + self, project_dir, temp_dir + ): + """Historical-agent reconciliation writes must be recorded in the + surviving preset's own ``registered_commands``, not just written + to disk and forgotten. + + Preset A is installed while gemini is active, then survives to be + active under opencode too (so A's ``registered_commands`` spans + both gemini and opencode). Preset B is installed *only* while + opencode is active — B's ``registered_commands`` is + ``{"opencode": [...]}`` and never mentions gemini. Removing A + triggers reconciliation that writes B's content into gemini's + directory (an agent B never wrote to before) via ``extra_agents``, + but if that write isn't merged back into B's own + ``registered_commands``, B's registry entry still only says + ``{"opencode": [...]}`` even though B's content now lives in + gemini's directory too. A later ``remove('b')`` then only cleans + up opencode, leaving the gemini file — reconciled there entirely + by side effect of removing A — as a permanent orphan with no + preset tracking it (#2948). + """ + self._write_init_options(project_dir, ai="gemini", ai_skills=False) + gemini_dir = project_dir / ".gemini" / "commands" + gemini_dir.mkdir(parents=True) + + preset_a_dir = self._create_command_preset( + temp_dir, "orphan-preset-a", "speckit.specify", + "Preset A", "preset A body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_a_dir, "0.1.5", priority=1) + + self._write_init_options(project_dir, ai="opencode", ai_skills=False) + opencode_dir = project_dir / ".opencode" / "commands" + opencode_dir.mkdir(parents=True, exist_ok=True) + manager.register_enabled_presets_for_agent("opencode") + + metadata_a = manager.registry.get("orphan-preset-a") + assert set(metadata_a.get("registered_commands", {})) == {"gemini", "opencode"}, ( + "sanity: preset A must be tracked under both agents" + ) + + # Preset B is installed only now, while opencode is the sole + # active agent — it never writes to or tracks gemini. + preset_b_dir = self._create_command_preset( + temp_dir, "orphan-preset-b", "speckit.specify", + "Preset B", "preset B body", + ) + manager.install_from_directory(preset_b_dir, "0.1.5", priority=10) + + metadata_b = manager.registry.get("orphan-preset-b") + assert set(metadata_b.get("registered_commands", {})) == {"opencode"}, ( + "sanity: preset B must only be tracked for opencode before " + "preset A is removed" + ) + + assert manager.remove("orphan-preset-a") is True + + # B is now written into gemini's directory as a side effect of + # reconciling A's removal, via the historical-agent extra_agents + # pass. + gemini_cmd_files = list(gemini_dir.glob("*specify*")) + assert gemini_cmd_files, "sanity: gemini's directory must have B's restored content" + assert "preset B body" in gemini_cmd_files[0].read_text() + + metadata_b = manager.registry.get("orphan-preset-b") + assert set(metadata_b.get("registered_commands", {})) == {"gemini", "opencode"}, ( + "preset B's own registered_commands must be updated to " + "include gemini once reconciliation actually writes content " + "there on its behalf — otherwise B's registry entry silently " + "lies about which directories it owns (#2948)" + ) + + assert manager.remove("orphan-preset-b") is True + + # No preset is installed any more, so gemini's file must have been + # reconciled down to the core bundled template (or removed + # entirely) — but it must NOT still contain B's stale content, + # which would mean B's write there was never tracked for cleanup. + remaining_gemini_files = list(gemini_dir.glob("*specify*")) + for f in remaining_gemini_files: + assert "preset B body" not in f.read_text(), ( + "removing preset B must clean up gemini's directory too, " + "since B's registered_commands was updated to include it " + "— otherwise B's stale content is orphaned there forever " + "with no preset left to track or clean it up (#2948)" + ) + def test_remove_reconciles_skill_for_every_historical_agent( self, project_dir, temp_dir ): @@ -5446,6 +5596,107 @@ class TestPresetSkills: "the currently active agent" ) + def test_remove_reconciliation_tracks_new_historical_skill_agent_for_survivor( + self, project_dir, temp_dir + ): + """Historical-agent skill reconciliation writes must be recorded in + the surviving preset's own ``registered_skills``, mirroring + ``test_remove_reconciliation_tracks_new_historical_agent_for_survivor`` + for the command side. + + Preset A is installed while claude is active, then survives to be + active under codex too (so A's ``registered_skills`` spans both + claude and codex). Preset B is installed *only* while codex is + active — B's ``registered_skills`` is ``{"codex": [...]}`` and + never mentions claude. Removing A triggers reconciliation that + renders B's SKILL.md into claude's directory (an agent B never + wrote to before) via ``extra_skills_dirs``, but if that write + isn't merged back into B's own ``registered_skills``, a later + ``remove('b')`` only cleans up codex, leaving claude's SKILL.md — + rendered there entirely by side effect of removing A — untracked + by any preset (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = project_dir / ".claude" / "skills" + # Pre-create the skill so _register_commands/_register_skills find + # an existing skill to overwrite (mirrors every other skill test + # in this class — native skill agents only overwrite already + # existing skill directories, they don't materialize brand-new + # ones outside of active-agent creation). + self._create_skill(claude_skills_dir, "speckit-specify") + + 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", + ) + + preset_a_dir = self._create_command_preset( + temp_dir, "orphan-skill-preset-a", "speckit.specify", + "Preset A", "preset A body", + ) + manager = PresetManager(project_dir) + manager.install_from_directory(preset_a_dir, "0.1.5", priority=1) + + self._write_init_options(project_dir, ai="codex", ai_skills=True) + codex_skills_dir = project_dir / ".agents" / "skills" + self._create_skill(codex_skills_dir, "speckit-specify") + manager.register_enabled_presets_for_agent("codex") + + metadata_a = manager.registry.get("orphan-skill-preset-a") + assert set(metadata_a.get("registered_skills", {})) == {"claude", "codex"}, ( + "sanity: preset A must be tracked under both agents" + ) + + # Preset B is installed only now, while codex is the sole active + # agent — it never writes to or tracks claude. + preset_b_dir = self._create_command_preset( + temp_dir, "orphan-skill-preset-b", "speckit.specify", + "Preset B", "preset B body", + ) + manager.install_from_directory(preset_b_dir, "0.1.5", priority=10) + + metadata_b = manager.registry.get("orphan-skill-preset-b") + assert set(metadata_b.get("registered_skills", {})) == {"codex"}, ( + "sanity: preset B must only be tracked for codex before " + "preset A is removed" + ) + + assert manager.remove("orphan-skill-preset-a") is True + + claude_skill_file = claude_skills_dir / "speckit-specify" / "SKILL.md" + assert claude_skill_file.exists(), ( + "sanity: claude's skill file must have been restored by " + "reconciliation" + ) + assert "preset:orphan-skill-preset-b" in claude_skill_file.read_text(), ( + "sanity: claude's SKILL.md must reflect preset B's content " + "after preset A is removed" + ) + + metadata_b = manager.registry.get("orphan-skill-preset-b") + assert set(metadata_b.get("registered_skills", {})) == {"claude", "codex"}, ( + "preset B's own registered_skills must be updated to include " + "claude once reconciliation actually renders content there " + "on its behalf — otherwise B's registry entry silently lies " + "about which directories it owns (#2948)" + ) + + assert manager.remove("orphan-skill-preset-b") is True + + # No preset is installed any more, so claude's SKILL.md must have + # been reconciled down to the core bundled template (or removed + # entirely) — but it must NOT still contain B's stale content, + # which would mean B's write there was never tracked for cleanup. + if claude_skill_file.exists(): + assert "preset:orphan-skill-preset-b" not in claude_skill_file.read_text(), ( + "removing preset B must clean up claude's directory too, " + "since B's registered_skills was updated to include it — " + "otherwise B's stale content is orphaned there forever " + "with no preset left to track or clean it up (#2948)" + ) + def test_symlinked_skill_subdir_rejected_on_restore(self, project_dir, temp_dir): """Restore must validate each per-skill subdirectory, not just its parent. @@ -5518,6 +5769,107 @@ class TestPresetSkills: "the symlink itself should be left alone" ) + def test_is_safe_registry_skill_name_rejects_unsafe_values(self, project_dir): + """Unit-test the centralized registry skill-name boundary guard. + + ``registered_skills`` entries are persisted registry data, not + manifest-derived, so every preset cleanup/provenance loop that + joins one onto a directory must first reject: non-strings, empty + strings, absolute paths, multi-component paths (containing ``/``), + and the literal traversal components ``"."``/``".."`` — the last + of which is *not* caught by a naive ``is_absolute() or + len(parts) != 1`` check alone, since ``Path("..").parts`` is a + single-element tuple (#2948). + """ + manager = PresetManager(project_dir) + is_safe = manager._is_safe_registry_skill_name + + assert is_safe("speckit-specify") is True + assert is_safe("") is False + assert is_safe(None) is False + assert is_safe(123) is False + assert is_safe(["speckit-specify"]) is False + assert is_safe(".") is False + assert is_safe("..") is False + assert is_safe("/etc/passwd") is False + assert is_safe(str(project_dir / "important-data")) is False + assert is_safe("foo/bar") is False + assert is_safe("foo/..") is False + assert is_safe("../foo") is False + + def test_unregister_skills_in_dir_rejects_absolute_registry_name( + self, project_dir + ): + """A corrupted ``registered_skills`` entry with an absolute path must not escape. + + ``Path`` join with an absolute right-hand operand discards the + left side entirely (``skills_dir / "/abs/path"`` == ``"/abs/path"``), + so an absolute in-project path stored in the registry would bypass + ``skills_dir`` altogether if not rejected before the join (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = project_dir / ".claude" / "skills" + claude_skills_dir.mkdir(parents=True) + + precious_dir = project_dir / "important-data" + precious_dir.mkdir() + precious_file = precious_dir / "SKILL.md" + precious_file.write_text("precious-absolute-target-marker") + + manager = PresetManager(project_dir) + manager._unregister_skills_in_dir( + [str(precious_dir)], claude_skills_dir, "claude" + ) + + assert precious_dir.is_dir(), ( + "an absolute registry entry must not let cleanup escape " + "skills_dir to an unrelated project directory (#2948)" + ) + assert precious_file.read_text() == "precious-absolute-target-marker" + + def test_infer_legacy_skill_provenance_rejects_absolute_registry_name( + self, project_dir + ): + """Legacy provenance inference must reject an absolute registry name. + + ``_infer_legacy_skill_provenance`` receives its ``skill_names`` + directly from a legacy flat-list ``registered_skills`` value — + registry data, not manifest-derived — and joins each name onto a + candidate agent's resolved skills directory the same way + ``_unregister_skills_in_dir`` does. An absolute in-project name + discards the candidate directory entirely (Python's ``/`` operator + drops the left side for an absolute right side), so it can read + an unrelated project directory's ``SKILL.md`` and, if its + frontmatter happens to carry a matching preset source marker, + falsely attribute an unrelated directory as this preset's own + skill override under whichever agent is being probed (#2948). + """ + self._write_init_options(project_dir, ai="claude", ai_skills=True) + claude_skills_dir = project_dir / ".claude" / "skills" + claude_skills_dir.mkdir(parents=True) + + precious_dir = project_dir / "important-data" + precious_dir.mkdir() + (precious_dir / "SKILL.md").write_text( + "---\n" + "metadata:\n" + " source: preset:some-pack\n" + "---\n\n" + "# Unrelated directory, not a real preset skill\n" + ) + + manager = PresetManager(project_dir) + inferred = manager._infer_legacy_skill_provenance( + [str(precious_dir)], "some-pack", "claude" + ) + + for names in inferred.values(): + assert str(precious_dir) not in names, ( + "an absolute registry entry must not be falsely attributed " + "as preset-owned provenance by probing outside the " + "intended skills subtree (#2948)" + ) + def test_copilot_skills_registration_restored_after_process_restart( self, project_dir, temp_dir ):