Skip to content

Commit 4ec4635

Browse files
authored
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.<event> 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.
1 parent 7106858 commit 4ec4635

5 files changed

Lines changed: 718 additions & 58 deletions

File tree

extensions/EXTENSION-API-REFERENCE.md

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,19 @@ provides:
5252
description: string
5353
required: boolean # Default: false
5454

55-
hooks: # Optional, event hooks
55+
hooks: # Optional, event hooks. Each event accepts either form below.
5656
event_name: # e.g., "after_specify", "after_plan", "after_tasks", "after_implement"
5757
command: string # Command to execute
58+
priority: integer # Optional, >= 1, default 10 (lower runs first)
5859
optional: boolean # Default: true
5960
prompt: string # Prompt text for optional hooks
6061
description: string # Hook description
6162
condition: string # Optional, condition expression
63+
another_event: # Any event may instead use a list of mappings (multiple commands)
64+
- command: string # Same fields as the single mapping, per entry
65+
priority: integer
66+
- command: string
67+
priority: integer
6268

6369
tags: # Optional, array of tags (2-10 recommended)
6470
- string
@@ -109,8 +115,10 @@ defaults: # Optional, default configuration values
109115

110116
- **Type**: object
111117
- **Keys**: Event names (e.g., `after_specify`, `after_plan`, `after_tasks`, `after_implement`, `before_analyze`)
118+
- **Value**: A single hook mapping, or a list of hook mappings to register multiple commands on one event
112119
- **Description**: Hooks that execute at lifecycle events
113120
- **Events**: Defined by core spec-kit commands
121+
- **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)
114122

115123
---
116124

@@ -535,7 +543,9 @@ Examples:
535543

536544
### Hook Definition
537545

538-
**In extension.yml**:
546+
Each event accepts either a single hook mapping or a list of mappings. A list registers multiple commands on the same event.
547+
548+
**Single mapping (in extension.yml)**:
539549

540550
```yaml
541551
hooks:
@@ -547,6 +557,24 @@ hooks:
547557
condition: null
548558
```
549559

560+
**List of mappings with priority**:
561+
562+
```yaml
563+
hooks:
564+
after_plan:
565+
- command: "speckit.my-ext.verify"
566+
priority: 5
567+
optional: false
568+
description: "Verify the plan"
569+
- command: "speckit.my-ext.report"
570+
priority: 10
571+
optional: true
572+
prompt: "Generate the report?"
573+
description: "Generate a report from the plan"
574+
```
575+
576+
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.
577+
550578
### Hook Events
551579

552580
Standard events (defined by core):

extensions/EXTENSION-DEVELOPMENT-GUIDE.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,9 +206,12 @@ Available hook points:
206206
- `before_constitution` / `after_constitution`: Before/after constitution update
207207
- `before_taskstoissues` / `after_taskstoissues`: Before/after tasks-to-issues conversion
208208

209+
Each event accepts a single hook object or a list of hook objects (multiple commands on one event).
210+
209211
Hook object:
210212

