From 520867972dd036a070e53897b69a3483cf194943 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 06:46:45 -0500 Subject: [PATCH 01/18] feat: register extension commands for generic integration Support generic command and skill registration in configured output directories, track generated artifacts for ownership-aware cleanup, and cover both layouts and failure cases. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/integration.md | 4 +- docs/reference/extensions.md | 2 +- docs/reference/integrations.md | 10 +- src/specify_cli/__init__.py | 15 +- src/specify_cli/agents.py | 46 +- src/specify_cli/extensions/__init__.py | 289 ++++++++++++- src/specify_cli/extensions/command_disable.py | 11 + src/specify_cli/extensions/command_enable.py | 18 + .../integrations/generic/__init__.py | 23 + .../integrations/test_integration_generic.py | 409 +++++++++++++++++- 10 files changed, 791 insertions(+), 36 deletions(-) diff --git a/design/integration.md b/design/integration.md index 2bcb516022..117641f0d9 100644 --- a/design/integration.md +++ b/design/integration.md @@ -71,7 +71,9 @@ selected variant. `py` is opt-in; non-interactive init defaults to `sh` on POSIX or `ps` on Windows. Maintain equivalent stdout behavior across all three. Bundled extension commands do not yet use this core-template script routing. `__AGENT__` and command references are resolved during rendering, not by -adding per-agent wrapper scripts. +adding per-agent wrapper scripts. For `generic`, extension registration +resolves the persisted `--commands-dir` rather than the static registry +placeholder; `--skills` emits skills into that same directory. ## Ownership and lifecycle diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 1c2c2d0179..8dbde190a8 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -29,7 +29,7 @@ specify extension add | `--force` | Overwrite if the extension is already installed | | `--priority `| Resolution priority (default: 10; lower = higher precedence) | -Installs an extension from the catalog, a URL, or a local directory. Extension commands are automatically registered with the currently installed AI coding agent integration. +Installs an extension from the catalog, a URL, or a local directory. Extension commands are registered with the active AI coding agent integration. For `generic`, invocations use the configured `--commands-dir`: flat command files by default, or `speckit-/SKILL.md` with `--skills`. The core `speckit.taskstoissues` command remains available alongside the GitHub extension's namespaced replacement during migration. > **Note:** All extension commands require a project already initialized with `specify init`. diff --git a/docs/reference/integrations.md b/docs/reference/integrations.md index 5f9cb80848..62b876c73f 100644 --- a/docs/reference/integrations.md +++ b/docs/reference/integrations.md @@ -259,7 +259,7 @@ Some integrations accept additional options via `--integration-options`: | Integration | Option | Description | | ----------- | ------------------- | -------------------------------------------------------------- | | `generic` | `--commands-dir` | Required. Directory for command files | -| `generic` | `--skills` | Render commands as `speckit-/SKILL.md` directories under `--commands-dir` instead of flat `speckit..md` files. Command references and next-step guidance switch to `/speckit-`. Generic's output directory is a runtime option rather than a static per-agent folder, so this does not enable extension/preset add-on skill registration in either layout. | +| `generic` | `--skills` | Render commands and installed extension invocations as `speckit-/SKILL.md` directories under `--commands-dir` instead of flat `speckit..md` files. Command references and next-step guidance switch to `/speckit-`. | | `kimi` | `--migrate-legacy` | Migrate legacy `.kimi/skills/` installs to `.kimi-code/skills/` (including dotted→hyphenated skill naming, e.g. `speckit.xxx` → `speckit-xxx`) | | `copilot` | `--commands` | Scaffold `.github/agents/*.agent.md` commands with `.github/prompts/*.prompt.md` companions and merge `.vscode/settings.json` instead of using the default skills layout. | | `copilot` | `--skills` | Force the default skills layout, overriding an existing commands layout during an explicit migration. | @@ -271,6 +271,14 @@ specify integration install generic --integration-options="--commands-dir .myage specify integration install generic --integration-options="--commands-dir .myagent/skills --skills" ``` +Once `generic` is the active integration, `specify extension add` registers +extension commands in its configured `--commands-dir` (as command files or +skills according to `--skills`). `specify extension remove` removes unchanged +extension-owned artifacts while leaving core commands, user files, and edited +extension files intact. The core `speckit.taskstoissues` command remains +available; installing the GitHub extension adds the namespaced replacement +without deprecating or removing the core command. + ## Scaffold a New Integration ```bash diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 8797a1dd0d..16e1463744 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -272,6 +272,12 @@ def _get_skills_dir(project_path: Path, selected_ai: str) -> Path: Returns ``project_path / / "skills"``, falling back to ``project_path / ".agents/skills"`` for unknown agents. """ + if selected_ai == "generic": + from .integrations.generic import registration_directory + + return project_path / registration_directory(project_path).relative_to( + project_path.resolve() + ) agent_config = AGENT_CONFIG.get(selected_ai, {}) agent_folder = agent_config.get("folder", "") if agent_folder: @@ -307,15 +313,6 @@ def resolve_active_skills_dir(project_root: Path) -> Path | None: if not isinstance(agent, str) or not agent: return None - # generic's output directory is a runtime --commands-dir CLI option, not - # a static per-agent folder (its config["folder"] is None), so there is - # no directory extension/preset skill registration could safely resolve - # here even when the project was scaffolded with --skills. Registration - # stays disabled for generic in both layouts, matching flat-mode generic - # (which never persists ai_skills=True and so never reaches this point). - if agent == "generic": - return None - ai_skills_enabled = _is_ai_skills_enabled(opts) if not ai_skills_enabled and agent != "kimi": return None diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 134b4e6d2f..8a301e93e8 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -6,6 +6,7 @@ command files into agent-specific directories in the correct format. """ +import hashlib import os import re from copy import deepcopy @@ -58,8 +59,23 @@ class CommandRegistrar: AGENT_CONFIGS: dict[str, dict[str, Any]] = {} _configs_loaded: bool = False - def __init__(self) -> None: + def __init__(self, project_root: Path | None = None) -> None: self._ensure_configs() + self.AGENT_CONFIGS = dict(self.AGENT_CONFIGS) + if project_root is not None: + from .integrations.generic import registration_directory + + from ._init_options import load_init_options + + opts = load_init_options(project_root) + if isinstance(opts, dict) and opts.get("ai") == "generic": + self.AGENT_CONFIGS["generic"] = { + "dir": str(registration_directory(project_root)), + "format": "markdown", + "args": "$ARGUMENTS", + "extension": ".md", + "invoke_separator": ".", + } def __init_subclass__(cls, **kwargs: Any) -> None: super().__init_subclass__(**kwargs) @@ -604,6 +620,24 @@ def _is_safe_command_name(name: str) -> bool: return False return os.path.normpath(name) == name + @staticmethod + def _generic_owned_output(path: Path, source_id: str, project_root: Path) -> bool: + """Only reuse a generated file if its installed extension still owns its bytes.""" + from .extensions import ExtensionRegistry + + metadata = ExtensionRegistry( + project_root / ".specify" / "extensions" + ).get(source_id) + hashes = metadata.get("generic_artifact_hashes", {}) if metadata else {} + if not isinstance(hashes, dict) or not path.is_file(): + return False + if path.is_symlink() and not path.resolve().is_relative_to( + (project_root / ".specify/extensions").resolve() + ): + return False + relative = path.relative_to(project_root.resolve()).as_posix() + return hashes.get(relative) == hashlib.sha256(path.read_bytes()).hexdigest() + @staticmethod def _same_lexical_path(left: Path, right: Path) -> bool: """Compare paths after lexical normalization without resolving symlinks.""" @@ -872,6 +906,11 @@ def register_commands( dest_file = commands_dir / f"{output_name}{agent_config['extension']}" self._ensure_inside(dest_file, commands_dir) + if agent_name == "generic" and (dest_file.exists() or dest_file.is_symlink()): + if not self._generic_owned_output(dest_file, source_id, project_root): + continue + registered.append(cmd_name) + continue dest_file.parent.mkdir(parents=True, exist_ok=True) self._write_registered_output( dest_file, @@ -954,6 +993,11 @@ def register_commands( commands_dir / f"{alias_output_name}{agent_config['extension']}" ) self._ensure_inside(alias_file, commands_dir) + if agent_name == "generic" and (alias_file.exists() or alias_file.is_symlink()): + if not self._generic_owned_output(alias_file, source_id, project_root): + continue + registered.append(alias) + continue alias_file.parent.mkdir(parents=True, exist_ok=True) self._write_registered_output( alias_file, diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index e4b9e7de9d..d0108b7cc6 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1371,7 +1371,7 @@ def _ensure_usable(skills_dir: Path) -> Optional[Path]: from ..agents import CommandRegistrar - registrar = CommandRegistrar() + registrar = CommandRegistrar(self.project_root) agent_config = registrar.AGENT_CONFIGS.get(selected_ai) ai_skills_enabled = is_ai_skills_enabled(opts) if not create: @@ -1445,7 +1445,7 @@ def _active_command_registration_scope(self) -> Optional[set[str]]: from ..agents import CommandRegistrar as AgentRegistrar - agent_config = AgentRegistrar().AGENT_CONFIGS.get(active_agent) + agent_config = AgentRegistrar(self.project_root).AGENT_CONFIGS.get(active_agent) if ( agent_config and is_ai_skills_enabled(load_init_options(self.project_root)) @@ -1460,7 +1460,7 @@ def _command_registration_targets(self) -> Dict[str, Path]: """Return current or recoverable command roots for a new install.""" from ..agents import CommandRegistrar as AgentRegistrar - registrar = AgentRegistrar() + registrar = AgentRegistrar(self.project_root) agent_scope = self._active_command_registration_scope() active_skills_agent = registrar._active_skills_agent(self.project_root) recoverable_active_skills_dir = ( @@ -1533,7 +1533,7 @@ def _register_commands_for_active_agent( Mapping of agent name to registered command names, matching the ``registered_commands`` registry shape. """ - registrar = CommandRegistrar() + registrar = CommandRegistrar(self.project_root) agent_scope = self._active_command_registration_scope() if agent_scope is None: @@ -1608,7 +1608,7 @@ def _register_extension_skills( selected_ai = opts.get("ai") if not isinstance(selected_ai, str) or not selected_ai: return [] - registrar = CommandRegistrar() + registrar = CommandRegistrar(self.project_root) agent_config = registrar.AGENT_CONFIGS.get(selected_ai, {}) integration = get_integration(selected_ai) ai_skills_enabled = is_ai_skills_enabled(opts) @@ -1669,6 +1669,12 @@ def _replacement(match: re.Match[str]) -> str: skill_subdir.exists() or skill_subdir.is_symlink() ) CommandRegistrar._ensure_inside(cache_file, cache_root) + if selected_ai == "generic" and skill_dir_preexists: + metadata = self.registry.get(manifest.id) or {} + if skill_name not in self._generic_owned_names( + metadata, [skill_name], skills=True + ): + continue if skill_file.exists() or skill_file.is_symlink(): is_expected_dev_symlink = self._is_expected_dev_symlink( skill_file, cache_file @@ -1902,7 +1908,15 @@ def add_candidate(candidate: Path) -> None: ) add_candidate(self.project_root / DEFAULT_SKILLS_DIR) - registrar = CommandRegistrar() + from ..integration_state import integration_setting, try_read_integration_json + + state, error = try_read_integration_json(self.project_root) + if error is None and integration_setting(state or {}, "generic"): + from ..integrations.generic import registration_directory + + add_candidate(registration_directory(self.project_root)) + + registrar = CommandRegistrar(self.project_root) for agent_name, agent_config in registrar.AGENT_CONFIGS.items(): if agent_config.get("extension") != "/SKILL.md": continue @@ -1914,11 +1928,122 @@ def add_candidate(candidate: Path) -> None: return candidates + def _generic_artifact_hashes( + self, + registered_commands: Dict[str, List[str]], + registered_skills: List[str], + previous: Optional[Dict[str, str]] = None, + ) -> Dict[str, str]: + """Record generic-owned output by path and hash for safe later removal.""" + from ..integrations.generic import registration_directory + + if not registered_commands.get("generic") and not registered_skills: + return previous or {} + output_dir = registration_directory(self.project_root) + paths = [ + output_dir / f"{name}.md" + for name in registered_commands.get("generic", []) + ] + [output_dir / name / "SKILL.md" for name in registered_skills] + hashes = dict(previous or {}) + for path in paths: + if not path.is_file(): + continue + relative = path.relative_to(self.project_root.resolve()).as_posix() + digest = hashlib.sha256(path.read_bytes()).hexdigest() + if relative not in hashes: + hashes[relative] = digest + return hashes + + def _generic_owned_names( + self, metadata: Dict[str, Any], names: List[str], *, skills: bool + ) -> List[str]: + """Keep customized or untracked generic artifacts out of cleanup.""" + from ..integrations.generic import registration_directory + + hashes = metadata.get("generic_artifact_hashes", {}) + if not isinstance(hashes, dict): + return [] + output_dir = registration_directory(self.project_root) + owned = [] + for name in names: + path = output_dir / name / "SKILL.md" if skills else output_dir / f"{name}.md" + if not path.is_file(): + continue + if path.is_symlink() and not path.resolve().is_relative_to( + self.extensions_dir.resolve() + ): + continue + relative = path.relative_to(self.project_root.resolve()).as_posix() + if hashes.get(relative) == hashlib.sha256(path.read_bytes()).hexdigest(): + owned.append(name) + return owned + + def _remove_generic_artifact_paths( + self, extension_id: str, metadata: Dict[str, Any], *, skills: bool = True + ) -> None: + """Clean recorded generic paths even after the configured directory moves.""" + manifest = self.get_extension(extension_id) + registered = metadata.get("registered_commands", {}) + command_names = set( + self._valid_name_list(registered.get("generic", [])) + ) if isinstance(registered, dict) else set() + skill_names = set(self._valid_name_list(metadata.get("registered_skills", []))) + if manifest is not None: + command_names.update( + name + for command in manifest.commands + for name in [command["name"], *(command.get("aliases") or [])] + ) + skill_names.update( + self._skill_name_for_command(command["name"]) + for command in manifest.commands + ) + hashes = metadata.get("generic_artifact_hashes", {}) + if not isinstance(hashes, dict): + return + root = self.project_root.resolve() + source = (self.extensions_dir / extension_id).resolve() + for relative, expected in hashes.items(): + if not isinstance(relative, str) or not isinstance(expected, str): + continue + name = Path(relative) + if name.is_absolute() or ".." in name.parts: + continue + skill_output = ( + name.name == "SKILL.md" and name.parent.name in skill_names + ) + if skill_output and not skills: + continue + if not skill_output and name.name not in { + f"{command}.md" for command in command_names + }: + continue + path = root / name + if not path.parent.resolve().is_relative_to(root): + continue + if not path.is_file(): + continue + if path.is_symlink() and not path.resolve().is_relative_to(source): + continue + content = path.read_bytes() + if hashlib.sha256(content).hexdigest() != expected: + if path.is_symlink(): + path.unlink() + path.write_bytes(content) + continue + path.unlink() + if skill_output: + try: + path.parent.rmdir() + except OSError: + pass + def _unregister_extension_skills( self, skill_names: List[str], extension_id: str, skills_dir: Optional[Path] = None, + generic_hashes: Optional[Dict[str, str]] = None, ) -> None: """Remove SKILL.md directories for extension skills. @@ -1941,9 +2066,30 @@ def _unregister_extension_skills( every configured agent's skills directory is scanned instead of resolving just the currently active one. """ + generic_roots = { + Path(path).parent.parent + for path in (generic_hashes or {}) + if isinstance(path, str) and path.endswith("/SKILL.md") + } for skill_subdir in self._find_extension_skill_dirs( skill_names, extension_id, skills_dir=skills_dir ): + skill_file = skill_subdir / "SKILL.md" + if generic_hashes is not None and skill_file.is_relative_to( + self.project_root.resolve() + ): + relative = skill_file.relative_to(self.project_root.resolve()).as_posix() + if Path(relative).parent.parent in generic_roots: + if generic_hashes.get(relative) != hashlib.sha256( + skill_file.read_bytes() + ).hexdigest(): + continue + skill_file.unlink() + try: + skill_subdir.rmdir() + except OSError: + pass + continue shutil.rmtree(skill_subdir) def _extension_owned_skill_names( @@ -2135,6 +2281,18 @@ def install_from_directory( # Reject manifests that would shadow core commands or installed extensions. self._validate_install_conflicts(manifest) + from .. import load_init_options + + active_options = load_init_options(self.project_root) + generic_active = isinstance(active_options, dict) and active_options.get("ai") == "generic" + if register_commands and generic_active and manifest.commands: + from ..integrations.generic import registration_directory + + try: + registration_directory(self.project_root) + except (OSError, ValueError) as exc: + raise ExtensionError(f"Cannot register generic extension commands: {exc}") from exc + # Refuse to install an extension from its own install destination — with # --force this would delete the source before copying it (issue #2990). dest_dir = self.extensions_dir / manifest.id @@ -2622,6 +2780,16 @@ def _restore_stranded_config_file( registered_skills = self._register_extension_skills( manifest, dest_dir, link_outputs=link_commands ) + if register_commands and generic_active and manifest.commands: + if not registered_commands.get("generic") and not registered_skills: + raise ExtensionError( + "Cannot register generic extension commands: no invocation artifacts " + "were written to the configured directory" + ) + generic_hashes = ( + self._generic_artifact_hashes(registered_commands, registered_skills) + if generic_active else {} + ) # Register hooks and update installed list in extensions.yml hook_executor = HookExecutor(self.project_root) @@ -2670,6 +2838,7 @@ def _restore_stranded_config_file( "priority": priority, "registered_commands": registered_commands, "registered_skills": registered_skills, + "generic_artifact_hashes": generic_hashes, }, ) @@ -2976,11 +3145,20 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: # Unregister commands from all AI agents if registered_commands: - registrar = CommandRegistrar() - registrar.unregister_commands(registered_commands, self.project_root) + registrar = CommandRegistrar(self.project_root) + safe_commands = dict(registered_commands) + if "generic" in safe_commands: + safe_commands.pop("generic") + registrar.unregister_commands(safe_commands, self.project_root) + if metadata: + self._remove_generic_artifact_paths(extension_id, metadata) # Unregister agent skills - self._unregister_extension_skills(registered_skills, extension_id) + self._unregister_extension_skills( + registered_skills, + extension_id, + generic_hashes=metadata.get("generic_artifact_hashes") if metadata else None, + ) if keep_config: # Preserve config files, only remove non-config files @@ -3031,6 +3209,57 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: return True + def disable_generic_extension_artifacts(self, extension_id: str) -> None: + """Retire generic invocations without removing the installed sources.""" + metadata = self.registry.get(extension_id) + if not metadata: + raise ExtensionError(f"Extension '{extension_id}' is not installed") + + registered = metadata.get("registered_commands", {}) + commands = self._valid_name_list(registered.get("generic")) if isinstance(registered, dict) else [] + skills = self._valid_name_list(metadata.get("registered_skills", [])) + from ..integrations.generic import registration_directory + + directory = registration_directory(self.project_root) + hashes = metadata.get("generic_artifact_hashes", {}) + if isinstance(hashes, dict): + for relative, expected in hashes.items(): + if not isinstance(relative, str) or not isinstance(expected, str): + continue + name = Path(relative) + if name.is_absolute() or ".." in name.parts: + continue + path = self.project_root.resolve() / name + if path.parent.resolve().is_relative_to(self.project_root.resolve()) and path.is_file(): + if hashlib.sha256(path.read_bytes()).hexdigest() != expected: + raise ExtensionError( + f"Cannot disable '{extension_id}': generic artifact {path} " + "was modified or is not owned; preserve it and remove it manually" + ) + for names, is_skill in ((commands, False), (skills, True)): + owned = self._generic_owned_names(metadata, names, skills=is_skill) + for name in names: + path = directory / name / "SKILL.md" if is_skill else directory / f"{name}.md" + if (path.exists() or path.is_symlink()) and name not in owned: + raise ExtensionError( + f"Cannot disable '{extension_id}': generic artifact {path} " + "was modified or is not owned; preserve it and remove it manually" + ) + + self._remove_generic_artifact_paths(extension_id, metadata) + if skills: + self._unregister_extension_skills( + skills, extension_id, skills_dir=directory, + generic_hashes=metadata.get("generic_artifact_hashes", {}), + ) + new_commands = dict(registered) if isinstance(registered, dict) else {} + new_commands.pop("generic", None) + self.registry.update(extension_id, { + "registered_commands": new_commands, + "registered_skills": self._extension_owned_skill_names(skills, extension_id), + "generic_artifact_hashes": {}, + }) + @staticmethod def _valid_name_list(value: Any) -> List[str]: """Return string entries from a registry list, ignoring corrupt values.""" @@ -3066,7 +3295,7 @@ def unregister_agent_artifacts( if not agent_name: return - registrar = CommandRegistrar() + registrar = CommandRegistrar(self.project_root) if agent_name not in registrar.AGENT_CONFIGS: return @@ -3081,6 +3310,10 @@ def unregister_agent_artifacts( if enabled_only and not metadata.get("enabled", True): continue + if agent_name == "generic": + self._remove_generic_artifact_paths( + ext_id, metadata, skills=not commands_only + ) updates: Dict[str, Any] = {} registered_commands = metadata.get("registered_commands", {}) @@ -3091,6 +3324,10 @@ def unregister_agent_artifacts( command_names = self._valid_name_list( registered_commands.get(agent_name) ) + if agent_name == "generic": + command_names = self._generic_owned_names( + metadata, command_names, skills=False + ) if command_names: registrar.unregister_commands( {agent_name: command_names}, self.project_root @@ -3104,6 +3341,10 @@ def unregister_agent_artifacts( metadata.get("registered_skills", []) ) if registered_skills and not commands_only: + if agent_name == "generic": + registered_skills = self._generic_owned_names( + metadata, registered_skills, skills=True + ) # Always pass the explicit, agent-scoped skills_dir — even # when it doesn't currently exist on disk. This method must # stay scoped to *this* agent only; omitting skills_dir (a @@ -3115,7 +3356,11 @@ def unregister_agent_artifacts( # to clean up; the fast path below is a safe no-op in that # case (every candidate skill_subdir.is_dir() check fails). self._unregister_extension_skills( - registered_skills, ext_id, skills_dir=agent_skills_dir + registered_skills, ext_id, skills_dir=agent_skills_dir, + generic_hashes=( + metadata.get("generic_artifact_hashes") + if agent_name == "generic" else None + ), ) # Only reconcile registry state when this agent's directory @@ -3177,7 +3422,7 @@ def _retire_legacy_flat_extension_commands( ): return [] - registrar = CommandRegistrar() + registrar = CommandRegistrar(self.project_root) agent_config = registrar.AGENT_CONFIGS.get(agent_name) if not agent_config or agent_config.get("extension") != "/SKILL.md": return [] @@ -3239,7 +3484,7 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool from .. import load_init_options - registrar = CommandRegistrar() + registrar = CommandRegistrar(self.project_root) agent_config = registrar.AGENT_CONFIGS.get(agent_name) init_options = load_init_options(self.project_root) if not isinstance(init_options, dict): @@ -3286,6 +3531,7 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool try: updates: Dict[str, Any] = {} registered: List[str] = [] + registered_skills: List[str] = [] # Set when a command -> skills toggle for this same agent # defers stale command-mode cleanup until the skills # replacement below confirms success (#2948). @@ -3485,6 +3731,10 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool ) ] if fully_replaced: + if agent_name == "generic": + fully_replaced = self._generic_owned_names( + metadata, fully_replaced, skills=False + ) registrar.unregister_commands( {agent_name: fully_replaced}, self.project_root ) @@ -3512,6 +3762,14 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool registered, ) + if agent_name == "generic": + hashes = self._generic_artifact_hashes( + {"generic": registered} if registered else {}, + registered_skills if agent_name == active_agent else [], + metadata.get("generic_artifact_hashes"), + ) + if hashes != metadata.get("generic_artifact_hashes"): + updates["generic_artifact_hashes"] = hashes if updates: self.registry.update(ext_id, updates) except Exception as ext_err: @@ -3623,10 +3881,11 @@ class CommandRegistrar: AGENT_CONFIGS = _AgentRegistrar.AGENT_CONFIGS - def __init__(self): + def __init__(self, project_root: Path | None = None): from ..agents import CommandRegistrar as _Registrar - self._registrar = _Registrar() + self._registrar = _Registrar(project_root) + self.AGENT_CONFIGS = self._registrar.AGENT_CONFIGS # Delegate static/utility methods @staticmethod diff --git a/src/specify_cli/extensions/command_disable.py b/src/specify_cli/extensions/command_disable.py index b9ee83134e..c1da4f68dc 100644 --- a/src/specify_cli/extensions/command_disable.py +++ b/src/specify_cli/extensions/command_disable.py @@ -42,6 +42,17 @@ def extension_disable( console.print(f"[yellow]Extension '{_escape_markup(str(display_name))}' is already disabled[/yellow]") raise typer.Exit(0) + from .. import load_init_options + + if load_init_options(project_root).get("ai") == "generic": + from . import ExtensionError + + try: + manager.disable_generic_extension_artifacts(extension_id) + except (ExtensionError, ValueError, OSError) as exc: + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc + manager.registry.update(extension_id, {"enabled": False}) # Disable hooks in extensions.yml diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index c338245905..6c19b9eefb 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -44,6 +44,24 @@ def extension_enable( manager.registry.update(extension_id, {"enabled": True}) + from .. import load_init_options + + if load_init_options(project_root).get("ai") == "generic": + manager.register_enabled_extensions_for_agent("generic") + refreshed = manager.registry.get(extension_id) or {} + commands = refreshed.get("registered_commands", {}) + manifest = manager.get_extension(extension_id) + if manifest and manifest.commands and not ( + (isinstance(commands, dict) and commands.get("generic")) + or refreshed.get("registered_skills") + ): + manager.registry.update(extension_id, {"enabled": False}) + console.print( + f"[red]Error:[/red] Could not register generic invocations " + f"for '{_escape_markup(str(extension_id))}'." + ) + raise typer.Exit(1) + # Enable hooks in extensions.yml config = hook_executor.get_project_config() if "hooks" in config: diff --git a/src/specify_cli/integrations/generic/__init__.py b/src/specify_cli/integrations/generic/__init__.py index fbceec20cb..166ba94a1d 100644 --- a/src/specify_cli/integrations/generic/__init__.py +++ b/src/specify_cli/integrations/generic/__init__.py @@ -18,6 +18,26 @@ from ..manifest import IntegrationManifest +def registration_directory(project_root: Path) -> Path: + """Resolve the installed generic output root, never the class-level placeholder.""" + from ...integration_state import integration_setting, try_read_integration_json + + state, error = try_read_integration_json(project_root) + if error is not None: + raise ValueError(f"Cannot read generic integration settings: {error.detail}") + settings = integration_setting(state or {}, "generic") + commands_dir = GenericIntegration._resolve_commands_dir( + settings.get("parsed_options"), {"raw_options": settings.get("raw_options")} + ) + if not isinstance(commands_dir, str): + raise ValueError("Invalid --commands-dir in generic integration settings") + root = project_root.resolve() + destination = (project_root / commands_dir).resolve() + if not destination.is_relative_to(root): + raise ValueError(f"Generic command directory {destination} escapes project root {root}") + return destination + + class _GenericSkillsHelper(SkillsIntegration): """Internal helper supplying skills-mode post-processing for ``GenericIntegration`` (e.g. the dot-to-hyphen hook invocation note). @@ -49,6 +69,9 @@ class GenericIntegration(MarkdownIntegration): "extension": ".md", } + def post_process_skill_content(self, content: str) -> str: + return _GenericSkillsHelper().post_process_skill_content(content) + def effective_invoke_separator( self, parsed_options: dict[str, Any] | None = None, diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 0b65802822..f84bc80499 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -3,10 +3,402 @@ import os import pytest +import yaml from specify_cli.integrations import get_integration from specify_cli.integrations.base import MarkdownIntegration from specify_cli.integrations.manifest import IntegrationManifest +from specify_cli.integration_state import write_integration_json +from specify_cli.extensions import ExtensionError, ExtensionManager +from specify_cli import save_init_options + + +@pytest.fixture +def generic_extension(tmp_path): + source = tmp_path / "source" + source.mkdir() + (source / "extension.yml").write_text(yaml.safe_dump({ + "schema_version": "1.0", + "extension": { + "id": "sample", "name": "Sample", "version": "1.0.0", + "description": "Sample commands", "author": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": {"commands": [{ + "name": "speckit.sample.run", + "file": "commands/run.md", + "description": "Run sample", + }]}, + }), encoding="utf-8") + (source / "commands").mkdir() + (source / "scripts").mkdir() + (source / "scripts" / "run.sh").write_text("echo sample\n", encoding="utf-8") + (source / "commands" / "run.md").write_text( + "---\ndescription: Run sample\nscripts:\n sh: scripts/run.sh\n---\n" + "Execute {SCRIPT} with {ARGS}; see scripts/run.sh. " + "Then __SPECKIT_COMMAND_TASKS__.\n", + encoding="utf-8", + ) + return source + + +def generic_project(tmp_path, *, skills=False, commands_dir=".custom/commands"): + project = tmp_path / "project" + project.mkdir() + opts = {"commands_dir": commands_dir, "skills": skills} + integration = get_integration("generic") + manifest = IntegrationManifest("generic", project) + integration.setup(project, manifest, parsed_options=opts) + manifest.save() + write_integration_json( + project, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": opts}}, + ) + save_init_options(project, {"ai": "generic", "ai_skills": skills, "script": "sh"}) + return project + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_lifecycle(tmp_path, generic_extension, skills): + project = generic_project( + tmp_path, skills=skills, + commands_dir=".custom/skills" if skills else ".custom/commands", + ) + output_dir = project / ".custom" / ("skills" if skills else "commands") + core = output_dir / ("speckit-taskstoissues/SKILL.md" if skills else "speckit.taskstoissues.md") + core_content = core.read_bytes() + unrelated = output_dir / ("user-skill/SKILL.md" if skills else "user.md") + unrelated.parent.mkdir(parents=True, exist_ok=True) + unrelated.write_text("user-owned", encoding="utf-8") + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = output_dir / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + assert artifact.is_file() + content = artifact.read_text(encoding="utf-8") + assert ".specify/extensions/sample/scripts/run.sh" in content + assert "{SCRIPT}" not in content + assert "$ARGUMENTS" in content + assert ("/speckit-tasks" if skills else "/speckit.tasks") in content + manager.register_enabled_extensions_for_agent("generic") + assert artifact.read_text(encoding="utf-8") == content + if skills: + sibling = artifact.parent / "user-notes.md" + sibling.write_text("keep this", encoding="utf-8") + assert manager.remove("sample") + assert not artifact.exists() + if skills: + assert sibling.read_text(encoding="utf-8") == "keep this" + assert core.read_bytes() == core_content + assert unrelated.read_text(encoding="utf-8") == "user-owned" + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_preserves_modified_artifact(tmp_path, generic_extension, skills): + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + artifact.write_text(artifact.read_text(encoding="utf-8") + "\nuser edit\n", encoding="utf-8") + assert manager.remove("sample") + assert artifact.read_text(encoding="utf-8").endswith("user edit\n") + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_rejects_modified_artifact_on_disable( + tmp_path, generic_extension, skills, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + artifact.write_text("user edit", encoding="utf-8") + manager.register_enabled_extensions_for_agent("generic") + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, ["extension", "disable", "sample"]) + finally: + os.chdir(old_cwd) + assert result.exit_code == 1 + assert "modified or is not owned" in result.output + assert artifact.read_text(encoding="utf-8") == "user edit" + assert manager.registry.get("sample")["enabled"] is True + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_reports_missing_registration_options( + tmp_path, generic_extension, skills, +): + project = generic_project(tmp_path, skills=skills) + (project / ".specify/integration.json").unlink() + with pytest.raises(ExtensionError, match="generic"): + ExtensionManager(project).install_from_directory(generic_extension, "1.0.0") + assert not ExtensionManager(project).registry.is_installed("sample") + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_after_cli_init(tmp_path, generic_extension, skills): + from typer.testing import CliRunner + from specify_cli import app + + project = tmp_path / "initialized" + project.mkdir() + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, [ + "init", "--here", "--integration", "generic", + "--integration-options=--commands-dir .myagent/commands" + ( + " --skills" if skills else "" + ), + "--script", "sh", + ], catch_exceptions=False) + finally: + os.chdir(old_cwd) + assert result.exit_code == 0, result.output + runner = CliRunner() + old_cwd = os.getcwd() + try: + os.chdir(project) + installed = runner.invoke( + app, ["extension", "add", str(generic_extension), "--dev"] + ) + assert installed.exit_code == 0, installed.output + finally: + os.chdir(old_cwd) + output = project / ".myagent/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + assert output.is_file() + try: + os.chdir(project) + removed = runner.invoke(app, ["extension", "remove", "sample", "--force"]) + assert removed.exit_code == 0, removed.output + finally: + os.chdir(old_cwd) + assert not output.exists() + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_remove_after_directory_upgrade( + tmp_path, generic_extension, skills, +): + from typer.testing import CliRunner + from specify_cli import app + + project = tmp_path / "upgrade" + project.mkdir() + runner = CliRunner() + suffix = " --skills" if skills else "" + old_cwd = os.getcwd() + try: + os.chdir(project) + init = runner.invoke(app, [ + "init", "--here", "--integration", "generic", + f"--integration-options=--commands-dir .old/output{suffix}", + "--script", "sh", + ], catch_exceptions=False) + assert init.exit_code == 0, init.output + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + name = "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + old_output = project / ".old/output" / name + assert old_output.is_file() + upgrade = runner.invoke(app, [ + "integration", "upgrade", "generic", "--force", + f"--integration-options=--commands-dir .new/output{suffix}", + ], catch_exceptions=False) + assert upgrade.exit_code == 0, upgrade.output + new_output = project / ".new/output" / name + assert new_output.is_file() + manager = ExtensionManager(project) + assert f".new/output/{name}" in manager.registry.get("sample")["generic_artifact_hashes"] + assert manager.remove("sample") + assert not old_output.exists() + assert not new_output.exists() + finally: + os.chdir(old_cwd) + + +@pytest.mark.parametrize("replacement_content", [ + "replacement with different bytes", + "---\nmetadata:\n source: extension:sample\n---\nedited generated skill", +]) +def test_generic_skills_upgrade_preserves_replaced_or_modified_skill( + tmp_path, generic_extension, replacement_content, +): + from typer.testing import CliRunner + from specify_cli import app + + project = tmp_path / "upgrade-preserves-edited-skill" + project.mkdir() + old_cwd = os.getcwd() + try: + os.chdir(project) + runner = CliRunner() + initialized = runner.invoke(app, [ + "init", "--here", "--integration", "generic", + "--integration-options=--commands-dir .custom/skills --skills", + "--script", "sh", + ], catch_exceptions=False) + assert initialized.exit_code == 0, initialized.output + + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + skill = project / ".custom/skills/speckit-sample-run/SKILL.md" + assert skill.is_file() + skill.write_text(replacement_content, encoding="utf-8") + + upgraded = runner.invoke(app, [ + "integration", "upgrade", "generic", "--force", + "--integration-options=--commands-dir .custom/skills --skills", + ], catch_exceptions=False) + assert upgraded.exit_code == 0, upgraded.output + assert skill.read_text(encoding="utf-8") == replacement_content + finally: + os.chdir(old_cwd) + + +def test_generic_extension_skills_accepts_project_alias(tmp_path, generic_extension): + project = generic_project(tmp_path, skills=True) + alias = tmp_path / "project-alias" + try: + alias.symlink_to(project, target_is_directory=True) + except OSError: + pytest.skip("directory symlinks are unavailable") + + manager = ExtensionManager(alias) + manager.install_from_directory(generic_extension, "1.0.0") + skill = alias / ".custom/commands/speckit-sample-run/SKILL.md" + assert skill.is_file() + assert manager.remove("sample") + assert not skill.exists() + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_does_not_overwrite_existing_command_or_skill( + tmp_path, generic_extension, skills, +): + project = generic_project(tmp_path, skills=skills) + output = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + output.parent.mkdir(parents=True, exist_ok=True) + output.write_text("\nmy own command", encoding="utf-8") + with pytest.raises(ExtensionError, match="no invocation artifacts"): + ExtensionManager(project).install_from_directory(generic_extension, "1.0.0") + assert output.read_text(encoding="utf-8") == "\nmy own command" + assert not ExtensionManager(project).registry.is_installed("sample") + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_rejects_escaping_directory( + tmp_path, generic_extension, skills, +): + project = generic_project(tmp_path, skills=skills) + write_integration_json( + project, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": {"commands_dir": "../outside"}}}, + ) + with pytest.raises(ExtensionError, match="escapes project root"): + ExtensionManager(project).install_from_directory(generic_extension, "1.0.0") + assert not (tmp_path / "outside").exists() + assert not ExtensionManager(project).registry.is_installed("sample") + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_dev_extension_removes_links(tmp_path, generic_extension, skills): + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0", link_commands=True) + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + assert artifact.is_file() + assert manager.remove("sample") + assert not artifact.exists() + assert not artifact.is_symlink() + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_dev_extension_preserves_edited_link(tmp_path, generic_extension, skills): + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0", link_commands=True) + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + if not artifact.is_symlink(): + pytest.skip("symlink creation is unavailable") + artifact.write_text("user edit", encoding="utf-8") + assert manager.remove("sample") + assert not artifact.is_symlink() + assert artifact.read_text(encoding="utf-8") == "user edit" + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_disable_and_enable(tmp_path, generic_extension, skills): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + old_cwd = os.getcwd() + try: + os.chdir(project) + runner = CliRunner() + disabled = runner.invoke(app, ["extension", "disable", "sample"]) + assert disabled.exit_code == 0, disabled.output + assert not output.exists() + assert (project / ".specify/extensions/sample/extension.yml").exists() + enabled = runner.invoke(app, ["extension", "enable", "sample"]) + assert enabled.exit_code == 0, enabled.output + assert output.is_file() + finally: + os.chdir(old_cwd) + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_enable_reports_colliding_user_file( + tmp_path, generic_extension, skills, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + old_cwd = os.getcwd() + try: + os.chdir(project) + runner = CliRunner() + disabled = runner.invoke(app, ["extension", "disable", "sample"]) + assert disabled.exit_code == 0, disabled.output + artifact.parent.mkdir(parents=True, exist_ok=True) + artifact.write_text("user-owned", encoding="utf-8") + enabled = runner.invoke(app, ["extension", "enable", "sample"]) + finally: + os.chdir(old_cwd) + assert enabled.exit_code == 1 + assert "Could not register generic invocations" in enabled.output + assert artifact.read_text(encoding="utf-8") == "user-owned" + assert ExtensionManager(project).registry.get("sample")["enabled"] is False class TestGenericIntegration: @@ -537,22 +929,23 @@ def test_shared_template_and_next_steps_use_dot_without_skills_flag( assert "/speckit.plan" in result.output assert "/speckit-plan" not in result.output - def test_generic_skills_mode_does_not_register_addon_skills_elsewhere( + def test_generic_skills_mode_resolves_configured_dir_not_default( self, tmp_path ): - """Copilot review (PR #4562): a generic --skills project persists - ai_skills=True, but generic's output directory is a runtime - --commands-dir option, not a static per-agent folder — there is no - directory extension/preset skill registration could safely resolve. - resolve_active_skills_dir() must stay disabled for generic rather - than silently falling back to .agents/skills.""" + """Skills register under the persisted generic directory, never .agents/skills.""" from specify_cli import resolve_active_skills_dir from specify_cli._init_options import save_init_options + write_integration_json( + tmp_path, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": { + "commands_dir": ".myagent/skills", "skills": True, + }}}, + ) save_init_options( tmp_path, {"ai": "generic", "ai_skills": True} ) - assert resolve_active_skills_dir(tmp_path) is None + assert resolve_active_skills_dir(tmp_path) == tmp_path / ".myagent/skills" assert not (tmp_path / ".agents" / "skills").exists() def test_complete_file_inventory_ps(self, tmp_path): From c5724ef5d7d328c318ebc7aecfd47171fecf2c23 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 07:07:37 -0500 Subject: [PATCH 02/18] fix: preserve generic extension lifecycle across updates Handle invalid generic settings during skill cleanup, refresh hashes after rewrites, preflight output collisions, and apply project-specific registrar configuration to extension-update backup and rollback. Add before-and-after regression coverage for each review finding. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/__init__.py | 42 +++- .../extensions/_command_update_transaction.py | 2 +- .../integrations/test_integration_generic.py | 180 +++++++++++++++++- 3 files changed, 211 insertions(+), 13 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index d0108b7cc6..06a95149c5 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1517,10 +1517,8 @@ def _register_commands_for_active_agent( Projects without a recorded active integration at all (pre-init-options 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. + recorded active generic key uses its persisted output directory, + not detection-based fallback to other agents. An init-options.json that exists but is corrupted, unreadable, or has a malformed/empty ``ai`` value (e.g. ``[]`` or ``null``) is also @@ -1914,9 +1912,13 @@ def add_candidate(candidate: Path) -> None: if error is None and integration_setting(state or {}, "generic"): from ..integrations.generic import registration_directory - add_candidate(registration_directory(self.project_root)) + try: + add_candidate(registration_directory(self.project_root)) + except (OSError, ValueError): + # Recorded paths and static roots still allow safe cleanup. + pass - registrar = CommandRegistrar(self.project_root) + registrar = CommandRegistrar() for agent_name, agent_config in registrar.AGENT_CONFIGS.items(): if agent_config.get("extension") != "/SKILL.md": continue @@ -1950,8 +1952,7 @@ def _generic_artifact_hashes( continue relative = path.relative_to(self.project_root.resolve()).as_posix() digest = hashlib.sha256(path.read_bytes()).hexdigest() - if relative not in hashes: - hashes[relative] = digest + hashes[relative] = digest return hashes def _generic_owned_names( @@ -2289,9 +2290,32 @@ def install_from_directory( from ..integrations.generic import registration_directory try: - registration_directory(self.project_root) + output_dir = registration_directory(self.project_root) except (OSError, ValueError) as exc: raise ExtensionError(f"Cannot register generic extension commands: {exc}") from exc + skills = is_ai_skills_enabled(active_options) + names = ( + {self._skill_name_for_command(command["name"]) for command in manifest.commands} + if skills else self._collect_manifest_command_names(manifest) + ) + owned = ( + set(self._generic_owned_names( + self.registry.get(manifest.id) or {}, list(names), skills=skills, + )) + if force and self.registry.is_installed(manifest.id) else set() + ) + for name in sorted(names): + target = output_dir / name if skills else output_dir / f"{name}.md" + if not (target.exists() or target.is_symlink()): + continue + if name in owned and ( + not skills or not any(child.name != "SKILL.md" for child in target.iterdir()) + ): + continue + raise ExtensionError( + "Cannot register generic extension commands: existing invocation " + f"artifact or directory '{target}' cannot be replaced safely" + ) # Refuse to install an extension from its own install destination — with # --force this would delete the source before copying it (issue #2990). diff --git a/src/specify_cli/extensions/_command_update_transaction.py b/src/specify_cli/extensions/_command_update_transaction.py index b5dd995c1c..932bb72df9 100644 --- a/src/specify_cli/extensions/_command_update_transaction.py +++ b/src/specify_cli/extensions/_command_update_transaction.py @@ -75,7 +75,7 @@ def run_update_command(extension: str | None) -> None: console.print() updated_extensions = [] failed_updates = [] - registrar = CommandRegistrar() + registrar = CommandRegistrar(project_root) hook_executor = HookExecutor(project_root) from ..agents import CommandRegistrar as _AgentReg # used in backup and rollback paths diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index f84bc80499..d7746533de 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -1,6 +1,11 @@ """Tests for GenericIntegration.""" import os +import shutil +import zipfile +from hashlib import sha256 +from pathlib import Path +from unittest.mock import patch import pytest import yaml @@ -9,7 +14,7 @@ from specify_cli.integrations.base import MarkdownIntegration from specify_cli.integrations.manifest import IntegrationManifest from specify_cli.integration_state import write_integration_json -from specify_cli.extensions import ExtensionError, ExtensionManager +from specify_cli.extensions import ExtensionCatalog, ExtensionError, ExtensionManager from specify_cli import save_init_options @@ -145,6 +150,29 @@ def test_generic_extension_reports_missing_registration_options( assert not ExtensionManager(project).registry.is_installed("sample") +@pytest.mark.parametrize("invalid_settings", ["missing", "malformed"]) +def test_generic_skills_remove_with_invalid_settings( + tmp_path, generic_extension, invalid_settings, +): + project = generic_project(tmp_path, skills=True) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + skill = project / ".custom/commands/speckit-sample-run/SKILL.md" + other_root = project / ".github/skills/speckit-sample-run/SKILL.md" + other_root.parent.mkdir(parents=True) + other_root.write_bytes(skill.read_bytes()) + state_file = project / ".specify/integration.json" + if invalid_settings == "missing": + state_file.unlink() + else: + state_file.write_text("{", encoding="utf-8") + + assert manager.remove("sample") + assert not skill.exists() + assert not other_root.exists() + assert not manager.registry.is_installed("sample") + + @pytest.mark.parametrize("skills", [False, True]) def test_generic_extension_after_cli_init(tmp_path, generic_extension, skills): from typer.testing import CliRunner @@ -268,6 +296,41 @@ def test_generic_skills_upgrade_preserves_replaced_or_modified_skill( os.chdir(old_cwd) +def test_generic_skills_upgrade_refreshes_artifact_digest( + tmp_path, generic_extension, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=True) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + skill = project / ".custom/commands/speckit-sample-run/SKILL.md" + relative = skill.relative_to(project).as_posix() + original_digest = manager.registry.get("sample")["generic_artifact_hashes"][relative] + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nupdated source\n", encoding="utf-8") + + old_cwd = os.getcwd() + try: + os.chdir(project) + upgraded = CliRunner().invoke(app, [ + "integration", "upgrade", "generic", "--force", + "--integration-options=--commands-dir .custom/commands --skills", + ], catch_exceptions=False) + assert upgraded.exit_code == 0, upgraded.output + finally: + os.chdir(old_cwd) + + assert "updated source" in skill.read_text(encoding="utf-8") + manager = ExtensionManager(project) + current_digest = manager.registry.get("sample")["generic_artifact_hashes"][relative] + assert current_digest != original_digest + assert current_digest == sha256(skill.read_bytes()).hexdigest() + assert manager.remove("sample") + assert not skill.exists() + + def test_generic_extension_skills_accepts_project_alias(tmp_path, generic_extension): project = generic_project(tmp_path, skills=True) alias = tmp_path / "project-alias" @@ -294,10 +357,121 @@ def test_generic_extension_does_not_overwrite_existing_command_or_skill( ) output.parent.mkdir(parents=True, exist_ok=True) output.write_text("\nmy own command", encoding="utf-8") - with pytest.raises(ExtensionError, match="no invocation artifacts"): + with pytest.raises(ExtensionError, match="cannot be replaced safely"): ExtensionManager(project).install_from_directory(generic_extension, "1.0.0") assert output.read_text(encoding="utf-8") == "\nmy own command" - assert not ExtensionManager(project).registry.is_installed("sample") + manager = ExtensionManager(project) + assert not manager.registry.is_installed("sample") + assert not (manager.extensions_dir / "sample").exists() + output.unlink() + if skills: + output.parent.rmdir() + manager.install_from_directory(generic_extension, "1.0.0") + assert manager.registry.is_installed("sample") + assert manager.remove("sample") + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_force_reinstall_only_replaces_owned_artifacts( + tmp_path, generic_extension, skills, +): + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + manager.install_from_directory(generic_extension, "1.0.0", force=True) + assert artifact.is_file() + assert manager.remove("sample") + assert not artifact.exists() + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_force_reinstall_preserves_edited_artifacts( + tmp_path, generic_extension, skills, +): + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + artifact.write_text("edited output", encoding="utf-8") + with pytest.raises(ExtensionError, match="cannot be replaced safely"): + manager.install_from_directory(generic_extension, "1.0.0", force=True) + assert artifact.read_text(encoding="utf-8") == "edited output" + assert manager.registry.is_installed("sample") + assert (manager.extensions_dir / "sample/extension.yml").is_file() + + +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("fail_install", [False, True]) +def test_generic_extension_update_uses_project_registrar( + tmp_path, generic_extension, skills, fail_install, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + original = artifact.read_bytes() + updated_source = tmp_path / "updated-source" + shutil.copytree(generic_extension, updated_source) + manifest_path = updated_source / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["extension"]["version"] = "2.0.0" + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + command_file = updated_source / "commands/run.md" + command_file.write_text( + command_file.read_text(encoding="utf-8") + "\nupdated command\n", + encoding="utf-8", + ) + archive = tmp_path / "sample-update.zip" + with zipfile.ZipFile(archive, "w") as zip_file: + for file in updated_source.rglob("*"): + if file.is_file(): + zip_file.write(file, file.relative_to(updated_source)) + + def install_update(self, _zip_path, speckit_version, *, catalog_name=None): + if fail_install: + raise RuntimeError("simulated update failure") + return self.install_from_directory( + updated_source, speckit_version, catalog_name=catalog_name + ) + + with ( + patch.object(Path, "cwd", return_value=project), + patch.object(ExtensionCatalog, "get_extension_info", return_value={ + "id": "sample", + "name": "Sample", + "version": "2.0.0", + "_install_allowed": True, + }), + patch.object(ExtensionCatalog, "download_extension", return_value=archive), + patch.object(ExtensionManager, "install_from_zip", install_update), + ): + result = CliRunner().invoke( + app, ["extension", "update", "sample"], input="y\n", + ) + + manager = ExtensionManager(project) + if fail_install: + assert result.exit_code == 1 + assert "simulated update failure" in result.output + assert manager.registry.get("sample")["version"] == "1.0.0" + assert artifact.read_bytes() == original + else: + assert result.exit_code == 0, result.output + assert manager.registry.get("sample")["version"] == "2.0.0" + assert artifact.read_bytes() != original + assert "updated command" in artifact.read_text(encoding="utf-8") + assert manager.remove("sample") + assert not artifact.exists() @pytest.mark.parametrize("skills", [False, True]) From cf484c9d01be0d081b6883be352b5502282bdb4e Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 07:24:29 -0500 Subject: [PATCH 03/18] fix: complete generic extension registration lifecycle Refresh owned commands and aliases, reject incomplete generic installs with artifact rollback, restore disabled state on enable errors, and verify the bundled GitHub extension registers in both layouts. Update its migration documentation and upstream regression expectations. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reference/agentic-sdd.md | 2 +- extensions/github/README.md | 18 +-- src/specify_cli/agents.py | 4 - src/specify_cli/extensions/__init__.py | 57 ++++++- src/specify_cli/extensions/command_enable.py | 12 +- .../github/test_github_extension.py | 33 ++-- .../integrations/test_integration_generic.py | 142 ++++++++++++++++++ 7 files changed, 236 insertions(+), 32 deletions(-) diff --git a/docs/reference/agentic-sdd.md b/docs/reference/agentic-sdd.md index 2ccb3bf77e..984c782c28 100644 --- a/docs/reference/agentic-sdd.md +++ b/docs/reference/agentic-sdd.md @@ -173,4 +173,4 @@ identified by that remote and checks existing task IDs to avoid duplicates. It is not required to implement tasks or converge on a feature. > [!NOTE] -> GitHub issue tracking is moving out of core into the bundled, opt-in [`github` extension](https://github.com/github/spec-kit/blob/main/extensions/github/README.md). `/speckit.taskstoissues` still works and is unchanged, but its replacement, `/speckit.github.taskstoissues`, is available via `specify extension add github` for integrations that register extension add-ons. The `generic` integration does not register them in either commands or skills mode; see the [installation and migration notes](https://github.com/github/spec-kit/blob/main/extensions/github/README.md#installation). +> GitHub issue tracking is moving out of core into the bundled, opt-in [`github` extension](https://github.com/github/spec-kit/blob/main/extensions/github/README.md). `/speckit.taskstoissues` still works and is unchanged. Install its namespaced replacement with `specify extension add github`; the `generic` integration registers it under its configured command directory in commands or skills mode. See the [installation and migration notes](https://github.com/github/spec-kit/blob/main/extensions/github/README.md#installation). diff --git a/extensions/github/README.md b/extensions/github/README.md index 01c148e5e3..ade406599b 100644 --- a/extensions/github/README.md +++ b/extensions/github/README.md @@ -20,12 +20,11 @@ From the root of an initialized Spec Kit project: specify extension add github ``` -The `generic` (bring your own agent) integration is an exception: it scaffolds -core commands or skills under `--commands-dir`, but does not currently register -extension add-ons in either layout. Installing `github` in a generic project -installs its sources but does **not** create a -`speckit.github.taskstoissues` command or skill. The core -`speckit.taskstoissues` command remains available. See +The `generic` (bring your own agent) integration registers this command under +its configured `--commands-dir` in both commands and skills layouts. Installing +`github` creates `speckit.github.taskstoissues.md` in commands mode or +`speckit-github-taskstoissues/SKILL.md` in skills mode. The core +`speckit.taskstoissues` command remains available and unchanged. See [integration-specific options](../../docs/reference/integrations.md#integration-specific-options). ## Removal @@ -92,10 +91,9 @@ To migrate, install the extension and use the namespaced command instead: specify extension add github ``` -This migration is not yet available for the `generic` integration: its core -command remains available, but extension add-ons do not register under its -custom `--commands-dir`. Generic registration needs a separate fix before the -core command can be removed for those projects. +The `generic` integration supports this migration in both commands and skills +layouts under its configured `--commands-dir`. The core command remains +available until a separate deprecation and removal decision. | Before | After | | ------------------------- | --------------------------------- | diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 8a301e93e8..0dd819c214 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -909,8 +909,6 @@ def register_commands( if agent_name == "generic" and (dest_file.exists() or dest_file.is_symlink()): if not self._generic_owned_output(dest_file, source_id, project_root): continue - registered.append(cmd_name) - continue dest_file.parent.mkdir(parents=True, exist_ok=True) self._write_registered_output( dest_file, @@ -996,8 +994,6 @@ def register_commands( if agent_name == "generic" and (alias_file.exists() or alias_file.is_symlink()): if not self._generic_owned_output(alias_file, source_id, project_root): continue - registered.append(alias) - continue alias_file.parent.mkdir(parents=True, exist_ok=True) self._write_registered_output( alias_file, diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 06a95149c5..4c7947bbfa 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2293,6 +2293,21 @@ def install_from_directory( output_dir = registration_directory(self.project_root) except (OSError, ValueError) as exc: raise ExtensionError(f"Cannot register generic extension commands: {exc}") from exc + source_root = source_dir.resolve() + for command in manifest.commands: + source_file = (source_root / command["file"]).resolve() + if not source_file.is_relative_to(source_root) or not source_file.is_file(): + raise ExtensionError( + "Cannot register generic extension commands: missing source " + f"'{command['file']}'" + ) + try: + source_file.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + raise ExtensionError( + "Cannot register generic extension commands: unreadable source " + f"'{command['file']}': {exc}" + ) from exc skills = is_ai_skills_enabled(active_options) names = ( {self._skill_name_for_command(command["name"]) for command in manifest.commands} @@ -2805,10 +2820,46 @@ def _restore_stranded_config_file( manifest, dest_dir, link_outputs=link_commands ) if register_commands and generic_active and manifest.commands: - if not registered_commands.get("generic") and not registered_skills: + expected = set(names) + actual = set(registered_skills if skills else registered_commands.get("generic", [])) + missing = expected - actual + if missing: + for name in actual: + path = output_dir / name / "SKILL.md" if skills else output_dir / f"{name}.md" + if path.is_file() or path.is_symlink(): + path.unlink() + if skills: + try: + path.parent.rmdir() + except OSError: + pass + + preserved = set(stranded_configs) + if did_remove: + backup_dir = self.extensions_dir / ".backup" / manifest.id + if backup_dir.is_dir() and not backup_dir.is_symlink(): + for config_file in backup_dir.iterdir(): + if ( + config_file.is_file() + and not config_file.is_symlink() + and config_file.name.endswith(("-config.yml", "-config.local.yml")) + ): + shutil.copy2(config_file, dest_dir / config_file.name) + preserved.add(config_file.name) + if preserved: + for child in dest_dir.iterdir(): + if child.name in preserved: + continue + if child.is_dir() and not child.is_symlink(): + shutil.rmtree(child) + else: + child.unlink() + (dest_dir / ".keep-config").write_text("", encoding="utf-8") + else: + shutil.rmtree(dest_dir) raise ExtensionError( - "Cannot register generic extension commands: no invocation artifacts " - "were written to the configured directory" + "Cannot register generic extension commands: missing invocation " + f"artifacts for {', '.join(sorted(missing))}" ) generic_hashes = ( self._generic_artifact_hashes(registered_commands, registered_skills) diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index 6c19b9eefb..ae05987c6b 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -17,7 +17,7 @@ def extension_enable( extension: str = typer.Argument(help="Extension ID or name to enable"), ): """Enable a disabled extension.""" - from . import ExtensionManager, HookExecutor + from . import ExtensionError, ExtensionManager, HookExecutor project_root = _commands._require_specify_project() manager = ExtensionManager(project_root) @@ -47,7 +47,15 @@ def extension_enable( from .. import load_init_options if load_init_options(project_root).get("ai") == "generic": - manager.register_enabled_extensions_for_agent("generic") + try: + manager.register_enabled_extensions_for_agent("generic") + except (ExtensionError, OSError, ValueError) as exc: + manager.registry.update(extension_id, {"enabled": False}) + console.print( + f"[red]Error:[/red] Could not register generic invocations " + f"for '{_escape_markup(str(extension_id))}': {_escape_markup(str(exc))}" + ) + raise typer.Exit(1) from exc refreshed = manager.registry.get(extension_id) or {} commands = refreshed.get("registered_commands", {}) manifest = manager.get_extension(extension_id) diff --git a/tests/extensions/github/test_github_extension.py b/tests/extensions/github/test_github_extension.py index deeadf5889..bc27dff386 100644 --- a/tests/extensions/github/test_github_extension.py +++ b/tests/extensions/github/test_github_extension.py @@ -104,11 +104,11 @@ def test_readme_documents_migration_from_core(self): assert "/speckit.taskstoissues" in text assert COMMAND_NAME in text - def test_readme_discloses_generic_registration_exception(self): + def test_readme_documents_generic_registration(self): text = (EXT_DIR / "README.md").read_text(encoding="utf-8") assert "`generic`" in text - assert "does **not** create" in text - assert "either layout" in text + assert "configured `--commands-dir`" in text + assert "commands and skills layouts" in text def test_command_file_exists(self): assert COMMAND_FILE.is_file() @@ -215,7 +215,7 @@ def test_core_command_remains_unchanged(self): class TestExtensionInstall: @pytest.mark.parametrize("skills_mode", [False, True], ids=["commands", "skills"]) - def test_generic_installs_sources_without_registering_an_artifact( + def test_generic_registers_and_removes_artifact_in_configured_directory( self, tmp_path: Path, skills_mode: bool ): from specify_cli.extensions import ExtensionManager @@ -248,6 +248,12 @@ def test_generic_installs_sources_without_registering_an_artifact( ) commands_dir = project / ".myagent" / "commands" commands_dir.mkdir(parents=True) + core = commands_dir / ( + "speckit-taskstoissues/SKILL.md" + if skills_mode else "speckit.taskstoissues.md" + ) + core.parent.mkdir(parents=True, exist_ok=True) + core.write_text("core command remains unchanged\n", encoding="utf-8") manager = ExtensionManager(project) manager.install_from_directory( @@ -258,17 +264,20 @@ def test_generic_installs_sources_without_registering_an_artifact( assert ( project / ".specify" / "extensions" / "github" / "extension.yml" ).is_file() - assert not ( - commands_dir - / ( - "speckit-github-taskstoissues/SKILL.md" - if skills_mode - else f"{COMMAND_NAME}.md" - ) - ).exists() + artifact = commands_dir / ( + "speckit-github-taskstoissues/SKILL.md" + if skills_mode else f"{COMMAND_NAME}.md" + ) + assert artifact.is_file() + content = artifact.read_text(encoding="utf-8") + assert ".specify/extensions/github/scripts/bash/resolve-tasks.sh" in content + assert "{SCRIPT}" not in content assert not ( project / ".agents" / "skills" / "speckit-github-taskstoissues" / "SKILL.md" ).exists() + assert manager.remove("github") + assert not artifact.exists() + assert core.read_text(encoding="utf-8") == "core command remains unchanged\n" def test_install_copies_command_and_scripts(self, tmp_path: Path): from specify_cli.extensions import ExtensionManager diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index d7746533de..e5ee7938c3 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -331,6 +331,114 @@ def test_generic_skills_upgrade_refreshes_artifact_digest( assert not skill.exists() +def test_generic_command_refreshes_owned_artifact_and_missing_alias( + tmp_path, generic_extension, +): + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"][0]["aliases"] = ["speckit.sample.alias"] + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" + primary = output / "speckit.sample.run.md" + alias = output / "speckit.sample.alias.md" + assert primary.is_file() and alias.is_file() + alias.unlink() + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nnew content\n", encoding="utf-8") + + manager.register_enabled_extensions_for_agent("generic") + + assert "new content" in primary.read_text(encoding="utf-8") + assert "new content" in alias.read_text(encoding="utf-8") + hashes = manager.registry.get("sample")["generic_artifact_hashes"] + assert hashes[primary.relative_to(project).as_posix()] == sha256(primary.read_bytes()).hexdigest() + assert hashes[alias.relative_to(project).as_posix()] == sha256(alias.read_bytes()).hexdigest() + assert manager.remove("sample") + assert not primary.exists() and not alias.exists() + + +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("ignored_source", [False, True]) +def test_generic_partial_registration_rolls_back_all_artifacts( + tmp_path, generic_extension, skills, ignored_source, +): + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"].append({ + "name": "speckit.sample.missing", + "file": "commands/missing.md", + "description": "Missing source", + }) + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + if ignored_source: + (generic_extension / "commands/missing.md").write_text( + "---\ndescription: Missing source\n---\ncontent\n", encoding="utf-8", + ) + (generic_extension / ".extensionignore").write_text( + "commands/missing.md\n", encoding="utf-8", + ) + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + + with pytest.raises(ExtensionError, match="missing"): + manager.install_from_directory(generic_extension, "1.0.0") + + output = project / ".custom/commands" + assert not (output / ("speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md")).exists() + assert not (manager.extensions_dir / "sample").exists() + assert not manager.registry.is_installed("sample") + (generic_extension / "commands/missing.md").write_text( + "---\ndescription: Resolved source\n---\ncontent\n", encoding="utf-8", + ) + (generic_extension / ".extensionignore").unlink(missing_ok=True) + manager.install_from_directory(generic_extension, "1.0.0") + assert manager.registry.is_installed("sample") + assert manager.remove("sample") + + +def test_generic_failed_registration_preserves_reinstall_config( + tmp_path, generic_extension, +): + (generic_extension / "sample-config.yml").write_text( + "default: true\n", encoding="utf-8", + ) + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0", register_commands=False) + assert manager.remove("sample", keep_config=True) + config = manager.extensions_dir / "sample/sample-config.yml" + config.write_text("user: preserved\n", encoding="utf-8") + + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"].append({ + "name": "speckit.sample.other", + "file": "commands/other.md", + "description": "Another command", + }) + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + (generic_extension / "commands/other.md").write_text( + "---\ndescription: Another command\n---\ncontent\n", encoding="utf-8", + ) + (generic_extension / ".extensionignore").write_text( + "commands/other.md\n", encoding="utf-8", + ) + with pytest.raises(ExtensionError, match="missing invocation artifacts"): + manager.install_from_directory(generic_extension, "1.0.0") + + assert config.read_text(encoding="utf-8") == "user: preserved\n" + assert (config.parent / ".keep-config").exists() + assert not (config.parent / "extension.yml").exists() + assert not (project / ".custom/commands/speckit.sample.run.md").exists() + (generic_extension / ".extensionignore").unlink() + manager.install_from_directory(generic_extension, "1.0.0") + assert config.read_text(encoding="utf-8") == "user: preserved\n" + assert manager.remove("sample") + + def test_generic_extension_skills_accepts_project_alias(tmp_path, generic_extension): project = generic_project(tmp_path, skills=True) alias = tmp_path / "project-alias" @@ -575,6 +683,40 @@ def test_generic_extension_enable_reports_colliding_user_file( assert ExtensionManager(project).registry.get("sample")["enabled"] is False +@pytest.mark.parametrize("invalid_settings", ["missing", "malformed"]) +def test_generic_extension_enable_failure_restores_disabled_state( + tmp_path, generic_extension, invalid_settings, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands/speckit.sample.run.md" + state_file = project / ".specify/integration.json" + original_state = state_file.read_bytes() + old_cwd = os.getcwd() + try: + os.chdir(project) + runner = CliRunner() + assert runner.invoke(app, ["extension", "disable", "sample"]).exit_code == 0 + if invalid_settings == "missing": + state_file.unlink() + else: + state_file.write_text("{", encoding="utf-8") + result = runner.invoke(app, ["extension", "enable", "sample"]) + assert result.exit_code == 1 + assert "Could not register generic invocations" in result.output + assert not output.exists() + assert ExtensionManager(project).registry.get("sample")["enabled"] is False + state_file.write_bytes(original_state) + assert runner.invoke(app, ["extension", "enable", "sample"]).exit_code == 0 + assert output.is_file() + finally: + os.chdir(old_cwd) + + class TestGenericIntegration: """Tests for GenericIntegration — requires --commands-dir option.""" From 13e46977f91b937e8ac4527f7abe5b08b3bf2aea Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 07:26:35 -0500 Subject: [PATCH 04/18] chore: bump bundled GitHub extension after migration docs change Keep the bundled extension manifest and catalog at 1.0.1 so updated documentation reaches existing installs and the extension version guard passes. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- extensions/catalog.json | 2 +- extensions/github/extension.yml | 2 +- tests/extensions/github/test_github_extension.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/catalog.json b/extensions/catalog.json index 6f1975264e..91c0140dba 100644 --- a/extensions/catalog.json +++ b/extensions/catalog.json @@ -66,7 +66,7 @@ "github": { "name": "GitHub Integration", "id": "github", - "version": "1.0.0", + "version": "1.0.1", "description": "GitHub platform integration for Spec Kit - create GitHub issues from a feature's task list", "author": "spec-kit-core", "repository": "https://github.com/github/spec-kit", diff --git a/extensions/github/extension.yml b/extensions/github/extension.yml index 4635b6accc..2e13c434ba 100644 --- a/extensions/github/extension.yml +++ b/extensions/github/extension.yml @@ -3,7 +3,7 @@ schema_version: "1.0" extension: id: github name: "GitHub Integration" - version: "1.0.0" + version: "1.0.1" description: "GitHub platform integration for Spec Kit - create GitHub issues from a feature's task list" author: spec-kit-core repository: https://github.com/github/spec-kit diff --git a/tests/extensions/github/test_github_extension.py b/tests/extensions/github/test_github_extension.py index bc27dff386..498abaf36d 100644 --- a/tests/extensions/github/test_github_extension.py +++ b/tests/extensions/github/test_github_extension.py @@ -158,7 +158,7 @@ def test_manifest_validates(self): m = ExtensionManifest(EXT_DIR / "extension.yml") assert m.id == "github" - assert m.version == "1.0.0" + assert m.version == "1.0.1" assert [c["name"] for c in m.commands] == [COMMAND_NAME] def test_manifest_command_files_exist(self): From 6c8bd76d063ba94fe2ce640326e02e5a2dc7b849 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 07:33:50 -0500 Subject: [PATCH 05/18] fix: preserve generic disable state through updates and enablement Retire generated generic artifacts when updating a disabled extension, and require complete hash-owned command or skill registration before enabling. Clean up partial output on failure and cover update success, rollback, retry, and both output layouts. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../extensions/_command_update_transaction.py | 8 ++ src/specify_cli/extensions/command_enable.py | 40 ++++++--- .../integrations/test_integration_generic.py | 87 ++++++++++++++++++- 3 files changed, 117 insertions(+), 18 deletions(-) diff --git a/src/specify_cli/extensions/_command_update_transaction.py b/src/specify_cli/extensions/_command_update_transaction.py index 932bb72df9..c9f116d363 100644 --- a/src/specify_cli/extensions/_command_update_transaction.py +++ b/src/specify_cli/extensions/_command_update_transaction.py @@ -538,6 +538,14 @@ def backup_extension_skills(skill_names, *, skills_dir=None): # 9. Restore metadata from backup (installed_at, enabled state) if backup_registry_entry and isinstance(backup_registry_entry, dict): + init_options = _commands.load_init_options(project_root) + if ( + not backup_registry_entry.get("enabled", True) + and isinstance(init_options, dict) + and init_options.get("ai") == "generic" + ): + manager.disable_generic_extension_artifacts(extension_id) + # Copy current registry entry to avoid mutating internal # registry state before explicit restore(). current_metadata = manager.registry.get(extension_id) diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index ae05987c6b..de837aee6e 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -46,9 +46,34 @@ def extension_enable( from .. import load_init_options - if load_init_options(project_root).get("ai") == "generic": + init_options = load_init_options(project_root) + if init_options.get("ai") == "generic": try: manager.register_enabled_extensions_for_agent("generic") + refreshed = manager.registry.get(extension_id) or {} + manifest = manager.get_extension(extension_id) + if manifest is None: + raise ExtensionError(f"Cannot read manifest for '{extension_id}'") + if manifest.commands: + from .._init_options import is_ai_skills_enabled + + skills = is_ai_skills_enabled(init_options) + expected = ( + { + manager._skill_name_for_command(command["name"]) + for command in manifest.commands + } + if skills else set(manager._collect_manifest_command_names(manifest)) + ) + owned = set(manager._generic_owned_names( + refreshed, list(expected), skills=skills, + )) + missing = expected - owned + if missing: + manager.disable_generic_extension_artifacts(extension_id) + raise ExtensionError( + "Missing invocation artifacts: " + ", ".join(sorted(missing)) + ) except (ExtensionError, OSError, ValueError) as exc: manager.registry.update(extension_id, {"enabled": False}) console.print( @@ -56,19 +81,6 @@ def extension_enable( f"for '{_escape_markup(str(extension_id))}': {_escape_markup(str(exc))}" ) raise typer.Exit(1) from exc - refreshed = manager.registry.get(extension_id) or {} - commands = refreshed.get("registered_commands", {}) - manifest = manager.get_extension(extension_id) - if manifest and manifest.commands and not ( - (isinstance(commands, dict) and commands.get("generic")) - or refreshed.get("registered_skills") - ): - manager.registry.update(extension_id, {"enabled": False}) - console.print( - f"[red]Error:[/red] Could not register generic invocations " - f"for '{_escape_markup(str(extension_id))}'." - ) - raise typer.Exit(1) # Enable hooks in extensions.yml config = hook_executor.get_project_config() diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index e5ee7938c3..4d463ef45d 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -515,8 +515,9 @@ def test_generic_extension_force_reinstall_preserves_edited_artifacts( @pytest.mark.parametrize("skills", [False, True]) @pytest.mark.parametrize("fail_install", [False, True]) +@pytest.mark.parametrize("disabled", [False, True]) def test_generic_extension_update_uses_project_registrar( - tmp_path, generic_extension, skills, fail_install, + tmp_path, generic_extension, skills, fail_install, disabled, ): from typer.testing import CliRunner from specify_cli import app @@ -528,6 +529,15 @@ def test_generic_extension_update_uses_project_registrar( "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" ) original = artifact.read_bytes() + if disabled: + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, ["extension", "disable", "sample"]) + assert result.exit_code == 0, result.output + finally: + os.chdir(old_cwd) + assert not artifact.exists() updated_source = tmp_path / "updated-source" shutil.copytree(generic_extension, updated_source) manifest_path = updated_source / "extension.yml" @@ -572,12 +582,30 @@ def install_update(self, _zip_path, speckit_version, *, catalog_name=None): assert result.exit_code == 1 assert "simulated update failure" in result.output assert manager.registry.get("sample")["version"] == "1.0.0" - assert artifact.read_bytes() == original + if disabled: + assert not artifact.exists() + else: + assert artifact.read_bytes() == original else: assert result.exit_code == 0, result.output assert manager.registry.get("sample")["version"] == "2.0.0" - assert artifact.read_bytes() != original - assert "updated command" in artifact.read_text(encoding="utf-8") + if disabled: + assert not artifact.exists() + old_cwd = os.getcwd() + try: + os.chdir(project) + enabled = CliRunner().invoke(app, ["extension", "enable", "sample"]) + assert enabled.exit_code == 0, enabled.output + finally: + os.chdir(old_cwd) + assert "updated command" in artifact.read_text(encoding="utf-8") + else: + assert artifact.read_bytes() != original + assert "updated command" in artifact.read_text(encoding="utf-8") + assert ExtensionManager(project).registry.get("sample")["enabled"] is ( + not (disabled and fail_install) + ) + manager = ExtensionManager(project) assert manager.remove("sample") assert not artifact.exists() @@ -683,6 +711,57 @@ def test_generic_extension_enable_reports_colliding_user_file( assert ExtensionManager(project).registry.get("sample")["enabled"] is False +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_enable_rejects_partial_registration( + tmp_path, generic_extension, skills, +): + from typer.testing import CliRunner + from specify_cli import app + + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"].append({ + "name": "speckit.sample.other", + "file": "commands/other.md", + "description": "Another command", + }) + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + (generic_extension / "commands/other.md").write_text( + "---\ndescription: Another command\n---\ncontent\n", encoding="utf-8", + ) + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" + collision = output / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + other = output / ( + "speckit-sample-other/SKILL.md" if skills else "speckit.sample.other.md" + ) + old_cwd = os.getcwd() + try: + os.chdir(project) + runner = CliRunner() + assert runner.invoke(app, ["extension", "disable", "sample"]).exit_code == 0 + collision.parent.mkdir(parents=True, exist_ok=True) + collision.write_text("edited replacement", encoding="utf-8") + enabled = runner.invoke(app, ["extension", "enable", "sample"]) + assert enabled.exit_code == 1 + assert "Could not register generic invocations" in enabled.output + assert collision.read_text(encoding="utf-8") == "edited replacement" + assert not other.exists() + assert ExtensionManager(project).registry.get("sample")["enabled"] is False + collision.unlink() + if skills: + collision.parent.rmdir() + enabled = runner.invoke(app, ["extension", "enable", "sample"]) + assert enabled.exit_code == 0, enabled.output + finally: + os.chdir(old_cwd) + assert collision.is_file() and other.is_file() + + @pytest.mark.parametrize("invalid_settings", ["missing", "malformed"]) def test_generic_extension_enable_failure_restores_disabled_state( tmp_path, generic_extension, invalid_settings, From 17d2bfffec09f243c3e0ac15a8ad24d06dad2374 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 07:41:54 -0500 Subject: [PATCH 06/18] fix: reject linked generic skill directories and roll back write errors Validate skill output parents before recognizing ownership or removing tracked files. Roll back partially generated generic artifacts and copied sources when registration raises, preserving retriable config state. Add failing-before symlink and write-error regressions for both layouts. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/__init__.py | 163 +++++++++++------- .../integrations/test_integration_generic.py | 80 +++++++++ 2 files changed, 185 insertions(+), 58 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 4c7947bbfa..5828539b07 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1938,19 +1938,25 @@ def _generic_artifact_hashes( ) -> Dict[str, str]: """Record generic-owned output by path and hash for safe later removal.""" from ..integrations.generic import registration_directory + from ..shared_infra import _validate_safe_shared_directory if not registered_commands.get("generic") and not registered_skills: return previous or {} output_dir = registration_directory(self.project_root) + root = self.project_root.resolve() paths = [ output_dir / f"{name}.md" for name in registered_commands.get("generic", []) ] + [output_dir / name / "SKILL.md" for name in registered_skills] hashes = dict(previous or {}) for path in paths: + try: + _validate_safe_shared_directory(root, path.parent) + except (OSError, ValueError): + continue if not path.is_file(): continue - relative = path.relative_to(self.project_root.resolve()).as_posix() + relative = path.relative_to(root).as_posix() digest = hashlib.sha256(path.read_bytes()).hexdigest() hashes[relative] = digest return hashes @@ -1960,21 +1966,27 @@ def _generic_owned_names( ) -> List[str]: """Keep customized or untracked generic artifacts out of cleanup.""" from ..integrations.generic import registration_directory + from ..shared_infra import _validate_safe_shared_directory hashes = metadata.get("generic_artifact_hashes", {}) if not isinstance(hashes, dict): return [] output_dir = registration_directory(self.project_root) + root = self.project_root.resolve() owned = [] for name in names: path = output_dir / name / "SKILL.md" if skills else output_dir / f"{name}.md" + try: + _validate_safe_shared_directory(root, path.parent) + except (OSError, ValueError): + continue if not path.is_file(): continue if path.is_symlink() and not path.resolve().is_relative_to( self.extensions_dir.resolve() ): continue - relative = path.relative_to(self.project_root.resolve()).as_posix() + relative = path.relative_to(root).as_posix() if hashes.get(relative) == hashlib.sha256(path.read_bytes()).hexdigest(): owned.append(name) return owned @@ -1983,6 +1995,8 @@ def _remove_generic_artifact_paths( self, extension_id: str, metadata: Dict[str, Any], *, skills: bool = True ) -> None: """Clean recorded generic paths even after the configured directory moves.""" + from ..shared_infra import _validate_safe_shared_directory + manifest = self.get_extension(extension_id) registered = metadata.get("registered_commands", {}) command_names = set( @@ -2020,7 +2034,9 @@ def _remove_generic_artifact_paths( }: continue path = root / name - if not path.parent.resolve().is_relative_to(root): + try: + _validate_safe_shared_directory(root, path.parent) + except (OSError, ValueError): continue if not path.is_file(): continue @@ -2807,64 +2823,95 @@ def _restore_stranded_config_file( # the restored user config with packaged defaults. Cleanup is deferred # until after registry.add() succeeds (see post-commit cleanup below). - # Register commands with AI agents (active integration only, #2948) - registered_commands = {} - if register_commands: - registered_commands = self._register_commands_for_active_agent( + def rollback_generic_registration() -> None: + from ..shared_infra import _validate_safe_shared_directory + + root = self.project_root.resolve() + installed_root = dest_dir.resolve() + for name in names: + path = output_dir / name / "SKILL.md" if skills else output_dir / f"{name}.md" + try: + _validate_safe_shared_directory(root, path.parent) + except (OSError, ValueError): + continue + if path.is_symlink(): + if not path.resolve().is_relative_to(installed_root): + continue + elif not path.is_file(): + if skills and path.parent.is_dir(): + try: + path.parent.rmdir() + except OSError: + pass + continue + path.unlink() + if skills: + try: + path.parent.rmdir() + except OSError: + pass + + preserved = set(stranded_configs) + if did_remove: + backup_dir = self.extensions_dir / ".backup" / manifest.id + if backup_dir.is_dir() and not backup_dir.is_symlink(): + for config_file in backup_dir.iterdir(): + if ( + config_file.is_file() + and not config_file.is_symlink() + and config_file.name.endswith(("-config.yml", "-config.local.yml")) + ): + shutil.copy2(config_file, dest_dir / config_file.name) + preserved.add(config_file.name) + if preserved: + for child in dest_dir.iterdir(): + if child.name in preserved: + continue + if child.is_dir() and not child.is_symlink(): + shutil.rmtree(child) + else: + child.unlink() + (dest_dir / ".keep-config").write_text("", encoding="utf-8") + else: + shutil.rmtree(dest_dir) + + try: + # Register commands with AI agents (active integration only, #2948) + registered_commands = {} + if register_commands: + registered_commands = self._register_commands_for_active_agent( + manifest, dest_dir, link_outputs=link_commands + ) + + # Auto-register extension commands as agent skills when skills mode + # was used during project initialisation (feature parity). + registered_skills = self._register_extension_skills( manifest, dest_dir, link_outputs=link_commands ) - - # Auto-register extension commands as agent skills when skills mode - # was used during project initialisation (feature parity). - registered_skills = self._register_extension_skills( - manifest, dest_dir, link_outputs=link_commands - ) - if register_commands and generic_active and manifest.commands: - expected = set(names) - actual = set(registered_skills if skills else registered_commands.get("generic", [])) - missing = expected - actual - if missing: - for name in actual: - path = output_dir / name / "SKILL.md" if skills else output_dir / f"{name}.md" - if path.is_file() or path.is_symlink(): - path.unlink() - if skills: - try: - path.parent.rmdir() - except OSError: - pass - - preserved = set(stranded_configs) - if did_remove: - backup_dir = self.extensions_dir / ".backup" / manifest.id - if backup_dir.is_dir() and not backup_dir.is_symlink(): - for config_file in backup_dir.iterdir(): - if ( - config_file.is_file() - and not config_file.is_symlink() - and config_file.name.endswith(("-config.yml", "-config.local.yml")) - ): - shutil.copy2(config_file, dest_dir / config_file.name) - preserved.add(config_file.name) - if preserved: - for child in dest_dir.iterdir(): - if child.name in preserved: - continue - if child.is_dir() and not child.is_symlink(): - shutil.rmtree(child) - else: - child.unlink() - (dest_dir / ".keep-config").write_text("", encoding="utf-8") - else: - shutil.rmtree(dest_dir) - raise ExtensionError( - "Cannot register generic extension commands: missing invocation " - f"artifacts for {', '.join(sorted(missing))}" + if register_commands and generic_active and manifest.commands: + expected = set(names) + actual = set( + registered_skills if skills else registered_commands.get("generic", []) ) - generic_hashes = ( - self._generic_artifact_hashes(registered_commands, registered_skills) - if generic_active else {} - ) + missing = expected - actual + if missing: + raise ExtensionError( + "Cannot register generic extension commands: missing invocation " + f"artifacts for {', '.join(sorted(missing))}" + ) + generic_hashes = ( + self._generic_artifact_hashes(registered_commands, registered_skills) + if generic_active else {} + ) + except (ExtensionError, OSError, ValueError, RuntimeError) as exc: + if register_commands and generic_active and manifest.commands: + try: + rollback_generic_registration() + except (OSError, ValueError) as rollback_error: + raise ExtensionError( + f"Generic registration failed ({exc}); rollback failed: {rollback_error}" + ) from rollback_error + raise # Register hooks and update installed list in extensions.yml hook_executor = HookExecutor(self.project_root) diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 4d463ef45d..65ec7e3436 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -112,6 +112,41 @@ def test_generic_extension_preserves_modified_artifact(tmp_path, generic_extensi assert artifact.read_text(encoding="utf-8").endswith("user edit\n") +@pytest.mark.parametrize("operation", ["resync", "remove", "force"]) +def test_generic_skill_symlinked_directory_is_not_owned( + tmp_path, generic_extension, operation, +): + project = generic_project(tmp_path, skills=True) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + skill_dir = project / ".custom/commands/speckit-sample-run" + other_dir = project / "moved-skill" + skill_dir.rename(other_dir) + try: + skill_dir.symlink_to(other_dir, target_is_directory=True) + except OSError: + pytest.skip("directory symlinks are unavailable") + other_skill = other_dir / "SKILL.md" + original = other_skill.read_bytes() + + if operation == "resync": + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nnew source\n", encoding="utf-8") + manager.register_enabled_extensions_for_agent("generic", force=True) + assert manager._generic_owned_names( + manager.registry.get("sample"), ["speckit-sample-run"], skills=True, + ) == [] + elif operation == "force": + with pytest.raises(ExtensionError, match="cannot be replaced safely"): + manager.install_from_directory(generic_extension, "1.0.0", force=True) + assert manager.registry.is_installed("sample") + + if operation != "force": + assert manager.remove("sample") + assert skill_dir.is_symlink() + assert other_skill.read_bytes() == original + + @pytest.mark.parametrize("skills", [False, True]) def test_generic_extension_rejects_modified_artifact_on_disable( tmp_path, generic_extension, skills, @@ -399,6 +434,51 @@ def test_generic_partial_registration_rolls_back_all_artifacts( assert manager.remove("sample") +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_registration_write_error_rolls_back_partial_install( + tmp_path, generic_extension, skills, monkeypatch, +): + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"].append({ + "name": "speckit.sample.other", + "file": "commands/other.md", + "description": "Another command", + }) + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + (generic_extension / "commands/other.md").write_text( + "---\ndescription: Another command\n---\ncontent\n", encoding="utf-8", + ) + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + output_dir = project / ".custom/commands" + first = output_dir / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + second = output_dir / ( + "speckit-sample-other/SKILL.md" if skills else "speckit.sample.other.md" + ) + original_write = Path.write_text + + def fail_second_write(path, *args, **kwargs): + if path == second: + raise OSError("simulated artifact write error") + return original_write(path, *args, **kwargs) + + monkeypatch.setattr(Path, "write_text", fail_second_write) + with pytest.raises(OSError, match="simulated artifact write error"): + manager.install_from_directory(generic_extension, "1.0.0") + monkeypatch.undo() + + assert not first.exists() + assert not second.exists() + assert not (manager.extensions_dir / "sample").exists() + assert not manager.registry.is_installed("sample") + manager.install_from_directory(generic_extension, "1.0.0") + assert first.is_file() and second.is_file() + assert manager.remove("sample") + + def test_generic_failed_registration_preserves_reinstall_config( tmp_path, generic_extension, ): From f2c5ede32f715c4f3f96791a206a3be87d6106aa Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 08:11:29 -0500 Subject: [PATCH 07/18] fix: remove generic commands without readable integration settings Retire hash-owned generic command artifacts without resolving missing or malformed generic output settings. Convert invalid integration state during post-removal event refresh to an explicit warning instead of a failure after successful removal. Cover normal and edited artifacts, both invalid-state cases, and native-hook preservation. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/events/__init__.py | 16 ++++-- src/specify_cli/extensions/__init__.py | 6 +- .../integrations/test_integration_generic.py | 55 +++++++++++++++++++ tests/specify_cli/events/test_events.py | 25 +++++++++ 4 files changed, 96 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/events/__init__.py b/src/specify_cli/events/__init__.py index 02f1e0ddcf..b47931e46f 100644 --- a/src/specify_cli/events/__init__.py +++ b/src/specify_cli/events/__init__.py @@ -1827,11 +1827,19 @@ def refresh_integration_events(project_root: Path) -> None: while a stale native hook may still be active (R3). """ from ..integrations import get_integration - from ..integrations._helpers import _read_integration_json, _resolve_integration_options + from ..integrations._helpers import _resolve_integration_options from ..integrations.manifest import IntegrationManifest - from ..integration_state import installed_integration_keys - - state = _read_integration_json(project_root) + from ..integration_state import installed_integration_keys, try_read_integration_json + + state, error = try_read_integration_json(project_root) + if error is not None: + detail = ( + f"unsupported schema {error.schema}" + if error.kind == "schema_too_new" + else f"{error.kind}: {error.detail}" + ) + raise EventRefreshError([(".specify/integration.json", detail)]) + state = state or {} failures: list[tuple[str, str]] = [] for key in installed_integration_keys(state): integration = get_integration(key) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 5828539b07..c2995fd7b1 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -3267,11 +3267,13 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: # Unregister commands from all AI agents if registered_commands: - registrar = CommandRegistrar(self.project_root) safe_commands = dict(registered_commands) if "generic" in safe_commands: safe_commands.pop("generic") - registrar.unregister_commands(safe_commands, self.project_root) + if safe_commands: + CommandRegistrar().unregister_commands( + safe_commands, self.project_root + ) if metadata: self._remove_generic_artifact_paths(extension_id, metadata) diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 65ec7e3436..5fad7ea12c 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -208,6 +208,61 @@ def test_generic_skills_remove_with_invalid_settings( assert not manager.registry.is_installed("sample") +@pytest.mark.parametrize("invalid_settings", ["missing", "malformed"]) +@pytest.mark.parametrize("modified", [False, True]) +def test_generic_commands_remove_with_invalid_settings( + tmp_path, generic_extension, invalid_settings, modified, +): + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + command = project / ".custom/commands/speckit.sample.run.md" + if modified: + command.write_text("user edit", encoding="utf-8") + state_file = project / ".specify/integration.json" + if invalid_settings == "missing": + state_file.unlink() + else: + state_file.write_text("{", encoding="utf-8") + + assert manager.remove("sample") + assert command.exists() is modified + if modified: + assert command.read_text(encoding="utf-8") == "user edit" + assert not (project / ".specify/extensions/sample").exists() + assert not manager.registry.is_installed("sample") + + +@pytest.mark.parametrize("invalid_settings", ["missing", "malformed"]) +def test_generic_commands_cli_remove_with_invalid_settings( + tmp_path, generic_extension, invalid_settings, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path) + ExtensionManager(project).install_from_directory(generic_extension, "1.0.0") + command = project / ".custom/commands/speckit.sample.run.md" + state_file = project / ".specify/integration.json" + if invalid_settings == "missing": + state_file.unlink() + else: + state_file.write_text("{", encoding="utf-8") + + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, ["extension", "remove", "sample", "--force"]) + finally: + os.chdir(old_cwd) + assert result.exit_code == 0, result.output + if invalid_settings == "malformed": + assert "event refresh failed" in result.output + assert ".specify/integration.json" in result.output + assert not command.exists() + assert not ExtensionManager(project).registry.is_installed("sample") + + @pytest.mark.parametrize("skills", [False, True]) def test_generic_extension_after_cli_init(tmp_path, generic_extension, skills): from typer.testing import CliRunner diff --git a/tests/specify_cli/events/test_events.py b/tests/specify_cli/events/test_events.py index cb10ad3855..368b56930e 100644 --- a/tests/specify_cli/events/test_events.py +++ b/tests/specify_cli/events/test_events.py @@ -2628,6 +2628,31 @@ class TestRefreshIntegrationEvents: """#1: refresh_integration_events regenerates native config after extension state changes.""" + @pytest.mark.parametrize("invalid_state,detail", [ + ("{", "decode"), + (json.dumps({"integration_state_schema": 999}), "unsupported schema 999"), + ]) + def test_refresh_reports_invalid_state_without_changing_hooks( + self, tmp_path, invalid_state, detail, + ): + from specify_cli.events import EventRefreshError, refresh_integration_events + + integration = ClaudeIntegration() + manifest = IntegrationManifest(integration.key, tmp_path, version="test") + install_integration_events( + integration, tmp_path, manifest, + {"pre_tool_use": [{"command": "speckit.my-ext.check"}]}, + ) + config_path = tmp_path / ".claude/settings.json" + original = config_path.read_bytes() + state_path = tmp_path / ".specify/integration.json" + state_path.write_text(invalid_state, encoding="utf-8") + + with pytest.raises(EventRefreshError, match=detail) as exc_info: + refresh_integration_events(tmp_path) + assert exc_info.value.failures[0][0] == ".specify/integration.json" + assert config_path.read_bytes() == original + def test_refresh_strips_removed_extension_events(self, tmp_path): from specify_cli.events import refresh_integration_events from specify_cli.integrations.manifest import IntegrationManifest From 25469828a76991cb87fe49ceee5e1a49299e1e87 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 08:43:33 -0500 Subject: [PATCH 08/18] fix: roll back generic installs through commit and preserve edited skills Keep registration rollback active through hook and registry writes, including partial state and rescued configuration; retire stale generic skills only when recorded hashes still match. Correct generic integration installation guidance and cover hook/registry failures and layout transitions with failing-before regressions. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/installation.md | 9 +- src/specify_cli/extensions/__init__.py | 143 +++++++++++------- .../integrations/test_integration_generic.py | 120 ++++++++++++++- 3 files changed, 216 insertions(+), 56 deletions(-) diff --git a/docs/installation.md b/docs/installation.md index b84007323c..fa777a6978 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -150,9 +150,12 @@ After initialization, you should see the following commands available in your co - `/speckit.taskstoissues` - Convert tasks to issues (moving to the bundled `github` extension as `/speckit.github.taskstoissues`; install it with `specify extension add github`) -The `generic` integration scaffolds core commands or skills, but does not -register extension add-ons; installing `github` there does not make its -replacement command invokable. See the [GitHub extension's installation notes](https://github.com/github/spec-kit/blob/main/extensions/github/README.md#installation). +The `generic` integration also registers extension commands in its configured +`--commands-dir`. Installing `github` makes `/speckit.github.taskstoissues` +available there as a command file, or as `/speckit-github-taskstoissues` when +`--skills` is enabled. The core `/speckit.taskstoissues` (or +`/speckit-taskstoissues` with `--skills`) remains available. +See the [GitHub extension's installation notes](https://github.com/github/spec-kit/blob/main/extensions/github/README.md#installation). Scripts are installed into a variant subdirectory matching the chosen script type: diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index c2995fd7b1..a09b50076f 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2875,6 +2875,8 @@ def rollback_generic_registration() -> None: else: shutil.rmtree(dest_dir) + hooks_started = False + registry_started = False try: # Register commands with AI agents (active integration only, #2948) registered_commands = {} @@ -2903,66 +2905,95 @@ def rollback_generic_registration() -> None: self._generic_artifact_hashes(registered_commands, registered_skills) if generic_active else {} ) - except (ExtensionError, OSError, ValueError, RuntimeError) as exc: + + # Register hooks and update installed list in extensions.yml + hook_executor = HookExecutor(self.project_root) + hooks_started = True + hook_executor.register_hooks(manifest) + + # Restore config files from backup when --force triggered a removal. + # Only restore *.yml config files to match what remove() backs up, + # so unexpected artifacts in .backup/ are not resurrected. + if did_remove: + backup_config_dir = self.extensions_dir / ".backup" / manifest.id + if backup_config_dir.is_symlink(): + backup_config_dir.unlink() + elif backup_config_dir.is_dir(): + for cfg_file in backup_config_dir.iterdir(): + if ( + cfg_file.is_file() + and not cfg_file.is_symlink() + and ( + cfg_file.name.endswith("-config.yml") + or cfg_file.name.endswith("-config.local.yml") + ) + ): + shutil.copy2(cfg_file, dest_dir / cfg_file.name) + elif backup_config_dir.exists(): + backup_config_dir.unlink() + + normalized_catalog_name = ( + catalog_name.strip() if isinstance(catalog_name, str) else "" + ) + source = ( + {"kind": "catalog", "catalog": normalized_catalog_name} + if normalized_catalog_name + else "local" + ) + registry_started = True + self.registry.add( + manifest.id, + { + "version": manifest.version, + "source": source, + "manifest_hash": manifest.get_hash(), + "enabled": True, + "priority": priority, + "registered_commands": registered_commands, + "registered_skills": registered_skills, + "generic_artifact_hashes": generic_hashes, + }, + ) + except Exception as exc: + # Any failed commit must retire outputs before the original error + # is re-raised, including errors from hook serialization. if register_commands and generic_active and manifest.commands: + rollback_errors = [] + if registry_started: + try: + self.registry.remove(manifest.id) + except Exception as error: + rollback_errors.append(f"registry: {error}") + if hooks_started: + try: + hook_executor.unregister_hooks(manifest.id) + except Exception as error: + rollback_errors.append(f"hooks: {error}") try: rollback_generic_registration() - except (OSError, ValueError) as rollback_error: + except Exception as error: + rollback_errors.append(f"artifacts: {error}") + if rollback_errors: raise ExtensionError( - f"Generic registration failed ({exc}); rollback failed: {rollback_error}" - ) from rollback_error + f"Generic registration failed ({exc}); rollback failed: " + + "; ".join(rollback_errors) + ) from exc raise - # Register hooks and update installed list in extensions.yml - hook_executor = HookExecutor(self.project_root) - hook_executor.register_hooks(manifest) - - # Restore config files from backup when --force triggered a removal. - # Only restore *.yml config files to match what remove() backs up, - # so unexpected artifacts in .backup/ are not resurrected. if did_remove: backup_config_dir = self.extensions_dir / ".backup" / manifest.id - # is_symlink first: is_dir() follows symlinks, but rmtree() - # raises on them — and we shouldn't follow symlinks to restore. - if backup_config_dir.is_symlink(): - backup_config_dir.unlink() - elif backup_config_dir.is_dir(): - for cfg_file in backup_config_dir.iterdir(): - if ( - cfg_file.is_file() - and not cfg_file.is_symlink() - and ( - cfg_file.name.endswith("-config.yml") - or cfg_file.name.endswith("-config.local.yml") - ) - ): - shutil.copy2(cfg_file, dest_dir / cfg_file.name) - shutil.rmtree(backup_config_dir) - elif backup_config_dir.exists(): - backup_config_dir.unlink() + if backup_config_dir.is_dir() and not backup_config_dir.is_symlink(): + # Retain the backup until registry commit so failed force + # reinstalls can still restore the user's configuration. + try: + shutil.rmtree(backup_config_dir) + except OSError as exc: + from .. import _print_cli_warning - # Update registry - normalized_catalog_name = ( - catalog_name.strip() if isinstance(catalog_name, str) else "" - ) - source = ( - {"kind": "catalog", "catalog": normalized_catalog_name} - if normalized_catalog_name - else "local" - ) - self.registry.add( - manifest.id, - { - "version": manifest.version, - "source": source, - "manifest_hash": manifest.get_hash(), - "enabled": True, - "priority": priority, - "registered_commands": registered_commands, - "registered_skills": registered_skills, - "generic_artifact_hashes": generic_hashes, - }, - ) + _print_cli_warning( + "remove", "configuration backup", str(backup_config_dir), + exc, continuing="The extension was installed; the backup remains.", + ) # Post-commit cleanup: the registry now records this extension as # installed, so the rescue guard (`not self.registry.is_installed`) @@ -3775,9 +3806,17 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool name for name in owned_here if name in replaced_skill_names ] + if agent_name == "generic": + to_remove = self._generic_owned_names( + metadata, to_remove, skills=True + ) if to_remove: self._unregister_extension_skills( - to_remove, ext_id, skills_dir=agent_skills_dir + to_remove, ext_id, skills_dir=agent_skills_dir, + generic_hashes=( + metadata.get("generic_artifact_hashes") + if agent_name == "generic" else None + ), ) # registered_skills is a single flat list # shared across every agent this extension diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 5fad7ea12c..d1d7812685 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -14,7 +14,7 @@ from specify_cli.integrations.base import MarkdownIntegration from specify_cli.integrations.manifest import IntegrationManifest from specify_cli.integration_state import write_integration_json -from specify_cli.extensions import ExtensionCatalog, ExtensionError, ExtensionManager +from specify_cli.extensions import ExtensionCatalog, ExtensionError, ExtensionManager, HookExecutor from specify_cli import save_init_options @@ -534,6 +534,124 @@ def fail_second_write(path, *args, **kwargs): assert manager.remove("sample") +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("failure", [ + "hooks-before", "hooks-after", "hooks-yaml", "registry-before", "registry-after", +]) +def test_generic_install_commit_error_rolls_back( + tmp_path, generic_extension, skills, failure, monkeypatch, +): + manifest_path = generic_extension / "extension.yml" + manifest_data = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest_data["hooks"] = { + "after_tasks": {"command": "speckit.sample.run", "optional": True} + } + manifest_path.write_text(yaml.safe_dump(manifest_data), encoding="utf-8") + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + error_type = yaml.YAMLError if failure == "hooks-yaml" else OSError + if failure.startswith("hooks"): + original_register = HookExecutor.register_hooks + + def fail_register_hooks(executor, manifest): + if failure == "hooks-after": + original_register(executor, manifest) + raise error_type("simulated hook write failure") + + monkeypatch.setattr(HookExecutor, "register_hooks", fail_register_hooks) + else: + original_save = manager.registry._save + + def fail_registry_save(): + monkeypatch.setattr(manager.registry, "_save", original_save) + if failure == "registry-after": + original_save() + raise OSError("simulated registry write failure") + + monkeypatch.setattr(manager.registry, "_save", fail_registry_save) + + with pytest.raises(error_type, match="simulated .* write failure"): + manager.install_from_directory(generic_extension, "1.0.0") + monkeypatch.undo() + + assert not artifact.exists() + assert not (manager.extensions_dir / "sample").exists() + assert not manager.registry.is_installed("sample") + assert not ExtensionManager(project).registry.is_installed("sample") + config = HookExecutor(project).get_project_config() + assert "sample" not in config["installed"] + assert not config["hooks"] + manager.install_from_directory(generic_extension, "1.0.0") + assert artifact.is_file() + assert manager.remove("sample") + + +@pytest.mark.parametrize("previous_install", ["kept-config", "forced"]) +def test_generic_commit_error_preserves_user_config( + tmp_path, generic_extension, previous_install, monkeypatch, +): + (generic_extension / "sample-config.yml").write_text( + "default: true\n", encoding="utf-8", + ) + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory( + generic_extension, "1.0.0", + register_commands=previous_install != "kept-config", + ) + if previous_install == "kept-config": + assert manager.remove("sample", keep_config=True) + config = manager.extensions_dir / "sample/sample-config.yml" + config.write_text("user: preserved\n", encoding="utf-8") + + def fail_register_hooks(executor, manifest): + raise OSError("simulated hook write failure") + + monkeypatch.setattr(HookExecutor, "register_hooks", fail_register_hooks) + with pytest.raises(OSError, match="simulated hook write failure"): + manager.install_from_directory( + generic_extension, "1.0.0", force=previous_install == "forced", + ) + monkeypatch.undo() + + assert config.read_text(encoding="utf-8") == "user: preserved\n" + assert (config.parent / ".keep-config").is_file() + assert not (config.parent / "extension.yml").exists() + assert not (project / ".custom/commands/speckit.sample.run.md").exists() + assert not manager.registry.is_installed("sample") + manager.install_from_directory(generic_extension, "1.0.0") + assert config.read_text(encoding="utf-8") == "user: preserved\n" + assert manager.remove("sample") + + +@pytest.mark.parametrize("edited", [False, True]) +def test_generic_skills_to_commands_preserves_edited_skill( + tmp_path, generic_extension, edited, +): + project = generic_project(tmp_path, skills=True) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output_dir = project / ".custom/commands" + skill = output_dir / "speckit-sample-run/SKILL.md" + original = skill.read_text(encoding="utf-8") + if edited: + skill.write_text(original + "\nuser edit\n", encoding="utf-8") + + save_init_options(project, {"ai": "generic", "ai_skills": False, "script": "sh"}) + manager.register_enabled_extensions_for_agent("generic") + + assert (output_dir / "speckit.sample.run.md").is_file() + assert skill.exists() is edited + if edited: + assert skill.read_text(encoding="utf-8") == original + "\nuser edit\n" + assert manager.remove("sample") + assert not (output_dir / "speckit.sample.run.md").exists() + assert skill.exists() is edited + + def test_generic_failed_registration_preserves_reinstall_config( tmp_path, generic_extension, ): From e3b33da5fdb5298f5ebfd83b0fc1193df925d66d Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 09:00:54 -0500 Subject: [PATCH 09/18] fix: explain unsupported generic integration state schema Include the saved and supported schema versions with upgrade guidance when generic registration encounters a newer integration state. Prove add and enable fail without mutating installed state, then succeed after the state is restored. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../integrations/generic/__init__.py | 13 +++++-- .../integrations/test_integration_generic.py | 36 +++++++++++++++++-- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/integrations/generic/__init__.py b/src/specify_cli/integrations/generic/__init__.py index 166ba94a1d..0dbe598ab7 100644 --- a/src/specify_cli/integrations/generic/__init__.py +++ b/src/specify_cli/integrations/generic/__init__.py @@ -20,11 +20,20 @@ def registration_directory(project_root: Path) -> Path: """Resolve the installed generic output root, never the class-level placeholder.""" - from ...integration_state import integration_setting, try_read_integration_json + from ...integration_state import ( + INTEGRATION_STATE_SCHEMA, + integration_setting, + try_read_integration_json, + ) state, error = try_read_integration_json(project_root) if error is not None: - raise ValueError(f"Cannot read generic integration settings: {error.detail}") + detail = ( + f"integration state schema {error.schema} is newer than supported " + f"schema {INTEGRATION_STATE_SCHEMA}; upgrade Spec Kit" + if error.kind == "schema_too_new" else error.detail + ) + raise ValueError(f"Cannot read generic integration settings: {detail}") settings = integration_setting(state or {}, "generic") commands_dir = GenericIntegration._resolve_commands_dir( settings.get("parsed_options"), {"raw_options": settings.get("raw_options")} diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index d1d7812685..b70bd60ece 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -1,5 +1,6 @@ """Tests for GenericIntegration.""" +import json import os import shutil import zipfile @@ -13,7 +14,7 @@ from specify_cli.integrations import get_integration from specify_cli.integrations.base import MarkdownIntegration from specify_cli.integrations.manifest import IntegrationManifest -from specify_cli.integration_state import write_integration_json +from specify_cli.integration_state import INTEGRATION_STATE_SCHEMA, write_integration_json from specify_cli.extensions import ExtensionCatalog, ExtensionError, ExtensionManager, HookExecutor from specify_cli import save_init_options @@ -185,6 +186,31 @@ def test_generic_extension_reports_missing_registration_options( assert not ExtensionManager(project).registry.is_installed("sample") +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_extension_reports_newer_integration_schema( + tmp_path, generic_extension, skills, +): + project = generic_project(tmp_path, skills=skills) + state_file = project / ".specify/integration.json" + original_state = state_file.read_text(encoding="utf-8") + state = json.loads(original_state) + state["integration_state_schema"] = INTEGRATION_STATE_SCHEMA + 1 + state_file.write_text(json.dumps(state), encoding="utf-8") + + manager = ExtensionManager(project) + with pytest.raises(ExtensionError, match="upgrade Spec Kit") as error: + manager.install_from_directory(generic_extension, "1.0.0") + assert str(state["integration_state_schema"]) in str(error.value) + assert str(INTEGRATION_STATE_SCHEMA) in str(error.value) + assert state_file.read_text(encoding="utf-8") == json.dumps(state) + assert not manager.registry.is_installed("sample") + assert not (manager.extensions_dir / "sample").exists() + + state_file.write_text(original_state, encoding="utf-8") + manager.install_from_directory(generic_extension, "1.0.0") + assert manager.remove("sample") + + @pytest.mark.parametrize("invalid_settings", ["missing", "malformed"]) def test_generic_skills_remove_with_invalid_settings( tmp_path, generic_extension, invalid_settings, @@ -1015,7 +1041,7 @@ def test_generic_extension_enable_rejects_partial_registration( assert collision.is_file() and other.is_file() -@pytest.mark.parametrize("invalid_settings", ["missing", "malformed"]) +@pytest.mark.parametrize("invalid_settings", ["missing", "malformed", "schema_too_new"]) def test_generic_extension_enable_failure_restores_disabled_state( tmp_path, generic_extension, invalid_settings, ): @@ -1035,11 +1061,17 @@ def test_generic_extension_enable_failure_restores_disabled_state( assert runner.invoke(app, ["extension", "disable", "sample"]).exit_code == 0 if invalid_settings == "missing": state_file.unlink() + elif invalid_settings == "schema_too_new": + state = json.loads(original_state) + state["integration_state_schema"] = INTEGRATION_STATE_SCHEMA + 1 + state_file.write_text(json.dumps(state), encoding="utf-8") else: state_file.write_text("{", encoding="utf-8") result = runner.invoke(app, ["extension", "enable", "sample"]) assert result.exit_code == 1 assert "Could not register generic invocations" in result.output + if invalid_settings == "schema_too_new": + assert "upgrade Spec Kit" in result.output assert not output.exists() assert ExtensionManager(project).registry.get("sample")["enabled"] is False state_file.write_bytes(original_state) From bd2a512aa0e2faca175ba5a8342762f3dbbf6cc8 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 11:26:16 -0500 Subject: [PATCH 10/18] fix: roll back partial generic extension refresh outputs Snapshot hash-owned artifacts and absent candidates before generic re-registration. On write or registry failures, restore previous command/skill bytes and symlinks, remove only newly generated output, and preserve the prior registry state; keep other extensions refreshing. Add failing-before coverage for command/skill layouts, partial writes, dev symlinks, layout transitions, and unrelated user files. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/__init__.py | 124 +++++++++++ .../integrations/test_integration_generic.py | 209 ++++++++++++++++++ 2 files changed, 333 insertions(+) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index a09b50076f..2337c81ad2 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1961,6 +1961,104 @@ def _generic_artifact_hashes( hashes[relative] = digest return hashes + def _snapshot_generic_refresh_artifacts( + self, manifest: ExtensionManifest, metadata: Dict[str, Any], + *, skills_mode_active: bool, + ) -> Dict[Path, tuple[bytes | None, str | None, bool]]: + """Remember owned outputs and absent candidates before a generic refresh.""" + from ..integrations.generic import registration_directory + from ..shared_infra import _validate_safe_shared_directory + + root = self.project_root.resolve() + output_dir = registration_directory(self.project_root) + source = (self.extensions_dir / manifest.id).resolve() + hashes = metadata.get("generic_artifact_hashes", {}) + if not isinstance(hashes, dict): + hashes = {} + registered = metadata.get("registered_commands", {}) + command_names = ( + set(self._collect_manifest_command_names(manifest)) + if not skills_mode_active else set() + ) + if isinstance(registered, dict): + command_names.update(self._valid_name_list(registered.get("generic"))) + skill_names = ( + {self._skill_name_for_command(command["name"]) for command in manifest.commands} + if skills_mode_active else set() + ) + skill_names.update(self._valid_name_list(metadata.get("registered_skills"))) + paths = {output_dir / f"{name}.md" for name in command_names} + paths.update(output_dir / name / "SKILL.md" for name in skill_names) + snapshot: Dict[Path, tuple[bytes | None, str | None, bool]] = {} + for path in paths: + try: + _validate_safe_shared_directory(root, path.parent) + except (OSError, ValueError): + # An unsafe candidate is not owned; do not block the other layout. + continue + if not path.exists() and not path.is_symlink(): + snapshot[path] = (None, None, path.parent.is_dir()) + continue + if not path.is_file(): + continue + if path.is_symlink() and not path.resolve().is_relative_to(source): + continue + content = path.read_bytes() + relative = path.relative_to(root).as_posix() + if hashes.get(relative) == hashlib.sha256(content).hexdigest(): + snapshot[path] = ( + content, os.readlink(path) if path.is_symlink() else None, True + ) + return snapshot + + def _restore_generic_refresh_artifacts( + self, snapshot: Dict[Path, tuple[bytes | None, str | None, bool]], + extension_id: str, + ) -> None: + """Restore prior owned files and remove only outputs absent before refresh.""" + from ..shared_infra import ( + _ensure_safe_shared_directory, + _validate_safe_shared_directory, + ) + + root = self.project_root.resolve() + source = (self.extensions_dir / extension_id).resolve() + errors = [] + for path, (content, link, parent_existed) in snapshot.items(): + try: + _validate_safe_shared_directory(root, path.parent) + if path.is_symlink(): + if content is None or not path.resolve().is_relative_to(source): + raise ValueError("unexpected symlink at output path") + if os.readlink(path) == link: + continue + path.unlink() + elif path.exists() and not path.is_file(): + raise ValueError("output path is no longer a file") + elif content is not None and link is None and path.is_file(): + if path.read_bytes() == content: + continue + if content is None: + if path.is_file(): + path.unlink() + if not parent_existed and path.parent.is_dir(): + try: + path.parent.rmdir() + except OSError: + pass + else: + _ensure_safe_shared_directory(root, path.parent) + if link is not None: + if path.is_file(): + path.unlink() + path.symlink_to(link) + else: + path.write_bytes(content) + except (OSError, ValueError) as exc: + errors.append(f"{path}: {exc}") + if errors: + raise ExtensionError("Could not restore generic artifacts: " + "; ".join(errors)) + def _generic_owned_names( self, metadata: Dict[str, Any], names: List[str], *, skills: bool ) -> List[str]: @@ -3683,7 +3781,13 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool # Isolate per-extension failures: one extension that fails to # register (e.g. an OSError writing a command file) must not abort # registration of the remaining enabled extensions for this agent. + generic_snapshot = None + registry_update_started = False try: + if agent_name == "generic": + generic_snapshot = self._snapshot_generic_refresh_artifacts( + manifest, metadata, skills_mode_active=skills_mode_active, + ) updates: Dict[str, Any] = {} registered: List[str] = [] registered_skills: List[str] = [] @@ -3751,6 +3855,8 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool # Skills are a companion artifact. If command registration # already succeeded, still persist it so later cleanup can # find those command files. + if agent_name == "generic": + raise from .. import _print_cli_warning _print_cli_warning( @@ -3934,12 +4040,30 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool if hashes != metadata.get("generic_artifact_hashes"): updates["generic_artifact_hashes"] = hashes if updates: + registry_update_started = True self.registry.update(ext_id, updates) except Exception as ext_err: # Best-effort per extension: warn and move on so a single bad # extension cannot silently drop the others. See #2950. from .. import _print_cli_warning + if generic_snapshot is not None: + rollback_errors = [] + try: + self._restore_generic_refresh_artifacts( + generic_snapshot, ext_id + ) + except (OSError, ValueError, ExtensionError) as error: + rollback_errors.append(f"artifacts: {error}") + if registry_update_started: + try: + self.registry.restore(ext_id, metadata) + except Exception as error: + rollback_errors.append(f"registry: {error}") + if rollback_errors: + ext_err = ExtensionError( + f"{ext_err}; rollback failed: {'; '.join(rollback_errors)}" + ) _print_cli_warning( "register extension artifacts for", "extension", diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index b70bd60ece..522c707bf3 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -476,6 +476,215 @@ def test_generic_command_refreshes_owned_artifact_and_missing_alias( assert not primary.exists() and not alias.exists() +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("first_present", [False, True]) +@pytest.mark.parametrize("dev_symlink", [False, True]) +@pytest.mark.parametrize("partial_second", [False, True]) +def test_generic_refresh_write_error_restores_artifacts_and_registry( + tmp_path, generic_extension, skills, first_present, dev_symlink, + partial_second, monkeypatch, +): + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"].append({ + "name": "speckit.sample.other", + "file": "commands/other.md", + "description": "Another command", + }) + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + (generic_extension / "commands/other.md").write_text( + "---\ndescription: Another command\n---\ncontent\n", encoding="utf-8", + ) + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory( + generic_extension, "1.0.0", link_commands=dev_symlink, + ) + output = project / ".custom/commands" + first = output / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + second = output / ( + "speckit-sample-other/SKILL.md" if skills else "speckit.sample.other.md" + ) + original = first.read_bytes() + if dev_symlink and not first.is_symlink(): + pytest.skip("dev-mode symlinks are unavailable") + metadata = manager.registry.get("sample") + if not first_present: + first.unlink() + if skills: + first.parent.rmdir() + second.unlink() + if skills: + second.parent.rmdir() + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nupdated source\n", encoding="utf-8") + original_write = Path.write_text + + def fail_second_write(path, *args, **kwargs): + if path == second: + if partial_second: + original_write(path, "partial generated output", encoding="utf-8") + raise OSError("simulated refresh write error") + return original_write(path, *args, **kwargs) + + monkeypatch.setattr(Path, "write_text", fail_second_write) + manager.register_enabled_extensions_for_agent("generic", force=skills) + monkeypatch.undo() + + assert first.exists() is first_present + if first_present: + assert first.read_bytes() == original + assert first.is_symlink() is dev_symlink + assert not second.exists() + assert manager.registry.get("sample") == metadata + unrelated = output / "user-owned.md" + unrelated.write_text("user content", encoding="utf-8") + + manager.register_enabled_extensions_for_agent("generic", force=skills) + assert "updated source" in first.read_text(encoding="utf-8") + assert second.is_file() + assert manager.remove("sample") + assert not first.exists() and not second.exists() + assert unrelated.read_text(encoding="utf-8") == "user content" + + +@pytest.mark.parametrize("save_before_error", [False, True]) +def test_generic_refresh_registry_write_error_restores_previous_state( + tmp_path, generic_extension, save_before_error, monkeypatch, +): + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands/speckit.sample.run.md" + original = output.read_bytes() + metadata = manager.registry.get("sample") + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nupdated source\n", encoding="utf-8") + original_save = manager.registry._save + called = False + + def fail_once(): + nonlocal called + called = True + monkeypatch.setattr(manager.registry, "_save", original_save) + if save_before_error: + original_save() + raise OSError("simulated refresh registry write error") + + monkeypatch.setattr(manager.registry, "_save", fail_once) + manager.register_enabled_extensions_for_agent("generic") + monkeypatch.undo() + + assert called + assert output.read_bytes() == original + assert manager.registry.get("sample") == metadata + assert ExtensionManager(project).registry.get("sample") == metadata + manager.register_enabled_extensions_for_agent("generic") + assert "updated source" in output.read_text(encoding="utf-8") + assert manager.remove("sample") + + +def test_generic_flat_refresh_ignores_unowned_skill_directory( + tmp_path, generic_extension, +): + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" + command = output / "speckit.sample.run.md" + user_skill = project / "user-skill" + user_skill.mkdir() + (user_skill / "SKILL.md").write_text("user content", encoding="utf-8") + skill_link = output / "speckit-sample-run" + try: + skill_link.symlink_to(user_skill, target_is_directory=True) + except OSError: + pytest.skip("directory symlinks are unavailable") + + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nupdated source\n", encoding="utf-8") + manager.register_enabled_extensions_for_agent("generic") + + assert "updated source" in command.read_text(encoding="utf-8") + assert skill_link.is_symlink() + assert (user_skill / "SKILL.md").read_text(encoding="utf-8") == "user content" + assert manager.remove("sample") + assert skill_link.is_symlink() + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_refresh_ignores_unrelated_other_layout_file( + tmp_path, generic_extension, skills, monkeypatch, +): + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" + current = output / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + other = output / ( + "speckit.sample.run.md" if skills else "speckit-sample-run/SKILL.md" + ) + other.parent.mkdir(parents=True, exist_ok=True) + other.write_text("user content", encoding="utf-8") + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nupdated source\n", encoding="utf-8") + original_read = Path.read_bytes + + def reject_other_layout_read(path, *args, **kwargs): + if path == other: + raise OSError("unrelated file must not be read") + return original_read(path, *args, **kwargs) + + monkeypatch.setattr(Path, "read_bytes", reject_other_layout_read) + manager.register_enabled_extensions_for_agent("generic", force=skills) + monkeypatch.undo() + + assert "updated source" in current.read_text(encoding="utf-8") + assert other.read_text(encoding="utf-8") == "user content" + assert manager.remove("sample") + assert other.read_text(encoding="utf-8") == "user content" + + +@pytest.mark.parametrize("skills_before", [False, True]) +def test_generic_layout_change_registry_error_restores_previous_artifacts( + tmp_path, generic_extension, skills_before, monkeypatch, +): + project = generic_project(tmp_path, skills=skills_before) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" + skill = output / "speckit-sample-run/SKILL.md" + command = output / "speckit.sample.run.md" + old_path, new_path = (skill, command) if skills_before else (command, skill) + original = old_path.read_bytes() + metadata = manager.registry.get("sample") + save_init_options( + project, {"ai": "generic", "ai_skills": not skills_before, "script": "sh"}, + ) + original_save = manager.registry._save + + def fail_once(): + monkeypatch.setattr(manager.registry, "_save", original_save) + raise OSError("simulated layout registry write error") + + monkeypatch.setattr(manager.registry, "_save", fail_once) + manager.register_enabled_extensions_for_agent("generic") + monkeypatch.undo() + + assert old_path.read_bytes() == original + assert not new_path.exists() + assert manager.registry.get("sample") == metadata + assert ExtensionManager(project).registry.get("sample") == metadata + manager.register_enabled_extensions_for_agent("generic") + assert new_path.is_file() + assert not old_path.exists() + assert manager.remove("sample") + + @pytest.mark.parametrize("skills", [False, True]) @pytest.mark.parametrize("ignored_source", [False, True]) def test_generic_partial_registration_rolls_back_all_artifacts( From d305bd9f9fff35e94ef3aaa996249f3cba34f2b1 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 12:03:57 -0500 Subject: [PATCH 11/18] fix: reject partial generic extension refreshes Require every generic invocation to be written or still hash-owned before committing refresh metadata. Keep hook-only extension disable independent of invalid generic output settings. Cover collisions, missing sources, retry, upgrade, and disabled hooks. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reference/extensions.md | 4 +- src/specify_cli/extensions/__init__.py | 45 +++- .../integrations/test_integration_generic.py | 232 ++++++++++++++++++ 3 files changed, 279 insertions(+), 2 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 8dbde190a8..6550cb0ce7 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -31,6 +31,8 @@ specify extension add Installs an extension from the catalog, a URL, or a local directory. Extension commands are registered with the active AI coding agent integration. For `generic`, invocations use the configured `--commands-dir`: flat command files by default, or `speckit-/SKILL.md` with `--skills`. The core `speckit.taskstoissues` command remains available alongside the GitHub extension's namespaced replacement during migration. +If a generic integration refresh cannot produce every extension invocation (for example, because a command or skill is user-modified or its source is missing), it warns and restores that extension's prior registered artifacts. Other extensions can still refresh. + > **Note:** All extension commands require a project already initialized with `specify init`. ## Remove an Extension @@ -100,7 +102,7 @@ specify extension enable specify extension disable ``` -Disable an extension without removing it. Disabled extensions are not loaded and their commands are not available. Re-enable with `enable`. +Disable an extension without removing it. Disabled extensions are not loaded and their commands are not available. Hook-only extensions can be disabled even if generic command-output settings are missing or invalid. Re-enable with `enable`. ## Set Extension Priority diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 2337c81ad2..69b98e7062 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2089,6 +2089,28 @@ def _generic_owned_names( owned.append(name) return owned + def _complete_generic_refresh( + self, + extension_id: str, + metadata: Dict[str, Any], + expected: List[str], + registered: List[str], + *, + skills: bool, + ) -> List[str]: + """Require each invocation to be newly written or still hash-owned.""" + missing = sorted(set(expected) - set(registered)) + retained = ( + self._generic_owned_names(metadata, missing, skills=skills) + if missing else [] + ) + absent = sorted(set(missing) - set(retained)) + if absent: + raise ExtensionError( + f"Missing invocation artifacts for '{extension_id}': {', '.join(absent)}" + ) + return list(dict.fromkeys(registered + retained)) + def _remove_generic_artifact_paths( self, extension_id: str, metadata: Dict[str, Any], *, skills: bool = True ) -> None: @@ -3471,10 +3493,12 @@ def disable_generic_extension_artifacts(self, extension_id: str) -> None: registered = metadata.get("registered_commands", {}) commands = self._valid_name_list(registered.get("generic")) if isinstance(registered, dict) else [] skills = self._valid_name_list(metadata.get("registered_skills", [])) + hashes = metadata.get("generic_artifact_hashes", {}) + if not commands and not skills and not hashes: + return from ..integrations.generic import registration_directory directory = registration_directory(self.project_root) - hashes = metadata.get("generic_artifact_hashes", {}) if isinstance(hashes, dict): for relative, expected in hashes.items(): if not isinstance(relative, str) or not isinstance(expected, str): @@ -3800,6 +3824,14 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool registered = registrar.register_commands_for_agent( agent_name, manifest, ext_dir, self.project_root ) + if agent_name == "generic": + registered = self._complete_generic_refresh( + ext_id, + metadata, + list(self._collect_manifest_command_names(manifest)), + registered, + skills=False, + ) registered_commands = metadata.get("registered_commands", {}) if not isinstance(registered_commands, dict): registered_commands = {} @@ -3851,6 +3883,17 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool registered_skills = self._register_extension_skills( manifest, ext_dir, force=force ) + if agent_name == "generic" and skills_mode_active: + registered_skills = self._complete_generic_refresh( + ext_id, + metadata, + [ + self._skill_name_for_command(cmd["name"]) + for cmd in manifest.commands + ], + registered_skills, + skills=True, + ) except Exception as skills_err: # Skills are a companion artifact. If command registration # already succeeded, still persist it so later cleanup can diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 522c707bf3..9fe6392496 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -408,6 +408,7 @@ def test_generic_skills_upgrade_preserves_replaced_or_modified_skill( ], catch_exceptions=False) assert upgraded.exit_code == 0, upgraded.output assert skill.read_text(encoding="utf-8") == replacement_content + assert "invocation artifacts" in upgraded.output finally: os.chdir(old_cwd) @@ -447,6 +448,57 @@ def test_generic_skills_upgrade_refreshes_artifact_digest( assert not skill.exists() +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_upgrade_warns_and_rolls_back_partial_extension_refresh( + tmp_path, generic_extension, skills, +): + from typer.testing import CliRunner + from specify_cli import app + + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"].append({ + "name": "speckit.sample.other", + "file": "commands/other.md", + "description": "Another command", + }) + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + (generic_extension / "commands/other.md").write_text( + "---\ndescription: Another command\n---\ncontent\n", encoding="utf-8", + ) + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" + first = output / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + second = output / ( + "speckit-sample-other/SKILL.md" if skills else "speckit.sample.other.md" + ) + original = first.read_bytes() + previous = manager.registry.get("sample") + second.write_text("user-edited invocation", encoding="utf-8") + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nupdated source\n", encoding="utf-8") + + old_cwd = os.getcwd() + try: + os.chdir(project) + upgraded = CliRunner().invoke(app, [ + "integration", "upgrade", "generic", "--force", + f"--integration-options=--commands-dir .custom/commands{' --skills' if skills else ''}", + ], catch_exceptions=False) + finally: + os.chdir(old_cwd) + + assert upgraded.exit_code == 0, upgraded.output + assert "invocation artifacts" in upgraded.output + assert first.read_bytes() == original + assert second.read_text(encoding="utf-8") == "user-edited invocation" + assert ExtensionManager(project).registry.get("sample") == previous + + def test_generic_command_refreshes_owned_artifact_and_missing_alias( tmp_path, generic_extension, ): @@ -476,6 +528,141 @@ def test_generic_command_refreshes_owned_artifact_and_missing_alias( assert not primary.exists() and not alias.exists() +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("collision_owned", [False, True]) +def test_generic_refresh_collision_rolls_back_and_warns( + tmp_path, generic_extension, skills, collision_owned, capsys, +): + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + if skills and collision_owned: + manifest["provides"]["commands"].append({ + "name": "speckit.sample.other", + "file": "commands/other.md", + "description": "Another command", + }) + (generic_extension / "commands/other.md").write_text( + "---\ndescription: Another command\n---\ncontent\n", encoding="utf-8", + ) + elif collision_owned: + manifest["provides"]["commands"][0]["aliases"] = ["speckit.sample.alias"] + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" + primary = output / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + collision = output / ( + "speckit-sample-other/SKILL.md" if skills else "speckit.sample.alias.md" + ) + if not collision_owned: + installed_manifest = manager.extensions_dir / "sample/extension.yml" + installed = yaml.safe_load(installed_manifest.read_text(encoding="utf-8")) + if skills: + installed["provides"]["commands"].append({ + "name": "speckit.sample.other", + "file": "commands/other.md", + "description": "Another command", + }) + (manager.extensions_dir / "sample/commands/other.md").write_text( + "---\ndescription: Another command\n---\ncontent\n", encoding="utf-8", + ) + collision.parent.mkdir() + else: + installed["provides"]["commands"][0]["aliases"] = ["speckit.sample.alias"] + installed_manifest.write_text(yaml.safe_dump(installed), encoding="utf-8") + original = primary.read_bytes() + collision.write_text("user-edited invocation", encoding="utf-8") + metadata = manager.registry.get("sample") + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nupdated source\n", encoding="utf-8") + + manager.register_enabled_extensions_for_agent("generic", force=skills) + + warning = capsys.readouterr().out + assert "Missing" in warning and "invocation artifacts" in warning + assert primary.read_bytes() == original + assert collision.read_text(encoding="utf-8") == "user-edited invocation" + assert manager.registry.get("sample") == metadata + assert manager.registry.get("sample")["enabled"] is True + + collision.unlink() + if skills: + collision.parent.rmdir() + manager.register_enabled_extensions_for_agent("generic", force=skills) + assert "updated source" in primary.read_text(encoding="utf-8") + assert collision.is_file() + assert manager.remove("sample") + + +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("retained", [False, True]) +def test_generic_refresh_missing_source_requires_output_or_prior_ownership( + tmp_path, generic_extension, skills, retained, capsys, +): + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"].append({ + "name": "speckit.sample.other", + "file": "commands/other.md", + "description": "Another command", + }) + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + second_source = generic_extension / "commands/other.md" + second_source.write_text( + "---\ndescription: Another command\n---\ncontent\n", encoding="utf-8", + ) + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + output = project / ".custom/commands" + first = output / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + second = output / ( + "speckit-sample-other/SKILL.md" if skills else "speckit.sample.other.md" + ) + original = first.read_bytes() + metadata = manager.registry.get("sample") + if not retained: + second.unlink() + if skills: + second.parent.rmdir() + (manager.extensions_dir / "sample/commands/other.md").unlink() + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nupdated source\n", encoding="utf-8") + + manager.register_enabled_extensions_for_agent("generic", force=skills) + + warning = capsys.readouterr().out + if retained: + assert "Missing" not in warning + assert "updated source" in first.read_text(encoding="utf-8") + assert second.is_file() + current = manager.registry.get("sample") + assert second.relative_to(project).as_posix() in current["generic_artifact_hashes"] + expected_name = "speckit-sample-other" if skills else "speckit.sample.other" + tracked = ( + current["registered_skills"] if skills + else current["registered_commands"]["generic"] + ) + assert expected_name in tracked + else: + assert "Missing" in warning and "invocation artifacts" in warning + assert first.read_bytes() == original + assert not second.exists() + assert manager.registry.get("sample") == metadata + (manager.extensions_dir / "sample/commands/other.md").write_bytes( + second_source.read_bytes() + ) + manager.register_enabled_extensions_for_agent("generic", force=skills) + assert "updated source" in first.read_text(encoding="utf-8") + assert second.is_file() + assert manager.remove("sample") + + @pytest.mark.parametrize("skills", [False, True]) @pytest.mark.parametrize("first_present", [False, True]) @pytest.mark.parametrize("dev_symlink", [False, True]) @@ -1290,6 +1477,51 @@ def test_generic_extension_enable_failure_restores_disabled_state( os.chdir(old_cwd) +@pytest.mark.parametrize("invalid_settings", ["missing", "malformed", "schema_too_new"]) +def test_generic_commandless_extension_disables_without_output_settings( + tmp_path, generic_extension, invalid_settings, +): + from typer.testing import CliRunner + from specify_cli import app + + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"] = [] + manifest["hooks"] = {"after_tasks": {"command": "echo sample"}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + installed_hooks = HookExecutor(project).get_project_config()["hooks"]["after_tasks"] + assert any( + hook["extension"] == "sample" and hook["enabled"] is True + for hook in installed_hooks + ) + state_file = project / ".specify/integration.json" + if invalid_settings == "missing": + state_file.unlink() + elif invalid_settings == "schema_too_new": + state = json.loads(state_file.read_text(encoding="utf-8")) + state["integration_state_schema"] = INTEGRATION_STATE_SCHEMA + 1 + state_file.write_text(json.dumps(state), encoding="utf-8") + else: + state_file.write_text("{", encoding="utf-8") + + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, ["extension", "disable", "sample"]) + finally: + os.chdir(old_cwd) + + assert result.exit_code == 0, result.output + updated = ExtensionManager(project).registry.get("sample") + assert updated["enabled"] is False + assert not updated["generic_artifact_hashes"] + hooks = HookExecutor(project).get_project_config()["hooks"]["after_tasks"] + assert any(hook["extension"] == "sample" and hook["enabled"] is False for hook in hooks) + + class TestGenericIntegration: """Tests for GenericIntegration — requires --commands-dir option.""" From b645dd510c831b77c85b3808b92f4215567b1121 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 15:39:01 -0500 Subject: [PATCH 12/18] fix: make generic extension lifecycle rollback complete Avoid generic registrar resolution for hook-only install and enable, restore command and skill artifacts and metadata if disable fails, and scope generic preset skill registration to extensions. Add before-and-after regressions for invalid settings, prior and moved outputs, and preset layout consistency. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reference/extensions.md | 2 +- docs/reference/presets.md | 2 +- src/specify_cli/extensions/__init__.py | 120 ++++++++++------ src/specify_cli/extensions/command_disable.py | 4 +- src/specify_cli/extensions/command_enable.py | 4 +- src/specify_cli/presets/_manager_skills.py | 4 +- .../integrations/test_integration_generic.py | 128 ++++++++++++++++++ 7 files changed, 216 insertions(+), 48 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 6550cb0ce7..62fbce3b07 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -102,7 +102,7 @@ specify extension enable specify extension disable ``` -Disable an extension without removing it. Disabled extensions are not loaded and their commands are not available. Hook-only extensions can be disabled even if generic command-output settings are missing or invalid. Re-enable with `enable`. +Disable an extension without removing it. Disabled extensions are not loaded and their commands are not available. Hook-only extensions can be installed, enabled, and disabled even if generic command-output settings are missing or invalid; extensions with commands still require valid settings. Re-enable with `enable`. ## Set Extension Priority diff --git a/docs/reference/presets.md b/docs/reference/presets.md index c3a253e21c..d51d3ace8a 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -27,7 +27,7 @@ specify preset add [] | `--from ` | Install from a custom URL instead of the catalog | | `--priority ` | Resolution priority (default: 10; lower = higher precedence) | -Installs a preset from the catalog, a URL, or a local directory. Preset commands are automatically registered with the currently installed AI coding agent integration. +Installs a preset from the catalog, a URL, or a local directory. Preset commands are automatically registered with supported active AI coding agent integrations. The generic integration currently delivers extension invocations but does not register preset command or skill overrides. > **Note:** All preset commands require a project already initialized with `specify init`. diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 69b98e7062..11d8639e63 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1531,6 +1531,8 @@ def _register_commands_for_active_agent( Mapping of agent name to registered command names, matching the ``registered_commands`` registry shape. """ + if not manifest.commands: + return {} registrar = CommandRegistrar(self.project_root) agent_scope = self._active_command_registration_scope() @@ -1590,6 +1592,8 @@ def _register_extension_skills( Returns: List of skill names that were created (for registry storage). """ + if not manifest.commands: + return [] skills_dir = self._get_skills_dir() if not skills_dir: return [] @@ -1962,10 +1966,10 @@ def _generic_artifact_hashes( return hashes def _snapshot_generic_refresh_artifacts( - self, manifest: ExtensionManifest, metadata: Dict[str, Any], + self, manifest: Optional[ExtensionManifest], metadata: Dict[str, Any], *, skills_mode_active: bool, ) -> Dict[Path, tuple[bytes | None, str | None, bool]]: - """Remember owned outputs and absent candidates before a generic refresh.""" + """Remember owned outputs and absent candidates for generic rollback.""" from ..integrations.generic import registration_directory from ..shared_infra import _validate_safe_shared_directory @@ -1978,17 +1982,23 @@ def _snapshot_generic_refresh_artifacts( registered = metadata.get("registered_commands", {}) command_names = ( set(self._collect_manifest_command_names(manifest)) - if not skills_mode_active else set() + if manifest is not None and not skills_mode_active else set() ) if isinstance(registered, dict): command_names.update(self._valid_name_list(registered.get("generic"))) skill_names = ( {self._skill_name_for_command(command["name"]) for command in manifest.commands} - if skills_mode_active else set() + if manifest is not None and skills_mode_active else set() ) skill_names.update(self._valid_name_list(metadata.get("registered_skills"))) paths = {output_dir / f"{name}.md" for name in command_names} paths.update(output_dir / name / "SKILL.md" for name in skill_names) + for relative in hashes: + if not isinstance(relative, str): + continue + name = Path(relative) + if not name.is_absolute() and ".." not in name.parts: + paths.add(root / name) snapshot: Dict[Path, tuple[bytes | None, str | None, bool]] = {} for path in paths: try: @@ -3485,7 +3495,7 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: return True def disable_generic_extension_artifacts(self, extension_id: str) -> None: - """Retire generic invocations without removing the installed sources.""" + """Disable and retire generic invocations without removing sources.""" metadata = self.registry.get(extension_id) if not metadata: raise ExtensionError(f"Extension '{extension_id}' is not installed") @@ -3494,48 +3504,76 @@ def disable_generic_extension_artifacts(self, extension_id: str) -> None: commands = self._valid_name_list(registered.get("generic")) if isinstance(registered, dict) else [] skills = self._valid_name_list(metadata.get("registered_skills", [])) hashes = metadata.get("generic_artifact_hashes", {}) - if not commands and not skills and not hashes: - return - from ..integrations.generic import registration_directory + has_artifacts = bool(commands or skills or hashes) + snapshot: Dict[Path, tuple[bytes | None, str | None, bool]] = {} + updates: Dict[str, Any] = {"enabled": False} + if has_artifacts: + from ..integrations.generic import registration_directory - directory = registration_directory(self.project_root) - if isinstance(hashes, dict): - for relative, expected in hashes.items(): - if not isinstance(relative, str) or not isinstance(expected, str): - continue - name = Path(relative) - if name.is_absolute() or ".." in name.parts: - continue - path = self.project_root.resolve() / name - if path.parent.resolve().is_relative_to(self.project_root.resolve()) and path.is_file(): - if hashlib.sha256(path.read_bytes()).hexdigest() != expected: + directory = registration_directory(self.project_root) + if isinstance(hashes, dict): + for relative, expected in hashes.items(): + if not isinstance(relative, str) or not isinstance(expected, str): + continue + name = Path(relative) + if name.is_absolute() or ".." in name.parts: + continue + path = self.project_root.resolve() / name + if path.parent.resolve().is_relative_to(self.project_root.resolve()) and path.is_file(): + if hashlib.sha256(path.read_bytes()).hexdigest() != expected: + raise ExtensionError( + f"Cannot disable '{extension_id}': generic artifact {path} " + "was modified or is not owned; preserve it and remove it manually" + ) + for names, is_skill in ((commands, False), (skills, True)): + owned = self._generic_owned_names(metadata, names, skills=is_skill) + for name in names: + path = directory / name / "SKILL.md" if is_skill else directory / f"{name}.md" + if (path.exists() or path.is_symlink()) and name not in owned: raise ExtensionError( f"Cannot disable '{extension_id}': generic artifact {path} " "was modified or is not owned; preserve it and remove it manually" ) - for names, is_skill in ((commands, False), (skills, True)): - owned = self._generic_owned_names(metadata, names, skills=is_skill) - for name in names: - path = directory / name / "SKILL.md" if is_skill else directory / f"{name}.md" - if (path.exists() or path.is_symlink()) and name not in owned: - raise ExtensionError( - f"Cannot disable '{extension_id}': generic artifact {path} " - "was modified or is not owned; preserve it and remove it manually" - ) - - self._remove_generic_artifact_paths(extension_id, metadata) - if skills: - self._unregister_extension_skills( - skills, extension_id, skills_dir=directory, - generic_hashes=metadata.get("generic_artifact_hashes", {}), + snapshot = self._snapshot_generic_refresh_artifacts( + self.get_extension(extension_id), metadata, + skills_mode_active=bool(skills), ) - new_commands = dict(registered) if isinstance(registered, dict) else {} - new_commands.pop("generic", None) - self.registry.update(extension_id, { - "registered_commands": new_commands, - "registered_skills": self._extension_owned_skill_names(skills, extension_id), - "generic_artifact_hashes": {}, - }) + + registry_update_started = False + try: + if has_artifacts: + self._remove_generic_artifact_paths(extension_id, metadata) + if skills: + self._unregister_extension_skills( + skills, extension_id, skills_dir=directory, + generic_hashes=metadata.get("generic_artifact_hashes", {}), + ) + new_commands = dict(registered) if isinstance(registered, dict) else {} + new_commands.pop("generic", None) + updates.update({ + "registered_commands": new_commands, + "registered_skills": self._extension_owned_skill_names(skills, extension_id), + "generic_artifact_hashes": {}, + }) + registry_update_started = True + self.registry.update(extension_id, updates) + except Exception as exc: + rollback_errors = [] + try: + self._restore_generic_refresh_artifacts(snapshot, extension_id) + except (OSError, ValueError, ExtensionError) as error: + rollback_errors.append(f"artifacts: {error}") + if registry_update_started: + try: + self.registry.restore(extension_id, metadata) + except Exception as error: + rollback_errors.append(f"registry: {error}") + if rollback_errors: + raise ExtensionError( + f"Cannot disable '{extension_id}': {exc}; " + f"rollback failed: {'; '.join(rollback_errors)}" + ) from exc + raise @staticmethod def _valid_name_list(value: Any) -> List[str]: diff --git a/src/specify_cli/extensions/command_disable.py b/src/specify_cli/extensions/command_disable.py index c1da4f68dc..16ef6683e1 100644 --- a/src/specify_cli/extensions/command_disable.py +++ b/src/specify_cli/extensions/command_disable.py @@ -52,8 +52,8 @@ def extension_disable( except (ExtensionError, ValueError, OSError) as exc: console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") raise typer.Exit(1) from exc - - manager.registry.update(extension_id, {"enabled": False}) + else: + manager.registry.update(extension_id, {"enabled": False}) # Disable hooks in extensions.yml config = hook_executor.get_project_config() diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index de837aee6e..32eb602fae 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -49,12 +49,12 @@ def extension_enable( init_options = load_init_options(project_root) if init_options.get("ai") == "generic": try: - manager.register_enabled_extensions_for_agent("generic") - refreshed = manager.registry.get(extension_id) or {} manifest = manager.get_extension(extension_id) if manifest is None: raise ExtensionError(f"Cannot read manifest for '{extension_id}'") if manifest.commands: + manager.register_enabled_extensions_for_agent("generic") + refreshed = manager.registry.get(extension_id) or {} from .._init_options import is_ai_skills_enabled skills = is_ai_skills_enabled(init_options) diff --git a/src/specify_cli/presets/_manager_skills.py b/src/specify_cli/presets/_manager_skills.py index fa7e715a31..e6246232af 100644 --- a/src/specify_cli/presets/_manager_skills.py +++ b/src/specify_cli/presets/_manager_skills.py @@ -455,6 +455,9 @@ def _get_skills_dir(self) -> Optional[Path]: resolve_active_skills_dir, ) from ..shared_infra import _ensure_safe_shared_directory + opts = load_init_options(self.project_root) + if isinstance(opts, dict) and opts.get("ai") == "generic": + return None try: skills_dir = resolve_active_skills_dir(self.project_root) except (ValueError, OSError) as exc: @@ -466,7 +469,6 @@ def _get_skills_dir(self) -> Optional[Path]: if skills_dir is None: return None - opts = load_init_options(self.project_root) selected_ai = opts.get("ai") if isinstance(opts, dict) else None if not isinstance(selected_ai, str) or not selected_ai: return skills_dir diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 9fe6392496..e4ca969b43 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -1522,6 +1522,134 @@ def test_generic_commandless_extension_disables_without_output_settings( assert any(hook["extension"] == "sample" and hook["enabled"] is False for hook in hooks) +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("invalid_settings", ["missing", "malformed", "schema_too_new"]) +def test_generic_commandless_install_and_enable_ignore_output_settings( + tmp_path, generic_extension, invalid_settings, skills, +): + from typer.testing import CliRunner + from specify_cli import app + + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"] = [] + manifest["hooks"] = {"after_tasks": {"command": "echo sample"}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + project = generic_project(tmp_path, skills=skills) + state_file = project / ".specify/integration.json" + if invalid_settings == "missing": + state_file.unlink() + elif invalid_settings == "malformed": + state_file.write_text("{", encoding="utf-8") + else: + state = json.loads(state_file.read_text(encoding="utf-8")) + state["integration_state_schema"] = INTEGRATION_STATE_SCHEMA + 1 + state_file.write_text(json.dumps(state), encoding="utf-8") + + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + assert manager.registry.get("sample")["enabled"] is True + old_cwd = os.getcwd() + try: + os.chdir(project) + runner = CliRunner() + disabled = runner.invoke(app, ["extension", "disable", "sample"]) + assert disabled.exit_code == 0, disabled.output + enabled = runner.invoke(app, ["extension", "enable", "sample"]) + finally: + os.chdir(old_cwd) + + assert enabled.exit_code == 0, enabled.output + assert ExtensionManager(project).registry.get("sample")["enabled"] is True + hooks = HookExecutor(project).get_project_config()["hooks"]["after_tasks"] + assert any(hook["extension"] == "sample" and hook["enabled"] is True for hook in hooks) + + +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("after_write", [False, True]) +@pytest.mark.parametrize("moved", [False, True]) +def test_generic_disable_registry_failure_restores_artifacts_and_metadata( + tmp_path, generic_extension, monkeypatch, skills, after_write, moved, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + original_content = artifact.read_bytes() + original_metadata = manager.registry.get("sample") + if moved: + write_integration_json( + project, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": { + "commands_dir": ".new/commands", "skills": skills, + }}}, + ) + original_save = manager.registry.__class__._save + failed = False + + def fail_disable_save(registry): + nonlocal failed + entry = registry.data["extensions"]["sample"] + if entry.get("enabled") is False and not failed: + failed = True + if after_write: + original_save(registry) + raise OSError("simulated disable registry failure") + return original_save(registry) + + monkeypatch.setattr(manager.registry.__class__, "_save", fail_disable_save) + old_cwd = os.getcwd() + try: + os.chdir(project) + runner = CliRunner() + disabled = runner.invoke(app, ["extension", "disable", "sample"]) + finally: + os.chdir(old_cwd) + monkeypatch.undo() + + assert disabled.exit_code != 0 + assert "simulated disable registry failure" in disabled.output + assert artifact.read_bytes() == original_content + assert ExtensionManager(project).registry.get("sample") == original_metadata + assert manager.registry.get("sample")["enabled"] is True + + old_cwd = os.getcwd() + try: + os.chdir(project) + retried = runner.invoke(app, ["extension", "disable", "sample"]) + finally: + os.chdir(old_cwd) + assert retried.exit_code == 0, retried.output + assert not artifact.exists() + assert ExtensionManager(project).registry.get("sample")["enabled"] is False + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_preset_registration_remains_outside_extension_scope( + tmp_path, skills, +): + from specify_cli.presets import PresetManager + from tests.specify_cli.presets._helpers import install_self_test_preset + + project = generic_project(tmp_path, skills=skills) + core = project / ".custom/commands" / ( + "speckit-specify/SKILL.md" if skills else "speckit.specify.md" + ) + original = core.read_bytes() + manager = PresetManager(project) + install_self_test_preset(manager) + + assert core.read_bytes() == original + metadata = manager.registry.get("self-test") + assert not metadata.get("registered_commands", {}).get("generic") + assert not metadata.get("registered_skills", {}).get("generic") + + class TestGenericIntegration: """Tests for GenericIntegration — requires --commands-dir option.""" From 8b2c44f29a9ffeae71cb52da146c60339e5c2142 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 16:17:35 -0500 Subject: [PATCH 13/18] fix: restore historical generic artifacts on update rollback Back up hash-owned generic outputs across previous directories even when another integration is active. Disable validates tracked ownership without rejecting unrelated same-named output after directory moves. Cover aliases, skills, multiple directories, collisions, modified output, and failed-update retry. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/reference/extensions.md | 4 + src/specify_cli/extensions/__init__.py | 48 ++-- .../extensions/_command_update_transaction.py | 28 ++ .../integrations/test_integration_generic.py | 241 ++++++++++++++++++ 4 files changed, 304 insertions(+), 17 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 62fbce3b07..81c5ad3ff4 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -95,6 +95,8 @@ Updates a specific extension, or all installed extensions if no name is given. Bundled extensions (such as `agent-context` and `git`) have no download URL; their updates install from the copy shipped with the running spec-kit release. When the catalog advertises a newer version than your spec-kit release ships, the update is reported as requiring a spec-kit upgrade first. +For `generic`, a failed update restores hash-owned invocations from previously configured `--commands-dir` locations as well as the current location, even if another integration is now active. + ## Enable / Disable an Extension ```bash @@ -104,6 +106,8 @@ specify extension disable Disable an extension without removing it. Disabled extensions are not loaded and their commands are not available. Hook-only extensions can be installed, enabled, and disabled even if generic command-output settings are missing or invalid; extensions with commands still require valid settings. Re-enable with `enable`. +For `generic`, disabling removes hash-owned invocations even after the output directory moves, but preserves unrelated same-named files in the new directory. + ## Set Extension Priority ```bash diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 11d8639e63..1998ba0568 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1966,16 +1966,20 @@ def _generic_artifact_hashes( return hashes def _snapshot_generic_refresh_artifacts( - self, manifest: Optional[ExtensionManifest], metadata: Dict[str, Any], - *, skills_mode_active: bool, + self, extension_id: str, manifest: Optional[ExtensionManifest], + metadata: Dict[str, Any], + *, skills_mode_active: bool, include_current_candidates: bool = True, ) -> Dict[Path, tuple[bytes | None, str | None, bool]]: """Remember owned outputs and absent candidates for generic rollback.""" from ..integrations.generic import registration_directory from ..shared_infra import _validate_safe_shared_directory root = self.project_root.resolve() - output_dir = registration_directory(self.project_root) - source = (self.extensions_dir / manifest.id).resolve() + output_dir = ( + registration_directory(self.project_root) + if include_current_candidates else None + ) + source = (self.extensions_dir / extension_id).resolve() hashes = metadata.get("generic_artifact_hashes", {}) if not isinstance(hashes, dict): hashes = {} @@ -1991,8 +1995,10 @@ def _snapshot_generic_refresh_artifacts( if manifest is not None and skills_mode_active else set() ) skill_names.update(self._valid_name_list(metadata.get("registered_skills"))) - paths = {output_dir / f"{name}.md" for name in command_names} - paths.update(output_dir / name / "SKILL.md" for name in skill_names) + paths: set[Path] = set() + if output_dir is not None: + paths.update(output_dir / f"{name}.md" for name in command_names) + paths.update(output_dir / name / "SKILL.md" for name in skill_names) for relative in hashes: if not isinstance(relative, str): continue @@ -3520,22 +3526,29 @@ def disable_generic_extension_artifacts(self, extension_id: str) -> None: continue path = self.project_root.resolve() / name if path.parent.resolve().is_relative_to(self.project_root.resolve()) and path.is_file(): + if path.is_symlink() and not path.resolve().is_relative_to( + (self.extensions_dir / extension_id).resolve() + ): + raise ExtensionError( + f"Cannot disable '{extension_id}': generic artifact {path} " + "was modified or is not owned; preserve it and remove it manually" + ) if hashlib.sha256(path.read_bytes()).hexdigest() != expected: raise ExtensionError( f"Cannot disable '{extension_id}': generic artifact {path} " "was modified or is not owned; preserve it and remove it manually" ) - for names, is_skill in ((commands, False), (skills, True)): - owned = self._generic_owned_names(metadata, names, skills=is_skill) - for name in names: - path = directory / name / "SKILL.md" if is_skill else directory / f"{name}.md" - if (path.exists() or path.is_symlink()) and name not in owned: - raise ExtensionError( - f"Cannot disable '{extension_id}': generic artifact {path} " - "was modified or is not owned; preserve it and remove it manually" - ) + if not hashes: + for names, is_skill in ((commands, False), (skills, True)): + for name in names: + path = directory / name / "SKILL.md" if is_skill else directory / f"{name}.md" + if path.exists() or path.is_symlink(): + raise ExtensionError( + f"Cannot disable '{extension_id}': generic artifact {path} " + "was modified or is not owned; preserve it and remove it manually" + ) snapshot = self._snapshot_generic_refresh_artifacts( - self.get_extension(extension_id), metadata, + extension_id, self.get_extension(extension_id), metadata, skills_mode_active=bool(skills), ) @@ -3848,7 +3861,8 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool try: if agent_name == "generic": generic_snapshot = self._snapshot_generic_refresh_artifacts( - manifest, metadata, skills_mode_active=skills_mode_active, + ext_id, manifest, metadata, + skills_mode_active=skills_mode_active, ) updates: Dict[str, Any] = {} registered: List[str] = [] diff --git a/src/specify_cli/extensions/_command_update_transaction.py b/src/specify_cli/extensions/_command_update_transaction.py index c9f116d363..df2264c639 100644 --- a/src/specify_cli/extensions/_command_update_transaction.py +++ b/src/specify_cli/extensions/_command_update_transaction.py @@ -104,6 +104,7 @@ def run_update_command(extension: str | None) -> None: # Store backup state backup_registry_entry = None # None means registry entry not yet captured + backup_generic_artifacts = None backup_installed = UNSET # Original installed list from extensions.yml backup_hooks = None # None means backup step 4 not yet reached; {} or {...} means backup was captured backed_up_command_files = {} @@ -221,6 +222,28 @@ def backup_extension_skills(skill_names, *, skills_dir=None): # 1. Backup registry entry (always, even if extension dir doesn't exist) backup_registry_entry = manager.registry.get(extension_id) + if ( + isinstance(backup_registry_entry, dict) + and ( + "generic" in registrar.AGENT_CONFIGS + or backup_registry_entry.get("generic_artifact_hashes") + ) + ): + generic_active = "generic" in registrar.AGENT_CONFIGS + backup_generic_artifacts = ( + manager._snapshot_generic_refresh_artifacts( + extension_id, + ( + manager.get_extension(extension_id) + if generic_active else None + ), + backup_registry_entry, + skills_mode_active=is_ai_skills_enabled( + _commands.load_init_options(project_root) + ), + include_current_candidates=generic_active, + ) + ) # 2. Backup extension directory extension_dir = manager.extensions_dir / extension_id @@ -760,6 +783,11 @@ def backup_extension_skills(skill_names, *, skills_dir=None): symlinks=True, ) + if backup_generic_artifacts is not None: + manager._restore_generic_refresh_artifacts( + backup_generic_artifacts, extension_id + ) + # Remove empty artifact directories that did not exist at # the destructive boundary. Do this after skill cleanup and # restoration so newly created skills roots and their diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index e4ca969b43..3254ceb661 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -1285,6 +1285,247 @@ def install_update(self, _zip_path, speckit_version, *, catalog_name=None): assert not artifact.exists() +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("collision", [False, True]) +@pytest.mark.parametrize("multiple_prior_dirs", [False, True]) +@pytest.mark.parametrize("inactive_generic", [False, True]) +def test_generic_update_rollback_restores_previous_output_directory( + tmp_path, generic_extension, skills, collision, multiple_prior_dirs, + inactive_generic, +): + from typer.testing import CliRunner + from specify_cli import app + + if not skills: + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"][0]["aliases"] = ["speckit.sample.alias"] + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + name = "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + old_file = project / ".custom/commands" / name + original_files = { + old_file: old_file.read_bytes(), + } + if not skills: + alias = project / ".custom/commands/speckit.sample.alias.md" + original_files[alias] = alias.read_bytes() + if multiple_prior_dirs: + write_integration_json( + project, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": { + "commands_dir": ".middle/commands", "skills": skills, + }}}, + ) + (project / ".middle/commands").mkdir(parents=True) + manager.register_enabled_extensions_for_agent("generic", force=skills) + middle_file = project / ".middle/commands" / name + original_files[middle_file] = middle_file.read_bytes() + if not skills: + middle_alias = project / ".middle/commands/speckit.sample.alias.md" + original_files[middle_alias] = middle_alias.read_bytes() + previous = manager.registry.get("sample") + new_dir = project / ".new/commands" + new_dir.mkdir(parents=True) + new_file = new_dir / name + if collision: + new_file.parent.mkdir(parents=True, exist_ok=True) + new_file.write_text("unrelated new output", encoding="utf-8") + write_integration_json( + project, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": { + "commands_dir": ".new/commands", "skills": skills, + }}}, + ) + if inactive_generic: + save_init_options(project, {"ai": "claude", "ai_skills": False, "script": "sh"}) + + updated_source = tmp_path / "updated-source" + shutil.copytree(generic_extension, updated_source) + manifest_path = updated_source / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["extension"]["version"] = "2.0.0" + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + updated_command = updated_source / "commands/run.md" + updated_command.write_text( + updated_command.read_text(encoding="utf-8") + "\nupdated command\n", + encoding="utf-8", + ) + archive = tmp_path / "sample-update.zip" + with zipfile.ZipFile(archive, "w") as zip_file: + for file in updated_source.rglob("*"): + if file.is_file(): + zip_file.write(file, file.relative_to(updated_source)) + + def install_update(self, _zip_path, speckit_version, *, catalog_name=None): + if fail_install: + raise RuntimeError("simulated update failure") + return self.install_from_directory( + updated_source, speckit_version, catalog_name=catalog_name + ) + + fail_install = True + with ( + patch.object(Path, "cwd", return_value=project), + patch.object(ExtensionCatalog, "get_extension_info", return_value={ + "id": "sample", "name": "Sample", "version": "2.0.0", + "_install_allowed": True, + }), + patch.object(ExtensionCatalog, "download_extension", return_value=archive), + patch.object(ExtensionManager, "install_from_zip", install_update), + ): + failed = CliRunner().invoke( + app, ["extension", "update", "sample"], input="y\n", + ) + assert failed.exit_code == 1 + assert "simulated update failure" in failed.output + assert {path: path.read_bytes() for path in original_files} == original_files + if collision: + assert new_file.read_text(encoding="utf-8") == "unrelated new output" + else: + assert not new_file.exists() + assert ExtensionManager(project).registry.get("sample") == previous + + if inactive_generic: + return + if collision: + new_file.unlink() + if skills: + new_file.parent.rmdir() + fail_install = False + with zipfile.ZipFile(archive, "w") as zip_file: + for file in updated_source.rglob("*"): + if file.is_file(): + zip_file.write(file, file.relative_to(updated_source)) + retried = CliRunner().invoke( + app, ["extension", "update", "sample"], input="y\n", + ) + + assert retried.exit_code == 0, retried.output + assert all(not path.exists() for path in original_files) + assert "updated command" in new_file.read_text(encoding="utf-8") + if not skills: + assert (new_dir / "speckit.sample.alias.md").is_file() + assert ExtensionManager(project).registry.get("sample")["version"] == "2.0.0" + + +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("modified_old", [False, True]) +def test_generic_disable_after_move_ignores_unrelated_new_output( + tmp_path, generic_extension, skills, modified_old, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + name = "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + old_file = project / ".custom/commands" / name + if modified_old: + old_file.write_text("user-edited old output", encoding="utf-8") + new_file = project / ".new/commands" / name + new_file.parent.mkdir(parents=True) + new_file.write_text("unrelated new output", encoding="utf-8") + write_integration_json( + project, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": { + "commands_dir": ".new/commands", "skills": skills, + }}}, + ) + + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, ["extension", "disable", "sample"]) + finally: + os.chdir(old_cwd) + + assert new_file.read_text(encoding="utf-8") == "unrelated new output" + state = ExtensionManager(project).registry.get("sample") + if modified_old: + assert result.exit_code == 1 + assert old_file.read_text(encoding="utf-8") == "user-edited old output" + assert state["enabled"] is True + else: + assert result.exit_code == 0, result.output + assert not old_file.exists() + assert state["enabled"] is False + assert not state["generic_artifact_hashes"] + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_disable_refuses_untracked_current_output( + tmp_path, generic_extension, skills, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + original = artifact.read_bytes() + manager.registry.update("sample", {"generic_artifact_hashes": {}}) + + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, ["extension", "disable", "sample"]) + finally: + os.chdir(old_cwd) + + assert result.exit_code == 1 + assert "not owned" in result.output + assert artifact.read_bytes() == original + assert ExtensionManager(project).registry.get("sample")["enabled"] is True + + +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_disable_rejects_retargeted_old_symlink_after_move( + tmp_path, generic_extension, skills, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + user_file = project / "user-output.md" + user_file.write_bytes(artifact.read_bytes()) + artifact.unlink() + try: + artifact.symlink_to(user_file) + except OSError: + pytest.skip("file symlinks are unavailable") + write_integration_json( + project, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": { + "commands_dir": ".new/commands", "skills": skills, + }}}, + ) + + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, ["extension", "disable", "sample"]) + finally: + os.chdir(old_cwd) + + assert result.exit_code == 1 + assert "not owned" in result.output + assert artifact.is_symlink() + assert user_file.read_bytes() == artifact.read_bytes() + assert ExtensionManager(project).registry.get("sample")["enabled"] is True + + @pytest.mark.parametrize("skills", [False, True]) def test_generic_extension_rejects_escaping_directory( tmp_path, generic_extension, skills, From e9159eef7b81c34bc3e02a0dd364836ffda67855 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 16:45:09 -0500 Subject: [PATCH 14/18] Reject inconsistent generic extension state and report update errors Cross-check the persisted generic default and skills layout against init options before installing commandful extensions. Convert eager update registrar configuration failures to handled CLI errors and cover both failure paths with regression tests. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/__init__.py | 41 +++++++ .../extensions/_command_update_transaction.py | 7 +- .../integrations/test_integration_generic.py | 111 +++++++++++++++++- 3 files changed, 157 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 1998ba0568..37fd4d46f9 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -47,6 +47,12 @@ from .._utils import dump_frontmatter, relative_extension_path_violation, version_satisfies from ..catalogs import CatalogEntry as BaseCatalogEntry from ..catalogs import CatalogStackBase +from ..integration_state import ( + INTEGRATION_STATE_SCHEMA, + default_integration_key, + integration_setting, + try_read_integration_json, +) from ..shared_infra import verify_archive_sha256 _FALLBACK_CORE_COMMAND_NAMES = frozenset( @@ -2438,6 +2444,41 @@ def install_from_directory( active_options = load_init_options(self.project_root) generic_active = isinstance(active_options, dict) and active_options.get("ai") == "generic" + if register_commands and manifest.commands: + state, state_error = try_read_integration_json(self.project_root) + if state_error is not None: + detail = ( + f"integration state schema {state_error.schema} is newer than supported " + f"schema {INTEGRATION_STATE_SCHEMA}; upgrade Spec Kit" + if state_error.kind == "schema_too_new" + else state_error.detail or state_error.kind + ) + raise ExtensionError( + "Cannot register extension commands: cannot read integration settings: " + f"{detail}" + ) + generic_default = default_integration_key(state) == "generic" if state else False + if state is not None and (generic_default or generic_active): + if generic_default != generic_active: + raise ExtensionError( + "Cannot register generic extension commands: generic integration " + "and init options disagree" + ) + parsed_options = integration_setting(state, "generic").get("parsed_options") + configured_skills = ( + parsed_options.get("skills", False) + if isinstance(parsed_options, dict) else False + ) + init_skills = active_options.get("ai_skills", False) + if ( + not isinstance(configured_skills, bool) + or not isinstance(init_skills, bool) + or configured_skills != init_skills + ): + raise ExtensionError( + "Cannot register generic extension commands: generic integration " + "and init options disagree on skills mode" + ) if register_commands and generic_active and manifest.commands: from ..integrations.generic import registration_directory diff --git a/src/specify_cli/extensions/_command_update_transaction.py b/src/specify_cli/extensions/_command_update_transaction.py index df2264c639..81c7199d55 100644 --- a/src/specify_cli/extensions/_command_update_transaction.py +++ b/src/specify_cli/extensions/_command_update_transaction.py @@ -75,7 +75,12 @@ def run_update_command(extension: str | None) -> None: console.print() updated_extensions = [] failed_updates = [] - registrar = CommandRegistrar(project_root) + try: + registrar = CommandRegistrar(project_root) + except (OSError, ValueError) as exc: + raise ExtensionError( + f"Cannot resolve extension update registration settings: {exc}" + ) from exc hook_executor = HookExecutor(project_root) from ..agents import CommandRegistrar as _AgentReg # used in backup and rollback paths diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 3254ceb661..6aaba2c5e1 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -186,6 +186,115 @@ def test_generic_extension_reports_missing_registration_options( assert not ExtensionManager(project).registry.is_installed("sample") +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize( + "invalid_options", + ["missing", "malformed", "missing_agent", "different_agent", "wrong_layout", "invalid_layout"], +) +def test_generic_command_install_rejects_inconsistent_init_options( + tmp_path, generic_extension, skills, invalid_options, +): + project = generic_project(tmp_path, skills=skills) + options_path = project / ".specify/init-options.json" + options = json.loads(options_path.read_text(encoding="utf-8")) + if invalid_options == "missing": + options_path.unlink() + elif invalid_options == "malformed": + options_path.write_text("{", encoding="utf-8") + else: + if invalid_options == "missing_agent": + options.pop("ai") + elif invalid_options == "different_agent": + options["ai"] = "claude" + elif invalid_options == "wrong_layout": + options["ai_skills"] = not skills + else: + options["ai_skills"] = "true" + options_path.write_text(json.dumps(options), encoding="utf-8") + + manager = ExtensionManager(project) + with pytest.raises(ExtensionError, match="generic.*init options"): + manager.install_from_directory(generic_extension, "1.0.0") + + assert not manager.registry.is_installed("sample") + assert not (manager.extensions_dir / "sample").exists() + assert not (project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + )).exists() + + +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("invalid_options", ["missing", "malformed"]) +def test_generic_hook_only_install_accepts_missing_init_options( + tmp_path, generic_extension, skills, invalid_options, +): + manifest_path = generic_extension / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"] = [] + manifest["hooks"] = {"after_tasks": {"command": "echo sample"}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + project = generic_project(tmp_path, skills=skills) + options_path = project / ".specify/init-options.json" + if invalid_options == "missing": + options_path.unlink() + else: + options_path.write_text("{", encoding="utf-8") + + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + assert manager.registry.get("sample")["enabled"] is True + assert not manager.registry.get("sample")["generic_artifact_hashes"] + hooks = HookExecutor(project).get_project_config()["hooks"]["after_tasks"] + assert any(hook["extension"] == "sample" and hook["enabled"] is True for hook in hooks) + + +@pytest.mark.parametrize("invalid_settings", ["missing", "malformed", "schema_too_new"]) +def test_generic_extension_update_reports_invalid_settings_without_crashing( + tmp_path, generic_extension, invalid_settings, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands/speckit.sample.run.md" + original = artifact.read_bytes() + previous = manager.registry.get("sample") + state_file = project / ".specify/integration.json" + if invalid_settings == "missing": + state_file.unlink() + elif invalid_settings == "malformed": + state_file.write_text("{", encoding="utf-8") + else: + state = json.loads(state_file.read_text(encoding="utf-8")) + state["integration_state_schema"] = INTEGRATION_STATE_SCHEMA + 1 + state_file.write_text(json.dumps(state), encoding="utf-8") + + old_cwd = os.getcwd() + try: + os.chdir(project) + with ( + patch.object(ExtensionCatalog, "get_extension_info", return_value={ + "id": "sample", "name": "Sample", "version": "2.0.0", + "_install_allowed": True, + }), + ): + result = CliRunner().invoke( + app, ["extension", "update", "sample"], input="y\n", + ) + finally: + os.chdir(old_cwd) + + assert result.exit_code == 1 + assert result.exception is not None + assert "Error:" in result.output + assert "extension update registration settings" in result.output + assert isinstance(result.exception, SystemExit) + assert artifact.read_bytes() == original + assert ExtensionManager(project).registry.get("sample") == previous + + @pytest.mark.parametrize("skills", [False, True]) def test_generic_extension_reports_newer_integration_schema( tmp_path, generic_extension, skills, @@ -1533,7 +1642,7 @@ def test_generic_extension_rejects_escaping_directory( project = generic_project(tmp_path, skills=skills) write_integration_json( project, version="1.0.0", integration_key="generic", - settings={"generic": {"parsed_options": {"commands_dir": "../outside"}}}, + settings={"generic": {"parsed_options": {"commands_dir": "../outside", "skills": skills}}}, ) with pytest.raises(ExtensionError, match="escapes project root"): ExtensionManager(project).install_from_directory(generic_extension, "1.0.0") From 71815df875d993f923f70501b6c45faf6b1eacea Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 17:17:10 -0500 Subject: [PATCH 15/18] Scope generic extension state handling to generic projects Preserve non-generic extension installation and event-refresh error handling when integration state is invalid. Store generic ownership metadata only for generic installs and add regressions for both sides of the boundary. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/events/__init__.py | 26 ++++++++++-------- src/specify_cli/extensions/__init__.py | 27 +++++++++---------- .../integrations/test_integration_generic.py | 27 ++++++++++++++++++- tests/specify_cli/events/test_events.py | 17 +++++++++--- 4 files changed, 67 insertions(+), 30 deletions(-) diff --git a/src/specify_cli/events/__init__.py b/src/specify_cli/events/__init__.py index b47931e46f..613dba5702 100644 --- a/src/specify_cli/events/__init__.py +++ b/src/specify_cli/events/__init__.py @@ -1827,19 +1827,23 @@ def refresh_integration_events(project_root: Path) -> None: while a stale native hook may still be active (R3). """ from ..integrations import get_integration - from ..integrations._helpers import _resolve_integration_options + from ..integrations._helpers import _read_integration_json, _resolve_integration_options from ..integrations.manifest import IntegrationManifest from ..integration_state import installed_integration_keys, try_read_integration_json - - state, error = try_read_integration_json(project_root) - if error is not None: - detail = ( - f"unsupported schema {error.schema}" - if error.kind == "schema_too_new" - else f"{error.kind}: {error.detail}" - ) - raise EventRefreshError([(".specify/integration.json", detail)]) - state = state or {} + from .._init_options import load_init_options + + if load_init_options(project_root).get("ai") == "generic": + state, error = try_read_integration_json(project_root) + if error is not None: + detail = ( + f"unsupported schema {error.schema}" + if error.kind == "schema_too_new" + else f"{error.kind}: {error.detail}" + ) + raise EventRefreshError([(".specify/integration.json", detail)]) + state = state or {} + else: + state = _read_integration_json(project_root) failures: list[tuple[str, str]] = [] for key in installed_integration_keys(state): integration = get_integration(key) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 37fd4d46f9..b559f91a2c 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2446,7 +2446,7 @@ def install_from_directory( generic_active = isinstance(active_options, dict) and active_options.get("ai") == "generic" if register_commands and manifest.commands: state, state_error = try_read_integration_json(self.project_root) - if state_error is not None: + if state_error is not None and generic_active: detail = ( f"integration state schema {state_error.schema} is newer than supported " f"schema {INTEGRATION_STATE_SCHEMA}; upgrade Spec Kit" @@ -3118,19 +3118,18 @@ def rollback_generic_registration() -> None: else "local" ) registry_started = True - self.registry.add( - manifest.id, - { - "version": manifest.version, - "source": source, - "manifest_hash": manifest.get_hash(), - "enabled": True, - "priority": priority, - "registered_commands": registered_commands, - "registered_skills": registered_skills, - "generic_artifact_hashes": generic_hashes, - }, - ) + registry_entry = { + "version": manifest.version, + "source": source, + "manifest_hash": manifest.get_hash(), + "enabled": True, + "priority": priority, + "registered_commands": registered_commands, + "registered_skills": registered_skills, + } + if generic_active: + registry_entry["generic_artifact_hashes"] = generic_hashes + self.registry.add(manifest.id, registry_entry) except Exception as exc: # Any failed commit must retire outputs before the original error # is re-raised, including errors from hook serialization. diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 6aaba2c5e1..db6bfce548 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -223,6 +223,31 @@ def test_generic_command_install_rejects_inconsistent_init_options( )).exists() +@pytest.mark.parametrize("invalid_settings", ["malformed", "schema_too_new"]) +def test_non_generic_command_install_keeps_legacy_integration_state_handling( + tmp_path, generic_extension, invalid_settings, +): + project = tmp_path / "project" + skills_dir = project / ".claude/skills" + skills_dir.mkdir(parents=True) + save_init_options(project, {"ai": "claude", "ai_skills": False, "script": "sh"}) + state_file = project / ".specify/integration.json" + if invalid_settings == "malformed": + state_file.write_text("{", encoding="utf-8") + else: + state_file.write_text( + json.dumps({"integration_state_schema": INTEGRATION_STATE_SCHEMA + 1}), + encoding="utf-8", + ) + + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + + assert manager.registry.is_installed("sample") + assert "generic_artifact_hashes" not in manager.registry.get("sample") + assert (skills_dir / "speckit-sample-run/SKILL.md").is_file() + + @pytest.mark.parametrize("skills", [False, True]) @pytest.mark.parametrize("invalid_options", ["missing", "malformed"]) def test_generic_hook_only_install_accepts_missing_init_options( @@ -243,7 +268,7 @@ def test_generic_hook_only_install_accepts_missing_init_options( manager = ExtensionManager(project) manager.install_from_directory(generic_extension, "1.0.0") assert manager.registry.get("sample")["enabled"] is True - assert not manager.registry.get("sample")["generic_artifact_hashes"] + assert not manager.registry.get("sample").get("generic_artifact_hashes") hooks = HookExecutor(project).get_project_config()["hooks"]["after_tasks"] assert any(hook["extension"] == "sample" and hook["enabled"] is True for hook in hooks) diff --git a/tests/specify_cli/events/test_events.py b/tests/specify_cli/events/test_events.py index 368b56930e..c92e9e30ad 100644 --- a/tests/specify_cli/events/test_events.py +++ b/tests/specify_cli/events/test_events.py @@ -2628,14 +2628,17 @@ class TestRefreshIntegrationEvents: """#1: refresh_integration_events regenerates native config after extension state changes.""" + @pytest.mark.parametrize("active_agent", ["generic", "claude"]) @pytest.mark.parametrize("invalid_state,detail", [ ("{", "decode"), (json.dumps({"integration_state_schema": 999}), "unsupported schema 999"), ]) def test_refresh_reports_invalid_state_without_changing_hooks( - self, tmp_path, invalid_state, detail, + self, tmp_path, invalid_state, detail, active_agent, ): + from specify_cli import save_init_options from specify_cli.events import EventRefreshError, refresh_integration_events + from typer import Exit integration = ClaudeIntegration() manifest = IntegrationManifest(integration.key, tmp_path, version="test") @@ -2645,12 +2648,18 @@ def test_refresh_reports_invalid_state_without_changing_hooks( ) config_path = tmp_path / ".claude/settings.json" original = config_path.read_bytes() + save_init_options(tmp_path, {"ai": active_agent}) state_path = tmp_path / ".specify/integration.json" state_path.write_text(invalid_state, encoding="utf-8") - with pytest.raises(EventRefreshError, match=detail) as exc_info: - refresh_integration_events(tmp_path) - assert exc_info.value.failures[0][0] == ".specify/integration.json" + if active_agent == "generic": + with pytest.raises(EventRefreshError, match=detail) as exc_info: + refresh_integration_events(tmp_path) + assert exc_info.value.failures[0][0] == ".specify/integration.json" + else: + with pytest.raises(Exit) as exc_info: + refresh_integration_events(tmp_path) + assert exc_info.value.exit_code == 1 assert config_path.read_bytes() == original def test_refresh_strips_removed_extension_events(self, tmp_path): From 9a8d2ef2f50221b06dfb52b40e7ecbd686041bea Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 17:27:19 -0500 Subject: [PATCH 16/18] Reject hard-linked generic outputs during refresh Treat generated files with multiple hard links as unowned before command or skill refresh, so a matching hash cannot authorize changes to unrelated linked files. Cover both layouts and successful refresh after unlinking. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/agents.py | 2 ++ src/specify_cli/extensions/__init__.py | 2 ++ .../integrations/test_integration_generic.py | 34 +++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 0dd819c214..64540bbca9 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -631,6 +631,8 @@ def _generic_owned_output(path: Path, source_id: str, project_root: Path) -> boo hashes = metadata.get("generic_artifact_hashes", {}) if metadata else {} if not isinstance(hashes, dict) or not path.is_file(): return False + if path.stat().st_nlink > 1: + return False if path.is_symlink() and not path.resolve().is_relative_to( (project_root / ".specify/extensions").resolve() ): diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index b559f91a2c..b7b8934d12 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2102,6 +2102,8 @@ def _generic_owned_names( continue if not path.is_file(): continue + if path.stat().st_nlink > 1: + continue if path.is_symlink() and not path.resolve().is_relative_to( self.extensions_dir.resolve() ): diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index db6bfce548..79012c47eb 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -113,6 +113,40 @@ def test_generic_extension_preserves_modified_artifact(tmp_path, generic_extensi assert artifact.read_text(encoding="utf-8").endswith("user edit\n") +@pytest.mark.parametrize("skills", [False, True]) +def test_generic_refresh_rejects_hard_linked_artifact( + tmp_path, generic_extension, skills, capsys, +): + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0") + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + unrelated = project / "unrelated.md" + try: + os.link(artifact, unrelated) + except OSError: + pytest.skip("hard links are unavailable") + original = artifact.read_bytes() + metadata = manager.registry.get("sample") + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nnew source\n", encoding="utf-8") + + manager.register_enabled_extensions_for_agent("generic", force=skills) + + warning = capsys.readouterr().out + assert "Missing" in warning and "invocation artifacts" in warning + assert artifact.read_bytes() == original + assert unrelated.read_bytes() == original + assert manager.registry.get("sample") == metadata + + unrelated.unlink() + manager.register_enabled_extensions_for_agent("generic", force=skills) + assert b"new source" in artifact.read_bytes() + assert manager.remove("sample") + + @pytest.mark.parametrize("operation", ["resync", "remove", "force"]) def test_generic_skill_symlinked_directory_is_not_owned( tmp_path, generic_extension, operation, From ca3d27ec81f6b56fca91fcd2ccc7a47efee9fb33 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 18:07:19 -0500 Subject: [PATCH 17/18] Require per-extension ownership for generic symlinks Reject cross-extension retargeted symlinks in generic command and skill ownership checks, and preserve them during skill cleanup. Exercise refresh, forced reinstall, removal, and update rollback with identical target bytes. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/agents.py | 2 +- src/specify_cli/extensions/__init__.py | 31 +++-- src/specify_cli/extensions/command_enable.py | 2 +- .../integrations/test_integration_generic.py | 108 +++++++++++++++++- 4 files changed, 131 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 64540bbca9..6016773e51 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -634,7 +634,7 @@ def _generic_owned_output(path: Path, source_id: str, project_root: Path) -> boo if path.stat().st_nlink > 1: return False if path.is_symlink() and not path.resolve().is_relative_to( - (project_root / ".specify/extensions").resolve() + (project_root / ".specify/extensions" / source_id).resolve() ): return False relative = path.relative_to(project_root.resolve()).as_posix() diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index b7b8934d12..f46328c14c 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1680,7 +1680,7 @@ def _replacement(match: re.Match[str]) -> str: if selected_ai == "generic" and skill_dir_preexists: metadata = self.registry.get(manifest.id) or {} if skill_name not in self._generic_owned_names( - metadata, [skill_name], skills=True + metadata, [skill_name], skills=True, extension_id=manifest.id ): continue if skill_file.exists() or skill_file.is_symlink(): @@ -2082,7 +2082,8 @@ def _restore_generic_refresh_artifacts( raise ExtensionError("Could not restore generic artifacts: " + "; ".join(errors)) def _generic_owned_names( - self, metadata: Dict[str, Any], names: List[str], *, skills: bool + self, metadata: Dict[str, Any], names: List[str], *, + skills: bool, extension_id: str, ) -> List[str]: """Keep customized or untracked generic artifacts out of cleanup.""" from ..integrations.generic import registration_directory @@ -2105,7 +2106,7 @@ def _generic_owned_names( if path.stat().st_nlink > 1: continue if path.is_symlink() and not path.resolve().is_relative_to( - self.extensions_dir.resolve() + (self.extensions_dir / extension_id).resolve() ): continue relative = path.relative_to(root).as_posix() @@ -2125,7 +2126,9 @@ def _complete_generic_refresh( """Require each invocation to be newly written or still hash-owned.""" missing = sorted(set(expected) - set(registered)) retained = ( - self._generic_owned_names(metadata, missing, skills=skills) + self._generic_owned_names( + metadata, missing, skills=skills, extension_id=extension_id + ) if missing else [] ) absent = sorted(set(missing) - set(retained)) @@ -2236,6 +2239,14 @@ def _unregister_extension_skills( skill_names, extension_id, skills_dir=skills_dir ): skill_file = skill_subdir / "SKILL.md" + if ( + generic_hashes is not None + and skill_file.is_symlink() + and not skill_file.resolve().is_relative_to( + (self.extensions_dir / extension_id).resolve() + ) + ): + continue if generic_hashes is not None and skill_file.is_relative_to( self.project_root.resolve() ): @@ -2510,7 +2521,8 @@ def install_from_directory( ) owned = ( set(self._generic_owned_names( - self.registry.get(manifest.id) or {}, list(names), skills=skills, + self.registry.get(manifest.id) or {}, list(names), + skills=skills, extension_id=manifest.id, )) if force and self.registry.is_installed(manifest.id) else set() ) @@ -3696,7 +3708,7 @@ def unregister_agent_artifacts( ) if agent_name == "generic": command_names = self._generic_owned_names( - metadata, command_names, skills=False + metadata, command_names, skills=False, extension_id=ext_id ) if command_names: registrar.unregister_commands( @@ -3713,7 +3725,7 @@ def unregister_agent_artifacts( if registered_skills and not commands_only: if agent_name == "generic": registered_skills = self._generic_owned_names( - metadata, registered_skills, skills=True + metadata, registered_skills, skills=True, extension_id=ext_id ) # Always pass the explicit, agent-scoped skills_dir — even # when it doesn't currently exist on disk. This method must @@ -4051,7 +4063,7 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool ] if agent_name == "generic": to_remove = self._generic_owned_names( - metadata, to_remove, skills=True + metadata, to_remove, skills=True, extension_id=ext_id ) if to_remove: self._unregister_extension_skills( @@ -4139,7 +4151,8 @@ def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool if fully_replaced: if agent_name == "generic": fully_replaced = self._generic_owned_names( - metadata, fully_replaced, skills=False + metadata, fully_replaced, skills=False, + extension_id=ext_id, ) registrar.unregister_commands( {agent_name: fully_replaced}, self.project_root diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index 32eb602fae..34bab2f279 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -66,7 +66,7 @@ def extension_enable( if skills else set(manager._collect_manifest_command_names(manifest)) ) owned = set(manager._generic_owned_names( - refreshed, list(expected), skills=skills, + refreshed, list(expected), skills=skills, extension_id=extension_id, )) missing = expected - owned if missing: diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 79012c47eb..e828d7ead1 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -169,7 +169,8 @@ def test_generic_skill_symlinked_directory_is_not_owned( source.write_text(source.read_text(encoding="utf-8") + "\nnew source\n", encoding="utf-8") manager.register_enabled_extensions_for_agent("generic", force=True) assert manager._generic_owned_names( - manager.registry.get("sample"), ["speckit-sample-run"], skills=True, + manager.registry.get("sample"), ["speckit-sample-run"], + skills=True, extension_id="sample", ) == [] elif operation == "force": with pytest.raises(ExtensionError, match="cannot be replaced safely"): @@ -1453,6 +1454,71 @@ def install_update(self, _zip_path, speckit_version, *, catalog_name=None): assert not artifact.exists() +def test_generic_update_rollback_preserves_cross_extension_skill_link( + tmp_path, generic_extension, monkeypatch, +): + from typer.testing import CliRunner + from specify_cli import app + + project = generic_project(tmp_path, skills=True) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0", link_commands=True) + artifact = project / ".custom/commands/speckit-sample-run/SKILL.md" + if not artifact.is_symlink(): + pytest.skip("dev-mode symlinks are unavailable") + other_file = manager.extensions_dir / "other/file.md" + other_file.parent.mkdir() + original = artifact.read_bytes() + other_file.write_bytes(original) + artifact.unlink() + artifact.symlink_to(os.path.relpath(other_file, artifact.parent)) + metadata = manager.registry.get("sample") + + updated_source = tmp_path / "updated-source" + shutil.copytree(generic_extension, updated_source) + manifest_path = updated_source / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["extension"]["version"] = "2.0.0" + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + archive = tmp_path / "sample-update.zip" + with zipfile.ZipFile(archive, "w") as zip_file: + for file in updated_source.rglob("*"): + if file.is_file(): + zip_file.write(file, file.relative_to(updated_source)) + + original_unlink = Path.unlink + removed_user_link = [] + + def track_unlink(path, *args, **kwargs): + if path == artifact: + removed_user_link.append(path) + return original_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", track_unlink) + with ( + patch.object(Path, "cwd", return_value=project), + patch.object(ExtensionCatalog, "get_extension_info", return_value={ + "id": "sample", + "name": "Sample", + "version": "2.0.0", + "_install_allowed": True, + }), + patch.object(ExtensionCatalog, "download_extension", return_value=archive), + patch.object(ExtensionManager, "install_from_zip", side_effect=RuntimeError("update failed")), + ): + result = CliRunner().invoke( + app, ["extension", "update", "sample"], input="y\n", + ) + + assert result.exit_code == 1 + assert "update failed" in result.output + assert not removed_user_link + assert artifact.is_symlink() + assert artifact.resolve() == other_file.resolve() + assert other_file.read_bytes() == original + assert ExtensionManager(project).registry.get("sample") == metadata + + @pytest.mark.parametrize("skills", [False, True]) @pytest.mark.parametrize("collision", [False, True]) @pytest.mark.parametrize("multiple_prior_dirs", [False, True]) @@ -1723,6 +1789,46 @@ def test_generic_dev_extension_removes_links(tmp_path, generic_extension, skills assert not artifact.is_symlink() +@pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("operation", ["refresh", "force", "remove"]) +def test_generic_retargeted_cross_extension_link_is_not_owned( + tmp_path, generic_extension, skills, operation, capsys, +): + project = generic_project(tmp_path, skills=skills) + manager = ExtensionManager(project) + manager.install_from_directory(generic_extension, "1.0.0", link_commands=True) + artifact = project / ".custom/commands" / ( + "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" + ) + if not artifact.is_symlink(): + pytest.skip("dev-mode symlinks are unavailable") + other_file = manager.extensions_dir / "other" / "file.md" + other_file.parent.mkdir() + original = artifact.read_bytes() + other_file.write_bytes(original) + artifact.unlink() + artifact.symlink_to(os.path.relpath(other_file, artifact.parent)) + metadata = manager.registry.get("sample") + + if operation == "refresh": + source = manager.extensions_dir / "sample/commands/run.md" + source.write_text(source.read_text(encoding="utf-8") + "\nnew source\n", encoding="utf-8") + manager.register_enabled_extensions_for_agent("generic", force=skills) + warning = capsys.readouterr().out + assert "Missing" in warning and "invocation artifacts" in warning + assert manager.registry.get("sample") == metadata + elif operation == "force": + with pytest.raises(ExtensionError, match="cannot be replaced safely"): + manager.install_from_directory(generic_extension, "1.0.0", force=True) + assert manager.registry.get("sample") == metadata + else: + assert manager.remove("sample") + + assert artifact.is_symlink() + assert artifact.resolve() == other_file.resolve() + assert other_file.read_bytes() == original + + @pytest.mark.parametrize("skills", [False, True]) def test_generic_dev_extension_preserves_edited_link(tmp_path, generic_extension, skills): project = generic_project(tmp_path, skills=skills) From 2fd9aaa1bfabb02687fbc404b2a1996934619943 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 29 Sep 2026 21:01:38 -0500 Subject: [PATCH 18/18] Cover owned generic dev links during force reinstall Exercise successful force reinstall for owned command and skill dev symlinks alongside regular outputs, complementing cross-extension retargeting regressions. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/integrations/test_integration_generic.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index e828d7ead1..c2eabfcaa7 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -1324,17 +1324,25 @@ def test_generic_extension_does_not_overwrite_existing_command_or_skill( @pytest.mark.parametrize("skills", [False, True]) +@pytest.mark.parametrize("dev_symlink", [False, True]) def test_generic_extension_force_reinstall_only_replaces_owned_artifacts( - tmp_path, generic_extension, skills, + tmp_path, generic_extension, skills, dev_symlink, ): project = generic_project(tmp_path, skills=skills) manager = ExtensionManager(project) - manager.install_from_directory(generic_extension, "1.0.0") + manager.install_from_directory(generic_extension, "1.0.0", link_commands=dev_symlink) artifact = project / ".custom/commands" / ( "speckit-sample-run/SKILL.md" if skills else "speckit.sample.run.md" ) - manager.install_from_directory(generic_extension, "1.0.0", force=True) + if dev_symlink and not artifact.is_symlink(): + pytest.skip("dev-mode symlinks are unavailable") + manager.install_from_directory( + generic_extension, "1.0.0", force=True, link_commands=dev_symlink, + ) assert artifact.is_file() + assert artifact.is_symlink() is dev_symlink + if dev_symlink: + assert artifact.resolve().is_relative_to((manager.extensions_dir / "sample").resolve()) assert manager.remove("sample") assert not artifact.exists()