From ea60056110b8b5f991d172f8be21eeefd2312819 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 09:08:43 -0500 Subject: [PATCH 01/14] feat: publish validated extension hook projections Keep current markdown hook behavior unchanged while generating an ordered, verified projection on extension lifecycle and shared-infrastructure refresh. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 14 ++ src/specify_cli/__init__.py | 11 +- src/specify_cli/extensions/__init__.py | 44 ++-- .../extensions/_command_update_transaction.py | 2 +- .../extensions/_hook_projection.py | 168 ++++++++++++++ src/specify_cli/extensions/command_add.py | 3 + src/specify_cli/extensions/command_disable.py | 27 ++- src/specify_cli/extensions/command_enable.py | 24 +- src/specify_cli/extensions/command_remove.py | 6 +- .../extensions/test_command_add.py | 39 ++++ .../test_command_update_transaction.py | 30 +-- .../extensions/test_hook_projection.py | 207 ++++++++++++++++++ tests/test_extensions.py | 12 +- 13 files changed, 534 insertions(+), 53 deletions(-) create mode 100644 src/specify_cli/extensions/_hook_projection.py create mode 100644 tests/specify_cli/extensions/test_hook_projection.py diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 676428a079..fefa90c7b3 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -304,6 +304,20 @@ 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. +Extension configuration saves also publish a validated projection in +`.specify/hook-dispatch/`: one `.json` response and SHA-256 digest per +event, an `events.txt` index and digest, and a `source.yml` copy of the +configuration. 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. 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. + ```yaml installed: - git 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..83baab5a1b 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2461,6 +2461,12 @@ def install_from_directory( # Load and validate manifest manifest_path = source_dir / "extension.yml" manifest = ExtensionManifest(manifest_path) + from ._hook_projection import _EVENT + + for event in manifest.hooks: + if not isinstance(event, str) or not _EVENT.fullmatch(event): + raise ValidationError(f"Invalid extension hook event: {event}") + HookExecutor(self.project_root).load_project_config_for_update() # Check compatibility self.check_compatibility(manifest, speckit_version) @@ -3543,6 +3549,8 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if not self.registry.is_installed(extension_id): return False + HookExecutor(self.project_root).load_project_config_for_update() + # Get registered commands and skills before removal metadata = self.registry.get(extension_id) registered_commands = ( @@ -5759,16 +5767,22 @@ 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 publish - 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", - ) + publish(self.config_file, config) + + def load_project_config_for_update(self) -> Dict[str, Any]: + from ._hook_projection import load_for_update + + return load_for_update(self.config_file) + + def migrate_project_config(self) -> None: + """Refresh the projection of an existing configuration without rewriting it.""" + from ._hook_projection import publish + + if 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) def register_extension(self, extension_id: str): """Add extension to the installed list in project config. @@ -5776,7 +5790,7 @@ def register_extension(self, extension_id: str): 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 +5809,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 = {} @@ -5872,7 +5886,7 @@ def register_hooks(self, manifest: ExtensionManifest): # Always ensure the extension is in the installed list self.register_extension(manifest.id) - config = self.get_project_config() + config = self.load_project_config_for_update() # Ensure config is a dict (defensive) changed = False @@ -5975,7 +5989,7 @@ def unregister_hooks(self, extension_id: str): # Always remove from installed list (Feedback from review) self.unregister_extension(extension_id) - config = self.get_project_config() + config = self.load_project_config_for_update() if not isinstance(config, dict): config = {} @@ -6251,7 +6265,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 +6284,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..02cb2f53e0 --- /dev/null +++ b/src/specify_cli/extensions/_hook_projection.py @@ -0,0 +1,168 @@ +"""Validated, transactional projection of extension hooks for future dispatchers.""" + +from __future__ import annotations + +import hashlib +import json +import os +import re +import shutil +import tempfile +from pathlib import Path +from typing import Any + +import yaml + +from ..integrations._file_changes import changing_file +from ..shared_infra import _exclusive_project_lock +from . import DEFAULT_HOOK_PRIORITY, normalize_priority + +_EVENT = re.compile(r"(before|after)_[a-z][a-z0-9_]*\Z") + + +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(): + if not isinstance(event, str) or not _EVENT.fullmatch(event): + raise ValueError(f"Invalid extension hook event: {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 _write_digest(directory: Path, name: str, payload: bytes) -> None: + (directory / name).write_bytes(payload) + (directory / f"{name}.sha256").write_text( + hashlib.sha256(payload).hexdigest() + "\n", encoding="ascii" + ) + + +def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool = False) -> None: + """Publish one complete projection and its YAML source, or restore the prior pair.""" + 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 + root.mkdir(parents=True, exist_ok=True) + with _exclusive_project_lock(root.parent, ".hook-dispatch.lock", context="hook projection"): + _check_paths(config_file) + source = config_file.read_bytes() if preserve_source else rendered + cache = _cache_path(config_file) + stage = Path(tempfile.mkdtemp(prefix=".hook-stage-", dir=root)) + backup = root / f"{stage.name}.backup" + old_config = config_file.read_bytes() if config_file.exists() else None + moved = False + installed = False + try: + if cache.exists(): + for child in cache.iterdir(): + if child.name not in {"source.yml", "events.txt", "events.txt.sha256"} and not child.name.endswith((".json", ".sha256")): + shutil.copy2(child, stage / child.name) + for event, payload in projected.items(): + _write_digest(stage, f"{event}.json", payload) + _write_digest(stage, "events.txt", "".join(f"{event}\n" for event in projected).encode("utf-8")) + (stage / "source.yml").write_bytes(source) + with changing_file(cache): + if cache.exists(): + os.replace(cache, backup) + moved = True + os.replace(stage, cache) + installed = True + if not preserve_source: + temporary = root / f"{stage.name}.yml" + try: + temporary.write_bytes(rendered) + with changing_file(config_file): + os.replace(temporary, config_file) + finally: + temporary.unlink(missing_ok=True) + except Exception as exc: + try: + if installed: + with changing_file(cache, removal=True): + shutil.rmtree(cache) + if moved: + with changing_file(cache): + os.replace(backup, cache) + if not preserve_source: + if old_config is None: + with changing_file(config_file, removal=True): + config_file.unlink(missing_ok=True) + elif config_file.read_bytes() != old_config: + with changing_file(config_file): + config_file.write_bytes(old_config) + except Exception as rollback_exc: + raise OSError(f"Hook projection publication failed ({exc}); rollback failed: {rollback_exc}") from exc + raise + finally: + if stage.exists(): + shutil.rmtree(stage) + if moved: + shutil.rmtree(backup) 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..fb31453ac4 100644 --- a/src/specify_cli/extensions/command_disable.py +++ b/src/specify_cli/extensions/command_disable.py @@ -17,7 +17,7 @@ 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, HookExecutor project_root = _commands._require_specify_project() manager = ExtensionManager(project_root) @@ -29,6 +29,12 @@ def extension_disable( extension, installed, "disable" ) + try: + config = hook_executor.load_project_config_for_update() + except (ValueError, OSError) as exc: + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc + # Update registry metadata = manager.registry.get(extension_id) if metadata is None or not isinstance(metadata, dict): @@ -45,8 +51,6 @@ def extension_disable( 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: @@ -56,13 +60,26 @@ def extension_disable( 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) + try: + hook_executor.save_project_config(config) + except (ValueError, OSError) as exc: + try: + manager.registry.update(extension_id, {"enabled": True}) + if load_init_options(project_root).get("ai") == "generic": + manager.register_enabled_extensions_for_agent("generic") + except (ExtensionError, ValueError, OSError) as rollback_exc: + console.print( + f"[red]Error:[/red] {_escape_markup(str(exc))}; " + f"rollback failed: {_escape_markup(str(rollback_exc))}" + ) + raise typer.Exit(1) from exc + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc console.print(f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' disabled") console.print("\nCommands will no longer be available. Hooks will not execute.") diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index 34bab2f279..b6cb82b4a6 100644 --- a/src/specify_cli/extensions/command_enable.py +++ b/src/specify_cli/extensions/command_enable.py @@ -29,6 +29,12 @@ def extension_enable( extension, installed, "enable" ) + try: + config = hook_executor.load_project_config_for_update() + except (ValueError, OSError) as exc: + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc + # Update registry metadata = manager.registry.get(extension_id) if metadata is None or not isinstance(metadata, dict): @@ -83,13 +89,27 @@ def extension_enable( 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) + try: + hook_executor.save_project_config(config) + except (ValueError, OSError) as exc: + try: + if init_options.get("ai") == "generic": + manager.disable_generic_extension_artifacts(extension_id) + else: + manager.registry.update(extension_id, {"enabled": False}) + except (ExtensionError, ValueError, OSError) as rollback_exc: + console.print( + f"[red]Error:[/red] {_escape_markup(str(exc))}; " + f"rollback failed: {_escape_markup(str(rollback_exc))}" + ) + raise typer.Exit(1) from exc + console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") + raise typer.Exit(1) from exc console.print(f"[green]✓[/green] Extension '{_escape_markup(str(display_name))}' enabled") diff --git a/src/specify_cli/extensions/command_remove.py b/src/specify_cli/extensions/command_remove.py index c210dae96b..52b30729bf 100644 --- a/src/specify_cli/extensions/command_remove.py +++ b/src/specify_cli/extensions/command_remove.py @@ -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 (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/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index cddcd6bf21..3d9819c33a 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -38,6 +38,45 @@ class TestExtensionAddCLI: """CLI tests for ``specify extension add``.""" + def test_add_publishes_hooks_without_changing_command_content(self, extension_dir, project_dir): + 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"}, + ] + } + 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 + rendered = (project_dir / ".github/agents/speckit.test-ext.hello.agent.md").read_text() + assert "hook-dispatch" not in rendered + + def test_add_rejects_invalid_existing_hooks_before_install(self, extension_dir, 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_dev_links_copilot_agent_when_supported( self, extension_dir, project_dir, temp_dir ): diff --git a/tests/specify_cli/extensions/test_command_update_transaction.py b/tests/specify_cli/extensions/test_command_update_transaction.py index 28efb46833..1f6c1d407c 100644 --- a/tests/specify_cli/extensions/test_command_update_transaction.py +++ b/tests/specify_cli/extensions/test_command_update_transaction.py @@ -153,21 +153,16 @@ 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.""" + """Malformed hooks must prevent update before any destructive phase.""" monkeypatch.chdir(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 +170,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..c98b4f2024 --- /dev/null +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -0,0 +1,207 @@ +"""Projection contract and publication failures without changing hook reads.""" + +import hashlib +import json +from pathlib import Path + +import pytest +import yaml + +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 _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_save_projects_only_unconditional_enabled_hooks_in_yaml_order(tmp_path): + executor = HookExecutor(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": {"../before_plan": []}}, + {"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 = HookExecutor(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 = HookExecutor(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_refuse_unsafe_cache_and_invalid_existing_config(tmp_path): + executor = HookExecutor(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 = HookExecutor(tmp_path) + executor.config_file.parent.mkdir() + 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_publication_failure_restores_prior_pair(tmp_path, monkeypatch): + import specify_cli.extensions._hook_projection as projection + + executor = HookExecutor(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_replace = projection.os.replace + + def fail_config_write(src, dst): + if Path(dst) == executor.config_file: + raise OSError("simulated config write failure") + return real_replace(src, dst) + + monkeypatch.setattr(projection.os, "replace", fail_config_write) + with pytest.raises(OSError, match="simulated config"): + 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 + _assert_snapshot(tmp_path) + assert not list((tmp_path / ".specify").glob(".hook-stage-*")) + + +def test_existing_yaml_refresh_keeps_legacy_read_semantics(tmp_path): + executor = HookExecutor(tmp_path) + executor.config_file.parent.mkdir() + 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 = HookExecutor(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 = HookExecutor(tmp_path) + executor.config_file.parent.mkdir() + 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 + + +@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_extensions.py b/tests/test_extensions.py index b512742389..c143f7809f 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -8154,8 +8154,7 @@ 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.""" + """A corrupt on-disk entry cannot be silently discarded by reinstall.""" executor = HookExecutor(project_dir) executor.register_hooks( _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.go"}}) @@ -8165,12 +8164,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 From 3b0fc502498024f303c78b13c684e9db502e5714 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 09:14:16 -0500 Subject: [PATCH 02/14] fix: journal projection lock during integration rollback Record newly created hook lock files in the integration lifecycle journal so a failed install restores the prior project snapshot. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- src/specify_cli/extensions/_hook_projection.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index 02cb2f53e0..4b8e076c53 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -111,7 +111,9 @@ def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool rendered = yaml.dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True).encode("utf-8") root = config_file.parent root.mkdir(parents=True, exist_ok=True) - with _exclusive_project_lock(root.parent, ".hook-dispatch.lock", context="hook projection"): + with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( + root.parent, ".hook-dispatch.lock", context="hook projection" + ): _check_paths(config_file) source = config_file.read_bytes() if preserve_source else rendered cache = _cache_path(config_file) From c5d255ef1351c923a61764b5c35d5b290188fd45 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 09:33:16 -0500 Subject: [PATCH 03/14] test: reject malformed hook state before lifecycle changes Keep registration regression coverage for corrupted configuration while asserting that publication now fails explicitly and preserves the original YAML. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- tests/test_extension_registration.py | 72 +++++++++++----------------- 1 file changed, 28 insertions(+), 44 deletions(-) diff --git a/tests/test_extension_registration.py b/tests/test_extension_registration.py index 4dc4d1beb2..df1f1f7c0d 100644 --- a/tests/test_extension_registration.py +++ b/tests/test_extension_registration.py @@ -171,20 +171,17 @@ def test_unregister_hooks_no_hooks_key(self, project_dir): 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.""" + """Invalid existing 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.""" @@ -300,7 +297,7 @@ def test_register_extension_mixed_type_installed(self, project_dir): 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().""" + """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 +307,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.""" + """A corrupt existing event list must prevent registration.""" executor = HookExecutor(project_dir) # Manually write a config with null hook event value @@ -349,13 +346,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.""" @@ -423,7 +418,7 @@ 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.""" + """Malformed hook entries must not be silently dropped on registration.""" executor = HookExecutor(project_dir) config_path = project_dir / ".specify" / "extensions.yml" @@ -454,33 +449,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) - - 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) + original = config_path.read_bytes() + with pytest.raises(ValueError, match="expected a mapping"): + executor.register_hooks(manifest) + assert config_path.read_bytes() == original def test_unregister_extension_scalar_root(self, project_dir): - """Hardening: unregister_extension should handle scalar root config.""" + """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.""" + """A scalar event list blocks removal without deleting the old state.""" executor = HookExecutor(project_dir) config_path = project_dir / ".specify" / "extensions.yml" @@ -489,9 +475,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 From 66de016240cfc6280d2b3d50a839b7c0566dc101 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 10:03:07 -0500 Subject: [PATCH 04/14] fix: preflight and recover hook projection publication Validate manifest hook fields before installation, avoid staging leaks and post-commit cleanup failures, and cover CLI rollback paths for generic artifacts. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- src/specify_cli/extensions/__init__.py | 15 ++++++--- .../extensions/_hook_projection.py | 12 +++++-- tests/specify_cli/extensions/_helpers.py | 32 +++++++++++++++++++ .../extensions/test_command_add.py | 21 ++++++++++++ .../extensions/test_command_disable.py | 17 ++++++++++ .../extensions/test_command_enable.py | 21 ++++++++++++ .../extensions/test_command_remove.py | 14 ++++++++ .../extensions/test_hook_projection.py | 32 +++++++++++++++++++ 8 files changed, 158 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 83baab5a1b..a7c0acc7a4 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2461,11 +2461,18 @@ def install_from_directory( # Load and validate manifest manifest_path = source_dir / "extension.yml" manifest = ExtensionManifest(manifest_path) - from ._hook_projection import _EVENT + from ._hook_projection import project_hooks - for event in manifest.hooks: - if not isinstance(event, str) or not _EVENT.fullmatch(event): - raise ValidationError(f"Invalid extension hook event: {event}") + 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 diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index 4b8e076c53..e952131f9b 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -4,6 +4,7 @@ import hashlib import json +import logging import os import re import shutil @@ -117,9 +118,9 @@ def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool _check_paths(config_file) source = config_file.read_bytes() if preserve_source else rendered cache = _cache_path(config_file) + old_config = config_file.read_bytes() if config_file.exists() else None stage = Path(tempfile.mkdtemp(prefix=".hook-stage-", dir=root)) backup = root / f"{stage.name}.backup" - old_config = config_file.read_bytes() if config_file.exists() else None moved = False installed = False try: @@ -167,4 +168,11 @@ def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool if stage.exists(): shutil.rmtree(stage) if moved: - shutil.rmtree(backup) + try: + shutil.rmtree(backup) + except OSError as exc: + logging.getLogger(__name__).warning( + "Hook projection committed; could not remove backup %s: %s", + backup, + exc, + ) diff --git a/tests/specify_cli/extensions/_helpers.py b/tests/specify_cli/extensions/_helpers.py index ef1bd845d2..8f2fce1cc4 100644 --- a/tests/specify_cli/extensions/_helpers.py +++ b/tests/specify_cli/extensions/_helpers.py @@ -4,6 +4,8 @@ import os from pathlib import Path +import yaml + MINIMAL_ZIP_BYTES = b"PK\x05\x06" + b"\x00" * 18 @@ -44,3 +46,33 @@ 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): + """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"}) + 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 3d9819c33a..1ab8207595 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -77,6 +77,27 @@ def test_add_rejects_invalid_existing_hooks_before_install(self, extension_dir, assert config.read_text() == "hooks:\n before_plan: invalid\n" 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 + ): + 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..1e60c01fa6 100644 --- a/tests/specify_cli/extensions/test_command_disable.py +++ b/tests/specify_cli/extensions/test_command_disable.py @@ -16,6 +16,7 @@ ExtensionRegistry, HookExecutor, ) +from tests.specify_cli.extensions._helpers import installed_generic_hook_extension class TestExtensionDisableCLI: @@ -53,3 +54,19 @@ 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) + 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 diff --git a/tests/specify_cli/extensions/test_command_enable.py b/tests/specify_cli/extensions/test_command_enable.py index 3543917af0..3f6131cba6 100644 --- a/tests/specify_cli/extensions/test_command_enable.py +++ b/tests/specify_cli/extensions/test_command_enable.py @@ -14,7 +14,9 @@ from specify_cli.extensions import ( ExtensionManager, ExtensionRegistry, + HookExecutor, ) +from tests.specify_cli.extensions._helpers import installed_generic_hook_extension class TestExtensionEnableCLI: @@ -50,3 +52,22 @@ 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) + 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 diff --git a/tests/specify_cli/extensions/test_command_remove.py b/tests/specify_cli/extensions/test_command_remove.py index a7a340b401..82df2de185 100644 --- a/tests/specify_cli/extensions/test_command_remove.py +++ b/tests/specify_cli/extensions/test_command_remove.py @@ -100,3 +100,17 @@ 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 + ): + 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" diff --git a/tests/specify_cli/extensions/test_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py index c98b4f2024..72b044a071 100644 --- a/tests/specify_cli/extensions/test_hook_projection.py +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -153,6 +153,38 @@ def fail_config_write(src, dst): assert not list((tmp_path / ".specify").glob(".hook-stage-*")) +def test_unreadable_existing_config_does_not_allocate_stage(tmp_path): + executor = HookExecutor(tmp_path) + executor.config_file.parent.mkdir() + executor.config_file.mkdir() + with pytest.raises(OSError): + executor.save_project_config(_config()) + assert not list(executor.config_file.parent.glob(".hook-stage-*")) + assert not (tmp_path / ".specify/hook-dispatch").exists() + + +def test_backup_cleanup_failure_warns_after_successful_publication(tmp_path, monkeypatch, caplog): + import specify_cli.extensions._hook_projection as projection + + executor = HookExecutor(tmp_path) + executor.save_project_config(_config()) + real_rmtree = projection.shutil.rmtree + + def fail_backup_cleanup(path, *args, **kwargs): + if str(path).endswith(".backup"): + raise OSError("backup cleanup failed") + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(projection.shutil, "rmtree", fail_backup_cleanup) + updated = {"installed": ["example"], "hooks": {"after_plan": []}} + executor.save_project_config(updated) + assert "backup cleanup failed" in caplog.text + 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 not (tmp_path / ".specify/hook-dispatch/before_plan.json").exists() + + def test_existing_yaml_refresh_keeps_legacy_read_semantics(tmp_path): executor = HookExecutor(tmp_path) executor.config_file.parent.mkdir() From d1b7e41ae8a510f2d11aab416c38bb7fe555f3f1 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 11:32:46 -0500 Subject: [PATCH 05/14] fix(extensions): gate hook projection and restore lifecycle on failure Preserve legacy markdown hook behavior by default; opt in to strict event projection with native_extension_hooks. Keep source and projection synchronized on refresh, roll back installation/removal publication failures, and surface unsuccessful artifact restoration. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 39 +++- src/specify_cli/extensions/__init__.py | 177 ++++++++++++------ .../extensions/_hook_projection.py | 63 ++++++- src/specify_cli/extensions/command_disable.py | 37 ++++ tests/specify_cli/extensions/_helpers.py | 17 +- .../extensions/test_command_add.py | 81 +++++++- .../extensions/test_command_disable.py | 17 +- .../extensions/test_command_enable.py | 4 +- .../extensions/test_command_remove.py | 46 +++++ .../extensions/test_command_update.py | 55 ++++++ .../test_command_update_transaction.py | 5 +- .../extensions/test_hook_projection.py | 172 +++++++++++++++-- tests/test_extension_registration.py | 22 ++- tests/test_extensions.py | 8 +- 14 files changed, 647 insertions(+), 96 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index fefa90c7b3..e6ef7facf6 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -304,15 +304,27 @@ 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. -Extension configuration saves also publish a validated projection in +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. + +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 and digest, and a `source.yml` copy of the -configuration. Only enabled hooks with no condition appear in the projection, +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. Current installed +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 @@ -364,7 +376,24 @@ 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. 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 lowercase letters, digits, underscores, or hyphens; 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/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index a7c0acc7a4..435735f04d 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2461,18 +2461,19 @@ def install_from_directory( # Load and validate manifest manifest_path = source_dir / "extension.yml" manifest = ExtensionManifest(manifest_path) - from ._hook_projection import project_hooks + from ._hook_projection import projection_enabled, project_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 + if projection_enabled(self.project_root): + 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 @@ -3068,33 +3069,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 + 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 - elif not path.is_file(): - if skills and path.parent.is_dir(): + 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: @@ -3121,10 +3123,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 @@ -3155,6 +3159,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, @@ -3200,9 +3205,13 @@ 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: + # Keep the existing non-generic retry semantics for failures after + # hooks have committed, but retire every agent's outputs when hook + # publication itself fails. + 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: @@ -3214,14 +3223,31 @@ def rollback_generic_registration() -> None: 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 @@ -3556,7 +3582,11 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if not self.registry.is_installed(extension_id): return False - HookExecutor(self.project_root).load_project_config_for_update() + from ._hook_projection import projection_enabled + + hook_executor = HookExecutor(self.project_root) + projection_active = projection_enabled(self.project_root) + hook_executor.load_project_config_for_update() # Get registered commands and skills before removal metadata = self.registry.get(extension_id) @@ -3572,6 +3602,12 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: extension_dir = self.extensions_dir / extension_id + # Publication can fail even after preflight (for example, on a disk + # error). Keep all invocation artifacts and sources intact until the + # extension has been removed from the hook configuration. + if projection_active: + hook_executor.unregister_hooks(extension_id) + # Unregister commands from all AI agents if registered_commands: safe_commands = dict(registered_commands) @@ -3631,9 +3667,8 @@ 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) + if not projection_active: + hook_executor.unregister_hooks(extension_id) # Update registry self.registry.remove(extension_id) @@ -5774,22 +5809,42 @@ def save_project_config(self, config: Dict[str, Any]): Args: config: Configuration dictionary to save """ - from ._hook_projection import publish + from ._hook_projection import projection_enabled, publish, retire - publish(self.config_file, config) + 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 + from ._hook_projection import load_for_update, projection_enabled - return load_for_update(self.config_file) + 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 publish + from ._hook_projection import projection_enabled, publish, retire - if self.config_file.exists() or self.config_file.is_symlink(): + 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. @@ -5890,9 +5945,11 @@ 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 + 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) @@ -5900,6 +5957,11 @@ def register_hooks(self, manifest: ExtensionManifest): 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): @@ -5993,18 +6055,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 + 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 diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index e952131f9b..998def9acb 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -14,11 +14,37 @@ import yaml +from .._init_options import load_init_options from ..integrations._file_changes import changing_file from ..shared_infra import _exclusive_project_lock from . import DEFAULT_HOOK_PRIORITY, normalize_priority -_EVENT = re.compile(r"(before|after)_[a-z][a-z0-9_]*\Z") +_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 " + "letters, digits, underscores, or hyphens. " + "Rename legacy events such as custom:after before installing or refreshing." + ) def _cache_path(config_file: Path) -> Path: @@ -45,8 +71,7 @@ def project_hooks(config: dict[str, Any]) -> dict[str, bytes]: raise ValueError("Invalid extension hooks: expected a mapping") projected: dict[str, bytes] = {} for event, entries in hooks.items(): - if not isinstance(event, str) or not _EVENT.fullmatch(event): - raise ValueError(f"Invalid extension hook event: {event}") + validate_event_name(event) if not isinstance(entries, list): raise ValueError(f"Invalid extension hooks.{event}: expected a list") selected = [] @@ -99,6 +124,21 @@ def load_for_update(config_file: Path) -> dict[str, Any]: 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 + root = config_file.parent + with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( + root.parent, ".hook-dispatch.lock", context="hook projection" + ): + _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: (directory / name).write_bytes(payload) (directory / f"{name}.sha256").write_text( @@ -108,15 +148,26 @@ def _write_digest(directory: Path, name: str, payload: bytes) -> None: def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool = False) -> None: """Publish one complete projection and its YAML source, or restore the prior pair.""" - projected = project_hooks(config) - rendered = yaml.dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True).encode("utf-8") + 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 root.mkdir(parents=True, exist_ok=True) with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( root.parent, ".hook-dispatch.lock", context="hook projection" ): _check_paths(config_file) - source = config_file.read_bytes() if preserve_source else rendered + 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) old_config = config_file.read_bytes() if config_file.exists() else None stage = Path(tempfile.mkdtemp(prefix=".hook-stage-", dir=root)) diff --git a/src/specify_cli/extensions/command_disable.py b/src/specify_cli/extensions/command_disable.py index fb31453ac4..0de37fa43b 100644 --- a/src/specify_cli/extensions/command_disable.py +++ b/src/specify_cli/extensions/command_disable.py @@ -72,6 +72,43 @@ def extension_disable( manager.registry.update(extension_id, {"enabled": True}) if load_init_options(project_root).get("ai") == "generic": manager.register_enabled_extensions_for_agent("generic") + restored = manager.registry.get(extension_id) or {} + if restored.get("enabled") is not True: + raise ExtensionError("Rollback did not restore enabled registry state") + previous_commands = metadata.get("registered_commands", {}) + command_names = ( + previous_commands.get("generic", []) + if isinstance(previous_commands, dict) else [] + ) + skill_names = metadata.get("registered_skills", []) + for names, skills in ((command_names, False), (skill_names, 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 = manager._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)) + ) except (ExtensionError, ValueError, OSError) as rollback_exc: console.print( f"[red]Error:[/red] {_escape_markup(str(exc))}; " diff --git a/tests/specify_cli/extensions/_helpers.py b/tests/specify_cli/extensions/_helpers.py index 8f2fce1cc4..8b47b76d99 100644 --- a/tests/specify_cli/extensions/_helpers.py +++ b/tests/specify_cli/extensions/_helpers.py @@ -10,6 +10,14 @@ 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 @@ -48,7 +56,9 @@ def can_create_symlink(tmp_path: Path) -> bool: return link.is_symlink() -def installed_generic_hook_extension(tmp_path: Path, extension_dir: Path): +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 @@ -70,7 +80,10 @@ def installed_generic_hook_extension(tmp_path: Path, extension_dir: Path): project, version="1.0.0", integration_key="generic", settings={"generic": {"parsed_options": options}}, ) - save_init_options(project, {"ai": "generic", "ai_skills": False, "script": "sh"}) + 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" diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index 1ab8207595..7055373a53 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, ) @@ -39,13 +41,18 @@ 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) @@ -62,10 +69,14 @@ def test_add_publishes_hooks_without_changing_command_content(self, extension_di 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 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): @@ -77,6 +88,73 @@ def test_add_rejects_invalid_existing_hooks_before_install(self, extension_dir, 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() + config = project_dir / ".specify/extensions.yml" + if config.exists(): + assert "test-ext" not in (config.read_text(encoding="utf-8")) + cache = project_dir / ".specify/hook-dispatch" + if cache.exists(): + assert (cache / "source.yml").read_bytes() == config.read_bytes() + + def test_default_add_accepts_legacy_hook_event_without_projection(self, extension_dir, project_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") + 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() + + def test_opted_in_add_rejects_legacy_hook_event_before_mutation( + self, extension_dir, project_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"] = {"custom:after": {"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 + 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", []), @@ -84,6 +162,7 @@ def test_add_rejects_invalid_existing_hooks_before_install(self, extension_dir, 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}} diff --git a/tests/specify_cli/extensions/test_command_disable.py b/tests/specify_cli/extensions/test_command_disable.py index 1e60c01fa6..60e5896bf7 100644 --- a/tests/specify_cli/extensions/test_command_disable.py +++ b/tests/specify_cli/extensions/test_command_disable.py @@ -58,7 +58,9 @@ def test_disable_reenable_hint_escapes_extension_id_markup(self, tmp_path): 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) + 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() @@ -70,3 +72,16 @@ def test_disable_publication_failure_restores_generic_registry_and_artifact( 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() diff --git a/tests/specify_cli/extensions/test_command_enable.py b/tests/specify_cli/extensions/test_command_enable.py index 3f6131cba6..c4df5624c7 100644 --- a/tests/specify_cli/extensions/test_command_enable.py +++ b/tests/specify_cli/extensions/test_command_enable.py @@ -56,7 +56,9 @@ def test_enable_registry_error_escapes_extension_id_markup(self, tmp_path): def test_enable_publication_failure_restores_generic_disabled_state( self, tmp_path, extension_dir ): - project, _, artifact = installed_generic_hook_extension(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 diff --git a/tests/specify_cli/extensions/test_command_remove.py b/tests/specify_cli/extensions/test_command_remove.py index 82df2de185..903bba9c8f 100644 --- a/tests/specify_cli/extensions/test_command_remove.py +++ b/tests/specify_cli/extensions/test_command_remove.py @@ -10,11 +10,17 @@ from unittest.mock import patch 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 +33,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.""" @@ -104,6 +122,7 @@ def test_remove_output_escapes_extension_id_markup(self, tmp_path): 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") @@ -114,3 +133,30 @@ def test_remove_reports_invalid_hooks_without_mutating_installation( 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_retains_generic_installation( + 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 = (artifact.read_bytes(), config.read_bytes(), (cache / "source.yml").read_bytes()) + original_save = HookExecutor.save_project_config + + def fail_hook_removal(executor, updated): + if not updated.get("hooks", {}).get("before_plan"): + raise OSError("projection failed") + return original_save(executor, updated) + + with patch.object(Path, "cwd", return_value=project), \ + patch.object(HookExecutor, "save_project_config", fail_hook_removal): + result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) + assert result.exit_code == 1 + assert "projection failed" in result.output + assert manager.registry.is_installed("test-ext") + assert extension_dir.is_dir() + assert (artifact.read_bytes(), config.read_bytes(), (cache / "source.yml").read_bytes()) == original 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 1f6c1d407c..aad5069b75 100644 --- a/tests/specify_cli/extensions/test_command_update_transaction.py +++ b/tests/specify_cli/extensions/test_command_update_transaction.py @@ -153,8 +153,11 @@ 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): - """Malformed hooks must prevent update before any destructive phase.""" + """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" # Read-only queries tolerate this, but mutations must not discard it. diff --git a/tests/specify_cli/extensions/test_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py index 72b044a071..b08709e2be 100644 --- a/tests/specify_cli/extensions/test_hook_projection.py +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -2,11 +2,13 @@ import hashlib import json +from contextlib import contextmanager 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 @@ -27,6 +29,11 @@ def _config() -> dict: } +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() @@ -39,8 +46,69 @@ def _assert_snapshot(root: Path, event: str = "before_plan") -> dict: return json.loads(payload) -def test_save_projects_only_unconditional_enabled_hooks_in_yaml_order(tmp_path): +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 == { @@ -62,14 +130,20 @@ def test_save_projects_only_unconditional_enabled_hooks_in_yaml_order(tmp_path): @pytest.mark.parametrize("bad", [ + {"hooks": {42: []}}, + {"hooks": {"custom:after": []}}, {"hooks": {"../before_plan": []}}, + {"hooks": {"before_speckit.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 = HookExecutor(tmp_path) + 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() @@ -80,7 +154,7 @@ def test_invalid_inputs_leave_existing_config_and_projection_unchanged(tmp_path, def test_update_and_refresh_retire_stale_events_and_preserve_crlf(tmp_path): - executor = HookExecutor(tmp_path) + executor = _enabled_executor(tmp_path) executor.save_project_config(_config()) updated = _config() updated["hooks"] = {"after_tasks": []} @@ -93,8 +167,33 @@ def test_update_and_refresh_retire_stale_events_and_preserve_crlf(tmp_path): assert b"\r\n" in executor.config_file.read_bytes() +def test_refresh_projects_exact_source_read_under_lock(tmp_path, monkeypatch): + import specify_cli.extensions._hook_projection as projection + + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + updated = {"installed": ["example"], "hooks": {"after_tasks": []}} + original_lock = projection._exclusive_project_lock + + @contextmanager + def concurrent_save(*args, **kwargs): + with original_lock(*args, **kwargs): + executor.config_file.write_text( + yaml.safe_dump(updated, sort_keys=False), encoding="utf-8" + ) + yield + + monkeypatch.setattr(projection, "_exclusive_project_lock", concurrent_save) + executor.migrate_project_config() + cache = tmp_path / ".specify/hook-dispatch" + assert (cache / "source.yml").read_bytes() == executor.config_file.read_bytes() + assert (cache / "events.txt").read_bytes() == b"after_tasks\n" + assert json.loads((cache / "after_tasks.json").read_text())["hooks"] == [] + assert not (cache / "before_plan.json").exists() + + def test_refuse_unsafe_cache_and_invalid_existing_config(tmp_path): - executor = HookExecutor(tmp_path) + executor = _enabled_executor(tmp_path) executor.save_project_config(_config()) cache = tmp_path / ".specify/hook-dispatch" (cache / "unexpected").mkdir() @@ -111,8 +210,8 @@ def test_refuse_unsafe_cache_and_invalid_existing_config(tmp_path): def test_refuse_symlinked_config_and_cache_before_mutation(tmp_path): - executor = HookExecutor(tmp_path) - executor.config_file.parent.mkdir() + 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: @@ -133,7 +232,7 @@ def test_refuse_symlinked_config_and_cache_before_mutation(tmp_path): def test_publication_failure_restores_prior_pair(tmp_path, monkeypatch): import specify_cli.extensions._hook_projection as projection - executor = HookExecutor(tmp_path) + 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() @@ -154,8 +253,8 @@ def fail_config_write(src, dst): def test_unreadable_existing_config_does_not_allocate_stage(tmp_path): - executor = HookExecutor(tmp_path) - executor.config_file.parent.mkdir() + 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()) @@ -166,7 +265,7 @@ def test_unreadable_existing_config_does_not_allocate_stage(tmp_path): def test_backup_cleanup_failure_warns_after_successful_publication(tmp_path, monkeypatch, caplog): import specify_cli.extensions._hook_projection as projection - executor = HookExecutor(tmp_path) + executor = _enabled_executor(tmp_path) executor.save_project_config(_config()) real_rmtree = projection.shutil.rmtree @@ -186,8 +285,8 @@ def fail_backup_cleanup(path, *args, **kwargs): def test_existing_yaml_refresh_keeps_legacy_read_semantics(tmp_path): - executor = HookExecutor(tmp_path) - executor.config_file.parent.mkdir() + 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") @@ -197,7 +296,7 @@ def test_existing_yaml_refresh_keeps_legacy_read_semantics(tmp_path): def test_enable_disable_remove_republish_without_stale_hooks(tmp_path): - executor = HookExecutor(tmp_path) + executor = _enabled_executor(tmp_path) executor.save_project_config(_config()) executor.disable_hooks("example") assert _assert_snapshot(tmp_path)["hooks"] == [] @@ -213,8 +312,8 @@ def test_enable_disable_remove_republish_without_stale_hooks(tmp_path): def test_shared_infrastructure_refresh_projects_existing_config(tmp_path, monkeypatch): import specify_cli - executor = HookExecutor(tmp_path) - executor.config_file.parent.mkdir() + 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) @@ -228,6 +327,49 @@ def test_project_hooks_is_pure(): 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_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_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), diff --git a/tests/test_extension_registration.py b/tests/test_extension_registration.py index df1f1f7c0d..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,8 +178,8 @@ 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): - """Invalid existing configuration must not be overwritten by removal.""" + 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"])) @@ -296,7 +304,7 @@ 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): + 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) @@ -313,7 +321,7 @@ def test_unregister_hooks_null_hook_values(self, project_dir): assert config_path.read_bytes() == original - def test_register_hooks_corrupted_hook_values(self, project_dir, tmp_path): + 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) @@ -417,7 +425,7 @@ 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): + 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) @@ -454,7 +462,7 @@ def test_register_hooks_mixed_type_hook_list(self, project_dir, tmp_path): executor.register_hooks(manifest) assert config_path.read_bytes() == original - def test_unregister_extension_scalar_root(self, project_dir): + 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" @@ -465,7 +473,7 @@ def test_unregister_extension_scalar_root(self, project_dir): executor.unregister_extension("any-ext") assert yaml.safe_load(config_path.read_text()) == 123 - def test_unregister_hooks_scalar_hook_values(self, project_dir): + 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" diff --git a/tests/test_extensions.py b/tests/test_extensions.py index c143f7809f..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,7 +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 on-disk entry cannot be silently discarded by reinstall.""" + """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"}}) From 6ec697ee4d349338a9574a7eb56d340fd0a29569 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 11:34:48 -0500 Subject: [PATCH 06/14] fix(extensions): require underscore-normalized hook events Reject hyphenated names only when native extension hooks are enabled; document explicit migration and retain legacy default acceptance. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 6 ++++-- src/specify_cli/extensions/_hook_projection.py | 7 ++++--- tests/specify_cli/extensions/test_command_add.py | 14 ++++++++++---- .../specify_cli/extensions/test_hook_projection.py | 5 ++++- 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index e6ef7facf6..2857cc4456 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -385,8 +385,10 @@ targets use `before_` / `after_` (for example, `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 lowercase letters, digits, underscores, or hyphens; the -full event name must fit in a projection filename. +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 diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index 998def9acb..a3b0e35a13 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -19,7 +19,7 @@ from ..shared_infra import _exclusive_project_lock from . import DEFAULT_HOOK_PRIORITY, normalize_priority -_EVENT = re.compile(r"(before|after)_[a-z0-9][a-z0-9_-]*\Z") +_EVENT = re.compile(r"(before|after)_[a-z0-9][a-z0-9_]*\Z") def projection_enabled(project_root: Path) -> bool: @@ -42,8 +42,9 @@ def validate_event_name(event: object) -> None: raise ValueError( f"Invalid extension hook event {event!r}: use before_ " "or after_ with a lowercase command name containing " - "letters, digits, underscores, or hyphens. " - "Rename legacy events such as custom:after before installing or refreshing." + "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." ) diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index 7055373a53..17d842add1 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -124,10 +124,13 @@ def fail_hook_save(executor, config): if cache.exists(): assert (cache / "source.yml").read_bytes() == config.read_bytes() - def test_default_add_accepts_legacy_hook_event_without_projection(self, extension_dir, project_dir): + @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"] = {"custom:after": {"command": "speckit.test-ext.hello"}} + 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( @@ -138,13 +141,14 @@ def test_default_add_accepts_legacy_hook_event_without_projection(self, extensio 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 + 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"] = {"custom:after": {"command": "speckit.test-ext.hello"}} + 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( @@ -152,6 +156,8 @@ def test_opted_in_add_rejects_legacy_hook_event_before_mutation( ) 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() diff --git a/tests/specify_cli/extensions/test_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py index b08709e2be..c7a3f86249 100644 --- a/tests/specify_cli/extensions/test_hook_projection.py +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -134,6 +134,8 @@ def test_save_projects_only_unconditional_enabled_hooks_in_yaml_order(tmp_path): {"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: []}}, @@ -338,6 +340,7 @@ def test_arbitrary_command_event_refresh_preserves_hook_reader_behavior(tmp_path "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)) @@ -353,7 +356,7 @@ def test_arbitrary_command_event_refresh_preserves_hook_reader_behavior(tmp_path 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_2fa_assess\n" + 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" From 7d33e9126012ebc49fa3db9eeb63a98846c8064c Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:39:07 -0500 Subject: [PATCH 07/14] Restore projected hooks after failed extension removal Preserve disabled manifest hooks in opted-in installs, move generic disable rollback verification into the domain owner, and clarify the opt-in naming contract. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 12 +- src/specify_cli/extensions/__init__.py | 177 ++++++++++++------ src/specify_cli/extensions/command_disable.py | 42 +---- src/specify_cli/extensions/command_remove.py | 4 +- .../extensions/test_command_add.py | 29 +++ .../extensions/test_command_disable.py | 18 ++ .../extensions/test_command_remove.py | 70 +++++++ 7 files changed, 248 insertions(+), 104 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 2857cc4456..0834780c41 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -330,6 +330,11 @@ 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. +If an opted-in removal fails during subsequent cleanup, Spec Kit restores the +prior hook configuration and projection or reports a rollback failure. This +does not make removal of command artifacts and extension sources atomic; +cleanup already completed may not be reversed. + ```yaml installed: - git @@ -376,9 +381,10 @@ 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. New manifests and project -configuration writes require `before_` or -`after_`, including commands beyond the built-in set. Core +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__` / diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 435735f04d..20853f055e 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -3586,7 +3586,7 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: hook_executor = HookExecutor(self.project_root) projection_active = projection_enabled(self.project_root) - hook_executor.load_project_config_for_update() + original_hook_config = hook_executor.load_project_config_for_update() # Get registered commands and skills before removal metadata = self.registry.get(extension_id) @@ -3608,73 +3608,128 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if projection_active: hook_executor.unregister_hooks(extension_id) - # Unregister commands from all AI agents - if registered_commands: - safe_commands = dict(registered_commands) - if "generic" in safe_commands: - safe_commands.pop("generic") - if safe_commands: - CommandRegistrar(self.project_root, include_generic=False).unregister_commands( - safe_commands, self.project_root - ) - if metadata: - self._remove_generic_artifact_paths(extension_id, metadata) + try: + # Unregister commands from all AI agents + if registered_commands: + safe_commands = dict(registered_commands) + if "generic" in safe_commands: + safe_commands.pop("generic") + if safe_commands: + CommandRegistrar(self.project_root, include_generic=False).unregister_commands( + safe_commands, self.project_root + ) + if metadata: + self._remove_generic_artifact_paths(extension_id, metadata) - # Unregister agent skills - self._unregister_extension_skills( - registered_skills, - extension_id, - generic_hashes=metadata.get("generic_artifact_hashes") if metadata else None, - ) + # Unregister agent skills + self._unregister_extension_skills( + registered_skills, + extension_id, + generic_hashes=metadata.get("generic_artifact_hashes") if metadata else None, + ) - if keep_config: - # Preserve config files, only remove non-config files - if extension_dir.exists(): - for child in extension_dir.iterdir(): - # Keep top-level *-config.yml and *-config.local.yml files - if child.is_file() and ( - child.name.endswith("-config.yml") - or child.name.endswith("-config.local.yml") - ): - continue - if child.is_dir(): - shutil.rmtree(child) - else: - child.unlink() - # Write a provenance marker so install_from_directory can - # distinguish this --keep-config leftover from a directory left - # by a partially-failed install (which must not have its - # packaged default configs treated as user-preserved data). - # Content is intentionally empty — only presence matters. - (extension_dir / ".keep-config").write_text("") - else: - # Backup config files before deleting - if extension_dir.exists(): - # Use subdirectory per extension to avoid name accumulation - # (e.g., jira-jira-config.yml on repeated remove/install cycles) - backup_dir = self.extensions_dir / ".backup" / extension_id - backup_dir.mkdir(parents=True, exist_ok=True) - - # Backup both primary and local override config files - config_files = list(extension_dir.glob("*-config.yml")) + list( - extension_dir.glob("*-config.local.yml") - ) - for config_file in config_files: - backup_path = backup_dir / config_file.name - shutil.copy2(config_file, backup_path) + if keep_config: + # Preserve config files, only remove non-config files + if extension_dir.exists(): + for child in extension_dir.iterdir(): + # Keep top-level *-config.yml and *-config.local.yml files + if child.is_file() and ( + child.name.endswith("-config.yml") + or child.name.endswith("-config.local.yml") + ): + continue + if child.is_dir(): + shutil.rmtree(child) + else: + child.unlink() + # Write a provenance marker so install_from_directory can + # distinguish this --keep-config leftover from a directory left + # by a partially-failed install (which must not have its + # packaged default configs treated as user-preserved data). + # Content is intentionally empty — only presence matters. + (extension_dir / ".keep-config").write_text("") + else: + # Backup config files before deleting + if extension_dir.exists(): + # Use subdirectory per extension to avoid name accumulation + # (e.g., jira-jira-config.yml on repeated remove/install cycles) + backup_dir = self.extensions_dir / ".backup" / extension_id + backup_dir.mkdir(parents=True, exist_ok=True) + + # Backup both primary and local override config files + config_files = list(extension_dir.glob("*-config.yml")) + list( + extension_dir.glob("*-config.local.yml") + ) + for config_file in config_files: + backup_path = backup_dir / config_file.name + shutil.copy2(config_file, backup_path) - # Remove extension directory - if extension_dir.exists(): - shutil.rmtree(extension_dir) + # Remove extension directory + if extension_dir.exists(): + shutil.rmtree(extension_dir) - if not projection_active: - hook_executor.unregister_hooks(extension_id) + if not projection_active: + hook_executor.unregister_hooks(extension_id) - # Update registry - self.registry.remove(extension_id) + # Update registry + self.registry.remove(extension_id) + except Exception as exc: + if projection_active: + try: + hook_executor.save_project_config(original_hook_config) + except (ValueError, OSError) as rollback_exc: + raise ExtensionError( + f"Extension removal failed ({exc}); hook rollback failed: {rollback_exc}" + ) from exc + raise 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) @@ -6017,7 +6072,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 diff --git a/src/specify_cli/extensions/command_disable.py b/src/specify_cli/extensions/command_disable.py index 0de37fa43b..7282a0658b 100644 --- a/src/specify_cli/extensions/command_disable.py +++ b/src/specify_cli/extensions/command_disable.py @@ -69,46 +69,12 @@ def extension_disable( hook_executor.save_project_config(config) except (ValueError, OSError) as exc: try: - manager.registry.update(extension_id, {"enabled": True}) if load_init_options(project_root).get("ai") == "generic": - manager.register_enabled_extensions_for_agent("generic") - restored = manager.registry.get(extension_id) or {} - if restored.get("enabled") is not True: - raise ExtensionError("Rollback did not restore enabled registry state") - previous_commands = metadata.get("registered_commands", {}) - command_names = ( - previous_commands.get("generic", []) - if isinstance(previous_commands, dict) else [] + manager.restore_generic_extension_after_failed_disable( + extension_id, metadata ) - skill_names = metadata.get("registered_skills", []) - for names, skills in ((command_names, False), (skill_names, 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 = manager._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)) - ) + else: + manager.registry.update(extension_id, {"enabled": True}) except (ExtensionError, ValueError, OSError) as rollback_exc: console.print( f"[red]Error:[/red] {_escape_markup(str(exc))}; " diff --git a/src/specify_cli/extensions/command_remove.py b/src/specify_cli/extensions/command_remove.py index 52b30729bf..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) @@ -70,7 +70,7 @@ def extension_remove( # Remove extension try: success = manager.remove(extension_id, keep_config=keep_config) - except (ValueError, OSError) as exc: + except (ExtensionError, ValueError, OSError) as exc: console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") raise typer.Exit(1) from exc diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index 17d842add1..8a17295644 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -75,6 +75,35 @@ def test_add_publishes_hooks_without_changing_command_content(self, extension_di 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_add_rejects_invalid_existing_hooks_before_install(self, extension_dir, project_dir): enable_native_hooks(project_dir) config = project_dir / ".specify/extensions.yml" diff --git a/tests/specify_cli/extensions/test_command_disable.py b/tests/specify_cli/extensions/test_command_disable.py index 60e5896bf7..51ffe94411 100644 --- a/tests/specify_cli/extensions/test_command_disable.py +++ b/tests/specify_cli/extensions/test_command_disable.py @@ -8,10 +8,12 @@ from pathlib import Path from unittest.mock import patch +import pytest from typer.testing import CliRunner from specify_cli import app from specify_cli.extensions import ( + ExtensionError, ExtensionManager, ExtensionRegistry, HookExecutor, @@ -85,3 +87,19 @@ def test_disable_reports_failed_generic_artifact_rollback(self, tmp_path, extens 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 + ) diff --git a/tests/specify_cli/extensions/test_command_remove.py b/tests/specify_cli/extensions/test_command_remove.py index 903bba9c8f..4566cec3af 100644 --- a/tests/specify_cli/extensions/test_command_remove.py +++ b/tests/specify_cli/extensions/test_command_remove.py @@ -160,3 +160,73 @@ def fail_hook_removal(executor, updated): assert manager.registry.is_installed("test-ext") assert extension_dir.is_dir() assert (artifact.read_bytes(), config.read_bytes(), (cache / "source.yml").read_bytes()) == original + + def test_remove_artifact_failure_restores_published_hooks( + 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_restores_hooks_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_reports_failed_hook_restoration( + self, tmp_path, extension_dir + ): + project, _, _ = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + original_save = HookExecutor.save_project_config + + def fail_restore(executor, updated): + if updated.get("hooks", {}).get("before_plan"): + raise OSError("restore failed") + return original_save(executor, updated) + + with patch.object(Path, "cwd", return_value=project), \ + patch.object(ExtensionManager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed")), \ + patch.object(HookExecutor, "save_project_config", fail_restore): + result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) + assert result.exit_code == 1 + assert "cleanup failed" in result.output + assert "hook rollback failed" in result.output + assert "restore failed" in " ".join(result.output.split()) From 9d7f9cadebbf707df1af7d26e9a302574f2410f5 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:59:03 -0500 Subject: [PATCH 08/14] Move extension disable transaction into domain owner Keep the CLI responsible only for resolution, output and error mapping; preserve generic rollback and opt-out behavior with direct operation tests. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- src/specify_cli/extensions/__init__.py | 45 ++++++++++ src/specify_cli/extensions/command_disable.py | 55 +----------- .../extensions/test_command_disable.py | 89 +++++++++++++++++++ 3 files changed, 138 insertions(+), 51 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 20853f055e..a2aa28c6a6 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -3685,6 +3685,51 @@ 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 + + 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 + + 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}) + + if "hooks" in config: + for hooks in config["hooks"].values(): + for hook in hooks: + if hook.get("extension") == extension_id: + hook["enabled"] = False + try: + hook_executor.save_project_config(config) + except (ValueError, OSError) as exc: + try: + if generic: + self.restore_generic_extension_after_failed_disable( + extension_id, metadata + ) + else: + 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 restore_generic_extension_after_failed_disable( self, extension_id: str, previous: Dict[str, Any] ) -> None: diff --git a/src/specify_cli/extensions/command_disable.py b/src/specify_cli/extensions/command_disable.py index 7282a0658b..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 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() @@ -30,60 +29,14 @@ def extension_disable( ) try: - config = hook_executor.load_project_config_for_update() - except (ValueError, OSError) as exc: + 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 - - # 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): + 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": - 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 - 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 - try: - hook_executor.save_project_config(config) - except (ValueError, OSError) as exc: - try: - if load_init_options(project_root).get("ai") == "generic": - manager.restore_generic_extension_after_failed_disable( - extension_id, metadata - ) - else: - manager.registry.update(extension_id, {"enabled": True}) - except (ExtensionError, ValueError, OSError) as rollback_exc: - console.print( - f"[red]Error:[/red] {_escape_markup(str(exc))}; " - f"rollback failed: {_escape_markup(str(rollback_exc))}" - ) - raise typer.Exit(1) from exc - console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") - raise typer.Exit(1) from exc - 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/tests/specify_cli/extensions/test_command_disable.py b/tests/specify_cli/extensions/test_command_disable.py index 51ffe94411..9015a710b4 100644 --- a/tests/specify_cli/extensions/test_command_disable.py +++ b/tests/specify_cli/extensions/test_command_disable.py @@ -9,6 +9,7 @@ from unittest.mock import patch import pytest +import yaml from typer.testing import CliRunner from specify_cli import app @@ -18,12 +19,27 @@ 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.""" @@ -103,3 +119,76 @@ def test_generic_rollback_verifies_owned_artifacts_at_domain_boundary( 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" From 5e8de8430c278e8afb6ca9c54a6793901919ce43 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 13:41:05 -0500 Subject: [PATCH 09/14] Restore exact hook state on opted-in lifecycle failures Preserve pre-existing same-ID hooks after failed install publication and restore source/cache bytes after removal cleanup failure, without changing opt-out or artifact rollback. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 11 ++- src/specify_cli/extensions/__init__.py | 30 ++++-- .../extensions/_hook_projection.py | 94 ++++++++++++++++++ .../extensions/test_command_add.py | 95 +++++++++++++++++++ .../extensions/test_command_remove.py | 52 ++++++++-- .../extensions/test_hook_projection.py | 24 +++++ 6 files changed, 286 insertions(+), 20 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 0834780c41..431f2d39ca 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -330,10 +330,13 @@ 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. -If an opted-in removal fails during subsequent cleanup, Spec Kit restores the -prior hook configuration and projection or reports a rollback failure. This -does not make removal of command artifacts and extension sources atomic; -cleanup already completed may not be reversed. +If hook publication fails during an opted-in install, Spec Kit restores the +prior hook configuration and projection, including pre-existing hooks for the +same extension ID. If an opted-in removal fails during subsequent cleanup, +it restores the original YAML and complete projection bytes (including +comments and line endings) or reports a rollback failure. This does not make +removal of command artifacts and extension sources atomic; cleanup already +completed may not be reversed. ```yaml installed: diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index a2aa28c6a6..dbd4f7abc5 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2463,7 +2463,8 @@ def install_from_directory( manifest = ExtensionManifest(manifest_path) from ._hook_projection import projection_enabled, project_hooks - if projection_enabled(self.project_root): + native_hooks = projection_enabled(self.project_root) + if native_hooks: try: project_hooks({"hooks": { event: [ @@ -3124,6 +3125,7 @@ def rollback_installation_files() -> None: hooks_started = False hooks_finished = False + hook_snapshot = None registry_started = False registered_commands = {} registered_skills = [] @@ -3157,6 +3159,10 @@ def rollback_installation_files() -> None: # Register hooks and update installed list in extensions.yml hook_executor = HookExecutor(self.project_root) + if native_hooks: + from ._hook_projection import snapshot + + hook_snapshot = snapshot(hook_executor.config_file) hooks_started = True hook_executor.register_hooks(manifest) hooks_finished = True @@ -3220,7 +3226,12 @@ def rollback_installation_files() -> None: rollback_errors.append(f"registry: {error}") if hooks_started: try: - hook_executor.unregister_hooks(manifest.id) + if hook_snapshot is not None and not hooks_finished: + from ._hook_projection import restore + + restore(hook_executor.config_file, hook_snapshot) + else: + hook_executor.unregister_hooks(manifest.id) except Exception as error: rollback_errors.append(f"hooks: {error}") if registered_commands: @@ -3582,11 +3593,11 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if not self.registry.is_installed(extension_id): return False - from ._hook_projection import projection_enabled + from ._hook_projection import projection_enabled, restore, snapshot hook_executor = HookExecutor(self.project_root) projection_active = projection_enabled(self.project_root) - original_hook_config = hook_executor.load_project_config_for_update() + hook_executor.load_project_config_for_update() # Get registered commands and skills before removal metadata = self.registry.get(extension_id) @@ -3601,14 +3612,15 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: registered_skills = [] extension_dir = self.extensions_dir / extension_id + hook_snapshot = snapshot(hook_executor.config_file) if projection_active else None # Publication can fail even after preflight (for example, on a disk # error). Keep all invocation artifacts and sources intact until the # extension has been removed from the hook configuration. - if projection_active: - hook_executor.unregister_hooks(extension_id) - try: + if projection_active: + hook_executor.unregister_hooks(extension_id) + # Unregister commands from all AI agents if registered_commands: safe_commands = dict(registered_commands) @@ -3674,9 +3686,9 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: # Update registry self.registry.remove(extension_id) except Exception as exc: - if projection_active: + if hook_snapshot is not None: try: - hook_executor.save_project_config(original_hook_config) + restore(hook_executor.config_file, hook_snapshot) except (ValueError, OSError) as rollback_exc: raise ExtensionError( f"Extension removal failed ({exc}); hook rollback failed: {rollback_exc}" diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index a3b0e35a13..a9ccf8de63 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -9,6 +9,7 @@ import re import shutil import tempfile +from dataclasses import dataclass from pathlib import Path from typing import Any @@ -52,6 +53,99 @@ def _cache_path(config_file: Path) -> Path: return config_file.parent / "hook-dispatch" +@dataclass(frozen=True) +class HookState: + source: bytes | None + cache: dict[str, bytes] | None + + +def _read_state(config_file: Path) -> HookState: + _check_paths(config_file) + cache = _cache_path(config_file) + return HookState( + config_file.read_bytes() if config_file.exists() else None, + {entry.name: entry.read_bytes() for entry in cache.iterdir()} + if cache.exists() else None, + ) + + +def snapshot(config_file: Path) -> HookState: + """Capture the exact on-disk YAML and projection under the publication lock.""" + root = config_file.parent + with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( + root.parent, ".hook-dispatch.lock", context="hook projection" + ): + return _read_state(config_file) + + +def restore(config_file: Path, previous: HookState) -> None: + """Restore an exact hook snapshot, including absent files and cache contents.""" + root = config_file.parent + cache = _cache_path(config_file) + with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( + root.parent, ".hook-dispatch.lock", context="hook projection" + ): + current = _read_state(config_file) + if current == previous: + return + stage = Path(tempfile.mkdtemp(prefix=".hook-restore-", dir=root)) + retain_stage = False + moved = False + installed = False + try: + if previous.cache is not None: + staged_cache = stage / "cache" + staged_cache.mkdir() + for name, content in previous.cache.items(): + (staged_cache / name).write_bytes(content) + if previous.source is not None: + (stage / "source.yml").write_bytes(previous.source) + with changing_file(cache, removal=previous.cache is None): + if cache.exists(): + os.replace(cache, stage / "previous-cache") + moved = True + if previous.cache is not None: + os.replace(stage / "cache", cache) + installed = True + if previous.source is None: + with changing_file(config_file, removal=True): + config_file.unlink(missing_ok=True) + else: + with changing_file(config_file): + os.replace(stage / "source.yml", config_file) + except Exception as exc: + try: + if installed: + with changing_file(cache, removal=True): + shutil.rmtree(cache) + if moved: + with changing_file(cache): + os.replace(stage / "previous-cache", cache) + if current.source is None: + with changing_file(config_file, removal=True): + config_file.unlink(missing_ok=True) + elif not config_file.is_file() or config_file.read_bytes() != current.source: + (stage / "rollback.yml").write_bytes(current.source) + with changing_file(config_file): + os.replace(stage / "rollback.yml", config_file) + except Exception as rollback_exc: + retain_stage = True + raise OSError( + f"Hook snapshot restoration failed ({exc}); rollback failed " + f"({rollback_exc}); backup retained at {stage}" + ) from exc + raise + finally: + if not retain_stage: + try: + shutil.rmtree(stage) + except OSError as exc: + logging.getLogger(__name__).warning( + "Could not remove hook snapshot staging %s: %s", + stage, exc, + ) + + def _check_paths(config_file: Path) -> None: root = config_file.parent cache = _cache_path(config_file) diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index 8a17295644..ed98c854eb 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -104,6 +104,101 @@ def test_add_disabled_manifest_hook_respects_opt_in( else: assert not cache.exists() + def test_install_publication_failure_keeps_preexisting_same_id_hooks( + self, extension_dir, project_dir, monkeypatch + ): + import specify_cli.extensions._hook_projection as projection + + 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_replace = projection.os.replace + failed = False + + def fail_config_write(source, target): + nonlocal failed + if Path(target) == config and not failed: + failed = True + raise OSError("simulated hook publication failure") + return real_replace(source, target) + + monkeypatch.setattr(projection.os, "replace", fail_config_write) + manager = ExtensionManager(project_dir) + with pytest.raises(OSError, match="simulated hook publication failure"): + 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_reports_failed_hook_snapshot_restoration( + 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( + "specify_cli.extensions._hook_projection.restore", + side_effect=OSError("restore failed"), + ): + with pytest.raises(ExtensionError) as error: + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + assert "publication failed" in str(error.value) + assert "rollback failed: hooks: restore failed" in str(error.value) + 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" diff --git a/tests/specify_cli/extensions/test_command_remove.py b/tests/specify_cli/extensions/test_command_remove.py index 4566cec3af..9e7881c2df 100644 --- a/tests/specify_cli/extensions/test_command_remove.py +++ b/tests/specify_cli/extensions/test_command_remove.py @@ -5,10 +5,12 @@ 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 @@ -209,22 +211,58 @@ def remove_then_fail(manager, extension_id, metadata): 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_restores_absent_yaml_and_cache( + 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_remove_reports_failed_hook_restoration( self, tmp_path, extension_dir ): project, _, _ = installed_generic_hook_extension( tmp_path, extension_dir, native_hooks=True ) - original_save = HookExecutor.save_project_config - - def fail_restore(executor, updated): - if updated.get("hooks", {}).get("before_plan"): - raise OSError("restore failed") - return original_save(executor, updated) with patch.object(Path, "cwd", return_value=project), \ patch.object(ExtensionManager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed")), \ - patch.object(HookExecutor, "save_project_config", fail_restore): + patch("specify_cli.extensions._hook_projection.restore", side_effect=OSError("restore failed")): result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) assert result.exit_code == 1 assert "cleanup failed" in result.output diff --git a/tests/specify_cli/extensions/test_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py index c7a3f86249..bafea4cd09 100644 --- a/tests/specify_cli/extensions/test_hook_projection.py +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -253,6 +253,30 @@ def fail_config_write(src, dst): _assert_snapshot(tmp_path) assert not list((tmp_path / ".specify").glob(".hook-stage-*")) +def test_snapshot_restoration_failure_retains_current_pair(tmp_path, monkeypatch): + import specify_cli.extensions._hook_projection as projection + + executor = _enabled_executor(tmp_path) + executor.save_project_config(_config()) + original = projection.snapshot(executor.config_file) + executor.save_project_config({"installed": [], "hooks": {}}) + current = projection.snapshot(executor.config_file) + real_replace = projection.os.replace + failed = False + + def fail_restore_write(source, target): + nonlocal failed + if Path(target) == executor.config_file and not failed: + failed = True + raise OSError("restore write failed") + return real_replace(source, target) + + monkeypatch.setattr(projection.os, "replace", fail_restore_write) + with pytest.raises(OSError, match="restore write failed"): + projection.restore(executor.config_file, original) + assert projection.snapshot(executor.config_file) == current + assert not list(executor.config_file.parent.glob(".hook-restore-*")) + def test_unreadable_existing_config_does_not_allocate_stage(tmp_path): executor = _enabled_executor(tmp_path) From 0832d91f76ae84f055362034cb2bbd12654de60f Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 14:03:14 -0500 Subject: [PATCH 10/14] Guard hook rollback against concurrent publication Use an expected-state check at the projection commit and compare the committed pair during compensation; report conflicts instead of overwriting newer hook data. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 4 +- src/specify_cli/extensions/__init__.py | 48 +++++++--- .../extensions/_hook_projection.py | 26 +++++- .../extensions/test_command_add.py | 65 +++++++++++++- .../extensions/test_command_remove.py | 90 ++++++++++++++++++- .../extensions/test_hook_projection.py | 2 +- 6 files changed, 213 insertions(+), 22 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 431f2d39ca..33064e9aa8 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -336,7 +336,9 @@ same extension ID. If an opted-in removal fails during subsequent cleanup, it restores the original YAML and complete projection bytes (including comments and line endings) or reports a rollback failure. This does not make removal of command artifacts and extension sources atomic; cleanup already -completed may not be reversed. +completed may not be reversed. If another hook publication wins before this +operation commits or publishes during cleanup, the conflicting write is +reported rather than overwritten by rollback. ```yaml installed: diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index dbd4f7abc5..d60cc8d5f3 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -24,13 +24,16 @@ from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path -from typing import Any, BinaryIO, Callable, Dict, List, Optional, Set +from typing import TYPE_CHECKING, Any, BinaryIO, Callable, Dict, List, Optional, Set import pathspec import yaml from packaging import version as pkg_version from packaging.specifiers import InvalidSpecifier, SpecifierSet +if TYPE_CHECKING: + from ._hook_projection import HookState + from .._assets import _locate_core_pack, _repo_root from .._download_security import ( archive_format_from_name, @@ -3164,7 +3167,10 @@ def rollback_installation_files() -> None: hook_snapshot = snapshot(hook_executor.config_file) hooks_started = True - hook_executor.register_hooks(manifest) + if native_hooks: + hook_executor.register_hooks(manifest, expected_state=hook_snapshot) + else: + hook_executor.register_hooks(manifest) hooks_finished = True # Restore config files from backup when --force triggered a removal. @@ -3229,7 +3235,10 @@ def rollback_installation_files() -> None: if hook_snapshot is not None and not hooks_finished: from ._hook_projection import restore - restore(hook_executor.config_file, hook_snapshot) + restore( + hook_executor.config_file, hook_snapshot, + expected=hook_snapshot, + ) else: hook_executor.unregister_hooks(manifest.id) except Exception as error: @@ -3613,13 +3622,16 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: extension_dir = self.extensions_dir / extension_id hook_snapshot = snapshot(hook_executor.config_file) if projection_active else None + post_hook_snapshot = hook_snapshot # Publication can fail even after preflight (for example, on a disk # error). Keep all invocation artifacts and sources intact until the # extension has been removed from the hook configuration. try: if projection_active: - hook_executor.unregister_hooks(extension_id) + post_hook_snapshot = hook_executor.unregister_hooks( + extension_id, expected_state=hook_snapshot + ) # Unregister commands from all AI agents if registered_commands: @@ -3688,7 +3700,10 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: except Exception as exc: if hook_snapshot is not None: try: - restore(hook_executor.config_file, hook_snapshot) + restore( + hook_executor.config_file, hook_snapshot, + expected=post_hook_snapshot, + ) except (ValueError, OSError) as rollback_exc: raise ExtensionError( f"Extension removal failed ({exc}); hook rollback failed: {rollback_exc}" @@ -5915,7 +5930,9 @@ def get_project_config(self) -> Dict[str, Any]: "hooks": {}, } - def save_project_config(self, config: Dict[str, Any]): + def save_project_config( + self, config: Dict[str, Any], *, expected_state: HookState | None = None + ) -> HookState | None: """Save project-level extension configuration. Args: @@ -5924,7 +5941,7 @@ def save_project_config(self, config: Dict[str, Any]): from ._hook_projection import projection_enabled, publish, retire if projection_enabled(self.project_root): - publish(self.config_file, config) + return publish(self.config_file, config, expected=expected_state) else: from ..integrations._file_changes import write_text @@ -6051,7 +6068,10 @@ def _sort_key(x: object) -> str: return sorted(seen.values(), key=_sort_key) - def register_hooks(self, manifest: ExtensionManifest): + def register_hooks( + self, manifest: ExtensionManifest, *, + expected_state: HookState | None = None, + ) -> HookState | None: """Register extension hooks in project config. Args: @@ -6159,9 +6179,11 @@ def register_hooks(self, manifest: ExtensionManifest): changed = True if changed: - self.save_project_config(config) + return self.save_project_config(config, expected_state=expected_state) - def unregister_hooks(self, extension_id: str): + def unregister_hooks( + self, extension_id: str, *, expected_state: HookState | None = None + ) -> HookState | None: """Remove extension hooks from project config. Args: @@ -6185,8 +6207,8 @@ def unregister_hooks(self, extension_id: str): if "hooks" not in config or not isinstance(config["hooks"], dict): if installed_changed: - self.save_project_config(config) - return + return self.save_project_config(config, expected_state=expected_state) + return expected_state # Remove hooks for this extension for hook_name in list(config["hooks"].keys()): @@ -6205,7 +6227,7 @@ def unregister_hooks(self, extension_id: str): name: hooks for name, hooks in config["hooks"].items() if hooks } - self.save_project_config(config) + return self.save_project_config(config, expected_state=expected_state) def get_hooks_for_event(self, event_name: str) -> List[Dict[str, Any]]: """Get all enabled hooks for a specific event, sorted by priority ascending. diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index a9ccf8de63..839ffd81de 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -78,8 +78,10 @@ def snapshot(config_file: Path) -> HookState: return _read_state(config_file) -def restore(config_file: Path, previous: HookState) -> None: - """Restore an exact hook snapshot, including absent files and cache contents.""" +def restore( + config_file: Path, previous: HookState, *, expected: HookState +) -> None: + """Restore exact hook state only if the published pair is still unchanged.""" root = config_file.parent cache = _cache_path(config_file) with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( @@ -88,6 +90,11 @@ def restore(config_file: Path, previous: HookState) -> None: current = _read_state(config_file) if current == previous: return + if current != expected: + raise ValueError( + "Hook configuration or projection changed since the lifecycle " + "operation; refusing to overwrite a concurrent update" + ) stage = Path(tempfile.mkdtemp(prefix=".hook-restore-", dir=root)) retain_stage = False moved = False @@ -241,7 +248,10 @@ def _write_digest(directory: Path, name: str, payload: bytes) -> None: ) -def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool = False) -> None: +def publish( + config_file: Path, config: dict[str, Any], *, + preserve_source: bool = False, expected: HookState | None = None, +) -> HookState | None: """Publish one complete projection and its YAML source, or restore the prior pair.""" if not preserve_source: projected = project_hooks(config) @@ -252,6 +262,11 @@ def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool root.parent, ".hook-dispatch.lock", context="hook projection" ): _check_paths(config_file) + if expected is not None and _read_state(config_file) != expected: + raise ValueError( + "Hook configuration or projection changed before publication; " + "refusing to overwrite a concurrent update" + ) if preserve_source: source = config_file.read_bytes() try: @@ -278,6 +293,10 @@ def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool _write_digest(stage, f"{event}.json", payload) _write_digest(stage, "events.txt", "".join(f"{event}\n" for event in projected).encode("utf-8")) (stage / "source.yml").write_bytes(source) + committed_cache = ( + {entry.name: entry.read_bytes() for entry in stage.iterdir()} + if expected is not None else None + ) with changing_file(cache): if cache.exists(): os.replace(cache, backup) @@ -322,3 +341,4 @@ def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool backup, exc, ) + return HookState(source, committed_cache) if committed_cache is not None else None diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index ed98c854eb..7bad55c189 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -199,6 +199,67 @@ def test_install_reports_failed_hook_snapshot_restoration( assert "rollback failed: hooks: restore failed" in str(error.value) assert not manager.registry.is_installed("test-ext") + def test_install_failed_publication_does_not_erase_concurrent_hook_save( + self, extension_dir, project_dir + ): + enable_native_hooks(project_dir) + manager = ExtensionManager(project_dir) + concurrent = {} + + def save_then_fail(executor, manifest, **kwargs): + updated = executor.load_project_config_for_update() + updated["hooks"]["after_plan"] = [ + {"extension": "another-ext", "command": "speckit.another-ext.run"} + ] + executor.save_project_config(updated) + cache = project_dir / ".specify/hook-dispatch" + concurrent["yaml"] = executor.config_file.read_bytes() + concurrent["cache"] = { + entry.name: entry.read_bytes() for entry in cache.iterdir() + } + raise OSError("publication failed") + + with patch.object(HookExecutor, "register_hooks", save_then_fail): + with pytest.raises((ExtensionError, OSError)) as error: + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + config = project_dir / ".specify/extensions.yml" + cache = project_dir / ".specify/hook-dispatch" + assert config.read_bytes() == concurrent["yaml"] + assert {entry.name: entry.read_bytes() for entry in cache.iterdir()} == concurrent["cache"] + assert "publication failed" in str(error.value) + assert "rollback failed" in str(error.value) + assert "refusing to overwrite a concurrent update" in str(error.value) + assert not manager.registry.is_installed("test-ext") + + def test_install_detects_hook_save_before_publisher_commit( + self, extension_dir, project_dir + ): + enable_native_hooks(project_dir) + manager = ExtensionManager(project_dir) + original_register = HookExecutor.register_hooks + concurrent = {} + + def save_then_register(executor, manifest, **kwargs): + updated = executor.load_project_config_for_update() + updated["hooks"]["after_plan"] = [ + {"extension": "another-ext", "command": "speckit.another-ext.run"} + ] + executor.save_project_config(updated) + concurrent["yaml"] = executor.config_file.read_bytes() + return original_register(executor, manifest, **kwargs) + + with patch.object(HookExecutor, "register_hooks", save_then_register): + with pytest.raises(ExtensionError, match="rollback failed") as error: + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + assert "changed before publication" in str(error.value) + assert "refusing to overwrite a concurrent update" in str(error.value) + assert (project_dir / ".specify/extensions.yml").read_bytes() == concurrent["yaml"] + 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" @@ -226,10 +287,10 @@ def test_add_rolls_back_non_generic_artifacts_on_publication_failure( (project_dir / ".github/agents").mkdir(parents=True) original_save = HookExecutor.save_project_config - def fail_hook_save(executor, config): + def fail_hook_save(executor, config, **kwargs): if config.get("hooks", {}).get("before_plan"): raise OSError("projection failed") - return original_save(executor, config) + return original_save(executor, config, **kwargs) with patch.object(Path, "cwd", return_value=project_dir), \ patch.object(HookExecutor, "save_project_config", fail_hook_save): diff --git a/tests/specify_cli/extensions/test_command_remove.py b/tests/specify_cli/extensions/test_command_remove.py index 9e7881c2df..1832af5978 100644 --- a/tests/specify_cli/extensions/test_command_remove.py +++ b/tests/specify_cli/extensions/test_command_remove.py @@ -16,6 +16,7 @@ from specify_cli import app from specify_cli.extensions import ( + ExtensionError, ExtensionManager, ExtensionRegistry, HookExecutor, @@ -149,10 +150,10 @@ def test_remove_publication_failure_retains_generic_installation( original = (artifact.read_bytes(), config.read_bytes(), (cache / "source.yml").read_bytes()) original_save = HookExecutor.save_project_config - def fail_hook_removal(executor, updated): + def fail_hook_removal(executor, updated, **kwargs): if not updated.get("hooks", {}).get("before_plan"): raise OSError("projection failed") - return original_save(executor, updated) + return original_save(executor, updated, **kwargs) with patch.object(Path, "cwd", return_value=project), \ patch.object(HookExecutor, "save_project_config", fail_hook_removal): @@ -253,6 +254,91 @@ def test_remove_cleanup_failure_restores_absent_yaml_and_cache( assert not config.exists() assert not cache.exists() + def test_remove_conflicting_hook_save_does_not_overwrite_concurrent_update( + self, tmp_path, extension_dir + ): + project, manager, _ = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + executor = HookExecutor(project) + cache = project / ".specify/hook-dispatch" + concurrent = {} + + def save_then_fail(extension_id, metadata): + updated = executor.load_project_config_for_update() + updated["hooks"]["after_plan"] = [ + {"extension": "another-ext", "command": "speckit.another-ext.run"} + ] + executor.save_project_config(updated) + concurrent["yaml"] = executor.config_file.read_bytes() + concurrent["cache"] = { + entry.name: entry.read_bytes() for entry in cache.iterdir() + } + raise OSError("cleanup failed") + + with patch.object(manager, "_remove_generic_artifact_paths", save_then_fail): + with pytest.raises((ExtensionError, OSError)) as error: + manager.remove("test-ext") + assert executor.config_file.read_bytes() == concurrent["yaml"] + assert {entry.name: entry.read_bytes() for entry in cache.iterdir()} == concurrent["cache"] + assert "cleanup failed" in str(error.value) + assert "rollback failed" in str(error.value) + assert "refusing to overwrite a concurrent update" in str(error.value) + assert manager.registry.is_installed("test-ext") + + def test_remove_detects_hook_save_before_publisher_commit( + self, tmp_path, extension_dir + ): + project, manager, _ = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + original_unregister = HookExecutor.unregister_hooks + concurrent = {} + + def save_then_unregister(executor, extension_id, **kwargs): + updated = executor.load_project_config_for_update() + updated["hooks"]["after_plan"] = [ + {"extension": "another-ext", "command": "speckit.another-ext.run"} + ] + executor.save_project_config(updated) + concurrent["yaml"] = executor.config_file.read_bytes() + return original_unregister(executor, extension_id, **kwargs) + + with patch.object(HookExecutor, "unregister_hooks", save_then_unregister): + with pytest.raises(ExtensionError, match="rollback failed") as error: + manager.remove("test-ext") + assert "changed before publication" in str(error.value) + assert "refusing to overwrite a concurrent update" in str(error.value) + assert (project / ".specify/extensions.yml").read_bytes() == concurrent["yaml"] + assert manager.registry.is_installed("test-ext") + + def test_remove_detects_hook_save_after_publisher_commit( + self, tmp_path, extension_dir + ): + project, manager, _ = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=True + ) + original_unregister = HookExecutor.unregister_hooks + concurrent = {} + + def unregister_then_save(executor, extension_id, **kwargs): + committed = original_unregister(executor, extension_id, **kwargs) + updated = executor.load_project_config_for_update() + updated["hooks"]["after_plan"] = [ + {"extension": "another-ext", "command": "speckit.another-ext.run"} + ] + executor.save_project_config(updated) + concurrent["yaml"] = executor.config_file.read_bytes() + return committed + + with patch.object(HookExecutor, "unregister_hooks", unregister_then_save), \ + patch.object(manager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed")): + with pytest.raises(ExtensionError, match="rollback failed") as error: + manager.remove("test-ext") + assert "refusing to overwrite a concurrent update" in str(error.value) + assert (project / ".specify/extensions.yml").read_bytes() == concurrent["yaml"] + assert manager.registry.is_installed("test-ext") + def test_remove_reports_failed_hook_restoration( self, tmp_path, extension_dir ): diff --git a/tests/specify_cli/extensions/test_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py index bafea4cd09..f80ef14be3 100644 --- a/tests/specify_cli/extensions/test_hook_projection.py +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -273,7 +273,7 @@ def fail_restore_write(source, target): monkeypatch.setattr(projection.os, "replace", fail_restore_write) with pytest.raises(OSError, match="restore write failed"): - projection.restore(executor.config_file, original) + projection.restore(executor.config_file, original, expected=current) assert projection.snapshot(executor.config_file) == current assert not list(executor.config_file.parent.glob(".hook-restore-*")) From e4e43099f4af5efc80254ded0498e7c9ae5aec7b Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 14:07:02 -0500 Subject: [PATCH 11/14] Revert out-of-scope concurrent hook publication guards Keep the two reviewed opt-in lifecycle rollback fixes without adding publisher CAS or multi-process conflict handling. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 4 +- src/specify_cli/extensions/__init__.py | 48 +++------- .../extensions/_hook_projection.py | 26 +----- .../extensions/test_command_add.py | 65 +------------- .../extensions/test_command_remove.py | 90 +------------------ .../extensions/test_hook_projection.py | 2 +- 6 files changed, 22 insertions(+), 213 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 33064e9aa8..431f2d39ca 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -336,9 +336,7 @@ same extension ID. If an opted-in removal fails during subsequent cleanup, it restores the original YAML and complete projection bytes (including comments and line endings) or reports a rollback failure. This does not make removal of command artifacts and extension sources atomic; cleanup already -completed may not be reversed. If another hook publication wins before this -operation commits or publishes during cleanup, the conflicting write is -reported rather than overwritten by rollback. +completed may not be reversed. ```yaml installed: diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index d60cc8d5f3..dbd4f7abc5 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -24,16 +24,13 @@ from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path -from typing import TYPE_CHECKING, Any, BinaryIO, Callable, Dict, List, Optional, Set +from typing import Any, BinaryIO, Callable, Dict, List, Optional, Set import pathspec import yaml from packaging import version as pkg_version from packaging.specifiers import InvalidSpecifier, SpecifierSet -if TYPE_CHECKING: - from ._hook_projection import HookState - from .._assets import _locate_core_pack, _repo_root from .._download_security import ( archive_format_from_name, @@ -3167,10 +3164,7 @@ def rollback_installation_files() -> None: hook_snapshot = snapshot(hook_executor.config_file) hooks_started = True - if native_hooks: - hook_executor.register_hooks(manifest, expected_state=hook_snapshot) - else: - hook_executor.register_hooks(manifest) + hook_executor.register_hooks(manifest) hooks_finished = True # Restore config files from backup when --force triggered a removal. @@ -3235,10 +3229,7 @@ def rollback_installation_files() -> None: if hook_snapshot is not None and not hooks_finished: from ._hook_projection import restore - restore( - hook_executor.config_file, hook_snapshot, - expected=hook_snapshot, - ) + restore(hook_executor.config_file, hook_snapshot) else: hook_executor.unregister_hooks(manifest.id) except Exception as error: @@ -3622,16 +3613,13 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: extension_dir = self.extensions_dir / extension_id hook_snapshot = snapshot(hook_executor.config_file) if projection_active else None - post_hook_snapshot = hook_snapshot # Publication can fail even after preflight (for example, on a disk # error). Keep all invocation artifacts and sources intact until the # extension has been removed from the hook configuration. try: if projection_active: - post_hook_snapshot = hook_executor.unregister_hooks( - extension_id, expected_state=hook_snapshot - ) + hook_executor.unregister_hooks(extension_id) # Unregister commands from all AI agents if registered_commands: @@ -3700,10 +3688,7 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: except Exception as exc: if hook_snapshot is not None: try: - restore( - hook_executor.config_file, hook_snapshot, - expected=post_hook_snapshot, - ) + restore(hook_executor.config_file, hook_snapshot) except (ValueError, OSError) as rollback_exc: raise ExtensionError( f"Extension removal failed ({exc}); hook rollback failed: {rollback_exc}" @@ -5930,9 +5915,7 @@ def get_project_config(self) -> Dict[str, Any]: "hooks": {}, } - def save_project_config( - self, config: Dict[str, Any], *, expected_state: HookState | None = None - ) -> HookState | None: + def save_project_config(self, config: Dict[str, Any]): """Save project-level extension configuration. Args: @@ -5941,7 +5924,7 @@ def save_project_config( from ._hook_projection import projection_enabled, publish, retire if projection_enabled(self.project_root): - return publish(self.config_file, config, expected=expected_state) + publish(self.config_file, config) else: from ..integrations._file_changes import write_text @@ -6068,10 +6051,7 @@ def _sort_key(x: object) -> str: return sorted(seen.values(), key=_sort_key) - def register_hooks( - self, manifest: ExtensionManifest, *, - expected_state: HookState | None = None, - ) -> HookState | None: + def register_hooks(self, manifest: ExtensionManifest): """Register extension hooks in project config. Args: @@ -6179,11 +6159,9 @@ def register_hooks( changed = True if changed: - return self.save_project_config(config, expected_state=expected_state) + self.save_project_config(config) - def unregister_hooks( - self, extension_id: str, *, expected_state: HookState | None = None - ) -> HookState | None: + def unregister_hooks(self, extension_id: str): """Remove extension hooks from project config. Args: @@ -6207,8 +6185,8 @@ def unregister_hooks( if "hooks" not in config or not isinstance(config["hooks"], dict): if installed_changed: - return self.save_project_config(config, expected_state=expected_state) - return expected_state + self.save_project_config(config) + return # Remove hooks for this extension for hook_name in list(config["hooks"].keys()): @@ -6227,7 +6205,7 @@ def unregister_hooks( name: hooks for name, hooks in config["hooks"].items() if hooks } - return self.save_project_config(config, expected_state=expected_state) + self.save_project_config(config) def get_hooks_for_event(self, event_name: str) -> List[Dict[str, Any]]: """Get all enabled hooks for a specific event, sorted by priority ascending. diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index 839ffd81de..a9ccf8de63 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -78,10 +78,8 @@ def snapshot(config_file: Path) -> HookState: return _read_state(config_file) -def restore( - config_file: Path, previous: HookState, *, expected: HookState -) -> None: - """Restore exact hook state only if the published pair is still unchanged.""" +def restore(config_file: Path, previous: HookState) -> None: + """Restore an exact hook snapshot, including absent files and cache contents.""" root = config_file.parent cache = _cache_path(config_file) with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( @@ -90,11 +88,6 @@ def restore( current = _read_state(config_file) if current == previous: return - if current != expected: - raise ValueError( - "Hook configuration or projection changed since the lifecycle " - "operation; refusing to overwrite a concurrent update" - ) stage = Path(tempfile.mkdtemp(prefix=".hook-restore-", dir=root)) retain_stage = False moved = False @@ -248,10 +241,7 @@ def _write_digest(directory: Path, name: str, payload: bytes) -> None: ) -def publish( - config_file: Path, config: dict[str, Any], *, - preserve_source: bool = False, expected: HookState | None = None, -) -> HookState | None: +def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool = False) -> None: """Publish one complete projection and its YAML source, or restore the prior pair.""" if not preserve_source: projected = project_hooks(config) @@ -262,11 +252,6 @@ def publish( root.parent, ".hook-dispatch.lock", context="hook projection" ): _check_paths(config_file) - if expected is not None and _read_state(config_file) != expected: - raise ValueError( - "Hook configuration or projection changed before publication; " - "refusing to overwrite a concurrent update" - ) if preserve_source: source = config_file.read_bytes() try: @@ -293,10 +278,6 @@ def publish( _write_digest(stage, f"{event}.json", payload) _write_digest(stage, "events.txt", "".join(f"{event}\n" for event in projected).encode("utf-8")) (stage / "source.yml").write_bytes(source) - committed_cache = ( - {entry.name: entry.read_bytes() for entry in stage.iterdir()} - if expected is not None else None - ) with changing_file(cache): if cache.exists(): os.replace(cache, backup) @@ -341,4 +322,3 @@ def publish( backup, exc, ) - return HookState(source, committed_cache) if committed_cache is not None else None diff --git a/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index 7bad55c189..ed98c854eb 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -199,67 +199,6 @@ def test_install_reports_failed_hook_snapshot_restoration( assert "rollback failed: hooks: restore failed" in str(error.value) assert not manager.registry.is_installed("test-ext") - def test_install_failed_publication_does_not_erase_concurrent_hook_save( - self, extension_dir, project_dir - ): - enable_native_hooks(project_dir) - manager = ExtensionManager(project_dir) - concurrent = {} - - def save_then_fail(executor, manifest, **kwargs): - updated = executor.load_project_config_for_update() - updated["hooks"]["after_plan"] = [ - {"extension": "another-ext", "command": "speckit.another-ext.run"} - ] - executor.save_project_config(updated) - cache = project_dir / ".specify/hook-dispatch" - concurrent["yaml"] = executor.config_file.read_bytes() - concurrent["cache"] = { - entry.name: entry.read_bytes() for entry in cache.iterdir() - } - raise OSError("publication failed") - - with patch.object(HookExecutor, "register_hooks", save_then_fail): - with pytest.raises((ExtensionError, OSError)) as error: - manager.install_from_directory( - extension_dir, "0.1.0", register_commands=False - ) - config = project_dir / ".specify/extensions.yml" - cache = project_dir / ".specify/hook-dispatch" - assert config.read_bytes() == concurrent["yaml"] - assert {entry.name: entry.read_bytes() for entry in cache.iterdir()} == concurrent["cache"] - assert "publication failed" in str(error.value) - assert "rollback failed" in str(error.value) - assert "refusing to overwrite a concurrent update" in str(error.value) - assert not manager.registry.is_installed("test-ext") - - def test_install_detects_hook_save_before_publisher_commit( - self, extension_dir, project_dir - ): - enable_native_hooks(project_dir) - manager = ExtensionManager(project_dir) - original_register = HookExecutor.register_hooks - concurrent = {} - - def save_then_register(executor, manifest, **kwargs): - updated = executor.load_project_config_for_update() - updated["hooks"]["after_plan"] = [ - {"extension": "another-ext", "command": "speckit.another-ext.run"} - ] - executor.save_project_config(updated) - concurrent["yaml"] = executor.config_file.read_bytes() - return original_register(executor, manifest, **kwargs) - - with patch.object(HookExecutor, "register_hooks", save_then_register): - with pytest.raises(ExtensionError, match="rollback failed") as error: - manager.install_from_directory( - extension_dir, "0.1.0", register_commands=False - ) - assert "changed before publication" in str(error.value) - assert "refusing to overwrite a concurrent update" in str(error.value) - assert (project_dir / ".specify/extensions.yml").read_bytes() == concurrent["yaml"] - 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" @@ -287,10 +226,10 @@ def test_add_rolls_back_non_generic_artifacts_on_publication_failure( (project_dir / ".github/agents").mkdir(parents=True) original_save = HookExecutor.save_project_config - def fail_hook_save(executor, config, **kwargs): + def fail_hook_save(executor, config): if config.get("hooks", {}).get("before_plan"): raise OSError("projection failed") - return original_save(executor, config, **kwargs) + return original_save(executor, config) with patch.object(Path, "cwd", return_value=project_dir), \ patch.object(HookExecutor, "save_project_config", fail_hook_save): diff --git a/tests/specify_cli/extensions/test_command_remove.py b/tests/specify_cli/extensions/test_command_remove.py index 1832af5978..9e7881c2df 100644 --- a/tests/specify_cli/extensions/test_command_remove.py +++ b/tests/specify_cli/extensions/test_command_remove.py @@ -16,7 +16,6 @@ from specify_cli import app from specify_cli.extensions import ( - ExtensionError, ExtensionManager, ExtensionRegistry, HookExecutor, @@ -150,10 +149,10 @@ def test_remove_publication_failure_retains_generic_installation( original = (artifact.read_bytes(), config.read_bytes(), (cache / "source.yml").read_bytes()) original_save = HookExecutor.save_project_config - def fail_hook_removal(executor, updated, **kwargs): + def fail_hook_removal(executor, updated): if not updated.get("hooks", {}).get("before_plan"): raise OSError("projection failed") - return original_save(executor, updated, **kwargs) + return original_save(executor, updated) with patch.object(Path, "cwd", return_value=project), \ patch.object(HookExecutor, "save_project_config", fail_hook_removal): @@ -254,91 +253,6 @@ def test_remove_cleanup_failure_restores_absent_yaml_and_cache( assert not config.exists() assert not cache.exists() - def test_remove_conflicting_hook_save_does_not_overwrite_concurrent_update( - self, tmp_path, extension_dir - ): - project, manager, _ = installed_generic_hook_extension( - tmp_path, extension_dir, native_hooks=True - ) - executor = HookExecutor(project) - cache = project / ".specify/hook-dispatch" - concurrent = {} - - def save_then_fail(extension_id, metadata): - updated = executor.load_project_config_for_update() - updated["hooks"]["after_plan"] = [ - {"extension": "another-ext", "command": "speckit.another-ext.run"} - ] - executor.save_project_config(updated) - concurrent["yaml"] = executor.config_file.read_bytes() - concurrent["cache"] = { - entry.name: entry.read_bytes() for entry in cache.iterdir() - } - raise OSError("cleanup failed") - - with patch.object(manager, "_remove_generic_artifact_paths", save_then_fail): - with pytest.raises((ExtensionError, OSError)) as error: - manager.remove("test-ext") - assert executor.config_file.read_bytes() == concurrent["yaml"] - assert {entry.name: entry.read_bytes() for entry in cache.iterdir()} == concurrent["cache"] - assert "cleanup failed" in str(error.value) - assert "rollback failed" in str(error.value) - assert "refusing to overwrite a concurrent update" in str(error.value) - assert manager.registry.is_installed("test-ext") - - def test_remove_detects_hook_save_before_publisher_commit( - self, tmp_path, extension_dir - ): - project, manager, _ = installed_generic_hook_extension( - tmp_path, extension_dir, native_hooks=True - ) - original_unregister = HookExecutor.unregister_hooks - concurrent = {} - - def save_then_unregister(executor, extension_id, **kwargs): - updated = executor.load_project_config_for_update() - updated["hooks"]["after_plan"] = [ - {"extension": "another-ext", "command": "speckit.another-ext.run"} - ] - executor.save_project_config(updated) - concurrent["yaml"] = executor.config_file.read_bytes() - return original_unregister(executor, extension_id, **kwargs) - - with patch.object(HookExecutor, "unregister_hooks", save_then_unregister): - with pytest.raises(ExtensionError, match="rollback failed") as error: - manager.remove("test-ext") - assert "changed before publication" in str(error.value) - assert "refusing to overwrite a concurrent update" in str(error.value) - assert (project / ".specify/extensions.yml").read_bytes() == concurrent["yaml"] - assert manager.registry.is_installed("test-ext") - - def test_remove_detects_hook_save_after_publisher_commit( - self, tmp_path, extension_dir - ): - project, manager, _ = installed_generic_hook_extension( - tmp_path, extension_dir, native_hooks=True - ) - original_unregister = HookExecutor.unregister_hooks - concurrent = {} - - def unregister_then_save(executor, extension_id, **kwargs): - committed = original_unregister(executor, extension_id, **kwargs) - updated = executor.load_project_config_for_update() - updated["hooks"]["after_plan"] = [ - {"extension": "another-ext", "command": "speckit.another-ext.run"} - ] - executor.save_project_config(updated) - concurrent["yaml"] = executor.config_file.read_bytes() - return committed - - with patch.object(HookExecutor, "unregister_hooks", unregister_then_save), \ - patch.object(manager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed")): - with pytest.raises(ExtensionError, match="rollback failed") as error: - manager.remove("test-ext") - assert "refusing to overwrite a concurrent update" in str(error.value) - assert (project / ".specify/extensions.yml").read_bytes() == concurrent["yaml"] - assert manager.registry.is_installed("test-ext") - def test_remove_reports_failed_hook_restoration( self, tmp_path, extension_dir ): diff --git a/tests/specify_cli/extensions/test_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py index f80ef14be3..bafea4cd09 100644 --- a/tests/specify_cli/extensions/test_hook_projection.py +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -273,7 +273,7 @@ def fail_restore_write(source, target): monkeypatch.setattr(projection.os, "replace", fail_restore_write) with pytest.raises(OSError, match="restore write failed"): - projection.restore(executor.config_file, original, expected=current) + projection.restore(executor.config_file, original) assert projection.snapshot(executor.config_file) == current assert not list(executor.config_file.parent.glob(".hook-restore-*")) From 1081bc03e83676b5f382a6fdc3e5bde95b752670 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 14:09:08 -0500 Subject: [PATCH 12/14] Drop projection lock for single-CLI lifecycle User-directed: simultaneous Specify CLI mutations of one project are unsupported. Preserve staged publication, byte-exact rollback, and refresh checks without a per-publication lock. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 3 +- .../extensions/_hook_projection.py | 269 +++++++++--------- .../extensions/test_hook_projection.py | 27 +- 3 files changed, 140 insertions(+), 159 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 431f2d39ca..fa370a757a 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -312,7 +312,8 @@ or perform an extension configuration update. Only the literal JSON boolean 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. +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 diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index a9ccf8de63..e25ebb17f2 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -17,7 +17,6 @@ from .._init_options import load_init_options from ..integrations._file_changes import changing_file -from ..shared_infra import _exclusive_project_lock from . import DEFAULT_HOOK_PRIORITY, normalize_priority _EVENT = re.compile(r"(before|after)_[a-z0-9][a-z0-9_]*\Z") @@ -70,80 +69,73 @@ def _read_state(config_file: Path) -> HookState: def snapshot(config_file: Path) -> HookState: - """Capture the exact on-disk YAML and projection under the publication lock.""" - root = config_file.parent - with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( - root.parent, ".hook-dispatch.lock", context="hook projection" - ): - return _read_state(config_file) + """Capture the exact on-disk YAML and projection for lifecycle rollback.""" + return _read_state(config_file) def restore(config_file: Path, previous: HookState) -> None: """Restore an exact hook snapshot, including absent files and cache contents.""" root = config_file.parent cache = _cache_path(config_file) - with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( - root.parent, ".hook-dispatch.lock", context="hook projection" - ): - current = _read_state(config_file) - if current == previous: - return - stage = Path(tempfile.mkdtemp(prefix=".hook-restore-", dir=root)) - retain_stage = False - moved = False - installed = False - try: + current = _read_state(config_file) + if current == previous: + return + stage = Path(tempfile.mkdtemp(prefix=".hook-restore-", dir=root)) + retain_stage = False + moved = False + installed = False + try: + if previous.cache is not None: + staged_cache = stage / "cache" + staged_cache.mkdir() + for name, content in previous.cache.items(): + (staged_cache / name).write_bytes(content) + if previous.source is not None: + (stage / "source.yml").write_bytes(previous.source) + with changing_file(cache, removal=previous.cache is None): + if cache.exists(): + os.replace(cache, stage / "previous-cache") + moved = True if previous.cache is not None: - staged_cache = stage / "cache" - staged_cache.mkdir() - for name, content in previous.cache.items(): - (staged_cache / name).write_bytes(content) - if previous.source is not None: - (stage / "source.yml").write_bytes(previous.source) - with changing_file(cache, removal=previous.cache is None): - if cache.exists(): - os.replace(cache, stage / "previous-cache") - moved = True - if previous.cache is not None: - os.replace(stage / "cache", cache) - installed = True - if previous.source is None: + os.replace(stage / "cache", cache) + installed = True + if previous.source is None: + with changing_file(config_file, removal=True): + config_file.unlink(missing_ok=True) + else: + with changing_file(config_file): + os.replace(stage / "source.yml", config_file) + except Exception as exc: + try: + if installed: + with changing_file(cache, removal=True): + shutil.rmtree(cache) + if moved: + with changing_file(cache): + os.replace(stage / "previous-cache", cache) + if current.source is None: with changing_file(config_file, removal=True): config_file.unlink(missing_ok=True) - else: + elif not config_file.is_file() or config_file.read_bytes() != current.source: + (stage / "rollback.yml").write_bytes(current.source) with changing_file(config_file): - os.replace(stage / "source.yml", config_file) - except Exception as exc: + os.replace(stage / "rollback.yml", config_file) + except Exception as rollback_exc: + retain_stage = True + raise OSError( + f"Hook snapshot restoration failed ({exc}); rollback failed " + f"({rollback_exc}); backup retained at {stage}" + ) from exc + raise + finally: + if not retain_stage: try: - if installed: - with changing_file(cache, removal=True): - shutil.rmtree(cache) - if moved: - with changing_file(cache): - os.replace(stage / "previous-cache", cache) - if current.source is None: - with changing_file(config_file, removal=True): - config_file.unlink(missing_ok=True) - elif not config_file.is_file() or config_file.read_bytes() != current.source: - (stage / "rollback.yml").write_bytes(current.source) - with changing_file(config_file): - os.replace(stage / "rollback.yml", config_file) - except Exception as rollback_exc: - retain_stage = True - raise OSError( - f"Hook snapshot restoration failed ({exc}); rollback failed " - f"({rollback_exc}); backup retained at {stage}" - ) from exc - raise - finally: - if not retain_stage: - try: - shutil.rmtree(stage) - except OSError as exc: - logging.getLogger(__name__).warning( - "Could not remove hook snapshot staging %s: %s", - stage, exc, - ) + shutil.rmtree(stage) + except OSError as exc: + logging.getLogger(__name__).warning( + "Could not remove hook snapshot staging %s: %s", + stage, exc, + ) def _check_paths(config_file: Path) -> None: @@ -224,14 +216,10 @@ def retire(config_file: Path) -> None: cache = _cache_path(config_file) if not cache.exists() and not cache.is_symlink(): return - root = config_file.parent - with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( - root.parent, ".hook-dispatch.lock", context="hook projection" - ): - _check_paths(config_file) - if cache.exists(): - with changing_file(cache, removal=True): - shutil.rmtree(cache) + _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: @@ -248,77 +236,74 @@ def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool rendered = yaml.dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True).encode("utf-8") root = config_file.parent root.mkdir(parents=True, exist_ok=True) - with changing_file(root / ".hook-dispatch.lock"), _exclusive_project_lock( - root.parent, ".hook-dispatch.lock", context="hook projection" - ): - _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) - old_config = config_file.read_bytes() if config_file.exists() else None - stage = Path(tempfile.mkdtemp(prefix=".hook-stage-", dir=root)) - backup = root / f"{stage.name}.backup" - moved = False - installed = False + _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) + old_config = config_file.read_bytes() if config_file.exists() else None + stage = Path(tempfile.mkdtemp(prefix=".hook-stage-", dir=root)) + backup = root / f"{stage.name}.backup" + moved = False + installed = False + try: + if cache.exists(): + for child in cache.iterdir(): + if child.name not in {"source.yml", "events.txt", "events.txt.sha256"} and not child.name.endswith((".json", ".sha256")): + shutil.copy2(child, stage / child.name) + for event, payload in projected.items(): + _write_digest(stage, f"{event}.json", payload) + _write_digest(stage, "events.txt", "".join(f"{event}\n" for event in projected).encode("utf-8")) + (stage / "source.yml").write_bytes(source) + with changing_file(cache): if cache.exists(): - for child in cache.iterdir(): - if child.name not in {"source.yml", "events.txt", "events.txt.sha256"} and not child.name.endswith((".json", ".sha256")): - shutil.copy2(child, stage / child.name) - for event, payload in projected.items(): - _write_digest(stage, f"{event}.json", payload) - _write_digest(stage, "events.txt", "".join(f"{event}\n" for event in projected).encode("utf-8")) - (stage / "source.yml").write_bytes(source) - with changing_file(cache): - if cache.exists(): - os.replace(cache, backup) - moved = True - os.replace(stage, cache) - installed = True + os.replace(cache, backup) + moved = True + os.replace(stage, cache) + installed = True + if not preserve_source: + temporary = root / f"{stage.name}.yml" + try: + temporary.write_bytes(rendered) + with changing_file(config_file): + os.replace(temporary, config_file) + finally: + temporary.unlink(missing_ok=True) + except Exception as exc: + try: + if installed: + with changing_file(cache, removal=True): + shutil.rmtree(cache) + if moved: + with changing_file(cache): + os.replace(backup, cache) if not preserve_source: - temporary = root / f"{stage.name}.yml" - try: - temporary.write_bytes(rendered) + if old_config is None: + with changing_file(config_file, removal=True): + config_file.unlink(missing_ok=True) + elif config_file.read_bytes() != old_config: with changing_file(config_file): - os.replace(temporary, config_file) - finally: - temporary.unlink(missing_ok=True) - except Exception as exc: - try: - if installed: - with changing_file(cache, removal=True): - shutil.rmtree(cache) - if moved: - with changing_file(cache): - os.replace(backup, cache) - if not preserve_source: - if old_config is None: - with changing_file(config_file, removal=True): - config_file.unlink(missing_ok=True) - elif config_file.read_bytes() != old_config: - with changing_file(config_file): - config_file.write_bytes(old_config) - except Exception as rollback_exc: - raise OSError(f"Hook projection publication failed ({exc}); rollback failed: {rollback_exc}") from exc - raise - finally: - if stage.exists(): - shutil.rmtree(stage) - if moved: - try: - shutil.rmtree(backup) - except OSError as exc: - logging.getLogger(__name__).warning( - "Hook projection committed; could not remove backup %s: %s", - backup, - exc, - ) + config_file.write_bytes(old_config) + except Exception as rollback_exc: + raise OSError(f"Hook projection publication failed ({exc}); rollback failed: {rollback_exc}") from exc + raise + finally: + if stage.exists(): + shutil.rmtree(stage) + if moved: + try: + shutil.rmtree(backup) + except OSError as exc: + logging.getLogger(__name__).warning( + "Hook projection committed; could not remove backup %s: %s", + backup, + exc, + ) diff --git a/tests/specify_cli/extensions/test_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py index bafea4cd09..3e786f12f7 100644 --- a/tests/specify_cli/extensions/test_hook_projection.py +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -2,7 +2,6 @@ import hashlib import json -from contextlib import contextmanager from pathlib import Path import pytest @@ -169,29 +168,25 @@ def test_update_and_refresh_retire_stale_events_and_preserve_crlf(tmp_path): assert b"\r\n" in executor.config_file.read_bytes() -def test_refresh_projects_exact_source_read_under_lock(tmp_path, monkeypatch): - import specify_cli.extensions._hook_projection as projection - +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": []}} - original_lock = projection._exclusive_project_lock - - @contextmanager - def concurrent_save(*args, **kwargs): - with original_lock(*args, **kwargs): - executor.config_file.write_text( - yaml.safe_dump(updated, sort_keys=False), encoding="utf-8" - ) - yield - - monkeypatch.setattr(projection, "_exclusive_project_lock", concurrent_save) - executor.migrate_project_config() 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): From 954998adea65b43c6e61c8208962abd119e835bc Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 14:24:36 -0500 Subject: [PATCH 13/14] Simplify opted-in hook projection to direct writes User-directed: remove projection-specific staging and rollback, retain validation and explicit partial-I/O recovery guidance, and keep ordinary cleanup before hook removal. Document the single-mutating-CLI writable-project assumption. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- AGENTS.md | 8 + docs/reference/extensions.md | 19 +- src/specify_cli/extensions/__init__.py | 156 ++++++--------- .../extensions/_hook_projection.py | 180 +++--------------- .../extensions/test_command_add.py | 38 ++-- .../extensions/test_command_remove.py | 51 ++--- .../extensions/test_hook_projection.py | 97 +++++----- 7 files changed, 201 insertions(+), 348 deletions(-) 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 fa370a757a..e3a70e0c18 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -331,13 +331,18 @@ 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. -If hook publication fails during an opted-in install, Spec Kit restores the -prior hook configuration and projection, including pre-existing hooks for the -same extension ID. If an opted-in removal fails during subsequent cleanup, -it restores the original YAML and complete projection bytes (including -comments and line endings) or reports a rollback failure. This does not make -removal of command artifacts and extension sources atomic; cleanup already -completed may not be reversed. +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: diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index dbd4f7abc5..2c98340787 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -3125,7 +3125,6 @@ def rollback_installation_files() -> None: hooks_started = False hooks_finished = False - hook_snapshot = None registry_started = False registered_commands = {} registered_skills = [] @@ -3159,10 +3158,6 @@ def rollback_installation_files() -> None: # Register hooks and update installed list in extensions.yml hook_executor = HookExecutor(self.project_root) - if native_hooks: - from ._hook_projection import snapshot - - hook_snapshot = snapshot(hook_executor.config_file) hooks_started = True hook_executor.register_hooks(manifest) hooks_finished = True @@ -3211,9 +3206,8 @@ def rollback_installation_files() -> None: registry_entry["generic_artifact_hashes"] = generic_hashes self.registry.add(manifest.id, registry_entry) except Exception as exc: - # Keep the existing non-generic retry semantics for failures after - # hooks have committed, but retire every agent's outputs when hook - # publication itself fails. + # 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) @@ -3224,14 +3218,9 @@ def rollback_installation_files() -> None: 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: - if hook_snapshot is not None and not hooks_finished: - from ._hook_projection import restore - - restore(hook_executor.config_file, hook_snapshot) - else: - hook_executor.unregister_hooks(manifest.id) + hook_executor.unregister_hooks(manifest.id) except Exception as error: rollback_errors.append(f"hooks: {error}") if registered_commands: @@ -3593,10 +3582,7 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: if not self.registry.is_installed(extension_id): return False - from ._hook_projection import projection_enabled, restore, snapshot - hook_executor = HookExecutor(self.project_root) - projection_active = projection_enabled(self.project_root) hook_executor.load_project_config_for_update() # Get registered commands and skills before removal @@ -3612,88 +3598,70 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: registered_skills = [] extension_dir = self.extensions_dir / extension_id - hook_snapshot = snapshot(hook_executor.config_file) if projection_active else None - # Publication can fail even after preflight (for example, on a disk - # error). Keep all invocation artifacts and sources intact until the - # extension has been removed from the hook configuration. - try: - if projection_active: - hook_executor.unregister_hooks(extension_id) - - # Unregister commands from all AI agents - if registered_commands: - safe_commands = dict(registered_commands) - if "generic" in safe_commands: - safe_commands.pop("generic") - if safe_commands: - CommandRegistrar(self.project_root, include_generic=False).unregister_commands( - safe_commands, self.project_root - ) - if metadata: - self._remove_generic_artifact_paths(extension_id, metadata) + # Unregister commands from all AI agents + if registered_commands: + safe_commands = dict(registered_commands) + if "generic" in safe_commands: + safe_commands.pop("generic") + if safe_commands: + CommandRegistrar(self.project_root, include_generic=False).unregister_commands( + safe_commands, self.project_root + ) + if metadata: + self._remove_generic_artifact_paths(extension_id, metadata) - # Unregister agent skills - self._unregister_extension_skills( - registered_skills, - extension_id, - generic_hashes=metadata.get("generic_artifact_hashes") if metadata else None, - ) + # Unregister agent skills + self._unregister_extension_skills( + registered_skills, + extension_id, + generic_hashes=metadata.get("generic_artifact_hashes") if metadata else None, + ) - if keep_config: - # Preserve config files, only remove non-config files - if extension_dir.exists(): - for child in extension_dir.iterdir(): - # Keep top-level *-config.yml and *-config.local.yml files - if child.is_file() and ( - child.name.endswith("-config.yml") - or child.name.endswith("-config.local.yml") - ): - continue - if child.is_dir(): - shutil.rmtree(child) - else: - child.unlink() - # Write a provenance marker so install_from_directory can - # distinguish this --keep-config leftover from a directory left - # by a partially-failed install (which must not have its - # packaged default configs treated as user-preserved data). - # Content is intentionally empty — only presence matters. - (extension_dir / ".keep-config").write_text("") - else: - # Backup config files before deleting - if extension_dir.exists(): - # Use subdirectory per extension to avoid name accumulation - # (e.g., jira-jira-config.yml on repeated remove/install cycles) - backup_dir = self.extensions_dir / ".backup" / extension_id - backup_dir.mkdir(parents=True, exist_ok=True) - - # Backup both primary and local override config files - config_files = list(extension_dir.glob("*-config.yml")) + list( - extension_dir.glob("*-config.local.yml") - ) - for config_file in config_files: - backup_path = backup_dir / config_file.name - shutil.copy2(config_file, backup_path) + if keep_config: + # Preserve config files, only remove non-config files + if extension_dir.exists(): + for child in extension_dir.iterdir(): + # Keep top-level *-config.yml and *-config.local.yml files + if child.is_file() and ( + child.name.endswith("-config.yml") + or child.name.endswith("-config.local.yml") + ): + continue + if child.is_dir(): + shutil.rmtree(child) + else: + child.unlink() + # Write a provenance marker so install_from_directory can + # distinguish this --keep-config leftover from a directory left + # by a partially-failed install (which must not have its + # packaged default configs treated as user-preserved data). + # Content is intentionally empty — only presence matters. + (extension_dir / ".keep-config").write_text("") + else: + # Backup config files before deleting + if extension_dir.exists(): + # Use subdirectory per extension to avoid name accumulation + # (e.g., jira-jira-config.yml on repeated remove/install cycles) + backup_dir = self.extensions_dir / ".backup" / extension_id + backup_dir.mkdir(parents=True, exist_ok=True) + + # Backup both primary and local override config files + config_files = list(extension_dir.glob("*-config.yml")) + list( + extension_dir.glob("*-config.local.yml") + ) + for config_file in config_files: + backup_path = backup_dir / config_file.name + shutil.copy2(config_file, backup_path) - # Remove extension directory - if extension_dir.exists(): - shutil.rmtree(extension_dir) + # Remove extension directory + if extension_dir.exists(): + shutil.rmtree(extension_dir) - if not projection_active: - hook_executor.unregister_hooks(extension_id) + hook_executor.unregister_hooks(extension_id) - # Update registry - self.registry.remove(extension_id) - except Exception as exc: - if hook_snapshot is not None: - try: - restore(hook_executor.config_file, hook_snapshot) - except (ValueError, OSError) as rollback_exc: - raise ExtensionError( - f"Extension removal failed ({exc}); hook rollback failed: {rollback_exc}" - ) from exc - raise + # Update registry + self.registry.remove(extension_id) return True diff --git a/src/specify_cli/extensions/_hook_projection.py b/src/specify_cli/extensions/_hook_projection.py index e25ebb17f2..251494e4af 100644 --- a/src/specify_cli/extensions/_hook_projection.py +++ b/src/specify_cli/extensions/_hook_projection.py @@ -1,22 +1,18 @@ -"""Validated, transactional projection of extension hooks for future dispatchers.""" +"""Validated projection of extension hooks for future dispatchers.""" from __future__ import annotations import hashlib import json -import logging -import os import re import shutil -import tempfile -from dataclasses import dataclass from pathlib import Path from typing import Any import yaml from .._init_options import load_init_options -from ..integrations._file_changes import changing_file +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") @@ -52,92 +48,6 @@ def _cache_path(config_file: Path) -> Path: return config_file.parent / "hook-dispatch" -@dataclass(frozen=True) -class HookState: - source: bytes | None - cache: dict[str, bytes] | None - - -def _read_state(config_file: Path) -> HookState: - _check_paths(config_file) - cache = _cache_path(config_file) - return HookState( - config_file.read_bytes() if config_file.exists() else None, - {entry.name: entry.read_bytes() for entry in cache.iterdir()} - if cache.exists() else None, - ) - - -def snapshot(config_file: Path) -> HookState: - """Capture the exact on-disk YAML and projection for lifecycle rollback.""" - return _read_state(config_file) - - -def restore(config_file: Path, previous: HookState) -> None: - """Restore an exact hook snapshot, including absent files and cache contents.""" - root = config_file.parent - cache = _cache_path(config_file) - current = _read_state(config_file) - if current == previous: - return - stage = Path(tempfile.mkdtemp(prefix=".hook-restore-", dir=root)) - retain_stage = False - moved = False - installed = False - try: - if previous.cache is not None: - staged_cache = stage / "cache" - staged_cache.mkdir() - for name, content in previous.cache.items(): - (staged_cache / name).write_bytes(content) - if previous.source is not None: - (stage / "source.yml").write_bytes(previous.source) - with changing_file(cache, removal=previous.cache is None): - if cache.exists(): - os.replace(cache, stage / "previous-cache") - moved = True - if previous.cache is not None: - os.replace(stage / "cache", cache) - installed = True - if previous.source is None: - with changing_file(config_file, removal=True): - config_file.unlink(missing_ok=True) - else: - with changing_file(config_file): - os.replace(stage / "source.yml", config_file) - except Exception as exc: - try: - if installed: - with changing_file(cache, removal=True): - shutil.rmtree(cache) - if moved: - with changing_file(cache): - os.replace(stage / "previous-cache", cache) - if current.source is None: - with changing_file(config_file, removal=True): - config_file.unlink(missing_ok=True) - elif not config_file.is_file() or config_file.read_bytes() != current.source: - (stage / "rollback.yml").write_bytes(current.source) - with changing_file(config_file): - os.replace(stage / "rollback.yml", config_file) - except Exception as rollback_exc: - retain_stage = True - raise OSError( - f"Hook snapshot restoration failed ({exc}); rollback failed " - f"({rollback_exc}); backup retained at {stage}" - ) from exc - raise - finally: - if not retain_stage: - try: - shutil.rmtree(stage) - except OSError as exc: - logging.getLogger(__name__).warning( - "Could not remove hook snapshot staging %s: %s", - stage, exc, - ) - - def _check_paths(config_file: Path) -> None: root = config_file.parent cache = _cache_path(config_file) @@ -223,19 +133,19 @@ def retire(config_file: Path) -> None: def _write_digest(directory: Path, name: str, payload: bytes) -> None: - (directory / name).write_bytes(payload) - (directory / f"{name}.sha256").write_text( - hashlib.sha256(payload).hexdigest() + "\n", encoding="ascii" + 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: - """Publish one complete projection and its YAML source, or restore the prior pair.""" + """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 - root.mkdir(parents=True, exist_ok=True) _check_paths(config_file) if preserve_source: source = config_file.read_bytes() @@ -249,61 +159,25 @@ def publish(config_file: Path, config: dict[str, Any], *, preserve_source: bool else: source = rendered cache = _cache_path(config_file) - old_config = config_file.read_bytes() if config_file.exists() else None - stage = Path(tempfile.mkdtemp(prefix=".hook-stage-", dir=root)) - backup = root / f"{stage.name}.backup" - moved = False - installed = False try: - if cache.exists(): - for child in cache.iterdir(): - if child.name not in {"source.yml", "events.txt", "events.txt.sha256"} and not child.name.endswith((".json", ".sha256")): - shutil.copy2(child, stage / child.name) - for event, payload in projected.items(): - _write_digest(stage, f"{event}.json", payload) - _write_digest(stage, "events.txt", "".join(f"{event}\n" for event in projected).encode("utf-8")) - (stage / "source.yml").write_bytes(source) - with changing_file(cache): - if cache.exists(): - os.replace(cache, backup) - moved = True - os.replace(stage, cache) - installed = True + root.mkdir(parents=True, exist_ok=True) if not preserve_source: - temporary = root / f"{stage.name}.yml" - try: - temporary.write_bytes(rendered) - with changing_file(config_file): - os.replace(temporary, config_file) - finally: - temporary.unlink(missing_ok=True) - except Exception as exc: - try: - if installed: - with changing_file(cache, removal=True): - shutil.rmtree(cache) - if moved: - with changing_file(cache): - os.replace(backup, cache) - if not preserve_source: - if old_config is None: - with changing_file(config_file, removal=True): - config_file.unlink(missing_ok=True) - elif config_file.read_bytes() != old_config: - with changing_file(config_file): - config_file.write_bytes(old_config) - except Exception as rollback_exc: - raise OSError(f"Hook projection publication failed ({exc}); rollback failed: {rollback_exc}") from exc - raise - finally: - if stage.exists(): - shutil.rmtree(stage) - if moved: - try: - shutil.rmtree(backup) - except OSError as exc: - logging.getLogger(__name__).warning( - "Hook projection committed; could not remove backup %s: %s", - backup, - exc, - ) + 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/tests/specify_cli/extensions/test_command_add.py b/tests/specify_cli/extensions/test_command_add.py index ed98c854eb..b088806eb6 100644 --- a/tests/specify_cli/extensions/test_command_add.py +++ b/tests/specify_cli/extensions/test_command_add.py @@ -107,8 +107,6 @@ def test_add_disabled_manifest_hook_respects_opt_in( def test_install_publication_failure_keeps_preexisting_same_id_hooks( self, extension_dir, project_dir, monkeypatch ): - import specify_cli.extensions._hook_projection as projection - enable_native_hooks(project_dir) config = project_dir / ".specify/extensions.yml" config.parent.mkdir(parents=True, exist_ok=True) @@ -123,19 +121,16 @@ def test_install_publication_failure_keeps_preexisting_same_id_hooks( cache = project_dir / ".specify/hook-dispatch" original = config.read_bytes() prior_cache = {entry.name: entry.read_bytes() for entry in cache.iterdir()} - real_replace = projection.os.replace - failed = False + real_write = Path.write_bytes - def fail_config_write(source, target): - nonlocal failed - if Path(target) == config and not failed: - failed = True + def fail_config_write(path, content): + if path == config: raise OSError("simulated hook publication failure") - return real_replace(source, target) + return real_write(path, content) - monkeypatch.setattr(projection.os, "replace", fail_config_write) + monkeypatch.setattr(Path, "write_bytes", fail_config_write) manager = ExtensionManager(project_dir) - with pytest.raises(OSError, match="simulated hook publication failure"): + with pytest.raises(OSError, match="simulated hook publication failure.*refresh-shared-infra"): manager.install_from_directory( extension_dir, "0.1.0", register_commands=False ) @@ -180,23 +175,17 @@ def test_install_replaces_preexisting_same_id_hooks_on_success( else: assert not cache.exists() - def test_install_reports_failed_hook_snapshot_restoration( + 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( - "specify_cli.extensions._hook_projection.restore", - side_effect=OSError("restore failed"), - ): - with pytest.raises(ExtensionError) as error: + 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 "publication failed" in str(error.value) - assert "rollback failed: hooks: restore failed" in str(error.value) assert not manager.registry.is_installed("test-ext") def test_add_rejects_invalid_existing_hooks_before_install(self, extension_dir, project_dir): @@ -241,12 +230,7 @@ def fail_hook_save(executor, config): 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() - config = project_dir / ".specify/extensions.yml" - if config.exists(): - assert "test-ext" not in (config.read_text(encoding="utf-8")) - cache = project_dir / ".specify/hook-dispatch" - if cache.exists(): - assert (cache / "source.yml").read_bytes() == config.read_bytes() + 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( diff --git a/tests/specify_cli/extensions/test_command_remove.py b/tests/specify_cli/extensions/test_command_remove.py index 9e7881c2df..4b0af828dc 100644 --- a/tests/specify_cli/extensions/test_command_remove.py +++ b/tests/specify_cli/extensions/test_command_remove.py @@ -136,7 +136,7 @@ def test_remove_reports_invalid_hooks_without_mutating_installation( 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_retains_generic_installation( + def test_remove_publication_failure_reports_partial_cleanup_and_recovery( self, tmp_path, extension_dir ): project, _, artifact = installed_generic_hook_extension( @@ -146,24 +146,27 @@ def test_remove_publication_failure_retains_generic_installation( extension_dir = 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()) - original_save = HookExecutor.save_project_config + original_source = (cache / "source.yml").read_bytes() + real_write = Path.write_bytes - def fail_hook_removal(executor, updated): - if not updated.get("hooks", {}).get("before_plan"): + def fail_index(path, content): + if path == cache / "events.txt": raise OSError("projection failed") - return original_save(executor, updated) + return real_write(path, content) with patch.object(Path, "cwd", return_value=project), \ - patch.object(HookExecutor, "save_project_config", fail_hook_removal): + 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 extension_dir.is_dir() - assert (artifact.read_bytes(), config.read_bytes(), (cache / "source.yml").read_bytes()) == original + 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_restores_published_hooks( + def test_remove_artifact_failure_leaves_published_hooks_untouched( self, tmp_path, extension_dir ): project, _, artifact = installed_generic_hook_extension( @@ -185,7 +188,7 @@ def test_remove_artifact_failure_restores_published_hooks( 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_restores_hooks_not_artifacts( + def test_remove_partial_cleanup_leaves_hooks_untouched_not_artifacts( self, tmp_path, extension_dir ): project, _, artifact = installed_generic_hook_extension( @@ -234,7 +237,7 @@ def test_remove_cleanup_failure_preserves_exact_yaml_and_cache( assert config.read_bytes() == original_yaml assert {entry.name: entry.read_bytes() for entry in cache.iterdir()} == original_cache - def test_remove_cleanup_failure_restores_absent_yaml_and_cache( + def test_remove_cleanup_failure_leaves_absent_yaml_and_cache_untouched( self, tmp_path, extension_dir ): project, manager, _ = installed_generic_hook_extension( @@ -253,18 +256,18 @@ def test_remove_cleanup_failure_restores_absent_yaml_and_cache( assert not config.exists() assert not cache.exists() - def test_remove_reports_failed_hook_restoration( + def test_default_remove_cleanup_failure_keeps_legacy_hook( self, tmp_path, extension_dir ): - project, _, _ = installed_generic_hook_extension( - tmp_path, extension_dir, native_hooks=True + project, manager, artifact = installed_generic_hook_extension( + tmp_path, extension_dir, native_hooks=False ) - - with patch.object(Path, "cwd", return_value=project), \ - patch.object(ExtensionManager, "_remove_generic_artifact_paths", side_effect=OSError("cleanup failed")), \ - patch("specify_cli.extensions._hook_projection.restore", side_effect=OSError("restore failed")): - result = CliRunner().invoke(app, ["extension", "remove", "test-ext", "--force"]) - assert result.exit_code == 1 - assert "cleanup failed" in result.output - assert "hook rollback failed" in result.output - assert "restore failed" in " ".join(result.output.split()) + 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_hook_projection.py b/tests/specify_cli/extensions/test_hook_projection.py index 3e786f12f7..3a9c7c9401 100644 --- a/tests/specify_cli/extensions/test_hook_projection.py +++ b/tests/specify_cli/extensions/test_hook_projection.py @@ -226,82 +226,93 @@ def test_refuse_symlinked_config_and_cache_before_mutation(tmp_path): assert executor.config_file.read_text() == "hooks: {}\n" -def test_publication_failure_restores_prior_pair(tmp_path, monkeypatch): - import specify_cli.extensions._hook_projection as projection - +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_replace = projection.os.replace + real_write = Path.write_bytes - def fail_config_write(src, dst): - if Path(dst) == executor.config_file: + 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_replace(src, dst) + return real_write(path, content) - monkeypatch.setattr(projection.os, "replace", fail_config_write) - with pytest.raises(OSError, match="simulated config"): + 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 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) - assert not list((tmp_path / ".specify").glob(".hook-stage-*")) -def test_snapshot_restoration_failure_retains_current_pair(tmp_path, monkeypatch): + +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()) - original = projection.snapshot(executor.config_file) - executor.save_project_config({"installed": [], "hooks": {}}) - current = projection.snapshot(executor.config_file) - real_replace = projection.os.replace - failed = False - - def fail_restore_write(source, target): - nonlocal failed - if Path(target) == executor.config_file and not failed: - failed = True - raise OSError("restore write failed") - return real_replace(source, target) - - monkeypatch.setattr(projection.os, "replace", fail_restore_write) - with pytest.raises(OSError, match="restore write failed"): - projection.restore(executor.config_file, original) - assert projection.snapshot(executor.config_file) == current - assert not list(executor.config_file.parent.glob(".hook-restore-*")) - - -def test_unreadable_existing_config_does_not_allocate_stage(tmp_path): + 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 list(executor.config_file.parent.glob(".hook-stage-*")) assert not (tmp_path / ".specify/hook-dispatch").exists() -def test_backup_cleanup_failure_warns_after_successful_publication(tmp_path, monkeypatch, caplog): +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()) - real_rmtree = projection.shutil.rmtree + cache = tmp_path / ".specify/hook-dispatch" + real_unlink = projection.unlink - def fail_backup_cleanup(path, *args, **kwargs): - if str(path).endswith(".backup"): - raise OSError("backup cleanup failed") - return real_rmtree(path, *args, **kwargs) + def fail_stale_cleanup(path): + if path == cache / "before_plan.json": + raise OSError("cannot remove stale event") + return real_unlink(path) - monkeypatch.setattr(projection.shutil, "rmtree", fail_backup_cleanup) + monkeypatch.setattr(projection, "unlink", fail_stale_cleanup) updated = {"installed": ["example"], "hooks": {"after_plan": []}} - executor.save_project_config(updated) - assert "backup cleanup failed" in caplog.text + 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() From 60b2456a0d868ee520b31f9ebae915ef7dd8dcf1 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:02:49 -0500 Subject: [PATCH 14/14] Preserve opted-in hook flags across extension enable Move the enable lifecycle transaction into ExtensionManager, recording each owned hook enabled value on disable and restoring it on re-enable without changing legacy opt-out behavior. User-directed review 5523638830 follow-up. Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f460283-8c52-47f4-8cfe-f67a833296cf --- docs/reference/extensions.md | 7 + src/specify_cli/extensions/__init__.py | 152 ++++++++++++++- src/specify_cli/extensions/command_enable.py | 82 +------- .../extensions/test_command_enable.py | 176 +++++++++++++++++- 4 files changed, 329 insertions(+), 88 deletions(-) diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index e3a70e0c18..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 diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 2c98340787..86ef233381 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -3671,6 +3671,7 @@ def disable(self, extension_id: str) -> bool: 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() @@ -3682,32 +3683,165 @@ def disable(self, extension_id: str) -> bool: 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: - for hooks in config["hooks"].values(): - for hook in hooks: - if hook.get("extension") == extension_id: - hook["enabled"] = False + 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.restore_generic_extension_after_failed_disable( - extension_id, metadata - ) - else: - self.registry.update(extension_id, {"enabled": True}) + 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( diff --git a/src/specify_cli/extensions/command_enable.py b/src/specify_cli/extensions/command_enable.py index b6cb82b4a6..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() @@ -30,87 +29,14 @@ def extension_enable( ) try: - config = hook_executor.load_project_config_for_update() - except (ValueError, OSError) as exc: + 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 - - # 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): + 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 - 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 - try: - hook_executor.save_project_config(config) - except (ValueError, OSError) as exc: - try: - if init_options.get("ai") == "generic": - manager.disable_generic_extension_artifacts(extension_id) - else: - manager.registry.update(extension_id, {"enabled": False}) - except (ExtensionError, ValueError, OSError) as rollback_exc: - console.print( - f"[red]Error:[/red] {_escape_markup(str(exc))}; " - f"rollback failed: {_escape_markup(str(rollback_exc))}" - ) - raise typer.Exit(1) from exc - console.print(f"[red]Error:[/red] {_escape_markup(str(exc))}") - raise typer.Exit(1) from exc - 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/tests/specify_cli/extensions/test_command_enable.py b/tests/specify_cli/extensions/test_command_enable.py index c4df5624c7..3a99a10a5e 100644 --- a/tests/specify_cli/extensions/test_command_enable.py +++ b/tests/specify_cli/extensions/test_command_enable.py @@ -5,23 +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 tests.specify_cli.extensions._helpers import installed_generic_hook_extension +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.""" @@ -73,3 +132,118 @@ def test_enable_publication_failure_restores_generic_disabled_state( 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"]