211213
- `command`: Command to execute (typically from `provides.commands`, but can reference any registered command)
214+
- `priority`: Run order within the event (integer ≥ 1, default 10; lower runs first; equal priorities keep authoring order)
212215
- `optional`: If true, prompt user before executing
213216
- `prompt`: Prompt text for optional hooks
214217
- `description`: Hook description
@@ -655,6 +658,23 @@ hooks:
655658
description: "Analyze tasks after generation"
656659
```
657660

661+
Multiple commands on one event, ordered by `priority` (lower runs first):
662+
663+
```yaml
664+
# extension.yml
665+
hooks:
666+
after_plan:
667+
- command: "speckit.my-ext.verify"
668+
priority: 5
669+
optional: false
670+
description: "Verify the plan"
671+
- command: "speckit.my-ext.report"
672+
priority: 10
673+
optional: true
674+
prompt: "Generate the report?"
675+
description: "Generate a report from the plan"
676+
```
677+
658678
---
659679

660680
## Troubleshooting

extensions/template/extension.yml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,14 @@ hooks:
7979
# optional: false # Auto-execute without prompting
8080
# description: "Runs automatically after implementation"
8181

82+
# MULTIPLE COMMANDS ON ONE EVENT: use a list of entries.
83+
# Add optional `priority` (integer >= 1, default 10) to order them, lowest first.
84+
# after_plan:
85+
# - command: "speckit.my-extension.verify"
86+
# priority: 5
87+
# - command: "speckit.my-extension.report"
88+
# priority: 10
89+
8290
# CUSTOMIZE: Add relevant tags (2-5 recommended)
8391
# Used for discovery in catalog
8492
tags:

src/specify_cli/extensions.py

Lines changed: 123 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@
4141
})
4242
EXTENSION_COMMAND_NAME_PATTERN = re.compile(r"^speckit\.([a-z0-9-]+)\.([a-z0-9-]+)$")
4343

44+
DEFAULT_HOOK_PRIORITY = 10
45+
4446
REINSTALL_COMMAND = "uv tool install specify-cli --force --from git+https://github.com/github/spec-kit.git"
4547

4648

@@ -89,26 +91,37 @@ class CompatibilityError(ExtensionError):
8991
pass
9092

9193

92-
def normalize_priority(value: Any, default: int = 10) -> int:
94+
def normalize_priority(value: Any, default: int = DEFAULT_HOOK_PRIORITY) -> int:
9395
"""Normalize a stored priority value for sorting and display.
9496
95-
Corrupted registry data may contain missing, non-numeric, or non-positive
96-
values. In those cases, fall back to the default priority.
97+
Corrupted registry data may contain missing, non-numeric, non-positive, or
98+
boolean values. In those cases, fall back to the default priority.
9799
98100
Args:
99101
value: Priority value to normalize (may be int, str, None, etc.)
100-
default: Default priority to use for invalid values (default: 10)
102+
default: Default priority to use for invalid values
101103
102104
Returns:
103105
Normalized priority as positive integer (>= 1)
104106
"""
107+
if isinstance(value, bool):
108+
return default
105109
try:
106110
priority = int(value)
107111
except (TypeError, ValueError):
108112
return default
109113
return priority if priority >= 1 else default
110114

111115

116+
def coerce_hook_entries(hook_config: Any) -> List[Any]:
117+
"""Return a hook event's config as a list of entries.
118+
119+
A hook event may be declared as a single mapping or a list of mappings.
120+
Both shapes are normalized to a list so callers can iterate uniformly.
121+
"""
122+
return hook_config if isinstance(hook_config, list) else [hook_config]
123+
124+
112125
@dataclass
113126
class CatalogEntry(BaseCatalogEntry):
114127
"""Represents a single catalog entry in the catalog stack."""
@@ -215,17 +228,36 @@ def _validate(self):
215228
"Extension must provide at least one command or hook"
216229
)
217230

218-
# Validate hook values (if present)
231+
# Validate hook values (if present).
232+
# Each event is a single mapping or a list of mappings.
219233
if hooks:
220234
for hook_name, hook_config in hooks.items():
221-
if not isinstance(hook_config, dict):
235+
if isinstance(hook_config, list) and not hook_config:
222236
raise ValidationError(
223-
f"Invalid hook '{hook_name}': expected a mapping"
224-
)
225-
if not hook_config.get("command"):
226-
raise ValidationError(
227-
f"Hook '{hook_name}' missing required 'command' field"
237+
f"Invalid hook '{hook_name}': list must contain at least one entry"
228238
)
239+
for entry in coerce_hook_entries(hook_config):
240+
if not isinstance(entry, dict):
241+
raise ValidationError(
242+
f"Invalid hook '{hook_name}': "
243+
"expected a mapping or list of mappings"
244+
)
245+
if not entry.get("command"):
246+
raise ValidationError(
247+
f"Hook '{hook_name}' missing required 'command' field"
248+
)
249+
if "priority" in entry:
250+
priority = entry["priority"]
251+
if not isinstance(priority, int) or isinstance(priority, bool):
252+
raise ValidationError(
253+
f"Hook '{hook_name}' has invalid 'priority': "
254+
"must be an integer"
255+
)
256+
if priority < 1:
257+
raise ValidationError(
258+
f"Hook '{hook_name}' has invalid 'priority': "
259+
"must be >= 1"
260+
)
229261

230262
# Validate commands; track renames so hook references can be rewritten.
231263
rename_map: Dict[str, str] = {}
@@ -275,28 +307,30 @@ def _validate(self):
275307
# an alias-form ref (ext.cmd → speckit.ext.cmd). Always emit a warning when
276308
# the reference is changed so extension authors know to update the manifest.
277309
for hook_name, hook_data in self.data.get("hooks", {}).items():
278-
if not isinstance(hook_data, dict):
279-
raise ValidationError(
280-
f"Hook '{hook_name}' must be a mapping, got {type(hook_data).__name__}"
281-
)
282-
command_ref = hook_data.get("command")
283-
if not isinstance(command_ref, str):
284-
continue
285-
# Step 1: apply any rename from the auto-correction pass.
286-
after_rename = rename_map.get(command_ref, command_ref)
287-
# Step 2: lift alias-form '{ext_id}.cmd' to canonical 'speckit.{ext_id}.cmd'.
288-
parts = after_rename.split(".")
289-
if len(parts) == 2 and parts[0] == ext["id"]:
290-
final_ref = f"speckit.{ext['id']}.{parts[1]}"
291-
else:
292-
final_ref = after_rename
293-
if final_ref != command_ref:
294-
hook_data["command"] = final_ref
295-
self.warnings.append(
296-
f"Hook '{hook_name}' referenced command '{command_ref}'; "
297-
f"updated to canonical form '{final_ref}'. "
298-
f"The extension author should update the manifest."
299-
)
310+
for entry in coerce_hook_entries(hook_data):
311+
if not isinstance(entry, dict):
312+
raise ValidationError(
313+
f"Hook '{hook_name}' must be a mapping or list of mappings, "
314+
f"got {type(entry).__name__}"
315+
)
316+
command_ref = entry.get("command")
317+
if not isinstance(command_ref, str):
318+
continue
319+
# Step 1: apply any rename from the auto-correction pass.
320+
after_rename = rename_map.get(command_ref, command_ref)
321+
# Step 2: lift alias-form '{ext_id}.cmd' to canonical 'speckit.{ext_id}.cmd'.
322+
parts = after_rename.split(".")
323+
if len(parts) == 2 and parts[0] == ext["id"]:
324+
final_ref = f"speckit.{ext['id']}.{parts[1]}"
325+
else:
326+
final_ref = after_rename
327+
if final_ref != command_ref:
328+
entry["command"] = final_ref
329+
self.warnings.append(
330+
f"Hook '{hook_name}' referenced command '{command_ref}'; "
331+
f"updated to canonical form '{final_ref}'. "
332+
f"The extension author should update the manifest."
333+
)
300334

301335
@staticmethod
302336
def _try_correct_command_name(name: str, ext_id: str) -> Optional[str]:
@@ -2734,9 +2768,6 @@ def register_hooks(self, manifest: ExtensionManifest):
27342768
# Always ensure the extension is in the installed list
27352769
self.register_extension(manifest.id)
27362770

2737-
if not hasattr(manifest, "hooks") or not manifest.hooks:
2738-
return
2739-
27402771
config = self.get_project_config()
27412772

27422773
# Ensure config is a dict (defensive)
@@ -2762,39 +2793,68 @@ def register_hooks(self, manifest: ExtensionManifest):
27622793
config["hooks"][h_name] = sanitized_h_list
27632794
changed = True
27642795

2796+
# Purge this extension's entries from events the new manifest no longer
2797+
# declares, so dropping an event on reinstall leaves no orphans.
2798+
declared_events = set(manifest.hooks.keys())
2799+
for h_name in list(config["hooks"].keys()):
2800+
if h_name in declared_events:
2801+
continue
2802+
kept = [
2803+
h for h in config["hooks"][h_name]
2804+
if not (isinstance(h, dict) and h.get("extension") == manifest.id)
2805+
]
2806+
if kept != config["hooks"][h_name]:
2807+
config["hooks"][h_name] = kept
2808+
changed = True
2809+
27652810
# Register each hook
27662811
for hook_name, hook_config in manifest.hooks.items():
27672812
if hook_name not in config["hooks"] or not isinstance(config["hooks"][hook_name], list):
27682813
config["hooks"][hook_name] = []
27692814
changed = True
27702815

2771-
# Add hook entry
2772-
hook_entry = {
2773-
"extension": manifest.id,
2774-
"command": hook_config.get("command"),
2775-
"enabled": True,
2776-
"optional": hook_config.get("optional", True),
2777-
"prompt": hook_config.get(
2778-
"prompt", f"Execute {hook_config.get('command')}?"
2779-
),
2780-
"description": hook_config.get("description", ""),
2781-
"condition": hook_config.get("condition"),
2782-
}
2816+
# Key by command to dedup within the manifest. Deleting before
2817+
# re-insert moves a duplicate to the end so "last wins" also breaks ties.
2818+
new_entries: Dict[str, Dict[str, Any]] = {}
2819+
for entry in coerce_hook_entries(hook_config):
2820+
if not isinstance(entry, dict):
2821+
continue
2822+
command = entry.get("command")
2823+
if not command:
2824+
continue
2825+
if command in new_entries:
2826+
del new_entries[command]
2827+
new_entries[command] = {
2828+
"extension": manifest.id,
2829+
"command": command,
2830+
"enabled": True,
2831+
"optional": entry.get("optional", True),
2832+
"priority": normalize_priority(
2833+
entry.get("priority"), DEFAULT_HOOK_PRIORITY
2834+
),
2835+
"prompt": entry.get("prompt", f"Execute {command}?"),
2836+
"description": entry.get("description", ""),
2837+
"condition": entry.get("condition"),
2838+
}
27832839

2784-
# Deduplicate: remove all existing entries for this extension on this
2785-
# hook event, then append the single canonical entry. This prevents
2786-
# multiple hooks firing when hand-edited or older versions leave
2787-
# duplicate entries behind. (Feedback from review)
2840+
# Purge then re-add all of this extension's entries for the event.
2841+
# A reinstall with a changed shape (single<->list or a shorter list)
2842+
# then leaves no orphaned entries behind.
27882843
original_list = config["hooks"][hook_name]
27892844
deduped = [
27902845
h for h in original_list
27912846
if not (isinstance(h, dict) and h.get("extension") == manifest.id)
27922847
]
2793-
deduped.append(hook_entry)
2848+
deduped.extend(new_entries.values())
27942849
if deduped != original_list:
27952850
config["hooks"][hook_name] = deduped
27962851
changed = True
27972852

2853+
non_empty = {name: hooks for name, hooks in config["hooks"].items() if hooks}
2854+
if non_empty != config["hooks"]:
2855+
config["hooks"] = non_empty
2856+
changed = True
2857+
27982858
if changed:
27992859
self.save_project_config(config)
28002860

@@ -2838,19 +2898,26 @@ def unregister_hooks(self, extension_id: str):
28382898
self.save_project_config(config)
28392899

28402900
def get_hooks_for_event(self, event_name: str) -> List[Dict[str, Any]]:
2841-
"""Get all registered hooks for a specific event.
2901+
"""Get all enabled hooks for a specific event, sorted by priority ascending.
2902+
2903+
Lower ``priority`` runs first. Ties keep insertion order via a stable
2904+
sort. Missing or corrupted on-disk priorities fall back to the default.
28422905
28432906
Args:
28442907
event_name: Name of the event (e.g., 'after_tasks')
28452908
28462909
Returns:
2847-
List of hook configurations
2910+
List of enabled hook configurations sorted by priority.
28482911
"""
28492912
config = self.get_project_config()
28502913
hooks = config.get("hooks", {}).get(event_name, [])
28512914

28522915
# Filter to enabled hooks only
2853-
return [h for h in hooks if h.get("enabled", True)]
2916+
enabled = [h for h in hooks if h.get("enabled", True)]
2917+
return sorted(
2918+
enabled,
2919+
key=lambda h: normalize_priority(h.get("priority"), DEFAULT_HOOK_PRIORITY),
2920+
)
28542921

28552922
def should_execute_hook(self, hook: Dict[str, Any]) -> bool:
28562923
"""Determine if a hook should be executed based on its condition.

0 commit comments

Comments
 (0)