From 4ec4635dd19b3e34dc3ae9ea61703ff683621f54 Mon Sep 17 00:00:00 2001 From: Seiya Kojima <20368071+seiya-koji@users.noreply.github.com> Date: Mon, 8 Jun 2026 22:03:46 +0900 Subject: [PATCH] feat(extensions): per-event hook lists with priority ordering (#2798) * feat(extensions): per-event hook lists with priority ordering The manifest validator restricted each hook event to a single mapping, even though HookExecutor stores entries as a list per event. This blocked an extension from running multiple commands on one event (e.g. a verification step plus a doc-generation step after speckit.plan), and get_hooks_for_event returned entries in raw insertion order with no way to influence execution order across or within extensions. This change: 1. Validator: accept hooks. as either a single mapping or a list of mappings. Each entry is validated individually and may carry an optional integer `priority` (>= 1, default 10; bool rejected). 2. Command-ref normalization: apply rename / alias->canonical rewriting to every entry in the list, not just the head. 3. register_hooks: expand list entries, persist `priority`, and purge-and-replace all entries owned by the extension on each event so a reinstall whose shape changed (single<->list, or a shorter list) leaves no orphaned entries behind. 4. get_hooks_for_event: sort enabled entries by `priority` ascending with a stable sort (ties keep insertion order). The existing normalize_priority helper is reused as the sort key so corrupted on-disk values fall back to the default instead of raising. Backward compatible: existing single-mapping manifests parse and register unchanged with priority defaulting to 10. The extension-level `priority` used by preset/template resolution is independent of the new hook-entry `priority`. Implements #2378 * fix(extensions): harden register_hooks per PR review - Skip non-dict hook entries before .get() so a manifest that bypasses validation can't crash register_hooks with AttributeError. - Normalize `priority` on save via normalize_priority so the on-disk config stays clean, mirroring the read-side defense in get_hooks_for_event. - Tests: cover the non-dict-entry skip and add encoding="utf-8" to the new tests' manifest writes. * fix(extensions): purge dropped-event hook orphans on reinstall register_hooks only purged events the new manifest still declared, so an extension that dropped an event on reinstall left stale entries for it in the project config. Purge this extension's entries from undeclared events (and prune emptied events) before registering; scoped to this extension, and a no-op for the install/update flow where unregister_hooks runs first. * fix(extensions): reject boolean priority and complete orphan purge - normalize_priority falls back to default for bool values - dedup deletes duplicate commands before re-insert for last-wins ties - register_hooks purges orphans even when all hooks are dropped * docs(extensions): document per-event hook lists and priority - EXTENSION-API-REFERENCE: hook event accepts a mapping or list; add priority field reference and last-wins dedup note - EXTENSION-DEVELOPMENT-GUIDE: add list-form example with priority * docs(extensions): show both single and list hook forms in schema snippet * docs(extensions): reference DEFAULT_HOOK_PRIORITY in normalize_priority normalize_priority hard-coded the default as the literal 10 in both its signature and docstring, duplicating DEFAULT_HOOK_PRIORITY. Reference the constant in the signature and drop the literal from the docstring so the default has a single source of truth. --- extensions/EXTENSION-API-REFERENCE.md | 32 +- extensions/EXTENSION-DEVELOPMENT-GUIDE.md | 20 + extensions/template/extension.yml | 8 + src/specify_cli/extensions.py | 179 +++++--- tests/test_extensions.py | 537 ++++++++++++++++++++++ 5 files changed, 718 insertions(+), 58 deletions(-) diff --git a/extensions/EXTENSION-API-REFERENCE.md b/extensions/EXTENSION-API-REFERENCE.md index ce0ff1775..bf85d1882 100644 --- a/extensions/EXTENSION-API-REFERENCE.md +++ b/extensions/EXTENSION-API-REFERENCE.md @@ -52,13 +52,19 @@ provides: description: string required: boolean # Default: false -hooks: # Optional, event hooks +hooks: # Optional, event hooks. Each event accepts either form below. event_name: # e.g., "after_specify", "after_plan", "after_tasks", "after_implement" command: string # Command to execute + priority: integer # Optional, >= 1, default 10 (lower runs first) optional: boolean # Default: true prompt: string # Prompt text for optional hooks description: string # Hook description condition: string # Optional, condition expression + another_event: # Any event may instead use a list of mappings (multiple commands) + - command: string # Same fields as the single mapping, per entry + priority: integer + - command: string + priority: integer tags: # Optional, array of tags (2-10 recommended) - string @@ -109,8 +115,10 @@ defaults: # Optional, default configuration values - **Type**: object - **Keys**: Event names (e.g., `after_specify`, `after_plan`, `after_tasks`, `after_implement`, `before_analyze`) +- **Value**: A single hook mapping, or a list of hook mappings to register multiple commands on one event - **Description**: Hooks that execute at lifecycle events - **Events**: Defined by core spec-kit commands +- **Ordering**: Within an event, hooks run by ascending `priority` (integer ≥ 1, default 10; lower runs first; equal priorities keep authoring order via a stable sort) --- @@ -535,7 +543,9 @@ Examples: ### Hook Definition -**In extension.yml**: +Each event accepts either a single hook mapping or a list of mappings. A list registers multiple commands on the same event. + +**Single mapping (in extension.yml)**: ```yaml hooks: @@ -547,6 +557,24 @@ hooks: condition: null ``` +**List of mappings with priority**: + +```yaml +hooks: + after_plan: + - command: "speckit.my-ext.verify" + priority: 5 + optional: false + description: "Verify the plan" + - command: "speckit.my-ext.report" + priority: 10 + optional: true + prompt: "Generate the report?" + description: "Generate a report from the plan" +``` + +Within a single manifest list, a repeated `command` is deduped as "last wins" and moved to the end, so it also breaks equal-priority ties in authoring order. + ### Hook Events Standard events (defined by core): diff --git a/extensions/EXTENSION-DEVELOPMENT-GUIDE.md b/extensions/EXTENSION-DEVELOPMENT-GUIDE.md index 5f24e71f0..877cfa0f9 100644 --- a/extensions/EXTENSION-DEVELOPMENT-GUIDE.md +++ b/extensions/EXTENSION-DEVELOPMENT-GUIDE.md @@ -206,9 +206,12 @@ Available hook points: - `before_constitution` / `after_constitution`: Before/after constitution update - `before_taskstoissues` / `after_taskstoissues`: Before/after tasks-to-issues conversion +Each event accepts a single hook object or a list of hook objects (multiple commands on one event). + Hook object: - `command`: Command to execute (typically from `provides.commands`, but can reference any registered command) +- `priority`: Run order within the event (integer ≥ 1, default 10; lower runs first; equal priorities keep authoring order) - `optional`: If true, prompt user before executing - `prompt`: Prompt text for optional hooks - `description`: Hook description @@ -655,6 +658,23 @@ hooks: description: "Analyze tasks after generation" ``` +Multiple commands on one event, ordered by `priority` (lower runs first): + +```yaml +# extension.yml +hooks: + after_plan: + - command: "speckit.my-ext.verify" + priority: 5 + optional: false + description: "Verify the plan" + - command: "speckit.my-ext.report" + priority: 10 + optional: true + prompt: "Generate the report?" + description: "Generate a report from the plan" +``` + --- ## Troubleshooting diff --git a/extensions/template/extension.yml b/extensions/template/extension.yml index abf7e45af..b907e0c78 100644 --- a/extensions/template/extension.yml +++ b/extensions/template/extension.yml @@ -79,6 +79,14 @@ hooks: # optional: false # Auto-execute without prompting # description: "Runs automatically after implementation" + # MULTIPLE COMMANDS ON ONE EVENT: use a list of entries. + # Add optional `priority` (integer >= 1, default 10) to order them, lowest first. + # after_plan: + # - command: "speckit.my-extension.verify" + # priority: 5 + # - command: "speckit.my-extension.report" + # priority: 10 + # CUSTOMIZE: Add relevant tags (2-5 recommended) # Used for discovery in catalog tags: diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index bddf637cb..adbbedcb9 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -41,6 +41,8 @@ _FALLBACK_CORE_COMMAND_NAMES = frozenset({ }) EXTENSION_COMMAND_NAME_PATTERN = re.compile(r"^speckit\.([a-z0-9-]+)\.([a-z0-9-]+)$") +DEFAULT_HOOK_PRIORITY = 10 + REINSTALL_COMMAND = "uv tool install specify-cli --force --from git+https://github.com/github/spec-kit.git" @@ -89,19 +91,21 @@ class CompatibilityError(ExtensionError): pass -def normalize_priority(value: Any, default: int = 10) -> int: +def normalize_priority(value: Any, default: int = DEFAULT_HOOK_PRIORITY) -> int: """Normalize a stored priority value for sorting and display. - Corrupted registry data may contain missing, non-numeric, or non-positive - values. In those cases, fall back to the default priority. + Corrupted registry data may contain missing, non-numeric, non-positive, or + boolean values. In those cases, fall back to the default priority. Args: value: Priority value to normalize (may be int, str, None, etc.) - default: Default priority to use for invalid values (default: 10) + default: Default priority to use for invalid values Returns: Normalized priority as positive integer (>= 1) """ + if isinstance(value, bool): + return default try: priority = int(value) except (TypeError, ValueError): @@ -109,6 +113,15 @@ def normalize_priority(value: Any, default: int = 10) -> int: return priority if priority >= 1 else default +def coerce_hook_entries(hook_config: Any) -> List[Any]: + """Return a hook event's config as a list of entries. + + A hook event may be declared as a single mapping or a list of mappings. + Both shapes are normalized to a list so callers can iterate uniformly. + """ + return hook_config if isinstance(hook_config, list) else [hook_config] + + @dataclass class CatalogEntry(BaseCatalogEntry): """Represents a single catalog entry in the catalog stack.""" @@ -215,17 +228,36 @@ class ExtensionManifest: "Extension must provide at least one command or hook" ) - # Validate hook values (if present) + # Validate hook values (if present). + # Each event is a single mapping or a list of mappings. if hooks: for hook_name, hook_config in hooks.items(): - if not isinstance(hook_config, dict): + if isinstance(hook_config, list) and not hook_config: raise ValidationError( - f"Invalid hook '{hook_name}': expected a mapping" - ) - if not hook_config.get("command"): - raise ValidationError( - f"Hook '{hook_name}' missing required 'command' field" + f"Invalid hook '{hook_name}': list must contain at least one entry" ) + for entry in coerce_hook_entries(hook_config): + if not isinstance(entry, dict): + raise ValidationError( + f"Invalid hook '{hook_name}': " + "expected a mapping or list of mappings" + ) + if not entry.get("command"): + raise ValidationError( + f"Hook '{hook_name}' missing required 'command' field" + ) + if "priority" in entry: + priority = entry["priority"] + if not isinstance(priority, int) or isinstance(priority, bool): + raise ValidationError( + f"Hook '{hook_name}' has invalid 'priority': " + "must be an integer" + ) + if priority < 1: + raise ValidationError( + f"Hook '{hook_name}' has invalid 'priority': " + "must be >= 1" + ) # Validate commands; track renames so hook references can be rewritten. rename_map: Dict[str, str] = {} @@ -275,28 +307,30 @@ class ExtensionManifest: # an alias-form ref (ext.cmd → speckit.ext.cmd). Always emit a warning when # the reference is changed so extension authors know to update the manifest. for hook_name, hook_data in self.data.get("hooks", {}).items(): - if not isinstance(hook_data, dict): - raise ValidationError( - f"Hook '{hook_name}' must be a mapping, got {type(hook_data).__name__}" - ) - command_ref = hook_data.get("command") - if not isinstance(command_ref, str): - continue - # Step 1: apply any rename from the auto-correction pass. - after_rename = rename_map.get(command_ref, command_ref) - # Step 2: lift alias-form '{ext_id}.cmd' to canonical 'speckit.{ext_id}.cmd'. - parts = after_rename.split(".") - if len(parts) == 2 and parts[0] == ext["id"]: - final_ref = f"speckit.{ext['id']}.{parts[1]}" - else: - final_ref = after_rename - if final_ref != command_ref: - hook_data["command"] = final_ref - self.warnings.append( - f"Hook '{hook_name}' referenced command '{command_ref}'; " - f"updated to canonical form '{final_ref}'. " - f"The extension author should update the manifest." - ) + for entry in coerce_hook_entries(hook_data): + if not isinstance(entry, dict): + raise ValidationError( + f"Hook '{hook_name}' must be a mapping or list of mappings, " + f"got {type(entry).__name__}" + ) + command_ref = entry.get("command") + if not isinstance(command_ref, str): + continue + # Step 1: apply any rename from the auto-correction pass. + after_rename = rename_map.get(command_ref, command_ref) + # Step 2: lift alias-form '{ext_id}.cmd' to canonical 'speckit.{ext_id}.cmd'. + parts = after_rename.split(".") + if len(parts) == 2 and parts[0] == ext["id"]: + final_ref = f"speckit.{ext['id']}.{parts[1]}" + else: + final_ref = after_rename + if final_ref != command_ref: + entry["command"] = final_ref + self.warnings.append( + f"Hook '{hook_name}' referenced command '{command_ref}'; " + f"updated to canonical form '{final_ref}'. " + f"The extension author should update the manifest." + ) @staticmethod def _try_correct_command_name(name: str, ext_id: str) -> Optional[str]: @@ -2734,9 +2768,6 @@ class HookExecutor: # Always ensure the extension is in the installed list self.register_extension(manifest.id) - if not hasattr(manifest, "hooks") or not manifest.hooks: - return - config = self.get_project_config() # Ensure config is a dict (defensive) @@ -2762,39 +2793,68 @@ class HookExecutor: config["hooks"][h_name] = sanitized_h_list changed = True + # Purge this extension's entries from events the new manifest no longer + # declares, so dropping an event on reinstall leaves no orphans. + declared_events = set(manifest.hooks.keys()) + for h_name in list(config["hooks"].keys()): + if h_name in declared_events: + continue + kept = [ + h for h in config["hooks"][h_name] + if not (isinstance(h, dict) and h.get("extension") == manifest.id) + ] + if kept != config["hooks"][h_name]: + config["hooks"][h_name] = kept + changed = True + # Register each hook for hook_name, hook_config in manifest.hooks.items(): if hook_name not in config["hooks"] or not isinstance(config["hooks"][hook_name], list): config["hooks"][hook_name] = [] changed = True - # Add hook entry - hook_entry = { - "extension": manifest.id, - "command": hook_config.get("command"), - "enabled": True, - "optional": hook_config.get("optional", True), - "prompt": hook_config.get( - "prompt", f"Execute {hook_config.get('command')}?" - ), - "description": hook_config.get("description", ""), - "condition": hook_config.get("condition"), - } + # Key by command to dedup within the manifest. Deleting before + # re-insert moves a duplicate to the end so "last wins" also breaks ties. + new_entries: Dict[str, Dict[str, Any]] = {} + for entry in coerce_hook_entries(hook_config): + if not isinstance(entry, dict): + continue + command = entry.get("command") + if not command: + continue + if command in new_entries: + del new_entries[command] + new_entries[command] = { + "extension": manifest.id, + "command": command, + "enabled": True, + "optional": entry.get("optional", True), + "priority": normalize_priority( + entry.get("priority"), DEFAULT_HOOK_PRIORITY + ), + "prompt": entry.get("prompt", f"Execute {command}?"), + "description": entry.get("description", ""), + "condition": entry.get("condition"), + } - # Deduplicate: remove all existing entries for this extension on this - # hook event, then append the single canonical entry. This prevents - # multiple hooks firing when hand-edited or older versions leave - # duplicate entries behind. (Feedback from review) + # Purge then re-add all of this extension's entries for the event. + # A reinstall with a changed shape (single<->list or a shorter list) + # then leaves no orphaned entries behind. original_list = config["hooks"][hook_name] deduped = [ h for h in original_list if not (isinstance(h, dict) and h.get("extension") == manifest.id) ] - deduped.append(hook_entry) + deduped.extend(new_entries.values()) if deduped != original_list: config["hooks"][hook_name] = deduped changed = True + non_empty = {name: hooks for name, hooks in config["hooks"].items() if hooks} + if non_empty != config["hooks"]: + config["hooks"] = non_empty + changed = True + if changed: self.save_project_config(config) @@ -2838,19 +2898,26 @@ class HookExecutor: self.save_project_config(config) def get_hooks_for_event(self, event_name: str) -> List[Dict[str, Any]]: - """Get all registered hooks for a specific event. + """Get all enabled hooks for a specific event, sorted by priority ascending. + + Lower ``priority`` runs first. Ties keep insertion order via a stable + sort. Missing or corrupted on-disk priorities fall back to the default. Args: event_name: Name of the event (e.g., 'after_tasks') Returns: - List of hook configurations + List of enabled hook configurations sorted by priority. """ config = self.get_project_config() hooks = config.get("hooks", {}).get(event_name, []) # Filter to enabled hooks only - return [h for h in hooks if h.get("enabled", True)] + enabled = [h for h in hooks if h.get("enabled", True)] + return sorted( + enabled, + key=lambda h: normalize_priority(h.get("priority"), DEFAULT_HOOK_PRIORITY), + ) def should_execute_hook(self, hook: Dict[str, Any]) -> bool: """Determine if a hook should be executed based on its condition. diff --git a/tests/test_extensions.py b/tests/test_extensions.py index e46fdf435..dd231de31 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -23,6 +23,7 @@ from tests.conftest import strip_ansi from specify_cli.extensions import ( CatalogEntry, CORE_COMMAND_NAMES, + DEFAULT_HOOK_PRIORITY, ExtensionManifest, ExtensionRegistry, ExtensionManager, @@ -190,6 +191,12 @@ class TestNormalizePriority: assert normalize_priority(None, default=20) == 20 assert normalize_priority("invalid", default=1) == 1 + def test_boolean_returns_default(self): + """Booleans fall back to the default rather than acting as int 0/1.""" + assert normalize_priority(True) == 10 + assert normalize_priority(False) == 10 + assert normalize_priority(True, default=5) == 5 + # ===== ExtensionManifest Tests ===== @@ -458,6 +465,137 @@ class TestExtensionManifest: with pytest.raises(ValidationError, match="Invalid hook 'after_tasks'"): ExtensionManifest(manifest_path) + def test_hook_single_mapping_still_accepted(self, extension_dir): + """Existing single-mapping hook manifests parse unchanged (regression).""" + manifest_path = extension_dir / "extension.yml" + manifest = ExtensionManifest(manifest_path) + + assert "after_tasks" in manifest.hooks + assert isinstance(manifest.hooks["after_tasks"], dict) + assert manifest.hooks["after_tasks"]["command"] == "speckit.test-ext.hello" + + def test_hook_list_of_mappings_accepted(self, temp_dir, valid_manifest_data): + """A hook event may be configured as a list of mappings.""" + import yaml + + valid_manifest_data["provides"]["commands"].append({ + "name": "speckit.test-ext.bye", + "file": "commands/bye.md", + "description": "Second test command", + }) + valid_manifest_data["hooks"]["after_tasks"] = [ + {"command": "speckit.test-ext.hello", "description": "first"}, + {"command": "speckit.test-ext.bye", "description": "second"}, + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + manifest = ExtensionManifest(manifest_path) + + entries = manifest.hooks["after_tasks"] + assert isinstance(entries, list) + assert [e["command"] for e in entries] == [ + "speckit.test-ext.hello", + "speckit.test-ext.bye", + ] + + def test_hook_list_with_non_mapping_entry_rejected(self, temp_dir, valid_manifest_data): + """A list entry that is not a mapping must raise ValidationError.""" + import yaml + + valid_manifest_data["hooks"]["after_tasks"] = [ + {"command": "speckit.test-ext.hello"}, + "not-a-mapping", + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises( + ValidationError, + match="Invalid hook 'after_tasks': expected a mapping or list of mappings", + ): + ExtensionManifest(manifest_path) + + def test_hook_list_command_refs_normalized(self, temp_dir, valid_manifest_data): + """Alias-form command refs are lifted to canonical form for every entry + in a list hook, each emitting a warning.""" + import yaml + + valid_manifest_data["provides"]["commands"].append({ + "name": "speckit.test-ext.bye", + "file": "commands/bye.md", + "description": "Second test command", + }) + valid_manifest_data["hooks"]["after_tasks"] = [ + {"command": "test-ext.hello"}, + {"command": "test-ext.bye"}, + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + manifest = ExtensionManifest(manifest_path) + + assert [e["command"] for e in manifest.hooks["after_tasks"]] == [ + "speckit.test-ext.hello", + "speckit.test-ext.bye", + ] + lifted = [w for w in manifest.warnings if "updated to canonical form" in w] + assert len(lifted) == 2 + + def test_hook_empty_list_rejected(self, temp_dir, valid_manifest_data): + """An empty list for a hook event is rejected rather than silently + registering nothing.""" + import yaml + + valid_manifest_data["hooks"]["after_tasks"] = [] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="must contain at least one entry"): + ExtensionManifest(manifest_path) + + def test_hook_priority_field_validation(self, temp_dir, valid_manifest_data): + """Hook entry ``priority`` must be a positive integer when provided.""" + import yaml + + manifest_path = temp_dir / "extension.yml" + + valid_manifest_data["hooks"]["after_tasks"] = { + "command": "speckit.test-ext.hello", + "priority": "high", + } + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + with pytest.raises(ValidationError, match="invalid 'priority'.*integer"): + ExtensionManifest(manifest_path) + + valid_manifest_data["hooks"]["after_tasks"]["priority"] = 0 + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + with pytest.raises(ValidationError, match="invalid 'priority'.*>= 1"): + ExtensionManifest(manifest_path) + + # bool is a subclass of int, so it must be rejected explicitly. + valid_manifest_data["hooks"]["after_tasks"]["priority"] = True + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + with pytest.raises(ValidationError, match="invalid 'priority'.*integer"): + ExtensionManifest(manifest_path) + + valid_manifest_data["hooks"]["after_tasks"]["priority"] = 5 + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + manifest = ExtensionManifest(manifest_path) + assert manifest.hooks["after_tasks"]["priority"] == 5 + def test_manifest_hash(self, extension_dir): """Test manifest hash calculation.""" manifest_path = extension_dir / "extension.yml" @@ -4906,6 +5044,405 @@ class TestExtensionPriorityBackwardsCompatibility: assert result[2][0] == "ext-low-priority" +class _StubManifest(ExtensionManifest): + """ExtensionManifest stub for HookExecutor tests. + + Subclasses the real manifest so it satisfies ``register_hooks``'s type + while bypassing the file-based parsing/validation pipeline. The inherited + ``id`` and ``hooks`` properties read from ``data``, so populating ``data`` + is enough. + """ + + def __init__(self, ext_id: str, hooks: dict): + self.data = {"extension": {"id": ext_id}, "hooks": hooks} + + +class TestHookExecutorRegistration: + """Tests for HookExecutor.register_hooks / get_hooks_for_event with + multi-entry hook events and per-entry priority ordering.""" + + def test_register_hooks_single_mapping_back_compat(self, project_dir): + """Single-mapping form continues to register exactly one entry with + default priority.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.go"}}) + ) + + config = executor.get_project_config() + entries = config["hooks"]["after_tasks"] + assert len(entries) == 1 + assert entries[0]["extension"] == "ext-a" + assert entries[0]["command"] == "speckit.ext-a.go" + assert entries[0]["priority"] == DEFAULT_HOOK_PRIORITY + + def test_register_hooks_multiple_entries_same_event(self, project_dir): + """A list of mappings registers each entry under the same event.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-a", + { + "after_tasks": [ + {"command": "speckit.ext-a.first", "description": "1st"}, + {"command": "speckit.ext-a.second", "description": "2nd"}, + ] + }, + ) + ) + + entries = executor.get_project_config()["hooks"]["after_tasks"] + assert len(entries) == 2 + assert [e["command"] for e in entries] == [ + "speckit.ext-a.first", + "speckit.ext-a.second", + ] + assert all(e["extension"] == "ext-a" for e in entries) + + def test_register_hooks_dedup_on_extension_and_command(self, project_dir): + """Re-registering the same (extension, command) updates in place + rather than appending a duplicate entry.""" + executor = HookExecutor(project_dir) + manifest = _StubManifest( + "ext-a", + { + "after_tasks": [ + {"command": "speckit.ext-a.first", "description": "v1"}, + {"command": "speckit.ext-a.second", "description": "v1"}, + ] + }, + ) + executor.register_hooks(manifest) + + manifest.hooks["after_tasks"][0]["description"] = "v2" + executor.register_hooks(manifest) + + entries = executor.get_project_config()["hooks"]["after_tasks"] + assert len(entries) == 2 + first = next(e for e in entries if e["command"] == "speckit.ext-a.first") + assert first["description"] == "v2" + + def test_register_hooks_shape_change_removes_orphans(self, project_dir): + """Reinstalling with a shorter hook shape (list → single mapping, or a + shrunk list) purges the dropped commands instead of leaving orphans.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-a", + { + "after_tasks": [ + {"command": "speckit.ext-a.first"}, + {"command": "speckit.ext-a.second"}, + ] + }, + ) + ) + + executor.register_hooks( + _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.first"}}) + ) + + entries = executor.get_project_config()["hooks"]["after_tasks"] + assert [e["command"] for e in entries] == ["speckit.ext-a.first"] + + def test_register_hooks_single_to_list_reinstall_adds_entries(self, project_dir): + """Reinstalling a single-mapping hook as a list adds the new entries.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.first"}}) + ) + executor.register_hooks( + _StubManifest( + "ext-a", + { + "after_tasks": [ + {"command": "speckit.ext-a.first"}, + {"command": "speckit.ext-a.second"}, + ] + }, + ) + ) + + entries = executor.get_project_config()["hooks"]["after_tasks"] + assert [e["command"] for e in entries] == [ + "speckit.ext-a.first", + "speckit.ext-a.second", + ] + + def test_register_hooks_skips_entry_without_command(self, project_dir): + """An entry lacking a command is skipped (defensive; validated + manifests never reach this state).""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-a", + { + "after_tasks": [ + {"command": "speckit.ext-a.go"}, + {"optional": True}, + ] + }, + ) + ) + + entries = executor.get_project_config()["hooks"]["after_tasks"] + assert [e["command"] for e in entries] == ["speckit.ext-a.go"] + + def test_register_hooks_skips_non_dict_entry(self, project_dir): + """A non-dict entry in a hook list is skipped rather than crashing + (defensive; validated manifests never reach this state).""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-a", + {"after_tasks": [{"command": "speckit.ext-a.go"}, "not-a-mapping"]}, + ) + ) + + entries = executor.get_project_config()["hooks"]["after_tasks"] + assert [e["command"] for e in entries] == ["speckit.ext-a.go"] + + def test_register_hooks_purges_dropped_event_orphans(self, project_dir): + """Re-registering without an event it previously declared purges this + extension's entries from that event, scoped to this extension.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-a", + { + "after_tasks": {"command": "speckit.ext-a.tasks"}, + "after_plan": {"command": "speckit.ext-a.plan"}, + "after_implement": {"command": "speckit.ext-a.impl"}, + }, + ) + ) + executor.register_hooks( + _StubManifest("ext-b", {"after_plan": {"command": "speckit.ext-b.plan"}}) + ) + + executor.register_hooks( + _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.tasks"}}) + ) + + hooks = executor.get_project_config()["hooks"] + assert [e["command"] for e in hooks["after_tasks"]] == ["speckit.ext-a.tasks"] + assert [e["command"] for e in hooks["after_plan"]] == ["speckit.ext-b.plan"] + assert "after_implement" not in hooks + + def test_register_hooks_dropping_all_hooks_purges_orphans(self, project_dir): + """Reinstalling with an empty hooks mapping still purges this + extension's entries, scoped to this extension.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.go"}}) + ) + executor.register_hooks( + _StubManifest("ext-b", {"after_tasks": {"command": "speckit.ext-b.go"}}) + ) + + 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"] + + 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.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.go"}}) + ) + executor.register_hooks( + _StubManifest("ext-b", {"after_tasks": {"command": "speckit.ext-b.go"}}) + ) + config = executor.get_project_config() + config["hooks"]["after_tasks"].append("corrupt-non-dict-entry") + executor.save_project_config(config) + + 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"] + + 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 + insertion position, so equal-priority tie order is 'last wins'.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-a", + { + "after_tasks": [ + {"command": "speckit.ext-a.dup", "description": "first"}, + {"command": "speckit.ext-a.other"}, + {"command": "speckit.ext-a.dup", "description": "last"}, + ] + }, + ) + ) + + entries = executor.get_project_config()["hooks"]["after_tasks"] + assert [e["command"] for e in entries] == [ + "speckit.ext-a.other", + "speckit.ext-a.dup", + ] + assert entries[-1]["description"] == "last" + + def test_register_hooks_preserves_other_extensions(self, project_dir): + """Re-registering one extension must not disturb another extension's + entries on the same event.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.go"}}) + ) + executor.register_hooks( + _StubManifest("ext-b", {"after_tasks": {"command": "speckit.ext-b.go"}}) + ) + + executor.register_hooks( + _StubManifest("ext-a", {"after_tasks": {"command": "speckit.ext-a.go"}}) + ) + + entries = executor.get_project_config()["hooks"]["after_tasks"] + assert sorted(e["extension"] for e in entries) == ["ext-a", "ext-b"] + + def test_get_hooks_for_event_sorts_by_priority(self, project_dir): + """Returned entries are sorted by priority ascending; equal priorities + preserve insertion order via stable sort.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-a", + { + "after_tasks": [ + {"command": "speckit.ext-a.mid", "priority": 10}, + {"command": "speckit.ext-a.first", "priority": 1}, + {"command": "speckit.ext-a.late", "priority": 20}, + {"command": "speckit.ext-a.mid-tied", "priority": 10}, + ] + }, + ) + ) + + ordered = executor.get_hooks_for_event("after_tasks") + assert [e["command"] for e in ordered] == [ + "speckit.ext-a.first", + "speckit.ext-a.mid", + "speckit.ext-a.mid-tied", + "speckit.ext-a.late", + ] + + def test_get_hooks_for_event_orders_across_extensions(self, project_dir): + """Priority controls execution order across extensions regardless of + install order (Issue #2378 use case).""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-report", + {"after_plan": {"command": "speckit.ext-report.run", "priority": 20}}, + ) + ) + executor.register_hooks( + _StubManifest( + "ext-verify", + {"after_plan": {"command": "speckit.ext-verify.run", "priority": 5}}, + ) + ) + + ordered = executor.get_hooks_for_event("after_plan") + assert [e["command"] for e in ordered] == [ + "speckit.ext-verify.run", + "speckit.ext-report.run", + ] + + def test_get_hooks_for_event_treats_missing_priority_as_default(self, project_dir): + """Entries persisted before priority was introduced should be sorted + as if their priority equaled DEFAULT_HOOK_PRIORITY.""" + executor = HookExecutor(project_dir) + # Legacy on-disk entry with no priority key. + # register_hooks now always sets one, so write this state directly. + executor.save_project_config({ + "installed": [], + "settings": {"auto_execute_hooks": True}, + "hooks": { + "after_tasks": [ + { + "extension": "legacy", + "command": "speckit.legacy.go", + "enabled": True, + }, + { + "extension": "newer", + "command": "speckit.newer.first", + "enabled": True, + "priority": 1, + }, + ] + }, + }) + + ordered = executor.get_hooks_for_event("after_tasks") + assert [e["command"] for e in ordered] == [ + "speckit.newer.first", + "speckit.legacy.go", + ] + + def test_get_hooks_for_event_tolerates_corrupted_priority(self, project_dir): + """A corrupted on-disk ``priority`` (non-numeric, None, or < 1) is + normalized to the default instead of raising during sort.""" + executor = HookExecutor(project_dir) + executor.save_project_config({ + "installed": [], + "settings": {"auto_execute_hooks": True}, + "hooks": { + "after_tasks": [ + { + "extension": "corrupt", + "command": "speckit.corrupt.go", + "enabled": True, + "priority": "not-a-number", + }, + { + "extension": "early", + "command": "speckit.early.go", + "enabled": True, + "priority": 1, + }, + ] + }, + }) + + ordered = executor.get_hooks_for_event("after_tasks") + assert [e["command"] for e in ordered] == [ + "speckit.early.go", + "speckit.corrupt.go", + ] + + def test_unregister_hooks_removes_all_extension_entries(self, project_dir): + """unregister_hooks removes every entry for the extension regardless + of how many were registered to a given event.""" + executor = HookExecutor(project_dir) + executor.register_hooks( + _StubManifest( + "ext-a", + { + "after_tasks": [ + {"command": "speckit.ext-a.first"}, + {"command": "speckit.ext-a.second"}, + ] + }, + ) + ) + executor.register_hooks( + _StubManifest("ext-b", {"after_tasks": {"command": "speckit.ext-b.solo"}}) + ) + + executor.unregister_hooks("ext-a") + + entries = executor.get_project_config()["hooks"].get("after_tasks", []) + assert [e["extension"] for e in entries] == ["ext-b"] + + class TestHookInvocationRendering: """Test hook invocation formatting for different agent modes."""