feat(extensions): per-event hook lists with priority ordering#2798
Open
seiya-koji wants to merge 4 commits into
Open
feat(extensions): per-event hook lists with priority ordering#2798seiya-koji wants to merge 4 commits into
seiya-koji wants to merge 4 commits into
Conversation
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 github#2378
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extends the extension hook system to support multiple hook entries per event and introduces per-entry priority ordering for deterministic execution.
Changes:
- Added
DEFAULT_HOOK_PRIORITYand priority-aware sorting for hooks. - Extended manifest parsing/validation to accept either a single hook mapping or a list of hook mappings.
- Updated the extension template and expanded tests to cover list hooks, priority validation, and ordering.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_extensions.py | Adds regression + new unit tests for list-form hooks, priority validation, and priority ordering in HookExecutor. |
| src/specify_cli/extensions.py | Implements list-form hook normalization, validates per-entry priority, persists priority in config, and sorts hooks by priority. |
| extensions/template/extension.yml | Documents list-form hook configuration and the new priority field. |
Comments suppressed due to low confidence (1)
src/specify_cli/extensions.py:93
DEFAULT_HOOK_PRIORITYis introduced as the canonical default (10), butnormalize_priority()still hard-codesdefault: int = 10in its signature. To avoid drift if the default ever changes, consider using the constant as the default parameter (or otherwise centralize the default in one place).
def normalize_priority(value: Any, default: int = 10) -> int:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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.
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.
- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extension manifests allowed only one command per hook event, and hooks fired
in insertion order with no way to control sequencing. Resolves #2378.
Each hook event in
extension.ymlmay now be a single mapping (existing form)or a list of mappings, with an optional integer
priority(>= 1, default 10;lower runs first, stable on ties). Existing single-mapping manifests are
unaffected. This hook-entry
priorityis independent of the extension-levelpriorityused for preset/template resolution.Testing
uv run specify --helpuv sync && uv run pytest18 new unit tests in
tests/test_extensions.pycover list/single validation,priority ordering, dedup, and backward compatibility. End-to-end: installed a
sample extension with a two-command
after_planlist viaspecify extension add— both registered with priorities and resolved in priority order.AI Disclosure
Implemented with AI assistance (Claude Code); reviewed by me before submission.