diff --git a/AGENTS.md b/AGENTS.md index 949f4999f9..8128976993 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,3 +64,11 @@ before acting. worktree's own virtualenv (`uv sync --extra test` then `.venv/bin/python -m pytest`). A bare `uv run pytest` can pick up an editable install from another worktree and fail to import new subpackages. +- **Mutating CLI workflows:** Assume the project directory is writable and + only one Specify CLI invocation mutates a project at a time. Prefer simple + direct writes and report exceptional filesystem failures plainly. Do not + introduce speculative locks, staging, compare-and-swap, or elaborate + recovery for concurrent invocations, read-only destinations, or disk + exhaustion without a concrete supported use case. Preserve semantic + validation and ordinary supported-workflow invariants; this guidance does + not call for removing existing safeguards in unrelated commands. diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 676428a079..b2ae481d2b 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -181,6 +181,13 @@ 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 opted-in native hook projection, disabling remembers each owned hook's +effective enabled value; enabling restores those values rather than activating +hooks that were individually disabled before the extension was disabled. For +extensions disabled before this state was recorded, enabling uses the +declarations in their installed manifest. Without the opt-in, the legacy +enable behavior continues to activate all owned hooks. + 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 @@ -304,6 +311,46 @@ Spec Kit stores project-level extension registration and hook configuration in: The file contains installed extensions, global settings, and hooks that are surfaced before or after Spec Kit commands. +Scripted extension hooks are **opt-in**. Set `"native_extension_hooks": true` +in `.specify/init-options.json`, then refresh shared infrastructure (for +example, with `specify integration switch --refresh-shared-infra`) +or perform an extension configuration update. Only the literal JSON boolean +`true` enables native hook projection; `false` or an absent key leaves existing +markdown/YAML hook behavior unchanged, without producing a projection. +Other values for this key are rejected. Turning the option off retires an +existing projection on the next configuration save or shared-infrastructure +refresh. Run only one Specify CLI mutation against a project at a time; +simultaneous CLI changes to the same project are unsupported. + +When opted in, extension configuration saves publish a validated projection in +`.specify/hook-dispatch/`: one `.json` response and SHA-256 digest per +event, an `events.txt` index of response filename stems and digest, and a +`source.yml` copy of the configuration. Each `events.txt` line is the name +of an event whose `.json` response contains the same `event` identity. +Only enabled hooks with no condition appear in the projection, +in their YAML declaration order (regardless of priority). Each response +includes `extension`, `command`, `optional`, `description`, `prompt`, and +`priority`. Existing projects receive a projection when their shared +infrastructure is refreshed; configuration writes regenerate it. Invalid +hook entries and unsafe projection paths prevent publication when opted in. Current installed +commands continue using their existing hook resolution; the projection does +not change execution behavior. For projection portability, missing or invalid +priorities and values above `2147483647` use the default `10`; the current +in-process hook reader retains its existing priority handling. + +Opted-in hook configuration and projection writes are direct, not atomic. If +an I/O error interrupts a write, Spec Kit reports the failure, but +`.specify/extensions.yml` or `.specify/hook-dispatch/` may be incomplete. +Fix the filesystem error, inspect and repair `extensions.yml` if needed, then +retry the extension operation or refresh shared infrastructure with +`specify integration switch --refresh-shared-infra`. A failed +install publication does not automatically unregister pre-existing hooks for +the same extension ID. Removal cleans up invocation artifacts and sources +before unregistering hooks; a cleanup failure can leave partially removed +artifacts but does not itself remove hooks. If hook publication then fails, +manual repair may be necessary. Git cannot recover uncommitted changes to +these files. + ```yaml installed: - git @@ -350,7 +397,27 @@ Each hook entry supports the following fields: | `description` | Human-readable explanation of what the hook does. | | `condition` | Optional expression evaluated by `HookExecutor` (using `config.` or `env.` with `is set`, `==`, or `!=`). Current command templates do not evaluate conditions and skip hooks with a non-empty condition. | -Hook event names identify when a hook is invoked. They generally use `before_` or `after_`, such as `before_implement`, `after_implement`, `before_tasks`, and `after_tasks`. +Hook event names identify when a hook is invoked. When native hook projection +is enabled, new manifests and project configuration writes require +`before_` or `after_`, including commands beyond +the built-in set. Core +targets use `before_` / `after_` (for example, +`before_plan`); extension-owned targets use +`before__` / +`after__` (for example, `before_bug_assess` for +`speckit.bug.assess`). The suffix identifies the command **being observed**, +not the extension providing the hook. It starts with a lowercase letter or +digit and contains only lowercase letters, digits, or underscores. Normalize +hyphens in target extension and command names to underscores (for example, +`my-extension` becomes `my_extension`); names are never rewritten implicitly. +The full event name must fit in a projection filename. + +**Opt-in migration:** Legacy names such as `custom:after` remain accepted by +default, but are rejected during extension installation, update, or +existing-project projection refresh after opting in. Rename the event in the extension's `extension.yml` and +any corresponding `.specify/extensions.yml` hooks to a matching +`before_` or `after_` name before retrying. +Invalid names fail explicitly rather than being silently omitted. Extension manifests reject invalid hook priorities during installation. For existing `.specify/extensions.yml` entries, `HookExecutor.get_hooks_for_event()` sorts with `normalize_priority()`: missing values, booleans, non-numeric values rejected by `int()`, and values less than `1` fall back to `10`; numeric strings and finite floats are coerced with `int()`, while non-finite floats are unsupported and may fail instead of falling back. diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 181b0feec1..a27e84ffe6 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -172,7 +172,13 @@ def _install_shared_infra( Returns ``True`` on success. """ - return _install_shared_infra_impl( + config_file = project_path / ".specify" / "extensions.yml" + if config_file.exists() or config_file.is_symlink(): + from .extensions import HookExecutor + + hook_executor = HookExecutor(project_path) + hook_executor.load_project_config_for_update() + result = _install_shared_infra_impl( project_path, script_type, version=get_speckit_version(), @@ -185,6 +191,9 @@ def _install_shared_infra( refresh_managed=refresh_managed, refresh_hint=refresh_hint, ) + if config_file.exists() or config_file.is_symlink(): + HookExecutor(project_path).migrate_project_config() + return result def _install_shared_infra_or_exit( diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index d03cb85770..86ef233381 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2461,6 +2461,21 @@ def install_from_directory( # Load and validate manifest manifest_path = source_dir / "extension.yml" manifest = ExtensionManifest(manifest_path) + from ._hook_projection import projection_enabled, project_hooks + + native_hooks = projection_enabled(self.project_root) + if native_hooks: + try: + project_hooks({"hooks": { + event: [ + {**entry, "extension": manifest.id} + for entry in coerce_hook_entries(entries) + ] + for event, entries in manifest.hooks.items() + }}) + except ValueError as exc: + raise ValidationError(f"Invalid manifest hooks: {exc}") from exc + HookExecutor(self.project_root).load_project_config_for_update() # Check compatibility self.check_compatibility(manifest, speckit_version) @@ -3055,33 +3070,34 @@ 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). - def rollback_generic_registration() -> None: + def rollback_installation_files() -> 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): + if register_commands and generic_active and manifest.commands: + 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 - elif not path.is_file(): - if skills and path.parent.is_dir(): + 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 - continue - path.unlink() - if skills: - try: - path.parent.rmdir() - except OSError: - pass preserved = set(stranded_configs) if did_remove: @@ -3108,10 +3124,12 @@ def rollback_generic_registration() -> None: shutil.rmtree(dest_dir) hooks_started = False + hooks_finished = False registry_started = False + registered_commands = {} + registered_skills = [] 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 @@ -3142,6 +3160,7 @@ def rollback_generic_registration() -> None: hook_executor = HookExecutor(self.project_root) hooks_started = True hook_executor.register_hooks(manifest) + hooks_finished = True # Restore config files from backup when --force triggered a removal. # Only restore *.yml config files to match what remove() backs up, @@ -3187,28 +3206,48 @@ def rollback_generic_registration() -> None: 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. - if register_commands and generic_active and manifest.commands: + # Retire invocation artifacts after a failed publication, but do + # not unregister pre-existing same-ID hooks from a partial write. + rollback_installation = ( + register_commands and generic_active and bool(manifest.commands) + ) or (hooks_started and not hooks_finished) + if rollback_installation: rollback_errors = [] if registry_started: try: self.registry.remove(manifest.id) except Exception as error: rollback_errors.append(f"registry: {error}") - if hooks_started: + if hooks_started and (hooks_finished or not native_hooks): try: hook_executor.unregister_hooks(manifest.id) except Exception as error: rollback_errors.append(f"hooks: {error}") + if registered_commands: + try: + non_generic = { + agent: commands for agent, commands in registered_commands.items() + if agent != "generic" + } + if non_generic: + CommandRegistrar(self.project_root, include_generic=False).unregister_commands( + non_generic, self.project_root + ) + except Exception as error: + rollback_errors.append(f"commands: {error}") + if registered_skills: + try: + self._unregister_extension_skills(registered_skills, manifest.id) + except Exception as error: + rollback_errors.append(f"skills: {error}") try: with changing_file(dest_dir, removal=True): - rollback_generic_registration() + rollback_installation_files() except Exception as error: rollback_errors.append(f"artifacts: {error}") if rollback_errors: raise ExtensionError( - f"Generic registration failed ({exc}); rollback failed: " + f"Extension installation failed ({exc}); rollback failed: " + "; ".join(rollback_errors) ) from exc raise @@ -3543,6 +3582,9 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if not self.registry.is_installed(extension_id): return False + hook_executor = HookExecutor(self.project_root) + hook_executor.load_project_config_for_update() + # Get registered commands and skills before removal metadata = self.registry.get(extension_id) registered_commands = ( @@ -3616,8 +3658,6 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if extension_dir.exists(): shutil.rmtree(extension_dir) - # Unregister hooks - hook_executor = HookExecutor(self.project_root) hook_executor.unregister_hooks(extension_id) # Update registry @@ -3625,6 +3665,230 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: return True + def disable(self, extension_id: str) -> bool: + """Disable an installed extension and its hooks, restoring invocations on failure. + + Returns False when the extension is already disabled. + """ + from .. import load_init_options + from ._hook_projection import projection_enabled + + hook_executor = HookExecutor(self.project_root) + config = hook_executor.load_project_config_for_update() + metadata = self.registry.get(extension_id) + if not isinstance(metadata, dict): + raise ExtensionError( + f"Extension '{extension_id}' not found in registry (corrupted state)" + ) + if not metadata.get("enabled", True): + return False + + native_hooks = projection_enabled(self.project_root) + previous_hook_states = [ + {"event": event, "command": hook["command"], "enabled": hook.get("enabled", True)} + for event, hooks in config.get("hooks", {}).items() + for hook in hooks + if hook.get("extension") == extension_id + ] if native_hooks else [] + generic = load_init_options(self.project_root).get("ai") == "generic" + if generic: + self.disable_generic_extension_artifacts(extension_id) + else: + self.registry.update(extension_id, {"enabled": False}) + + try: + if native_hooks: + self.registry.update(extension_id, {"disabled_hook_states": previous_hook_states}) + if "hooks" in config: + for hooks in config["hooks"].values(): + for hook in hooks: + if hook.get("extension") == extension_id: + hook["enabled"] = False + hook_executor.save_project_config(config) + except (ValueError, OSError, ExtensionError) as exc: + try: + if generic: + self.restore_generic_extension_after_failed_disable( + extension_id, metadata + ) + if native_hooks: + self.registry.restore(extension_id, metadata) + elif not generic: + self.registry.update(extension_id, {"enabled": True}) + except (ExtensionError, ValueError, OSError) as rollback_exc: + raise ExtensionError( + f"{exc}; rollback failed: {rollback_exc}" + ) from exc + raise + return True + + def enable(self, extension_id: str) -> bool: + """Enable an installed extension, its generic invocations, and prior hooks. + + Returns False when the extension is already enabled. + """ + from .. import load_init_options + from .._init_options import is_ai_skills_enabled + from ._hook_projection import projection_enabled + + hook_executor = HookExecutor(self.project_root) + config = hook_executor.load_project_config_for_update() + metadata = self.registry.get(extension_id) + if not isinstance(metadata, dict): + raise ExtensionError( + f"Extension '{extension_id}' not found in registry (corrupted state)" + ) + if metadata.get("enabled", True): + return False + + native_hooks = projection_enabled(self.project_root) + if native_hooks: + previous = metadata.get("disabled_hook_states") + if previous is None: + manifest = self.get_extension(extension_id) + if manifest is None: + raise ExtensionError(f"Cannot read manifest for '{extension_id}'") + previous = [ + {"event": event, "command": entry["command"], "enabled": entry.get("enabled", True)} + for event, entries in manifest.hooks.items() + for entry in coerce_hook_entries(entries) + if isinstance(entry, dict) and entry.get("command") + ] + owned = [ + (event, hook) + for event, hooks in config.get("hooks", {}).items() + for hook in hooks + if hook.get("extension") == extension_id + ] + if ( + not isinstance(previous, list) + or len(previous) != len(owned) + or any( + not isinstance(state, dict) + or state.get("event") != event + or state.get("command") != hook.get("command") + or not isinstance(state.get("enabled"), bool) + for state, (event, hook) in zip(previous, owned) + ) + ): + raise ExtensionError( + f"Cannot enable '{extension_id}': owned hooks changed since disable; " + "review the hook configuration before enabling" + ) + + init_options = load_init_options(self.project_root) + generic = init_options.get("ai") == "generic" + self.registry.update(extension_id, {"enabled": True}) + if generic: + try: + manifest = self.get_extension(extension_id) + if manifest is None: + raise ExtensionError(f"Cannot read manifest for '{extension_id}'") + if manifest.commands: + self.register_enabled_extensions_for_agent("generic") + refreshed = self.registry.get(extension_id) or {} + skills = is_ai_skills_enabled(init_options) + expected = ( + {self._skill_name_for_command(command["name"]) for command in manifest.commands} + if skills else set(self._collect_manifest_command_names(manifest)) + ) + owned_names = set(self._generic_owned_names( + refreshed, list(expected), skills=skills, extension_id=extension_id, + )) + missing = expected - owned_names + if missing: + self.disable_generic_extension_artifacts(extension_id) + raise ExtensionError( + "Missing invocation artifacts: " + ", ".join(sorted(missing)) + ) + except (ExtensionError, OSError, ValueError) as exc: + try: + self.registry.update(extension_id, {"enabled": False}) + except (ExtensionError, OSError, ValueError) as rollback_exc: + raise ExtensionError( + f"Could not register generic invocations for '{extension_id}': " + f"{exc}; rollback failed: {rollback_exc}" + ) from exc + raise ExtensionError( + f"Could not register generic invocations for '{extension_id}': {exc}" + ) from exc + + if "hooks" in config: + if native_hooks: + for state, (_, hook) in zip(previous, owned): + hook["enabled"] = state["enabled"] + else: + for hooks in config["hooks"].values(): + for hook in hooks: + if hook.get("extension") == extension_id: + hook["enabled"] = True + try: + hook_executor.save_project_config(config) + except (ValueError, OSError) as exc: + try: + if generic: + self.disable_generic_extension_artifacts(extension_id) + if native_hooks: + self.registry.restore(extension_id, metadata) + elif not generic: + self.registry.update(extension_id, {"enabled": False}) + except (ExtensionError, ValueError, OSError) as rollback_exc: + raise ExtensionError( + f"{exc}; rollback failed: {rollback_exc}" + ) from exc + raise + if native_hooks: + updated = self.registry.get(extension_id) or {} + if "disabled_hook_states" in updated: + updated.pop("disabled_hook_states") + self.registry.restore(extension_id, updated) + return True + + def restore_generic_extension_after_failed_disable( + self, extension_id: str, previous: Dict[str, Any] + ) -> None: + """Restore generic invocations and verify the registry and owned files.""" + self.registry.update(extension_id, {"enabled": True}) + self.register_enabled_extensions_for_agent("generic") + restored = self.registry.get(extension_id) or {} + if restored.get("enabled") is not True: + raise ExtensionError("Rollback did not restore enabled registry state") + previous_commands = previous.get("registered_commands", {}) + commands = ( + previous_commands.get("generic", []) + if isinstance(previous_commands, dict) else [] + ) + for names, skills in ( + (commands, False), (previous.get("registered_skills", []), True) + ): + if not isinstance(names, list) or any( + not isinstance(name, str) for name in names + ): + raise ExtensionError("Invalid generic artifact metadata during rollback") + restored_commands = restored.get("registered_commands", {}) + registered = ( + restored.get("registered_skills", []) + if skills else ( + restored_commands.get("generic", []) + if isinstance(restored_commands, dict) else None + ) + ) + if ( + not isinstance(registered, list) + or any(not isinstance(name, str) for name in registered) + or set(names) - set(registered) + ): + raise ExtensionError("Rollback did not restore generic artifact registry") + owned = self._generic_owned_names( + restored, names, skills=skills, extension_id=extension_id + ) + missing = set(names) - set(owned) + if missing: + raise ExtensionError( + "Rollback did not restore generic invocation artifacts: " + + ", ".join(sorted(missing)) + ) + def disable_generic_extension_artifacts(self, extension_id: str) -> None: """Disable and retire generic invocations without removing sources.""" metadata = self.registry.get(extension_id) @@ -5759,24 +6023,50 @@ def save_project_config(self, config: Dict[str, Any]): Args: config: Configuration dictionary to save """ - from ..integrations._file_changes import write_text + from ._hook_projection import projection_enabled, publish, retire - self.config_file.parent.mkdir(parents=True, exist_ok=True) - write_text( - self.config_file, - yaml.dump( - config, default_flow_style=False, sort_keys=False, allow_unicode=True - ), - encoding="utf-8", + if projection_enabled(self.project_root): + publish(self.config_file, config) + else: + from ..integrations._file_changes import write_text + + retire(self.config_file) + self.config_file.parent.mkdir(parents=True, exist_ok=True) + write_text( + self.config_file, + yaml.dump( + config, default_flow_style=False, sort_keys=False, allow_unicode=True + ), + encoding="utf-8", + ) + + def load_project_config_for_update(self) -> Dict[str, Any]: + from ._hook_projection import load_for_update, projection_enabled + + return ( + load_for_update(self.config_file) + if projection_enabled(self.project_root) else self.get_project_config() ) + def migrate_project_config(self) -> None: + """Refresh the projection of an existing configuration without rewriting it.""" + from ._hook_projection import projection_enabled, publish, retire + + if projection_enabled(self.project_root) and ( + self.config_file.exists() or self.config_file.is_symlink() + ): + config = self.load_project_config_for_update() + publish(self.config_file, config, preserve_source=True) + else: + retire(self.config_file) + def register_extension(self, extension_id: str): """Add extension to the installed list in project config. Args: extension_id: ID of extension to register """ - config = self.get_project_config() + config = self.load_project_config_for_update() # Ensure config is a dict (defensive) if not isinstance(config, dict): @@ -5795,7 +6085,7 @@ def unregister_extension(self, extension_id: str): Args: extension_id: ID of extension to unregister """ - config = self.get_project_config() + config = self.load_project_config_for_update() if not isinstance(config, dict): config = {} @@ -5869,16 +6159,23 @@ def register_hooks(self, manifest: ExtensionManifest): Args: manifest: Extension manifest with hooks to register """ - # Always ensure the extension is in the installed list - self.register_extension(manifest.id) + from ._hook_projection import projection_enabled - config = self.get_project_config() + native_hooks = projection_enabled(self.project_root) + if not native_hooks: + self.register_extension(manifest.id) + config = self.load_project_config_for_update() # Ensure config is a dict (defensive) changed = False if not isinstance(config, dict): config = {} changed = True + if native_hooks: + installed = self._sanitize_installed_list(config.get("installed"), add_id=manifest.id) + if installed != config.get("installed"): + config["installed"] = installed + changed = True # Ensure hooks dict exists and is a mapping if "hooks" not in config or not isinstance(config["hooks"], dict): @@ -5934,7 +6231,7 @@ def register_hooks(self, manifest: ExtensionManifest): new_entries[command] = { "extension": manifest.id, "command": command, - "enabled": True, + "enabled": entry.get("enabled", True) if native_hooks else True, "optional": entry.get("optional", True), "priority": normalize_priority( entry.get("priority"), DEFAULT_HOOK_PRIORITY @@ -5972,18 +6269,25 @@ def unregister_hooks(self, extension_id: str): Args: extension_id: ID of extension to unregister """ - # Always remove from installed list (Feedback from review) - self.unregister_extension(extension_id) + from ._hook_projection import projection_enabled - config = self.get_project_config() + native_hooks = projection_enabled(self.project_root) + if not native_hooks: + self.unregister_extension(extension_id) + config = self.load_project_config_for_update() if not isinstance(config, dict): config = {} - # We don't save yet, as there are no hooks to unregister, - # but unregister_extension above might have already saved a normalized config. - return + installed_changed = False + if native_hooks: + installed = self._sanitize_installed_list(config.get("installed"), remove_id=extension_id) + installed_changed = installed != config.get("installed") + if installed_changed: + config["installed"] = installed if "hooks" not in config or not isinstance(config["hooks"], dict): + if installed_changed: + self.save_project_config(config) return # Remove hooks for this extension @@ -6251,7 +6555,7 @@ def enable_hooks(self, extension_id: str): Args: extension_id: Extension ID """ - config = self.get_project_config() + config = self.load_project_config_for_update() if "hooks" not in config: return @@ -6270,7 +6574,7 @@ def disable_hooks(self, extension_id: str): Args: extension_id: Extension ID """ - config = self.get_project_config() + config = self.load_project_config_for_update() if "hooks" not in config: return diff --git a/src/specify_cli/extensions/_command_update_transaction.py b/src/specify_cli/extensions/_command_update_transaction.py index 81c7199d55..9dfea05297 100644 --- a/src/specify_cli/extensions/_command_update_transaction.py +++ b/src/specify_cli/extensions/_command_update_transaction.py @@ -366,7 +366,7 @@ def backup_extension_skills(skill_names, *, skills_dir=None): # 4. Backup hooks and installed list from extensions.yml # get_project_config() always normalizes installed->[] and hooks->{}, # so no sentinel is needed to distinguish key-absent from key-empty. - config = hook_executor.get_project_config() + config = hook_executor.load_project_config_for_update() if isinstance(config, dict): import copy # Deep-copy so nested mapping entries (e.g. version-pin dicts) diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py new file mode 100644 index 0000000000..251494e4af --- /dev/null +++ b/src/specify_cli/extensions/_hook_projection.py @@ -0,0 +1,183 @@ +"""Validated projection of extension hooks for future dispatchers.""" + +from __future__ import annotations + +import hashlib +import json +import re +import shutil +from pathlib import Path +from typing import Any + +import yaml + +from .._init_options import load_init_options +from ..integrations._file_changes import changing_file, unlink, write_bytes +from . import DEFAULT_HOOK_PRIORITY, normalize_priority + +_EVENT = re.compile(r"(before|after)_[a-z0-9][a-z0-9_]*\Z") + + +def projection_enabled(project_root: Path) -> bool: + options = load_init_options(project_root) + enabled = options.get("native_extension_hooks", False) + if not isinstance(enabled, bool): + raise ValueError( + "Invalid native_extension_hooks in .specify/init-options.json: " + "expected true or false" + ) + return enabled + + +def validate_event_name(event: object) -> None: + if ( + not isinstance(event, str) + or not _EVENT.fullmatch(event) + or len(event) + len(".json.sha256") > 255 + ): + raise ValueError( + f"Invalid extension hook event {event!r}: use before_ " + "or after_ with a lowercase command name containing " + "only letters, digits, or underscores. Normalize hyphens to underscores " + "(for example, my-extension -> my_extension). Rename legacy events such " + "as custom:after before installing or refreshing." + ) + + +def _cache_path(config_file: Path) -> Path: + return config_file.parent / "hook-dispatch" + + +def _check_paths(config_file: Path) -> None: + root = config_file.parent + cache = _cache_path(config_file) + if root.is_symlink() or config_file.is_symlink(): + raise ValueError("Refusing symlinked extension configuration") + if cache.is_symlink() or (cache.exists() and not cache.is_dir()): + raise ValueError("Refusing non-directory or symlinked hook projection") + if cache.exists(): + for child in cache.iterdir(): + if child.is_symlink() or not child.is_file(): + raise ValueError(f"Refusing non-file hook projection entry: {child.name}") + + +def project_hooks(config: dict[str, Any]) -> dict[str, bytes]: + """Render unconditionally eligible hooks in YAML declaration order.""" + hooks = config.get("hooks", {}) + if not isinstance(hooks, dict): + raise ValueError("Invalid extension hooks: expected a mapping") + projected: dict[str, bytes] = {} + for event, entries in hooks.items(): + validate_event_name(event) + if not isinstance(entries, list): + raise ValueError(f"Invalid extension hooks.{event}: expected a list") + selected = [] + for index, entry in enumerate(entries): + label = f"hooks.{event}[{index}]" + if not isinstance(entry, dict): + raise ValueError(f"Invalid {label}: expected a mapping") + for field in ("extension", "command"): + if not isinstance(entry.get(field), str) or not entry[field]: + raise ValueError(f"Invalid {label}: missing {field}") + for field in ("enabled", "optional"): + if field in entry and not isinstance(entry[field], bool): + raise ValueError(f"Invalid {label}.{field}: expected a boolean") + for field in ("condition", "description", "prompt"): + if entry.get(field) is not None and not isinstance(entry[field], str): + raise ValueError(f"Invalid {label}.{field}: expected a string or null") + if any(isinstance(value, str) and "\0" in value for value in entry.values()): + raise ValueError(f"Invalid {label}: NUL character") + if entry.get("enabled", True) is False or entry.get("condition"): + continue + priority = normalize_priority(entry.get("priority")) + if priority > 2147483647: + priority = DEFAULT_HOOK_PRIORITY + selected.append({ + "extension": entry["extension"], + "command": entry["command"], + "optional": entry.get("optional", True), + "description": entry.get("description") or "", + "prompt": entry.get("prompt") or "", + "priority": priority, + }) + projected[event] = ( + json.dumps({"event": event, "hooks": selected}, ensure_ascii=True, separators=(",", ":")) + "\n" + ).encode("utf-8") + return projected + + +def load_for_update(config_file: Path) -> dict[str, Any]: + """Reject malformed existing state before extension lifecycle mutations.""" + _check_paths(config_file) + if not config_file.exists(): + return {"installed": [], "settings": {"auto_execute_hooks": True}, "hooks": {}} + try: + config = yaml.safe_load(config_file.read_text(encoding="utf-8")) + except (OSError, UnicodeError, yaml.YAMLError) as exc: + raise ValueError(f"Invalid extension configuration: {exc}") from exc + if not isinstance(config, dict): + raise ValueError("Invalid extension configuration: expected a mapping") + project_hooks(config) + return config + + +def retire(config_file: Path) -> None: + """Retire a prior projection when native hooks are no longer enabled.""" + cache = _cache_path(config_file) + if not cache.exists() and not cache.is_symlink(): + return + _check_paths(config_file) + if cache.exists(): + with changing_file(cache, removal=True): + shutil.rmtree(cache) + + +def _write_digest(directory: Path, name: str, payload: bytes) -> None: + write_bytes(directory / name, payload) + write_bytes( + directory / f"{name}.sha256", + (hashlib.sha256(payload).hexdigest() + "\n").encode("ascii"), + ) + + +def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool = False) -> None: + """Write the validated hook configuration and projection directly.""" + if not preserve_source: + projected = project_hooks(config) + rendered = yaml.dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True).encode("utf-8") + root = config_file.parent + _check_paths(config_file) + if preserve_source: + source = config_file.read_bytes() + try: + current = yaml.safe_load(source.decode("utf-8")) + except (UnicodeError, yaml.YAMLError) as exc: + raise ValueError(f"Invalid extension configuration: {exc}") from exc + if not isinstance(current, dict): + raise ValueError("Invalid extension configuration: expected a mapping") + projected = project_hooks(current) + else: + source = rendered + cache = _cache_path(config_file) + try: + root.mkdir(parents=True, exist_ok=True) + if not preserve_source: + write_bytes(config_file, rendered) + cache.mkdir(exist_ok=True) + retained = {"source.yml", "events.txt", "events.txt.sha256"} + for event, payload in projected.items(): + name = f"{event}.json" + _write_digest(cache, name, payload) + retained.update((name, f"{name}.sha256")) + _write_digest(cache, "events.txt", "".join(f"{event}\n" for event in projected).encode("utf-8")) + write_bytes(cache / "source.yml", source) + for child in cache.iterdir(): + if child.name not in retained and child.name.endswith((".json", ".sha256")): + unlink(child) + except OSError as exc: + raise OSError( + f"Hook projection write failed ({exc}); .specify/extensions.yml or " + ".specify/hook-dispatch/ may be incomplete. Repair the filesystem and " + "extensions.yml if needed, then retry the extension operation or run " + "`specify integration switch --refresh-shared-infra`." + ) from exc diff --git a/src/specify_cli/extensions/command_add.py b/src/specify_cli/extensions/command_add.py index 30a4ff717c..14d6150d19 100644 --- a/src/specify_cli/extensions/command_add.py +++ b/src/specify_cli/extensions/command_add.py @@ -357,3 +357,6 @@ def extension_add( except ExtensionError as e: console.print(f"\n[red]Error:[/red] {_escape_markup(str(e))}") raise typer.Exit(1) + except (ValueError, OSError) as e: + console.print(f"\n[red]Error:[/red] {_escape_markup(str(e))}") + raise typer.Exit(1) from e diff --git a/src/specify_cli/extensions/command_disable.py b/src/specify_cli/extensions/command_disable.py index 16ef6683e1..8213bf5e2d 100644 --- a/src/specify_cli/extensions/command_disable.py +++ b/src/specify_cli/extensions/command_disable.py @@ -17,11 +17,10 @@ def extension_disable( extension: str = typer.Argument(help="Extension ID or name to disable"), ): """Disable an extension without removing it.""" - from . import ExtensionManager, HookExecutor + from . import ExtensionError, ExtensionManager project_root = _commands._require_specify_project() manager = ExtensionManager(project_root) - hook_executor = HookExecutor(project_root) # Resolve extension ID from argument (handles ambiguous names) installed = manager.list_installed() @@ -29,41 +28,15 @@ def extension_disable( extension, installed, "disable" ) - # Update registry - metadata = manager.registry.get(extension_id) - if metadata is None or not isinstance(metadata, dict): - console.print( - f"[red]Error:[/red] Extension '{_escape_markup(str(extension_id))}' " - "not found in registry (corrupted state)" - ) - raise typer.Exit(1) - - if not metadata.get("enabled", True): + try: + disabled = manager.disable(extension_id) + except (ExtensionError, ValueError, OSError) as exc: + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc + if not disabled: 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 - else: - manager.registry.update(extension_id, {"enabled": False}) - - # Disable hooks in extensions.yml - config = hook_executor.get_project_config() - if "hooks" in config: - for hook_name in config["hooks"]: - for hook in config["hooks"][hook_name]: - if hook.get("extension") == extension_id: - hook["enabled"] = False - hook_executor.save_project_config(config) - console.print(f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' disabled") console.print("\nCommands will no longer be available. Hooks will not execute.") console.print(f"To re-enable: specify extension enable {_escape_markup(str(extension_id))}") diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index 34bab2f279..036a9292f4 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -17,11 +17,10 @@ def extension_enable( extension: str = typer.Argument(help="Extension ID or name to enable"), ): """Enable a disabled extension.""" - from . import ExtensionError, ExtensionManager, HookExecutor + from . import ExtensionError, ExtensionManager project_root = _commands._require_specify_project() manager = ExtensionManager(project_root) - hook_executor = HookExecutor(project_root) # Resolve extension ID from argument (handles ambiguous names) installed = manager.list_installed() @@ -29,68 +28,15 @@ def extension_enable( extension, installed, "enable" ) - # Update registry - metadata = manager.registry.get(extension_id) - if metadata is None or not isinstance(metadata, dict): - console.print( - f"[red]Error:[/red] Extension '{_escape_markup(str(extension_id))}' " - "not found in registry (corrupted state)" - ) - raise typer.Exit(1) - - if metadata.get("enabled", True): + try: + enabled = manager.enable(extension_id) + except (ExtensionError, ValueError, OSError) as exc: + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc + if not enabled: console.print(f"[yellow]Extension '{_escape_markup(str(display_name))}' is already enabled[/yellow]") raise typer.Exit(0) - manager.registry.update(extension_id, {"enabled": True}) - - from .. import load_init_options - - init_options = load_init_options(project_root) - if init_options.get("ai") == "generic": - try: - 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) - 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, extension_id=extension_id, - )) - 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( - 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 - - # Enable hooks in extensions.yml - config = hook_executor.get_project_config() - if "hooks" in config: - for hook_name in config["hooks"]: - for hook in config["hooks"][hook_name]: - if hook.get("extension") == extension_id: - hook["enabled"] = True - hook_executor.save_project_config(config) - console.print(f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' enabled") # #1: regenerate native event config so the enabled extension's events diff --git a/src/specify_cli/extensions/command_remove.py b/src/specify_cli/extensions/command_remove.py index c210dae96b..b3fdc4c8be 100644 --- a/src/specify_cli/extensions/command_remove.py +++ b/src/specify_cli/extensions/command_remove.py @@ -19,7 +19,7 @@ def extension_remove( force: bool = typer.Option(False, "--force", help="Skip confirmation"), ): """Uninstall an extension.""" - from . import ExtensionManager + from . import ExtensionError, ExtensionManager project_root = _commands._require_specify_project() manager = ExtensionManager(project_root) @@ -68,7 +68,11 @@ def extension_remove( raise typer.Exit(0) # Remove extension - success = manager.remove(extension_id, keep_config=keep_config) + try: + success = manager.remove(extension_id, keep_config=keep_config) + except (ExtensionError, ValueError, OSError) as exc: + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc if success: console.print(f"\n[green]✓[/green] Extension '{_escape_markup(str(display_name))}' removed successfully") diff --git a/tests/specify_cli/extensions/_helpers.py b/tests/specify_cli/extensions/_helpers.py index ef1bd845d2..8b47b76d99 100644 --- a/tests/specify_cli/extensions/_helpers.py +++ b/tests/specify_cli/extensions/_helpers.py @@ -4,10 +4,20 @@ import os from pathlib import Path +import yaml + MINIMAL_ZIP_BYTES = b"PK\x05\x06" + b"\x00" * 18 +def enable_native_hooks(project: Path) -> None: + from specify_cli import load_init_options, save_init_options + + save_init_options( + project, {**load_init_options(project), "native_extension_hooks": True} + ) + + def open_test_download_zip(project_root, download_dir, zip_filename): """Create a transient download file using platform-appropriate semantics.""" target = download_dir / zip_filename @@ -44,3 +54,38 @@ def can_create_symlink(tmp_path: Path) -> bool: except OSError: return False return link.is_symlink() + + +def installed_generic_hook_extension( + tmp_path: Path, extension_dir: Path, *, native_hooks: bool = False +): + """Install a hook-bearing extension with a real generic invocation artifact.""" + from specify_cli import save_init_options + from specify_cli.extensions import ExtensionManager + from specify_cli.integration_state import write_integration_json + from specify_cli.integrations import get_integration + from specify_cli.integrations.manifest import IntegrationManifest + + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = {"before_plan": {"command": "speckit.test-ext.hello"}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + project = tmp_path / "generic-project" + project.mkdir() + options = {"commands_dir": ".custom/commands", "skills": False} + installation = IntegrationManifest("generic", project) + get_integration("generic").setup(project, installation, parsed_options=options) + installation.save() + write_integration_json( + project, version="1.0.0", integration_key="generic", + settings={"generic": {"parsed_options": options}}, + ) + save_init_options(project, { + "ai": "generic", "ai_skills": False, "script": "sh", + "native_extension_hooks": native_hooks, + }) + manager = ExtensionManager(project) + manager.install_from_directory(extension_dir, "1.0.0") + artifact = project / ".custom/commands/speckit.test-ext.hello.md" + assert artifact.is_file() + return project, manager, artifact diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index cddcd6bf21..b088806eb6 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -24,12 +24,14 @@ ExtensionError, ExtensionManager, ExtensionRegistry, + HookExecutor, ValidationError, ) from tests.conftest import strip_ansi from tests.specify_cli.extensions._helpers import ( MINIMAL_ZIP_BYTES as _MINIMAL_ZIP_BYTES, can_create_symlink, + enable_native_hooks, open_test_download_zip as _open_test_download_zip, validate_safe_cache_dir as _validate_safe_cache_dir_test_stand_in, ) @@ -38,6 +40,257 @@ class TestExtensionAddCLI: """CLI tests for ``specify extension add``.""" + def test_add_publishes_hooks_without_changing_command_content(self, extension_dir, project_dir): + enable_native_hooks(project_dir) + from specify_cli import load_init_options, save_init_options + + save_init_options(project_dir, {**load_init_options(project_dir), "ai": "copilot"}) + manifest = extension_dir / "extension.yml" + payload = yaml.safe_load(manifest.read_text(encoding="utf-8")) + payload["hooks"] = { + "before_plan": [ + {"command": "speckit.test-ext.hello", "optional": False, "priority": 30}, + {"command": "speckit.test-ext.skip", "condition": "env.DISABLED is set"}, + ], + "before_bug_assess": {"command": "speckit.test-ext.hello"}, + } + manifest.write_text(yaml.safe_dump(payload), encoding="utf-8") + (project_dir / ".github" / "agents").mkdir(parents=True) + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["extension", "add", str(extension_dir), "--dev"], catch_exceptions=True + ) + assert result.exit_code == 0, result.output + cache = project_dir / ".specify/hook-dispatch" + assert (cache / "source.yml").read_bytes() == ( + project_dir / ".specify/extensions.yml" + ).read_bytes() + hooks = json.loads((cache / "before_plan.json").read_text(encoding="utf-8"))["hooks"] + assert len(hooks) == 1 + assert hooks[0]["command"] == "speckit.test-ext.hello" + assert hooks[0]["optional"] is False + assert json.loads((cache / "before_bug_assess.json").read_text())["event"] == ( + "before_bug_assess" + ) + rendered = (project_dir / ".github/agents/speckit.test-ext.hello.agent.md").read_text() + assert "hook-dispatch" not in rendered + + @pytest.mark.parametrize("native_hooks", [False, True]) + def test_add_disabled_manifest_hook_respects_opt_in( + self, extension_dir, project_dir, native_hooks + ): + if native_hooks: + enable_native_hooks(project_dir) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = { + "before_plan": { + "command": "speckit.test-ext.hello", "enabled": False, + } + } + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["extension", "add", str(extension_dir), "--dev"] + ) + assert result.exit_code == 0, result.output + config = yaml.safe_load( + (project_dir / ".specify/extensions.yml").read_text(encoding="utf-8") + ) + assert config["hooks"]["before_plan"][0]["enabled"] is not native_hooks + cache = project_dir / ".specify/hook-dispatch" + if native_hooks: + assert json.loads((cache / "before_plan.json").read_text())["hooks"] == [] + else: + assert not cache.exists() + + def test_install_publication_failure_keeps_preexisting_same_id_hooks( + self, extension_dir, project_dir, monkeypatch + ): + enable_native_hooks(project_dir) + config = project_dir / ".specify/extensions.yml" + config.parent.mkdir(parents=True, exist_ok=True) + config.write_bytes( + b"# existing project hooks\r\ninstalled:\r\n - test-ext\r\n" + b"hooks:\r\n before_plan:\r\n" + b" - extension: test-ext\r\n" + b" command: speckit.test-ext.hello\r\n" + b" enabled: true\r\n" + ) + HookExecutor(project_dir).migrate_project_config() + cache = project_dir / ".specify/hook-dispatch" + original = config.read_bytes() + prior_cache = {entry.name: entry.read_bytes() for entry in cache.iterdir()} + real_write = Path.write_bytes + + def fail_config_write(path, content): + if path == config: + raise OSError("simulated hook publication failure") + return real_write(path, content) + + monkeypatch.setattr(Path, "write_bytes", fail_config_write) + manager = ExtensionManager(project_dir) + with pytest.raises(OSError, match="simulated hook publication failure.*refresh-shared-infra"): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + assert not manager.registry.is_installed("test-ext") + assert config.read_bytes() == original + assert {entry.name: entry.read_bytes() for entry in cache.iterdir()} == prior_cache + + @pytest.mark.parametrize("native_hooks", [False, True]) + def test_install_replaces_preexisting_same_id_hooks_on_success( + self, extension_dir, project_dir, native_hooks + ): + if native_hooks: + enable_native_hooks(project_dir) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = { + "before_plan": {"command": "speckit.test-ext.hello"} + } + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + config = project_dir / ".specify/extensions.yml" + config.parent.mkdir(parents=True, exist_ok=True) + config.write_text( + "installed:\n - test-ext\nhooks:\n before_plan:\n" + " - extension: test-ext\n command: speckit.test-ext.old\n", + encoding="utf-8", + ) + manager = ExtensionManager(project_dir) + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + stored = yaml.safe_load(config.read_text(encoding="utf-8")) + assert stored["installed"] == ["test-ext"] + assert [entry["command"] for entry in stored["hooks"]["before_plan"]] == [ + "speckit.test-ext.hello" + ] + cache = project_dir / ".specify/hook-dispatch" + if native_hooks: + assert cache.is_dir() + assert json.loads((cache / "before_plan.json").read_text())["hooks"][0]["command"] == ( + "speckit.test-ext.hello" + ) + else: + assert not cache.exists() + + def test_install_publication_failure_reports_error_without_unregistration( + self, extension_dir, project_dir + ): + enable_native_hooks(project_dir) + manager = ExtensionManager(project_dir) + with patch.object(HookExecutor, "register_hooks", side_effect=OSError("publication failed")), \ + patch.object(HookExecutor, "unregister_hooks", side_effect=AssertionError("must not unregister")): + with pytest.raises(OSError, match="publication failed"): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + assert not manager.registry.is_installed("test-ext") + + def test_add_rejects_invalid_existing_hooks_before_install(self, extension_dir, project_dir): + enable_native_hooks(project_dir) + config = project_dir / ".specify/extensions.yml" + config.write_text("hooks:\n before_plan: invalid\n", encoding="utf-8") + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["extension", "add", str(extension_dir), "--dev"], catch_exceptions=True + ) + assert result.exit_code == 1 + assert "expected a list" in result.output + assert config.read_text() == "hooks:\n before_plan: invalid\n" + assert not (project_dir / ".specify/extensions/test-ext").exists() + + def test_add_rolls_back_non_generic_artifacts_on_publication_failure( + self, extension_dir, project_dir + ): + enable_native_hooks(project_dir) + from specify_cli import load_init_options, save_init_options + + save_init_options(project_dir, {**load_init_options(project_dir), "ai": "copilot"}) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = {"before_plan": {"command": "speckit.test-ext.hello"}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + (project_dir / ".github/agents").mkdir(parents=True) + original_save = HookExecutor.save_project_config + + def fail_hook_save(executor, config): + if config.get("hooks", {}).get("before_plan"): + raise OSError("projection failed") + return original_save(executor, config) + + with patch.object(Path, "cwd", return_value=project_dir), \ + patch.object(HookExecutor, "save_project_config", fail_hook_save): + result = CliRunner().invoke( + app, ["extension", "add", str(extension_dir), "--dev"] + ) + assert result.exit_code == 1 + assert "projection failed" in result.output + assert not ExtensionManager(project_dir).registry.is_installed("test-ext") + assert not (project_dir / ".specify/extensions/test-ext").exists() + assert not (project_dir / ".github/agents/speckit.test-ext.hello.agent.md").exists() + assert not (project_dir / ".specify/extensions.yml").exists() + + @pytest.mark.parametrize("event", ["custom:after", "before_my-extension_assess"]) + def test_default_add_accepts_legacy_hook_event_without_projection( + self, extension_dir, project_dir, event + ): + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = {event: {"command": "speckit.test-ext.hello"}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["extension", "add", str(extension_dir), "--dev"] + ) + assert result.exit_code == 0, result.output + assert ExtensionManager(project_dir).registry.is_installed("test-ext") + assert (project_dir / ".specify/extensions/test-ext").exists() + assert not (project_dir / ".specify/hook-dispatch").exists() + + @pytest.mark.parametrize("event", ["custom:after", "before_my-extension_assess"]) + def test_opted_in_add_rejects_legacy_hook_event_before_mutation( + self, extension_dir, project_dir, event + ): + enable_native_hooks(project_dir) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = {event: {"command": "speckit.test-ext.hello"}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["extension", "add", str(extension_dir), "--dev"] + ) + assert result.exit_code == 1 + assert "Rename legacy events" in result.output + if "-" in event: + assert "my-extension -> my_extension" in result.output.replace("\n", " ") + assert not ExtensionManager(project_dir).registry.is_installed("test-ext") + assert not (project_dir / ".specify/extensions/test-ext").exists() + + @pytest.mark.parametrize("field,value", [ + ("command", 17), ("optional", "yes"), ("enabled", "yes"), + ("condition", 42), ("description", False), ("prompt", []), + ]) + def test_add_rejects_invalid_manifest_hook_before_mutation( + self, extension_dir, project_dir, field, value + ): + enable_native_hooks(project_dir) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = {"before_plan": {"command": "speckit.test-ext.hello", field: value}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke( + app, ["extension", "add", str(extension_dir), "--dev"], catch_exceptions=True + ) + assert result.exit_code == 1 + assert "hook" in result.output.lower() + assert not (project_dir / ".specify/extensions/test-ext").exists() + assert not (project_dir / ".specify/extensions.yml").exists() + assert not (project_dir / ".specify/hook-dispatch").exists() + def test_add_dev_links_copilot_agent_when_supported( self, extension_dir, project_dir, temp_dir ): diff --git a/tests/specify_cli/extensions/test_command_disable.py b/tests/specify_cli/extensions/test_command_disable.py index c790c3b184..9015a710b4 100644 --- a/tests/specify_cli/extensions/test_command_disable.py +++ b/tests/specify_cli/extensions/test_command_disable.py @@ -8,19 +8,38 @@ from pathlib import Path from unittest.mock import patch +import pytest +import yaml from typer.testing import CliRunner from specify_cli import app from specify_cli.extensions import ( + ExtensionError, ExtensionManager, ExtensionRegistry, HookExecutor, ) +from specify_cli.extensions import _commands +from tests.specify_cli.extensions._helpers import installed_generic_hook_extension class TestExtensionDisableCLI: """CLI tests for ``specify extension disable``.""" + def test_disable_cli_delegates_transaction_to_domain(self, tmp_path): + project = tmp_path / "project" + (project / ".specify").mkdir(parents=True) + installed = [{"id": "test-ext", "name": "Test Extension", "enabled": True}] + with patch.object(Path, "cwd", return_value=project), \ + patch.object(ExtensionManager, "list_installed", return_value=installed), \ + patch.object(ExtensionManager, "disable", return_value=True) as operation, \ + patch.object(HookExecutor, "load_project_config_for_update", side_effect=AssertionError("CLI loaded hook config")), \ + patch.object(ExtensionRegistry, "update", side_effect=AssertionError("CLI updated registry")), \ + patch.object(_commands, "_refresh_events_and_warn"): + result = CliRunner().invoke(app, ["extension", "disable", "test-ext"]) + assert result.exit_code == 0, result.output + operation.assert_called_once_with("test-ext") + def test_disable_reenable_hint_escapes_extension_id_markup(self, tmp_path): """Disable success hints should not parse extension IDs as markup.""" @@ -53,3 +72,123 @@ def test_disable_reenable_hint_escapes_extension_id_markup(self, tmp_path): assert result.exit_code == 0, result.output assert "specify extension enable [red]bad[/red]" in result.output + + def test_disable_publication_failure_restores_generic_registry_and_artifact( + self, tmp_path, extension_dir + ): + project, _, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + original_artifact = artifact.read_bytes() + config = project / ".specify/extensions.yml" + original_config = config.read_bytes() + with patch.object(Path, "cwd", return_value=project), \ + patch.object(HookExecutor, "save_project_config", side_effect=OSError("projection failed")): + result = CliRunner().invoke(app, ["extension", "disable", "test-ext"]) + assert result.exit_code == 1 + assert "projection failed" in result.output + assert ExtensionManager(project).registry.get("test-ext")["enabled"] is True + assert artifact.read_bytes() == original_artifact + assert config.read_bytes() == original_config + + def test_disable_reports_failed_generic_artifact_rollback(self, tmp_path, extension_dir): + project, _, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + with patch.object(Path, "cwd", return_value=project), \ + patch.object(HookExecutor, "save_project_config", side_effect=OSError("projection failed")), \ + patch.object(ExtensionManager, "register_enabled_extensions_for_agent"): + result = CliRunner().invoke(app, ["extension", "disable", "test-ext"]) + assert result.exit_code == 1 + assert "rollback failed" in result.output + assert "did not restore generic" in result.output + assert not artifact.exists() + + def test_generic_rollback_verifies_owned_artifacts_at_domain_boundary( + self, tmp_path, extension_dir + ): + project, _, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + manager = ExtensionManager(project) + previous = manager.registry.get("test-ext") + manager.disable_generic_extension_artifacts("test-ext") + assert not artifact.exists() + with patch.object(manager, "register_enabled_extensions_for_agent"): + with pytest.raises(ExtensionError, match="did not restore generic"): + manager.restore_generic_extension_after_failed_disable( + "test-ext", previous + ) + + @pytest.mark.parametrize("native_hooks", [False, True]) + def test_domain_disable_updates_hooks_and_preserves_opt_out( + self, tmp_path, extension_dir, native_hooks + ): + project, manager, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=native_hooks + ) + assert manager.disable("test-ext") is True + assert manager.disable("test-ext") is False + assert manager.registry.get("test-ext")["enabled"] is False + assert not artifact.exists() + config = project / ".specify/extensions.yml" + assert yaml.safe_load(config.read_text())["hooks"]["before_plan"][0]["enabled"] is False + cache = project / ".specify/hook-dispatch" + if native_hooks: + assert (cache / "source.yml").read_bytes() == config.read_bytes() + assert '"hooks":[]' in (cache / "before_plan.json").read_text() + else: + assert not cache.exists() + + @pytest.mark.parametrize("native_hooks", [False, True]) + def test_domain_disable_publication_failure_restores_previous_state( + self, tmp_path, extension_dir, native_hooks + ): + project, manager, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=native_hooks + ) + original_artifact = artifact.read_bytes() + config = project / ".specify/extensions.yml" + original_config = config.read_bytes() + with patch.object( + HookExecutor, "save_project_config", side_effect=OSError("save failed") + ): + with pytest.raises(OSError, match="save failed"): + manager.disable("test-ext") + assert manager.registry.get("test-ext")["enabled"] is True + assert artifact.read_bytes() == original_artifact + assert config.read_bytes() == original_config + + def test_domain_disable_non_generic_failure_restores_registry( + self, tmp_path, extension_dir + ): + from specify_cli import load_init_options, save_init_options + + project, manager, _ = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + save_init_options(project, {**load_init_options(project), "ai": "copilot"}) + config = project / ".specify/extensions.yml" + original = config.read_bytes() + with patch.object( + HookExecutor, "save_project_config", side_effect=OSError("save failed") + ): + with pytest.raises(OSError, match="save failed"): + manager.disable("test-ext") + assert manager.registry.get("test-ext")["enabled"] is True + assert config.read_bytes() == original + + def test_domain_disable_rejects_invalid_opted_in_hooks_before_mutation( + self, tmp_path, extension_dir + ): + project, manager, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + original_artifact = artifact.read_bytes() + config = project / ".specify/extensions.yml" + config.write_text("hooks:\n before_plan: invalid\n", encoding="utf-8") + with pytest.raises(ValueError, match="expected a list"): + manager.disable("test-ext") + assert manager.registry.get("test-ext")["enabled"] is True + assert artifact.read_bytes() == original_artifact + assert config.read_text() == "hooks:\n before_plan: invalid\n" diff --git a/tests/specify_cli/extensions/test_command_enable.py b/tests/specify_cli/extensions/test_command_enable.py index 3543917af0..3a99a10a5e 100644 --- a/tests/specify_cli/extensions/test_command_enable.py +++ b/tests/specify_cli/extensions/test_command_enable.py @@ -5,21 +5,82 @@ from __future__ import annotations +import json from pathlib import Path from unittest.mock import patch +import pytest from typer.testing import CliRunner +import yaml from specify_cli import app from specify_cli.extensions import ( + ExtensionError, ExtensionManager, ExtensionRegistry, + HookExecutor, ) +from specify_cli.extensions import _commands +from tests.specify_cli.extensions._helpers import enable_native_hooks, installed_generic_hook_extension class TestExtensionEnableCLI: """CLI tests for ``specify extension enable``.""" + def test_enable_cli_delegates_transaction_to_domain(self, tmp_path): + project = tmp_path / "project" + (project / ".specify").mkdir(parents=True) + installed = [{"id": "test-ext", "name": "Test Extension", "enabled": False}] + with patch.object(Path, "cwd", return_value=project), \ + patch.object(ExtensionManager, "list_installed", return_value=installed), \ + patch.object(ExtensionManager, "enable", return_value=True) as operation, \ + patch.object(HookExecutor, "load_project_config_for_update", side_effect=AssertionError("CLI loaded hook config")), \ + patch.object(ExtensionRegistry, "update", side_effect=AssertionError("CLI updated registry")), \ + patch.object(_commands, "_refresh_events_and_warn"), \ + patch.object(ExtensionManager, "scaffold_config", return_value=([], [], [])): + result = CliRunner().invoke(app, ["extension", "enable", "test-ext"]) + assert result.exit_code == 0, result.output + operation.assert_called_once_with("test-ext") + + @pytest.mark.parametrize("native_hooks", [False, True]) + def test_disable_enable_preserves_declaration_disabled_hook_only_when_opted_in( + self, project_dir, extension_dir, native_hooks + ): + if native_hooks: + enable_native_hooks(project_dir) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = { + "before_plan": [ + {"command": "speckit.test-ext.hello", "enabled": False}, + {"command": "speckit.test-ext.other", "enabled": True}, + ] + } + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + config_path = project_dir / ".specify/extensions.yml" + before = yaml.safe_load(config_path.read_text(encoding="utf-8"))["hooks"]["before_plan"] + assert [hook["enabled"] for hook in before] == ([False, True] if native_hooks else [True, True]) + + assert manager.disable("test-ext") is True + assert all( + not hook["enabled"] + for hook in yaml.safe_load(config_path.read_text(encoding="utf-8"))["hooks"]["before_plan"] + ) + assert manager.enable("test-ext") is True + assert manager.enable("test-ext") is False + after = yaml.safe_load(config_path.read_text(encoding="utf-8"))["hooks"]["before_plan"] + assert [hook["enabled"] for hook in after] == [hook["enabled"] for hook in before] + cache = project_dir / ".specify/hook-dispatch" + if native_hooks: + assert [ + hook["command"] + for hook in json.loads((cache / "before_plan.json").read_text())["hooks"] + ] == ["speckit.test-ext.other"] + else: + assert not cache.exists() + def test_enable_registry_error_escapes_extension_id_markup(self, tmp_path): """Registry-corruption errors should render extension IDs literally.""" @@ -50,3 +111,139 @@ def test_enable_registry_error_escapes_extension_id_markup(self, tmp_path): assert result.exit_code == 1, result.output assert "Extension '[red]bad[/red]' not found in registry" in result.output + + def test_enable_publication_failure_restores_generic_disabled_state( + self, tmp_path, extension_dir + ): + project, _, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + with patch.object(Path, "cwd", return_value=project): + disabled = CliRunner().invoke(app, ["extension", "disable", "test-ext"]) + assert disabled.exit_code == 0, disabled.output + assert not artifact.exists() + config = project / ".specify/extensions.yml" + original_config = config.read_bytes() + with patch.object(Path, "cwd", return_value=project), \ + patch.object(HookExecutor, "save_project_config", side_effect=OSError("projection failed")): + result = CliRunner().invoke(app, ["extension", "enable", "test-ext"]) + assert result.exit_code == 1 + assert "projection failed" in result.output + assert ExtensionManager(project).registry.get("test-ext")["enabled"] is False, result.output + assert not artifact.exists() + assert config.read_bytes() == original_config + + def test_domain_enable_rejects_changed_hook_identity_before_mutation( + self, tmp_path, extension_dir + ): + project, manager, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + assert manager.disable("test-ext") + metadata = manager.registry.get("test-ext") + metadata["disabled_hook_states"][0]["command"] = "speckit.test-ext.other" + manager.registry.restore("test-ext", metadata) + config = project / ".specify/extensions.yml" + original = config.read_bytes() + with pytest.raises(ExtensionError, match="owned hooks changed since disable"): + manager.enable("test-ext") + assert manager.registry.get("test-ext")["enabled"] is False + assert not artifact.exists() + assert config.read_bytes() == original + + def test_domain_enable_publication_failure_retains_disabled_hook_state( + self, tmp_path, extension_dir + ): + project, manager, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + assert manager.disable("test-ext") + previous = manager.registry.get("test-ext") + config = project / ".specify/extensions.yml" + original = config.read_bytes() + with patch.object(HookExecutor, "save_project_config", side_effect=OSError("save failed")): + with pytest.raises(OSError, match="save failed"): + manager.enable("test-ext") + assert manager.registry.get("test-ext") == previous + assert not artifact.exists() + assert config.read_bytes() == original + + def test_enable_older_disabled_extension_uses_manifest_declared_states( + self, project_dir, extension_dir + ): + enable_native_hooks(project_dir) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = { + "before_plan": [ + {"command": "speckit.test-ext.hello", "enabled": False}, + {"command": "speckit.test-ext.other", "enabled": True}, + ] + } + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + assert manager.disable("test-ext") + previous = manager.registry.get("test-ext") + previous.pop("disabled_hook_states") + manager.registry.restore("test-ext", previous) + assert manager.enable("test-ext") + config = yaml.safe_load((project_dir / ".specify/extensions.yml").read_text()) + assert [hook["enabled"] for hook in config["hooks"]["before_plan"]] == [False, True] + + def test_cli_enable_keeps_manifest_disabled_hook_out_of_projection( + self, project_dir, extension_dir + ): + enable_native_hooks(project_dir) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = { + "before_plan": [ + {"command": "speckit.test-ext.hello", "enabled": False}, + {"command": "speckit.test-ext.other", "enabled": True}, + ] + } + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + assert manager.disable("test-ext") + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke(app, ["extension", "enable", "test-ext"]) + assert result.exit_code == 0, result.output + config = yaml.safe_load((project_dir / ".specify/extensions.yml").read_text()) + assert [hook["enabled"] for hook in config["hooks"]["before_plan"]] == [False, True] + cache = project_dir / ".specify/hook-dispatch/before_plan.json" + assert [ + hook["command"] for hook in json.loads(cache.read_text())["hooks"] + ] == ["speckit.test-ext.other"] + + def test_opted_in_enable_restores_user_override_not_manifest_default( + self, project_dir, extension_dir + ): + enable_native_hooks(project_dir) + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = { + "before_plan": [ + {"command": "speckit.test-ext.hello", "enabled": False}, + {"command": "speckit.test-ext.other", "enabled": True}, + ] + } + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + executor = HookExecutor(project_dir) + config = executor.load_project_config_for_update() + config["hooks"]["before_plan"][0]["enabled"] = True + config["hooks"]["before_plan"][1]["enabled"] = False + executor.save_project_config(config) + + assert manager.disable("test-ext") + assert manager.enable("test-ext") + restored = yaml.safe_load(executor.config_file.read_text()) + assert [hook["enabled"] for hook in restored["hooks"]["before_plan"]] == [True, False] + assert [ + hook["command"] for hook in json.loads( + (project_dir / ".specify/hook-dispatch/before_plan.json").read_text() + )["hooks"] + ] == ["speckit.test-ext.hello"] diff --git a/tests/specify_cli/extensions/test_command_remove.py b/tests/specify_cli/extensions/test_command_remove.py index a7a340b401..4b0af828dc 100644 --- a/tests/specify_cli/extensions/test_command_remove.py +++ b/tests/specify_cli/extensions/test_command_remove.py @@ -5,16 +5,24 @@ from __future__ import annotations +import shutil from pathlib import Path from types import SimpleNamespace from unittest.mock import patch +import pytest from typer.testing import CliRunner +import yaml from specify_cli import app from specify_cli.extensions import ( ExtensionManager, ExtensionRegistry, + HookExecutor, +) +from tests.specify_cli.extensions._helpers import ( + enable_native_hooks, + installed_generic_hook_extension, ) @@ -27,6 +35,18 @@ def _install_ext(self, project_dir, ext_dir): manager.install_from_directory(ext_dir, "0.1.0", register_commands=False) return manager + def test_default_remove_legacy_hook_without_projection(self, project_dir, extension_dir): + manifest_path = extension_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["hooks"] = {"custom:after": {"command": "speckit.test-ext.hello"}} + manifest_path.write_text(yaml.safe_dump(manifest), encoding="utf-8") + self._install_ext(project_dir, extension_dir) + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) + assert result.exit_code == 0, result.output + assert not ExtensionManager(project_dir).registry.is_installed("test-ext") + assert not (project_dir / ".specify/hook-dispatch").exists() + def test_remove_confirmation_singular_command(self, tmp_path, extension_dir): """Confirmation prompt should say '1 command' (singular) when one command registered.""" @@ -100,3 +120,154 @@ def test_remove_output_escapes_extension_id_markup(self, tmp_path): assert result.exit_code == 0, result.output assert ".specify/extensions/.backup/[red]bad[/red]/" in result.output assert "specify extension add [red]bad[/red]" in result.output + + def test_remove_reports_invalid_hooks_without_mutating_installation( + self, project_dir, extension_dir + ): + enable_native_hooks(project_dir) + manager = self._install_ext(project_dir, extension_dir) + config = project_dir / ".specify/extensions.yml" + config.write_text("hooks:\n before_plan: invalid\n", encoding="utf-8") + with patch.object(Path, "cwd", return_value=project_dir): + result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) + assert result.exit_code == 1 + assert "expected a list" in result.output + assert manager.registry.is_installed("test-ext") + assert (project_dir / ".specify/extensions/test-ext").exists() + assert config.read_text(encoding="utf-8") == "hooks:\n before_plan: invalid\n" + + def test_remove_publication_failure_reports_partial_cleanup_and_recovery( + self, tmp_path, extension_dir + ): + project, _, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + manager = ExtensionManager(project) + extension_dir = project / ".specify/extensions/test-ext" + config = project / ".specify/extensions.yml" + cache = project / ".specify/hook-dispatch" + original_source = (cache / "source.yml").read_bytes() + real_write = Path.write_bytes + + def fail_index(path, content): + if path == cache / "events.txt": + raise OSError("projection failed") + return real_write(path, content) + + with patch.object(Path, "cwd", return_value=project), \ + patch.object(Path, "write_bytes", fail_index): + result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) + assert result.exit_code == 1 + assert "projection failed" in result.output + assert "refresh-shared-infra" in result.output + assert manager.registry.is_installed("test-ext") + assert not extension_dir.exists() + assert not artifact.exists() + assert config.read_bytes() != original_source + assert (cache / "source.yml").read_bytes() == original_source + + def test_remove_artifact_failure_leaves_published_hooks_untouched( + self, tmp_path, extension_dir + ): + project, _, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + manager = ExtensionManager(project) + source = project / ".specify/extensions/test-ext" + config = project / ".specify/extensions.yml" + cache = project / ".specify/hook-dispatch" + original = (artifact.read_bytes(), config.read_bytes(), (cache / "source.yml").read_bytes()) + with patch.object(Path, "cwd", return_value=project), \ + patch.object(ExtensionManager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed")): + result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) + assert result.exit_code == 1 + assert "cleanup failed" in result.output + assert manager.registry.is_installed("test-ext") + assert source.is_dir() + assert (artifact.read_bytes(), config.read_bytes(), (cache / "source.yml").read_bytes()) == original + assert yaml.safe_load(config.read_text())["hooks"]["before_plan"][0]["extension"] == "test-ext" + assert (cache / "before_plan.json").is_file() + + def test_remove_partial_cleanup_leaves_hooks_untouched_not_artifacts( + self, tmp_path, extension_dir + ): + project, _, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + manager = ExtensionManager(project) + config = project / ".specify/extensions.yml" + cache = project / ".specify/hook-dispatch" + original = (config.read_bytes(), (cache / "before_plan.json").read_bytes()) + original_remove = ExtensionManager._remove_generic_artifact_paths + + def remove_then_fail(manager, extension_id, metadata): + original_remove(manager, extension_id, metadata) + raise OSError("cleanup failed after artifact removal") + + with patch.object(Path, "cwd", return_value=project), \ + patch.object(ExtensionManager, "_remove_generic_artifact_paths", remove_then_fail): + result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) + assert result.exit_code == 1 + assert "cleanup failed after artifact removal" in result.output + assert manager.registry.is_installed("test-ext") + assert not artifact.exists() + assert (config.read_bytes(), (cache / "before_plan.json").read_bytes()) == original + assert (cache / "source.yml").read_bytes() == config.read_bytes() + + def test_remove_cleanup_failure_preserves_exact_yaml_and_cache( + self, tmp_path, extension_dir + ): + project, manager, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + config = project / ".specify/extensions.yml" + config.write_bytes(b"# preserve this comment\r\n" + config.read_bytes().replace(b"\n", b"\r\n")) + HookExecutor(project).migrate_project_config() + cache = project / ".specify/hook-dispatch" + original_yaml = config.read_bytes() + original_cache = {entry.name: entry.read_bytes() for entry in cache.iterdir()} + + with patch.object( + manager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed") + ): + with pytest.raises(OSError, match="cleanup failed"): + manager.remove("test-ext") + assert manager.registry.is_installed("test-ext") + assert artifact.exists() + assert config.read_bytes() == original_yaml + assert {entry.name: entry.read_bytes() for entry in cache.iterdir()} == original_cache + + def test_remove_cleanup_failure_leaves_absent_yaml_and_cache_untouched( + self, tmp_path, extension_dir + ): + project, manager, _ = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + config = project / ".specify/extensions.yml" + cache = project / ".specify/hook-dispatch" + config.unlink() + shutil.rmtree(cache) + with patch.object( + manager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed") + ): + with pytest.raises(OSError, match="cleanup failed"): + manager.remove("test-ext") + assert manager.registry.is_installed("test-ext") + assert not config.exists() + assert not cache.exists() + + def test_default_remove_cleanup_failure_keeps_legacy_hook( + self, tmp_path, extension_dir + ): + project, manager, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=False + ) + config = project / ".specify/extensions.yml" + original = config.read_bytes() + with patch.object(manager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed")): + with pytest.raises(OSError, match="cleanup failed"): + manager.remove("test-ext") + assert artifact.exists() + assert manager.registry.is_installed("test-ext") + assert config.read_bytes() == original + assert not (project / ".specify/hook-dispatch").exists() diff --git a/tests/specify_cli/extensions/test_command_update.py b/tests/specify_cli/extensions/test_command_update.py index c1d5f1743c..2505395343 100644 --- a/tests/specify_cli/extensions/test_command_update.py +++ b/tests/specify_cli/extensions/test_command_update.py @@ -20,6 +20,7 @@ ExtensionManager, HookExecutor, ) +from tests.specify_cli.extensions._helpers import enable_native_hooks class TestExtensionUpdateCLI: @@ -94,6 +95,60 @@ def _create_catalog_zip( if extra_manifest_path is not None: zf.writestr(extra_manifest_path, manifest_text) + @pytest.mark.parametrize("native_hooks", [False, True]) + def test_update_legacy_event_follows_opt_in_mode(self, tmp_path, native_hooks): + project = tmp_path / "project" + project.mkdir() + (project / ".specify").mkdir() + (project / ".claude/skills").mkdir(parents=True) + if native_hooks: + enable_native_hooks(project) + manager = ExtensionManager(project) + original_source = self._create_extension_source(tmp_path, "1.0.0") + manager.install_from_directory( + original_source, "0.1.0", catalog_name="original-catalog" + ) + original_entry = manager.registry.get("test-ext") + original_config = yaml.safe_load((project / ".specify/extensions.yml").read_text()) + updated_source = self._create_extension_source(tmp_path, "2.0.0") + manifest_path = updated_source / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text()) + manifest["hooks"] = {"custom:after": {"command": "speckit.test-ext.hello"}} + manifest_path.write_text(yaml.safe_dump(manifest)) + zip_path = tmp_path / "test-ext-update.zip" + self._create_catalog_zip(zip_path, "2.0.0") + + def install_invalid_update(self_obj, _zip_path, version, *, catalog_name=None): + return self_obj.install_from_directory( + updated_source, version, catalog_name=catalog_name + ) + + with patch.object(Path, "cwd", return_value=project), \ + patch.object(ExtensionCatalog, "get_extension_info", return_value={ + "id": "test-ext", "name": "Test Extension", "version": "2.0.0", + "_install_allowed": True, + }), \ + patch.object(ExtensionCatalog, "download_extension", return_value=zip_path), \ + patch.object(ExtensionManager, "install_from_zip", install_invalid_update): + result = CliRunner().invoke( + app, ["extension", "update", "test-ext"], input="y\n" + ) + if native_hooks: + assert result.exit_code == 1, result.output + assert "Rename legacy events" in result.output + assert ExtensionManager(project).registry.get("test-ext") == original_entry + assert yaml.safe_load( + (project / ".specify/extensions.yml").read_text() + ) == original_config + assert (project / ".specify/hook-dispatch").exists() + else: + assert result.exit_code == 0, result.output + assert ExtensionManager(project).registry.get("test-ext")["version"] == "2.0.0" + assert "custom:after" in yaml.safe_load( + (project / ".specify/extensions.yml").read_text() + )["hooks"] + assert not (project / ".specify/hook-dispatch").exists() + @pytest.mark.parametrize( "manifest_path", [ diff --git a/tests/specify_cli/extensions/test_command_update_transaction.py b/tests/specify_cli/extensions/test_command_update_transaction.py index 28efb46833..aad5069b75 100644 --- a/tests/specify_cli/extensions/test_command_update_transaction.py +++ b/tests/specify_cli/extensions/test_command_update_transaction.py @@ -153,21 +153,19 @@ def test_extension_update_corrupted_hooks_value(project_dir, monkeypatch): assert not isinstance(result.exception, AttributeError) def test_extension_update_rollback_corrupted_config(project_dir, monkeypatch): - """Regression: extension update rollback must handle corrupted extensions.yml.""" + """Opted-in malformed hooks must prevent update before mutation.""" + from tests.specify_cli.extensions._helpers import enable_native_hooks + monkeypatch.chdir(project_dir) + enable_native_hooks(project_dir) config_path = project_dir / ".specify" / "extensions.yml" - # Write config with hooks: null; get_project_config() normalizes this to {} - # so the backup captures {} and the restored config will have hooks: {}. + # Read-only queries tolerate this, but mutations must not discard it. config_path.write_text(yaml.dump({"installed": ["test-ext"], "hooks": None})) - # Mock update process to fail after backup monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: [{"id": "test-ext", "name": "Test Ext", "version": "1.0.0"}]) monkeypatch.setattr(ExtensionRegistry, "get", lambda self, ext_id: {"version": "1.0.0", "enabled": True}) - # Reach the destructive update phase, then fail the install so rollback is - # both necessary and safe to exercise. Download/preflight failures leave - # the installation untouched and deliberately skip destructive rollback. mock_zip = project_dir / "mock.zip" _write_update_zip(mock_zip) monkeypatch.setattr( @@ -175,32 +173,23 @@ def test_extension_update_rollback_corrupted_config(project_dir, monkeypatch): "download_extension", lambda self, ext_id: mock_zip, ) - monkeypatch.setattr( - ExtensionManager, - "remove", - lambda self, ext_id, keep_config=False: None, - ) - - def mock_install_fail(*args, **kwargs): - config_path.write_text(yaml.dump("CORRUPTED")) - raise Exception("Install failed") + def unexpected_mutation(*args, **kwargs): + pytest.fail("invalid hook config must fail before mutation") monkeypatch.setattr(ExtensionCatalog, "get_extension_info", lambda self, ext_id: {"id": "test-ext", "name": "Test Ext", "version": "1.1.0", "download_url": "https://example.com/ext.zip"}) - monkeypatch.setattr(ExtensionManager, "install_from_zip", mock_install_fail) + monkeypatch.setattr(ExtensionManager, "install_from_zip", unexpected_mutation) + monkeypatch.setattr(ExtensionManager, "remove", unexpected_mutation) monkeypatch.setattr("typer.confirm", lambda _: True) result = runner.invoke(app, ["extension", "update", "test-ext"], obj={"project_root": project_dir}) - # Should handle Exception and NOT crash with AttributeError during rollback assert result.exit_code == 1 - assert "Install failed" in result.output + assert "expected a mapping" in result.output assert not isinstance(result.exception, AttributeError) - # Verify hooks key was preserved (normalized to {} if it was null/corrupted) restored_config = yaml.safe_load(config_path.read_text()) assert isinstance(restored_config, dict) - assert "hooks" in restored_config - assert restored_config["hooks"] == {} + assert restored_config["hooks"] is None def test_extension_update_skills_backup_no_collision(project_dir, monkeypatch): diff --git a/tests/specify_cli/extensions/test_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py new file mode 100644 index 0000000000..3a9c7c9401 --- /dev/null +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -0,0 +1,414 @@ +"""Projection contract and publication failures without changing hook reads.""" + +import hashlib +import json +from pathlib import Path + +import pytest +import yaml + +from specify_cli import save_init_options +from specify_cli.extensions import HookExecutor +from specify_cli.extensions._hook_projection import project_hooks + + +def _config() -> dict: + return { + "installed": ["example"], + "settings": {"auto_execute_hooks": True}, + "hooks": { + "before_plan": [ + {"extension": "example", "command": "speckit.example.first", "priority": 90, "optional": False}, + {"extension": "example", "command": "speckit.example.skipped", "enabled": False}, + {"extension": "example", "command": "speckit.example.dynamic", "condition": "env.RUN is set"}, + {"extension": "example", "command": "speckit.example.last", "priority": 1, "prompt": "Proceed?"}, + ], + "after_plan": [], + }, + } + + +def _enabled_executor(root: Path) -> HookExecutor: + save_init_options(root, {"native_extension_hooks": True}) + return HookExecutor(root) + + +def _assert_snapshot(root: Path, event: str = "before_plan") -> dict: + cache = root / ".specify" / "hook-dispatch" + source = (root / ".specify" / "extensions.yml").read_bytes() + assert (cache / "source.yml").read_bytes() == source + index = (cache / "events.txt").read_bytes() + assert index == b"before_plan\nafter_plan\n" + assert (cache / "events.txt.sha256").read_text().strip() == hashlib.sha256(index).hexdigest() + payload = (cache / f"{event}.json").read_bytes() + assert (cache / f"{event}.json.sha256").read_text().strip() == hashlib.sha256(payload).hexdigest() + return json.loads(payload) + + +def test_default_save_and_refresh_preserve_legacy_hooks_without_projection(tmp_path): + executor = HookExecutor(tmp_path) + config = {"installed": ["example"], "hooks": {"custom:after": [ + {"extension": "example", "command": "speckit.example.legacy"} + ]}} + executor.save_project_config(config) + source = executor.config_file.read_bytes() + executor.migrate_project_config() + assert executor.config_file.read_bytes() == source + assert executor.get_hooks_for_event("custom:after")[0]["command"] == ( + "speckit.example.legacy" + ) + assert not (tmp_path / ".specify/hook-dispatch").exists() + + +def test_default_hook_lifecycle_keeps_legacy_event_without_projection(tmp_path): + executor = HookExecutor(tmp_path) + config = {"installed": ["example"], "hooks": {"custom:after": [ + {"extension": "example", "command": "speckit.example.legacy"} + ]}} + executor.save_project_config(config) + executor.disable_hooks("example") + assert not executor.get_hooks_for_event("custom:after") + executor.enable_hooks("example") + assert executor.get_hooks_for_event("custom:after") + executor.unregister_hooks("example") + assert not executor.get_hooks_for_event("custom:after") + assert not (tmp_path / ".specify/hook-dispatch").exists() + + +@pytest.mark.parametrize("invalid", ["true", 1, None, []]) +def test_non_boolean_opt_in_is_rejected_without_writing_projection(tmp_path, invalid): + save_init_options(tmp_path, {"native_extension_hooks": invalid}) + executor = HookExecutor(tmp_path) + with pytest.raises(ValueError, match="native_extension_hooks.*expected true or false"): + executor.save_project_config(_config()) + assert not executor.config_file.exists() + assert not (tmp_path / ".specify/hook-dispatch").exists() + + +def test_opt_out_retires_prior_projection_and_restores_legacy_writes(tmp_path): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + cache = tmp_path / ".specify/hook-dispatch" + assert cache.exists() + save_init_options(tmp_path, {"native_extension_hooks": False}) + legacy = {"hooks": {"custom:after": [ + {"extension": "example", "command": "speckit.example.legacy"} + ]}} + executor.save_project_config(legacy) + executor.migrate_project_config() + assert not cache.exists() + assert executor.get_hooks_for_event("custom:after")[0]["command"] == ( + "speckit.example.legacy" + ) + save_init_options(tmp_path, {"native_extension_hooks": True}) + with pytest.raises(ValueError, match="Rename legacy events"): + executor.migrate_project_config() + assert not cache.exists() + + +def test_save_projects_only_unconditional_enabled_hooks_in_yaml_order(tmp_path): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + result = _assert_snapshot(tmp_path) + assert result == { + "event": "before_plan", + "hooks": [ + {"extension": "example", "command": "speckit.example.first", "optional": False, + "description": "", "prompt": "", "priority": 90}, + {"extension": "example", "command": "speckit.example.last", "optional": True, + "description": "", "prompt": "Proceed?", "priority": 1}, + ], + } + assert json.loads((tmp_path / ".specify/hook-dispatch/after_plan.json").read_text()) == { + "event": "after_plan", "hooks": [] + } + assert [hook["command"] for hook in executor.get_hooks_for_event("before_plan")] == [ + "speckit.example.last", "speckit.example.dynamic", + "speckit.example.first", + ] + + +@pytest.mark.parametrize("bad", [ + {"hooks": {42: []}}, + {"hooks": {"custom:after": []}}, + {"hooks": {"../before_plan": []}}, + {"hooks": {"before_speckit.bug.assess": []}}, + {"hooks": {"before_my-extension_assess": []}}, + {"hooks": {"after_bug-assess": []}}, + {"hooks": {"before_": []}}, + {"hooks": {"after_Bug": []}}, + {"hooks": {"before_" + "a" * 250: []}}, + {"hooks": {"before_plan": [None]}}, + {"hooks": {"before_plan": [{"extension": "a", "command": "b", "optional": "yes"}]}}, + {"hooks": {"before_plan": [{"extension": "a", "command": "b", "condition": 7}]}}, + {"hooks": {"before_plan": [{"extension": "a", "command": "b\0"}]}}, +]) +def test_invalid_inputs_leave_existing_config_and_projection_unchanged(tmp_path, bad): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + old_source = executor.config_file.read_bytes() + old_projection = (tmp_path / ".specify/hook-dispatch/before_plan.json").read_bytes() + with pytest.raises(ValueError): + executor.save_project_config(bad) + assert executor.config_file.read_bytes() == old_source + assert (tmp_path / ".specify/hook-dispatch/before_plan.json").read_bytes() == old_projection + + +def test_update_and_refresh_retire_stale_events_and_preserve_crlf(tmp_path): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + updated = _config() + updated["hooks"] = {"after_tasks": []} + executor.save_project_config(updated) + assert not (tmp_path / ".specify/hook-dispatch/before_plan.json").exists() + assert (tmp_path / ".specify/hook-dispatch/events.txt").read_bytes() == b"after_tasks\n" + executor.config_file.write_bytes(executor.config_file.read_bytes().replace(b"\n", b"\r\n")) + executor.migrate_project_config() + assert (tmp_path / ".specify/hook-dispatch/source.yml").read_bytes() == executor.config_file.read_bytes() + assert b"\r\n" in executor.config_file.read_bytes() + + +def test_refresh_replaces_stale_cache_after_manual_yaml_edit(tmp_path): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + updated = {"installed": ["example"], "hooks": {"after_tasks": []}} + cache = tmp_path / ".specify/hook-dispatch" + executor.config_file.write_text( + yaml.safe_dump(updated, sort_keys=False), encoding="utf-8" + ) + assert (cache / "source.yml").read_bytes() != executor.config_file.read_bytes() + assert (cache / "before_plan.json").exists() + executor.migrate_project_config() + assert (cache / "source.yml").read_bytes() == executor.config_file.read_bytes() + assert (cache / "events.txt").read_bytes() == b"after_tasks\n" + assert (cache / "events.txt.sha256").read_text().strip() == hashlib.sha256( + b"after_tasks\n" + ).hexdigest() + assert json.loads((cache / "after_tasks.json").read_text())["hooks"] == [] + assert not (cache / "before_plan.json").exists() + assert not (tmp_path / ".specify/.hook-dispatch.lock").exists() + + +def test_refuse_unsafe_cache_and_invalid_existing_config(tmp_path): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + cache = tmp_path / ".specify/hook-dispatch" + (cache / "unexpected").mkdir() + original = executor.config_file.read_bytes() + with pytest.raises(ValueError, match="non-file"): + executor.load_project_config_for_update() + with pytest.raises(ValueError, match="non-file"): + executor.save_project_config(_config()) + assert executor.config_file.read_bytes() == original + (cache / "unexpected").rmdir() + executor.config_file.write_text("hooks:\n before_plan: invalid\n") + with pytest.raises(ValueError, match="expected a list"): + executor.load_project_config_for_update() + + +def test_refuse_symlinked_config_and_cache_before_mutation(tmp_path): + executor = _enabled_executor(tmp_path) + executor.config_file.parent.mkdir(exist_ok=True) + external = tmp_path / "external.yml" + external.write_text("hooks: {}\n", encoding="utf-8") + try: + executor.config_file.symlink_to(external) + except (OSError, NotImplementedError): + pytest.skip("symlinks unavailable") + with pytest.raises(ValueError, match="symlinked"): + executor.save_project_config(_config()) + assert external.read_text() == "hooks: {}\n" + executor.config_file.unlink() + executor.config_file.write_text("hooks: {}\n") + (tmp_path / ".specify/hook-dispatch").symlink_to(tmp_path, target_is_directory=True) + with pytest.raises(ValueError, match="symlinked"): + executor.load_project_config_for_update() + assert executor.config_file.read_text() == "hooks: {}\n" + + +def test_partial_direct_config_write_requires_manual_repair_and_refresh(tmp_path, monkeypatch): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + source = executor.config_file.read_bytes() + payload = (tmp_path / ".specify/hook-dispatch/before_plan.json").read_bytes() + real_write = Path.write_bytes + + def fail_config_write(path, content): + if path == executor.config_file: + real_write(path, b"hooks:\n before_plan: [\n") + raise OSError("simulated config write failure") + return real_write(path, content) + + monkeypatch.setattr(Path, "write_bytes", fail_config_write) + with pytest.raises(OSError, match="simulated config write failure.*refresh-shared-infra"): + executor.save_project_config({"installed": [], "hooks": {}}) + assert executor.config_file.read_bytes() != source + assert (tmp_path / ".specify/hook-dispatch/before_plan.json").read_bytes() == payload + with pytest.raises(ValueError, match="Invalid extension configuration"): + executor.migrate_project_config() + monkeypatch.setattr(Path, "write_bytes", real_write) + executor.config_file.write_bytes(source) + executor.migrate_project_config() + _assert_snapshot(tmp_path) + + +def test_direct_cache_write_failure_leaves_partial_state_then_refresh_repairs(tmp_path, monkeypatch): + import specify_cli.extensions._hook_projection as projection + + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + cache = tmp_path / ".specify/hook-dispatch" + old_source = executor.config_file.read_bytes() + old_index = (cache / "events.txt").read_bytes() + updated = {"installed": ["example"], "hooks": {"after_tasks": []}} + real_write = projection.write_bytes + + def fail_index(path, content): + if path == cache / "events.txt": + raise OSError("disk full") + return real_write(path, content) + + monkeypatch.setattr(projection, "write_bytes", fail_index) + with pytest.raises(OSError, match="disk full.*refresh-shared-infra"): + executor.save_project_config(updated) + assert executor.config_file.read_bytes() != old_source + assert (cache / "source.yml").read_bytes() == old_source + assert (cache / "events.txt").read_bytes() == old_index + monkeypatch.setattr(projection, "write_bytes", real_write) + executor.migrate_project_config() + assert (cache / "source.yml").read_bytes() == executor.config_file.read_bytes() + assert (cache / "events.txt").read_bytes() == b"after_tasks\n" + assert not (cache / "before_plan.json").exists() + + +def test_unreadable_existing_config_does_not_create_projection(tmp_path): + executor = _enabled_executor(tmp_path) + executor.config_file.parent.mkdir(exist_ok=True) + executor.config_file.mkdir() + with pytest.raises(OSError): + executor.save_project_config(_config()) + assert not (tmp_path / ".specify/hook-dispatch").exists() + + +def test_direct_stale_event_cleanup_failure_reports_and_refresh_repairs(tmp_path, monkeypatch): + import specify_cli.extensions._hook_projection as projection + + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + cache = tmp_path / ".specify/hook-dispatch" + real_unlink = projection.unlink + + def fail_stale_cleanup(path): + if path == cache / "before_plan.json": + raise OSError("cannot remove stale event") + return real_unlink(path) + + monkeypatch.setattr(projection, "unlink", fail_stale_cleanup) + updated = {"installed": ["example"], "hooks": {"after_plan": []}} + with pytest.raises(OSError, match="cannot remove stale event.*refresh-shared-infra"): + executor.save_project_config(updated) + assert yaml.safe_load(executor.config_file.read_text(encoding="utf-8")) == updated + assert (tmp_path / ".specify/hook-dispatch/source.yml").read_bytes() == executor.config_file.read_bytes() + assert (tmp_path / ".specify/hook-dispatch/events.txt").read_bytes() == b"after_plan\n" + assert (cache / "before_plan.json").exists() + monkeypatch.setattr(projection, "unlink", real_unlink) + executor.migrate_project_config() + assert not (tmp_path / ".specify/hook-dispatch/before_plan.json").exists() + + +def test_existing_yaml_refresh_keeps_legacy_read_semantics(tmp_path): + executor = _enabled_executor(tmp_path) + executor.config_file.parent.mkdir(exist_ok=True) + existing = _config() + executor.config_file.write_text(yaml.safe_dump(existing, sort_keys=False)) + old = executor.check_hooks_for_event("before_plan") + executor.migrate_project_config() + assert executor.check_hooks_for_event("before_plan") == old + _assert_snapshot(tmp_path) + + +def test_enable_disable_remove_republish_without_stale_hooks(tmp_path): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + executor.disable_hooks("example") + assert _assert_snapshot(tmp_path)["hooks"] == [] + executor.enable_hooks("example") + assert [hook["command"] for hook in _assert_snapshot(tmp_path)["hooks"]] == [ + "speckit.example.first", "speckit.example.skipped", "speckit.example.last" + ] + executor.unregister_hooks("example") + assert not (tmp_path / ".specify/hook-dispatch/before_plan.json").exists() + assert (tmp_path / ".specify/hook-dispatch/events.txt").read_bytes() == b"" + + +def test_shared_infrastructure_refresh_projects_existing_config(tmp_path, monkeypatch): + import specify_cli + + executor = _enabled_executor(tmp_path) + executor.config_file.parent.mkdir(exist_ok=True) + executor.config_file.write_text(yaml.safe_dump(_config(), sort_keys=False), encoding="utf-8") + monkeypatch.setattr(specify_cli, "_install_shared_infra_impl", lambda *args, **kwargs: True) + assert specify_cli._install_shared_infra(tmp_path, "sh", refresh_managed=True) + _assert_snapshot(tmp_path) + + +def test_project_hooks_is_pure(): + config = _config() + original = yaml.safe_dump(config) + project_hooks(config) + assert yaml.safe_dump(config) == original + + +def test_arbitrary_command_event_refresh_preserves_hook_reader_behavior(tmp_path): + executor = _enabled_executor(tmp_path) + executor.config_file.parent.mkdir(exist_ok=True) + existing = {"hooks": { + "before_my_command": [ + {"extension": "example", "command": "speckit.example.one", "priority": 30}, + {"extension": "example", "command": "speckit.example.two", "priority": 2}, + ], + "after_bug_assess": [ + {"extension": "example", "command": "speckit.example.extension"} + ], + "before_my_extension_assess": [], + "before_2fa_assess": [], + }} + executor.config_file.write_text(yaml.safe_dump(existing, sort_keys=False)) + before = { + event: executor.get_hooks_for_event(event) + for event in existing["hooks"] + } + executor.migrate_project_config() + assert { + event: executor.get_hooks_for_event(event) + for event in existing["hooks"] + } == before + cache = tmp_path / ".specify/hook-dispatch" + assert json.loads((cache / "before_my_command.json").read_text())["event"] == "before_my_command" + assert (cache / "events.txt").read_bytes() == ( + b"before_my_command\nafter_bug_assess\nbefore_my_extension_assess\nbefore_2fa_assess\n" + ) + assert json.loads((cache / "after_bug_assess.json").read_text())["event"] == "after_bug_assess" + + +def test_refresh_rejects_legacy_event_with_actionable_error(tmp_path): + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + cache = tmp_path / ".specify/hook-dispatch" + old_index = (cache / "events.txt").read_bytes() + executor.config_file.write_text("hooks:\n custom:after: []\n", encoding="utf-8") + with pytest.raises(ValueError, match="Rename legacy events"): + executor.migrate_project_config() + assert executor.config_file.read_text() == "hooks:\n custom:after: []\n" + assert (cache / "events.txt").read_bytes() == old_index + + +@pytest.mark.parametrize("value,expected", [ + ("7", 7), (False, 10), ("invalid", 10), (-1, 10), + (2147483648, 10), (float("inf"), 10), +]) +def test_project_hooks_normalizes_legacy_priorities(value, expected): + config = {"hooks": {"after_plan": [ + {"extension": "example", "command": "speckit.example.run", "priority": value} + ]}} + assert json.loads(project_hooks(config)["after_plan"])["hooks"][0]["priority"] == expected diff --git a/tests/test_extension_registration.py b/tests/test_extension_registration.py index 4dc4d1beb2..9eee891766 100644 --- a/tests/test_extension_registration.py +++ b/tests/test_extension_registration.py @@ -10,6 +10,14 @@ def project_dir(tmp_path): (proj_dir / ".specify").mkdir() return proj_dir + +@pytest.fixture +def native_hooks(project_dir): + from specify_cli import save_init_options + + save_init_options(project_dir, {"native_extension_hooks": True}) + + class TestExtensionRegistration: """Tests for the 'installed' list management in HookExecutor.""" @@ -170,21 +178,18 @@ def test_unregister_hooks_no_hooks_key(self, project_dir): config = executor.get_project_config() assert "ext-no-hooks" not in config["installed"] - def test_unregister_hooks_corrupted_config(self, project_dir): - """Resilience: unregister_hooks should gracefully handle corrupted config.""" + def test_unregister_hooks_corrupted_config(self, project_dir, native_hooks): + """Opted-in invalid configuration must not be overwritten by removal.""" # Create a corrupted config (root is a list) config_path = project_dir / ".specify" / "extensions.yml" config_path.write_text(yaml.dump(["corrupted", "list"])) executor = HookExecutor(project_dir) - # Should not raise even with corrupted config - executor.unregister_hooks("non-existent") + with pytest.raises(ValueError, match="expected a mapping"): + executor.unregister_hooks("non-existent") - # Config should remain as-is or be handled gracefully - config = executor.get_project_config() - # If it's corrupted, it's returned as-is or handled by defensive logic - assert config is not None + assert yaml.safe_load(config_path.read_text()) == ["corrupted", "list"] def test_unregister_hooks_with_multiple_extensions(self, project_dir, tmp_path): """Multiple Extensions: unregister_hooks should only remove target extension's hooks.""" @@ -299,8 +304,8 @@ def test_register_extension_mixed_type_installed(self, project_dir): assert 1 not in config["installed"] assert True not in config["installed"] - def test_unregister_hooks_null_hook_values(self, project_dir): - """Regression: hooks: {after_tasks: null} must not crash in unregister_hooks().""" + def test_unregister_hooks_null_hook_values(self, project_dir, native_hooks): + """A null event list must block removal rather than lose registered hooks.""" executor = HookExecutor(project_dir) # Manually write a config with null hook event value @@ -310,14 +315,14 @@ def test_unregister_hooks_null_hook_values(self, project_dir): "hooks": {"after_tasks": None} })) - # Should not raise TypeError when iterating None - executor.unregister_hooks("broken-ext") + original = config_path.read_bytes() + with pytest.raises(ValueError, match="expected a list"): + executor.unregister_hooks("broken-ext") - config = executor.get_project_config() - assert "broken-ext" not in config["installed"] + assert config_path.read_bytes() == original - def test_register_hooks_corrupted_hook_values(self, project_dir, tmp_path): - """Regression: register_hooks() must handle non-list hook event values in config.""" + def test_register_hooks_corrupted_hook_values(self, project_dir, tmp_path, native_hooks): + """A corrupt existing event list must prevent registration.""" executor = HookExecutor(project_dir) # Manually write a config with null hook event value @@ -349,13 +354,11 @@ def test_register_hooks_corrupted_hook_values(self, project_dir, tmp_path): manifest = ExtensionManifest(manifest_path) - # Should not raise TypeError when trying to append to None - executor.register_hooks(manifest) + original = config_path.read_bytes() + with pytest.raises(ValueError, match="expected a list"): + executor.register_hooks(manifest) - config = executor.get_project_config() - assert "new-ext" in config["installed"] - assert isinstance(config["hooks"]["after_tasks"], list) - assert any(h["extension"] == "new-ext" for h in config["hooks"]["after_tasks"]) + assert config_path.read_bytes() == original def test_register_extension_already_present_in_corrupted_list(self, project_dir): """Regression: if extension is already present but list has non-strings, it must still be sanitized.""" @@ -422,8 +425,8 @@ def test_unregister_extension_corrupted_installed(self, project_dir): config = executor.get_project_config() assert config["installed"] == [] - def test_register_hooks_mixed_type_hook_list(self, project_dir, tmp_path): - """Regression: register_hooks() must sanitize hook event lists by dropping non-dicts.""" + def test_register_hooks_mixed_type_hook_list(self, project_dir, tmp_path, native_hooks): + """Malformed hook entries must not be silently dropped on registration.""" executor = HookExecutor(project_dir) config_path = project_dir / ".specify" / "extensions.yml" @@ -454,33 +457,24 @@ def test_register_hooks_mixed_type_hook_list(self, project_dir, tmp_path): manifest_path.write_text(yaml.dump(manifest_data)) manifest = ExtensionManifest(manifest_path) - executor.register_hooks(manifest) + original = config_path.read_bytes() + with pytest.raises(ValueError, match="expected a mapping"): + executor.register_hooks(manifest) + assert config_path.read_bytes() == original - config = executor.get_project_config() - hooks = config["hooks"]["after_tasks"] - - # Should have 2 valid dict hooks, and 0 non-dict items - assert len(hooks) == 2 - assert all(isinstance(h, dict) for h in hooks) - assert any(h.get("extension") == "other" for h in hooks) - assert any(h.get("extension") == "new-ext" for h in hooks) - - def test_unregister_extension_scalar_root(self, project_dir): - """Hardening: unregister_extension should handle scalar root config.""" + def test_unregister_extension_scalar_root(self, project_dir, native_hooks): + """A scalar root cannot be safely updated without losing configuration.""" executor = HookExecutor(project_dir) config_path = project_dir / ".specify" / "extensions.yml" config_path.write_text(yaml.dump(123)) - # Should not crash and should normalize to {} - executor.unregister_extension("any-ext") - - config = executor.get_project_config() - assert isinstance(config, dict) - assert config["installed"] == [] + with pytest.raises(ValueError, match="expected a mapping"): + executor.unregister_extension("any-ext") + assert yaml.safe_load(config_path.read_text()) == 123 - def test_unregister_hooks_scalar_hook_values(self, project_dir): - """Regression: unregister_hooks() must handle scalar hook event values.""" + def test_unregister_hooks_scalar_hook_values(self, project_dir, native_hooks): + """A scalar event list blocks removal without deleting the old state.""" executor = HookExecutor(project_dir) config_path = project_dir / ".specify" / "extensions.yml" @@ -489,9 +483,7 @@ def test_unregister_hooks_scalar_hook_values(self, project_dir): "hooks": {"after_tasks": 123} })) - # Should not raise TypeError when iterating - executor.unregister_hooks("some-ext") - - config = executor.get_project_config() - assert "some-ext" not in config["installed"] - assert "after_tasks" not in config["hooks"] + original = config_path.read_bytes() + with pytest.raises(ValueError, match="expected a list"): + executor.unregister_hooks("some-ext") + assert config_path.read_bytes() == original diff --git a/tests/test_extensions.py b/tests/test_extensions.py index b512742389..7fea2d5be8 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -976,7 +976,7 @@ def test_hook_empty_list_rejected(self, temp_dir, valid_manifest_data): ExtensionManifest(manifest_path) def test_hook_colons_remain_accepted(self, temp_dir, valid_manifest_data): - """Artifact identifiers must not narrow the existing hook manifest contract.""" + """Legacy manifests remain valid unless scripted hook dispatch is enabled.""" import yaml valid_manifest_data["hooks"] = { @@ -987,7 +987,6 @@ def test_hook_colons_remain_accepted(self, temp_dir, valid_manifest_data): yaml.dump(valid_manifest_data, f) manifest = ExtensionManifest(manifest_path) - assert manifest.hooks["custom:after"]["command"] == ( "/skill:speckit-test-ext-hello" ) @@ -8154,8 +8153,10 @@ def test_register_hooks_dropping_all_hooks_purges_orphans(self, project_dir): assert [e["command"] for e in hooks["after_tasks"]] == ["speckit.ext-b.go"] def test_register_hooks_empty_hooks_purge_survives_corrupt_entry(self, project_dir): - """A corrupt non-dict entry already on disk does not break the - empty-hooks orphan purge; it is dropped and valid entries survive.""" + """An opted-in project cannot silently discard corrupt hook entries.""" + from specify_cli import save_init_options + + save_init_options(project_dir, {"native_extension_hooks": True}) executor = HookExecutor(project_dir) executor.register_hooks( _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.go"}}) @@ -8165,12 +8166,13 @@ def test_register_hooks_empty_hooks_purge_survives_corrupt_entry(self, project_d ) config = executor.get_project_config() config["hooks"]["after_tasks"].append("corrupt-non-dict-entry") - executor.save_project_config(config) + executor.config_file.write_text(yaml.safe_dump(config), encoding="utf-8") + original = executor.config_file.read_bytes() - executor.register_hooks(_StubManifest("ext-a", {})) + with pytest.raises(ValueError, match="expected a mapping"): + executor.register_hooks(_StubManifest("ext-a", {})) - hooks = executor.get_project_config()["hooks"] - assert [e["command"] for e in hooks["after_tasks"]] == ["speckit.ext-b.go"] + assert executor.config_file.read_bytes() == original def test_register_hooks_duplicate_command_moves_to_end(self, project_dir): """A command repeated in one manifest keeps the last value and the last