From 99b5d014eca3bfa15f31d9a303d58ac6aaf3e3ae Mon Sep 17 00:00:00 2001 From: Abishek Yadav Date: Mon, 13 Apr 2026 17:40:34 -0500 Subject: [PATCH] feat: add modifies_hooks support for extension hook overrides Extensions can now declare a `modifies_hooks` section in extension.yml to disable or set_optional hooks from other extensions during installation. This solves conflicts like the worktree-parallel extension needing to disable the git extension's `before_specify -> speckit.git.feature` hook, which forces branch switching and breaks the worktree-first workflow. Safety model: - Requires explicit user consent (Y/N prompt) before applying modifications - Non-TTY (CI) defaults to NO (fail-closed) - Granular targeting: hook + extension + command triple required - Limited actions: only disable and set_optional (no removal) - Reason field is mandatory and shown in consent prompt - Extra warning when modifying community (non-bundled) extension hooks - Fully reversible: original state restored on extension removal - Audit trail in registry via _modified_hooks metadata Schema: modifies_hooks: - hook: before_specify extension: git command: speckit.git.feature action: disable reason: "Worktree-parallel keeps primary checkout stable" Closes: related to #61, #1476 Made-with: Cursor --- src/specify_cli/extensions.py | 331 ++++++++++++++++++++++++++++++---- tests/test_extensions.py | 329 +++++++++++++++++++++++++++++++++ 2 files changed, 621 insertions(+), 39 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 8f45c425ad..ae4ab111d6 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -9,6 +9,7 @@ import json import hashlib import os +import sys import tempfile import zipfile import shutil @@ -217,6 +218,32 @@ def _validate(self): f"Hook '{hook_name}' missing required 'command' field" ) + # Validate modifies_hooks (if present) + modifies_hooks = self.data.get("modifies_hooks") + if modifies_hooks is not None: + if not isinstance(modifies_hooks, list): + raise ValidationError( + "Invalid modifies_hooks: expected a list" + ) + _VALID_MODIFY_ACTIONS = {"disable", "set_optional"} + _REQUIRED_MODIFY_FIELDS = {"hook", "extension", "command", "action", "reason"} + for idx, entry in enumerate(modifies_hooks): + if not isinstance(entry, dict): + raise ValidationError( + f"Invalid modifies_hooks[{idx}]: expected a mapping" + ) + missing = _REQUIRED_MODIFY_FIELDS - set(entry.keys()) + if missing: + raise ValidationError( + f"modifies_hooks[{idx}] missing required fields: {', '.join(sorted(missing))}" + ) + action = entry["action"] + if action not in _VALID_MODIFY_ACTIONS: + raise ValidationError( + f"modifies_hooks[{idx}] invalid action '{action}': " + f"must be one of {', '.join(sorted(_VALID_MODIFY_ACTIONS))}" + ) + # Validate commands (if present) for cmd in commands: if "name" not in cmd or "file" not in cmd: @@ -264,6 +291,15 @@ def hooks(self) -> Dict[str, Any]: """Get hook definitions.""" return self.data.get("hooks", {}) + @property + def modifies_hooks(self) -> List[Dict[str, Any]]: + """Get hook modification declarations. + + Each entry targets a specific hook from another extension and + requests a limited action (disable / set_optional) with a reason. + """ + return self.data.get("modifies_hooks", []) + def get_hash(self) -> str: """Calculate SHA256 hash of manifest file.""" with open(self.path, 'rb') as f: @@ -1080,9 +1116,9 @@ def install_from_directory( # was used during project initialisation (feature parity). registered_skills = self._register_extension_skills(manifest, dest_dir) - # Register hooks + # Register hooks and apply any modifies_hooks declarations hook_executor = HookExecutor(self.project_root) - hook_executor.register_hooks(manifest) + applied_modifications = hook_executor.register_hooks(manifest) # Update registry self.registry.add(manifest.id, { @@ -1093,6 +1129,7 @@ def install_from_directory( "priority": priority, "registered_commands": registered_commands, "registered_skills": registered_skills, + "_modified_hooks": applied_modifications, }) return manifest @@ -1224,9 +1261,12 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if extension_dir.exists(): shutil.rmtree(extension_dir) - # Unregister hooks + # Unregister hooks and restore any hooks this extension modified + modified_hooks = metadata.get("_modified_hooks", []) if metadata else [] + if not isinstance(modified_hooks, list): + modified_hooks = [] hook_executor = HookExecutor(self.project_root) - hook_executor.unregister_hooks(extension_id) + hook_executor.unregister_hooks(extension_id, modified_hooks=modified_hooks) # Update registry self.registry.remove(extension_id) @@ -2228,67 +2268,280 @@ def save_project_config(self, config: Dict[str, Any]): encoding="utf-8", ) - def register_hooks(self, manifest: ExtensionManifest): + def register_hooks( + self, + manifest: ExtensionManifest, + ) -> List[Dict[str, Any]]: """Register extension hooks in project config. + Also processes ``modifies_hooks`` declarations: each matching + target hook is modified (disable / set_optional) after explicit + user consent. Returns a list of applied modifications so the + caller can persist them in the extension registry for rollback. + Args: manifest: Extension manifest with hooks to register + + Returns: + List of dicts describing applied hook modifications (empty + when the extension declares no ``modifies_hooks`` or the + user declines). """ - if not hasattr(manifest, "hooks") or not manifest.hooks: - return + has_hooks = hasattr(manifest, "hooks") and manifest.hooks + has_modifies = hasattr(manifest, "modifies_hooks") and manifest.modifies_hooks + + if not has_hooks and not has_modifies: + return [] config = self.get_project_config() - # Ensure hooks dict exists if "hooks" not in config: config["hooks"] = {} - # Register each hook - for hook_name, hook_config in manifest.hooks.items(): - if hook_name not in config["hooks"]: - config["hooks"][hook_name] = [] - - # Add hook entry - hook_entry = { - "extension": manifest.id, - "command": hook_config.get("command"), - "enabled": True, - "optional": hook_config.get("optional", True), - "prompt": hook_config.get( - "prompt", f"Execute {hook_config.get('command')}?" - ), - "description": hook_config.get("description", ""), - "condition": hook_config.get("condition"), - } + # --- 1. Register the extension's own hooks --- + if has_hooks: + for hook_name, hook_config in manifest.hooks.items(): + if hook_name not in config["hooks"]: + config["hooks"][hook_name] = [] + + hook_entry = { + "extension": manifest.id, + "command": hook_config.get("command"), + "enabled": True, + "optional": hook_config.get("optional", True), + "prompt": hook_config.get( + "prompt", f"Execute {hook_config.get('command')}?" + ), + "description": hook_config.get("description", ""), + "condition": hook_config.get("condition"), + } - # Check if already registered - existing = [ - h - for h in config["hooks"][hook_name] - if h.get("extension") == manifest.id - ] + existing = [ + h + for h in config["hooks"][hook_name] + if h.get("extension") == manifest.id + ] - if not existing: - config["hooks"][hook_name].append(hook_entry) - else: - # Update existing - for i, h in enumerate(config["hooks"][hook_name]): - if h.get("extension") == manifest.id: - config["hooks"][hook_name][i] = hook_entry + if not existing: + config["hooks"][hook_name].append(hook_entry) + else: + for i, h in enumerate(config["hooks"][hook_name]): + if h.get("extension") == manifest.id: + config["hooks"][hook_name][i] = hook_entry + + # --- 2. Process modifies_hooks --- + applied_modifications: List[Dict[str, Any]] = [] + + if has_modifies: + applied_modifications = self._apply_hook_modifications( + manifest, config + ) self.save_project_config(config) + return applied_modifications + + # ------------------------------------------------------------------ + # Hook modification helpers + # ------------------------------------------------------------------ + + @staticmethod + def _is_bundled_extension(extension_id: str) -> bool: + """Return True if the extension ships with spec-kit core.""" + return extension_id == "git" + + def _apply_hook_modifications( + self, + manifest: ExtensionManifest, + config: Dict[str, Any], + ) -> List[Dict[str, Any]]: + """Resolve, prompt, and apply ``modifies_hooks`` entries. + + Mutates *config* in-place when the user consents. + + Returns: + List of applied modification records (for registry storage). + """ + pending: List[Dict[str, Any]] = [] + + for entry in manifest.modifies_hooks: + hook_name = entry["hook"] + target_ext = entry["extension"] + target_cmd = entry["command"] + action = entry["action"] + reason = entry["reason"] + + hooks_list = config.get("hooks", {}).get(hook_name, []) + match_idx = None + for idx, h in enumerate(hooks_list): + if ( + h.get("extension") == target_ext + and h.get("command") == target_cmd + ): + match_idx = idx + break + + if match_idx is None: + print( + f"[{manifest.id}] Warning: target hook " + f"{hook_name} -> {target_ext}/{target_cmd} not found, skipping.", + file=sys.stderr, + ) + continue + + target_hook = hooks_list[match_idx] + + if action == "disable" and not target_hook.get("enabled", True): + continue + if action == "set_optional" and target_hook.get("optional", True): + continue + + pending.append({ + "hook": hook_name, + "extension": target_ext, + "command": target_cmd, + "action": action, + "reason": reason, + "original_enabled": target_hook.get("enabled", True), + "original_optional": target_hook.get("optional", True), + "_idx": match_idx, + }) + + if not pending: + return [] + + if not self._prompt_hook_modifications(manifest.id, pending): + print( + f"[{manifest.id}] Hook modifications skipped. " + "You can apply them manually in .specify/extensions.yml", + file=sys.stderr, + ) + return [] + + applied: List[Dict[str, Any]] = [] + for mod in pending: + hook_name = mod["hook"] + hooks_list = config["hooks"][hook_name] + target_hook = hooks_list[mod["_idx"]] + + if mod["action"] == "disable": + target_hook["enabled"] = False + elif mod["action"] == "set_optional": + target_hook["optional"] = True + + applied.append({ + "hook": mod["hook"], + "extension": mod["extension"], + "command": mod["command"], + "action": mod["action"], + "original_enabled": mod["original_enabled"], + "original_optional": mod["original_optional"], + }) + print( + f"[{manifest.id}] {mod['action']}d hook " + f"{mod['hook']} -> {mod['extension']}/{mod['command']}: " + f"{mod['reason']}", + file=sys.stderr, + ) + + return applied + + @staticmethod + def _prompt_hook_modifications( + installing_ext: str, + pending: List[Dict[str, Any]], + ) -> bool: + """Print a summary and ask for explicit consent. - def unregister_hooks(self, extension_id: str): + Returns False if the user declines or stdin is not a TTY (CI). + """ + print( + f"\nExtension '{installing_ext}' requests the following " + "hook modifications:\n", + file=sys.stderr, + ) + header = f" {'Hook':<22} {'Target Extension':<18} {'Command':<28} {'Action':<12} Reason" + print(header, file=sys.stderr) + print(" " + "-" * (len(header) - 2), file=sys.stderr) + for mod in pending: + is_bundled = HookExecutor._is_bundled_extension(mod["extension"]) + ext_label = mod["extension"] + if not is_bundled: + ext_label += " (community)" + reason_short = mod["reason"][:60] + ("..." if len(mod["reason"]) > 60 else "") + print( + f" {mod['hook']:<22} {ext_label:<18} {mod['command']:<28} {mod['action']:<12} {reason_short}", + file=sys.stderr, + ) + + has_community = any( + not HookExecutor._is_bundled_extension(m["extension"]) for m in pending + ) + if has_community: + print( + "\n Warning: This extension modifies hooks from a community " + "extension which may have been configured intentionally.", + file=sys.stderr, + ) + + if not sys.stdin.isatty(): + print( + "\n Non-interactive mode detected. Defaulting to NO " + "(hook modifications not applied).", + file=sys.stderr, + ) + return False + + try: + answer = input("\nApply these modifications? [Y/n]: ").strip().lower() + except (EOFError, KeyboardInterrupt): + return False + return answer in ("y", "yes", "") + + def unregister_hooks( + self, + extension_id: str, + modified_hooks: Optional[List[Dict[str, Any]]] = None, + ): """Remove extension hooks from project config. + Also restores any hooks that were modified by this extension + during installation (tracked in *modified_hooks*). + Args: extension_id: ID of extension to unregister + modified_hooks: List of modification records from registry + ``_modified_hooks`` (may be None) """ config = self.get_project_config() if "hooks" not in config: return + # Restore hooks that were modified by this extension + if modified_hooks: + for mod in modified_hooks: + hook_name = mod.get("hook") + target_ext = mod.get("extension") + target_cmd = mod.get("command") + if not hook_name or not target_ext or not target_cmd: + continue + + hooks_list = config.get("hooks", {}).get(hook_name, []) + for h in hooks_list: + if ( + h.get("extension") == target_ext + and h.get("command") == target_cmd + ): + h["enabled"] = mod.get("original_enabled", True) + h["optional"] = mod.get("original_optional", True) + print( + f"[{extension_id}] Restored hook " + f"{hook_name} -> {target_ext}/{target_cmd} " + f"(enabled={h['enabled']}, optional={h['optional']})", + file=sys.stderr, + ) + break + # Remove hooks for this extension for hook_name in config["hooks"]: config["hooks"][hook_name] = [ diff --git a/tests/test_extensions.py b/tests/test_extensions.py index a6ddff8e1a..779941c64c 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3853,3 +3853,332 @@ def test_hook_message_falls_back_when_invocation_is_empty(self, project_dir): assert "Executing: `/`" in message assert "EXECUTE_COMMAND: " in message assert "EXECUTE_COMMAND_INVOCATION: /" in message + + +# ===== modifies_hooks Tests ===== + + +class TestModifiesHooks: + """Tests for the modifies_hooks extension.yml feature.""" + + @pytest.fixture + def project_dir(self, tmp_path): + specify_dir = tmp_path / ".specify" + specify_dir.mkdir() + extensions_dir = specify_dir / "extensions" + extensions_dir.mkdir() + return tmp_path + + @pytest.fixture + def extensions_yml_with_git_hook(self, project_dir): + """Seed extensions.yml with a git before_specify hook.""" + import yaml + + config = { + "installed": [], + "settings": {"auto_execute_hooks": True}, + "hooks": { + "before_specify": [ + { + "extension": "git", + "command": "speckit.git.feature", + "enabled": True, + "optional": False, + "prompt": "Execute speckit.git.feature?", + "description": "Create feature branch before specification", + "condition": None, + } + ] + }, + } + config_path = project_dir / ".specify" / "extensions.yml" + config_path.write_text(yaml.dump(config, default_flow_style=False)) + return config_path + + @pytest.fixture + def manifest_with_modifies(self, tmp_path): + """Extension manifest that declares modifies_hooks.""" + import yaml + + ext_dir = tmp_path / "worktrees-src" + ext_dir.mkdir() + data = { + "schema_version": "1.0", + "extension": { + "id": "worktrees", + "name": "Worktrees", + "version": "1.0.0", + "description": "Worktree isolation", + }, + "requires": {"speckit_version": ">=0.4.0"}, + "provides": { + "commands": [ + { + "name": "speckit.worktrees.create", + "file": "commands/speckit.worktrees.create.md", + "description": "Spawn a worktree", + } + ] + }, + "hooks": { + "after_specify": { + "command": "speckit.worktrees.create", + "optional": False, + "description": "Auto-spawn worktree", + } + }, + "modifies_hooks": [ + { + "hook": "before_specify", + "extension": "git", + "command": "speckit.git.feature", + "action": "disable", + "reason": "Worktree-parallel model keeps primary checkout stable", + } + ], + } + manifest_path = ext_dir / "extension.yml" + manifest_path.write_text(yaml.dump(data, default_flow_style=False)) + # Create the command file so manifest validation passes + cmd_dir = ext_dir / "commands" + cmd_dir.mkdir() + (cmd_dir / "speckit.worktrees.create.md").write_text("# Create worktree") + return ExtensionManifest(manifest_path) + + def test_validation_accepts_valid_modifies_hooks(self, manifest_with_modifies): + assert len(manifest_with_modifies.modifies_hooks) == 1 + assert manifest_with_modifies.modifies_hooks[0]["action"] == "disable" + + def test_validation_rejects_missing_fields(self, tmp_path): + import yaml + + data = { + "schema_version": "1.0", + "extension": { + "id": "bad-ext", + "name": "Bad", + "version": "1.0.0", + "description": "Missing fields", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + {"name": "speckit.bad-ext.cmd", "file": "cmd.md", "description": "x"} + ] + }, + "modifies_hooks": [ + {"hook": "before_specify", "extension": "git"} + ], + } + path = tmp_path / "extension.yml" + path.write_text(yaml.dump(data, default_flow_style=False)) + with pytest.raises(ValidationError, match="missing required fields"): + ExtensionManifest(path) + + def test_validation_rejects_bad_action(self, tmp_path): + import yaml + + data = { + "schema_version": "1.0", + "extension": { + "id": "bad-ext", + "name": "Bad", + "version": "1.0.0", + "description": "Bad action", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + {"name": "speckit.bad-ext.cmd", "file": "cmd.md", "description": "x"} + ] + }, + "modifies_hooks": [ + { + "hook": "before_specify", + "extension": "git", + "command": "speckit.git.feature", + "action": "remove", + "reason": "bad", + } + ], + } + path = tmp_path / "extension.yml" + path.write_text(yaml.dump(data, default_flow_style=False)) + with pytest.raises(ValidationError, match="invalid action"): + ExtensionManifest(path) + + def test_register_hooks_disables_target_with_consent( + self, project_dir, extensions_yml_with_git_hook, manifest_with_modifies, monkeypatch + ): + """When user consents, the target hook is disabled.""" + import yaml + + monkeypatch.setattr("sys.stdin", type("FakeTTY", (), {"isatty": lambda self: True})()) + monkeypatch.setattr("builtins.input", lambda _: "y") + + executor = HookExecutor(project_dir) + applied = executor.register_hooks(manifest_with_modifies) + + assert len(applied) == 1 + assert applied[0]["action"] == "disable" + assert applied[0]["original_enabled"] is True + + config = yaml.safe_load(extensions_yml_with_git_hook.read_text()) + git_hook = config["hooks"]["before_specify"][0] + assert git_hook["enabled"] is False + + def test_register_hooks_skips_when_consent_declined( + self, project_dir, extensions_yml_with_git_hook, manifest_with_modifies, monkeypatch + ): + """When user declines, the target hook is not modified.""" + import yaml + + monkeypatch.setattr("sys.stdin", type("FakeTTY", (), {"isatty": lambda self: True})()) + monkeypatch.setattr("builtins.input", lambda _: "n") + + executor = HookExecutor(project_dir) + applied = executor.register_hooks(manifest_with_modifies) + + assert len(applied) == 0 + + config = yaml.safe_load(extensions_yml_with_git_hook.read_text()) + git_hook = config["hooks"]["before_specify"][0] + assert git_hook["enabled"] is True + + def test_register_hooks_skips_in_non_tty( + self, project_dir, extensions_yml_with_git_hook, manifest_with_modifies, monkeypatch + ): + """In CI (non-TTY), modifications default to NO.""" + import yaml + + monkeypatch.setattr("sys.stdin", type("FakeNonTTY", (), {"isatty": lambda self: False})()) + + executor = HookExecutor(project_dir) + applied = executor.register_hooks(manifest_with_modifies) + + assert len(applied) == 0 + + config = yaml.safe_load(extensions_yml_with_git_hook.read_text()) + git_hook = config["hooks"]["before_specify"][0] + assert git_hook["enabled"] is True + + def test_unregister_hooks_restores_original_state( + self, project_dir, extensions_yml_with_git_hook, manifest_with_modifies, monkeypatch + ): + """Removing the extension restores the target hook to its original state.""" + import yaml + + monkeypatch.setattr("sys.stdin", type("FakeTTY", (), {"isatty": lambda self: True})()) + monkeypatch.setattr("builtins.input", lambda _: "y") + + executor = HookExecutor(project_dir) + applied = executor.register_hooks(manifest_with_modifies) + + config = yaml.safe_load(extensions_yml_with_git_hook.read_text()) + assert config["hooks"]["before_specify"][0]["enabled"] is False + + executor.unregister_hooks("worktrees", modified_hooks=applied) + + config = yaml.safe_load(extensions_yml_with_git_hook.read_text()) + git_hook = config["hooks"]["before_specify"][0] + assert git_hook["enabled"] is True + + def test_target_hook_not_found_warns_not_errors( + self, project_dir, manifest_with_modifies, capsys + ): + """Missing target hook prints warning but doesn't raise.""" + import yaml + + empty_config = { + "installed": [], + "settings": {"auto_execute_hooks": True}, + "hooks": {}, + } + config_path = project_dir / ".specify" / "extensions.yml" + config_path.write_text(yaml.dump(empty_config, default_flow_style=False)) + + executor = HookExecutor(project_dir) + applied = executor.register_hooks(manifest_with_modifies) + + assert len(applied) == 0 + captured = capsys.readouterr() + assert "not found" in captured.err + + def test_already_disabled_hook_is_noop( + self, project_dir, monkeypatch + ): + """If target hook is already disabled, no consent prompt is shown.""" + import yaml + + config = { + "installed": [], + "settings": {"auto_execute_hooks": True}, + "hooks": { + "before_specify": [ + { + "extension": "git", + "command": "speckit.git.feature", + "enabled": False, + "optional": False, + } + ] + }, + } + config_path = project_dir / ".specify" / "extensions.yml" + config_path.write_text(yaml.dump(config, default_flow_style=False)) + + ext_dir = project_dir / "worktrees-src" + ext_dir.mkdir() + data = { + "schema_version": "1.0", + "extension": { + "id": "worktrees", + "name": "Worktrees", + "version": "1.0.0", + "description": "Test", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + {"name": "speckit.worktrees.create", "file": "cmd.md", "description": "x"} + ] + }, + "hooks": { + "after_specify": { + "command": "speckit.worktrees.create", + "optional": False, + "description": "test", + } + }, + "modifies_hooks": [ + { + "hook": "before_specify", + "extension": "git", + "command": "speckit.git.feature", + "action": "disable", + "reason": "Already disabled test", + } + ], + } + manifest_path = ext_dir / "extension.yml" + manifest_path.write_text(yaml.dump(data, default_flow_style=False)) + (ext_dir / "commands").mkdir() + (ext_dir / "commands" / "speckit.worktrees.create.md").write_text("# test") + cmd_path = ext_dir / "cmd.md" + cmd_path.write_text("# test") + manifest = ExtensionManifest(manifest_path) + + input_called = False + + def fail_input(_): + nonlocal input_called + input_called = True + return "n" + + monkeypatch.setattr("builtins.input", fail_input) + + executor = HookExecutor(project_dir) + applied = executor.register_hooks(manifest) + + assert len(applied) == 0 + assert not input_called