From 96546b134a3b51c1157e11993c8038152908f2f7 Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 26 Aug 2026 22:53:07 -0500 Subject: [PATCH 1/9] feat: expose hook contributions via specify artifact Extends `specify artifact list --json` and `specify artifact info --json` to surface hook contributions as a fourth ArtifactKind alongside command / template / script. Adds a parallel iterator + stack builder for hooks (their name legitimately contains `:`, breaking the existing (kind, name) tuple grammar), preserving strictly additive JSON envelope changes for the existing three kinds. Hook rows carry top-level `eventName`, `targetCommand`, `optional`, `priority`, and `registered` fields. Stack entries use `strategy: replace` and `lookupId` from `derive_hook_id`. The `registered` flag mirrors the runtime by reading `.specify/extensions.yml` bindings via a new `HookExecutor.is_hook_registered` helper. Declared-but-unbound hooks still appear with `registered: false`. The shorthand `hook:{eventName}:{targetCommand}` round-trips through `artifact info`. Includes 30 new tests covering surfacing, sort order, active-winner selection, registered semantics, the layer invariant, and no-regression on existing kinds. Docs at `docs/reference/artifacts.md` extended with a Hook artifacts subsection. Closes #4343 Assisted-by: GitHub Copilot (model: Claude Opus 4.7, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- docs/reference/artifacts.md | 95 +++- src/specify_cli/artifacts/__init__.py | 414 ++++++++++++++++- src/specify_cli/artifacts/_commands.py | 10 +- src/specify_cli/extensions/__init__.py | 37 ++ tests/test_artifact_command.py | 592 +++++++++++++++++++++++++ 5 files changed, 1125 insertions(+), 23 deletions(-) diff --git a/docs/reference/artifacts.md b/docs/reference/artifacts.md index fba74efa81..0cf11932fc 100644 --- a/docs/reference/artifacts.md +++ b/docs/reference/artifacts.md @@ -1,6 +1,6 @@ # Artifacts -An **artifact** is any command, template, or script Spec Kit exposes in a project, regardless of which layer contributes it — built-in assets, an installed preset, an installed extension, or a project-local override in `.specify/templates/overrides/`. +An **artifact** is any command, template, script, or hook Spec Kit exposes in a project, regardless of which layer contributes it — built-in assets, an installed preset, an installed extension, or a project-local override in `.specify/templates/overrides/`. The `specify artifact` command group is the read-only introspection surface for that inventory. `specify preset resolve ` answers "which file wins for this preset-managed name?"; `specify artifact` answers "what exists at all, and what is the full composition stack behind it?" — including built-in artifacts that no preset touches. @@ -16,7 +16,7 @@ specify artifact list --json | -------- | -------------------------------------------------------- | | `--json` | Required. Emit the inventory as a JSON array on stdout. | -Prints the full inventory of every visible artifact — one row per `(kind, name)` pair, including its composition `stack` — sorted by kind (`command`, then `template`, then `script`) and then by name. +Prints the full inventory of every visible artifact — one row per `(kind, name)` pair, including its composition `stack` — sorted by kind (`command`, then `template`, then `script`, then `hook`) and then by name. ```json [ @@ -65,12 +65,14 @@ Prints the full inventory of every visible artifact — one row per `(kind, name | Field | Description | | ------------- | ------------------------------------------------------------------------- | -| `id` | `{kind}:{name}` — the shorthand `artifact info` accepts as its argument | -| `name` | Logical artifact name (commands use the `speckit.` namespace) | -| `kind` | One of `command`, `template`, `script` | +| `id` | `{kind}:{name}` — the shorthand `artifact info` accepts as its argument (for hooks, `hook:{eventName}:{targetCommand}`) | +| `name` | Logical artifact name (commands use the `speckit.` namespace; hooks use `{eventName}:{targetCommand}`) | +| `kind` | One of `command`, `template`, `script`, `hook` | | `description` | Description from the highest-precedence layer that declares one, else `""` | | `stack` | Composition stack for this artifact, using the same row shape as `artifact info` | +Hook rows carry additional top-level scalar fields that mirror the priority-sorted winner of the composition stack — see [Hook artifacts](#hook-artifacts) below. + Built-in artifacts always appear, even when nothing overrides them. Descriptions come from the highest-priority layer that has one — a preset or project override that hides a built-in command reports its own description, not the hidden built-in text. Skills (`.github/skills/**/SKILL.md`) are excluded: they are integration-specific output, not a shipped asset family. ## Artifact Info @@ -82,9 +84,9 @@ specify artifact info --json | Option | Description | | ---------------- | ------------------------------------------------------------------- | | `--json` | Required. Emit the composition stack as a JSON object on stdout. | -| `--kind ` | Narrow the lookup to `command`, `template`, or `script` | +| `--kind ` | Narrow the lookup to `command`, `template`, `script`, or `hook` | -`` accepts either a bare name (`speckit.specify`) or the `kind:name` shorthand (`command:speckit.specify`). When both the shorthand and `--kind` are supplied they must agree. +`` accepts either a bare name (`speckit.specify`) or the `kind:name` shorthand (`command:speckit.specify`). For hooks, the shorthand is `hook:{eventName}:{targetCommand}` — the bare hook name (`{eventName}:{targetCommand}`) is also accepted. When both the shorthand and `--kind` are supplied they must agree. ```json { @@ -144,19 +146,92 @@ The top-level `id`, `name`, `kind`, `description`, and `stack` fields match the Lookup IDs use the same grammar as [preset contribution identifiers](presets.md#contribution-identifiers), so a `lookupId` from this command joins directly to `PresetManifest.iter_contributions()` / `ExtensionManifest.iter_contributions()` for manifest-declared layers. Project-local overrides carry a synthetic `project:_:{kind}:{name}` ID that intentionally matches no manifest contribution. `lookupId` is manifest-backed layer provenance, not the round-trip key — use `id` for that. +## Hook artifacts + +Hook rows extend the shape above with a few fields that only apply to hooks. A hook row's public identifier is `hook:{eventName}:{targetCommand}` — that string is the round-trip key that `artifact info` accepts, and `name` is the same value with the `hook:` prefix stripped. + +```json +{ + "id": "hook:before_specify:speckit.compliance.pre-check", + "name": "before_specify:speckit.compliance.pre-check", + "kind": "hook", + "description": "Compliance pre-check guard", + "eventName": "before_specify", + "targetCommand": "speckit.compliance.pre-check", + "optional": false, + "priority": 5, + "registered": true, + "stack": [ + { + "id": "hook:before_specify:speckit.compliance.pre-check", + "layer": "extension", + "sourceId": "compliance", + "strategy": "replace", + "active": true, + "lookupId": "extension:compliance:hook:before_specify:speckit.compliance.pre-check", + "priority": 5, + "optional": false + } + ] +} +``` + +### Top-level fields + +| Field | Description | +| --------------- | ----------------------------------------------------------------------------------------------- | +| `eventName` | The event whose fires trigger this hook (`before_specify`, `after_plan`, …) | +| `targetCommand` | The command the hook proposes to run when the event fires | +| `optional` | Mirrors the active winner's `optional` scalar — the value the runtime will actually see | +| `priority` | Mirrors the active winner's `priority` scalar | +| `registered` | `true` when a matching `.specify/extensions.yml` binding exists and is not `enabled: false` | + +`optional` and `priority` on the row always agree with the entry marked `active: true` on the stack — they are the values the runtime will actually execute for this `(eventName, targetCommand)` pair. Per-contributor `priority` / `optional` remain visible on every stack entry so callers can audit why one contributor won. + +### Hook stack entries + +Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` (all of which are meaningless for hooks) and add per-contributor `priority` and `optional`. `strategy` is always `"replace"` — the runtime has no composable hook-strategy vocabulary today, so the field is present for shape parity but carries no semantics beyond "this hook overrides earlier hooks in the same slot". + +| Field | Description | +| ----------- | -------------------------------------------------------------------------------------------- | +| `id` | The row-shorthand `hook:{eventName}:{targetCommand}`, identical on every entry | +| `layer` | Always `preset` or `extension` (never `null`, never `project`, never the built-in tier) | +| `sourceId` | The contributing pack's manifest id | +| `strategy` | Always `"replace"` — see note above | +| `active` | `true` only on the priority-sorted winner (index `0`) | +| `lookupId` | The manifest identifier: `{layer}:{sourceId}:hook:{eventName}:{targetCommand}` | +| `priority` | Per-contributor priority (ascending = higher precedence; falls back to the runtime default) | +| `optional` | Per-contributor optional flag | + +### `registered` semantics + +`registered` reflects the project's runtime binding state under `.specify/extensions.yml` and MUST match the runtime's own execution decision. It is `true` when at least one entry in the event's binding array (a) names one of the row's contributing sources via `extension` and (b) is not explicitly `enabled: false`. A binding entry with a matching `extension` but no `command` field counts as a wildcard — same rule the runtime's `enable_hooks` / `disable_hooks` apply. + +A declared hook whose contributors have **no** matching binding entry still appears in the inventory with `registered: false`. This is intentional: `artifact list --json` describes what an extension declares, and `registered` tells you whether the runtime will actually invoke it. A structurally invalid `.specify/extensions.yml` (parse error, wrong top-level type, missing `hooks:` key) is silently normalized to an empty bindings map — every declared hook then reports `registered: false` and no error is raised to callers. + +### Layer invariant + +Hooks only appear on `preset` or `extension` stack entries. There is no built-in ("core") hook tier — the identifier grammar itself refuses to build hook IDs on any other layer, and `derive_hook_id` will raise `IdentifierComponentError` for a `layer` outside `{preset, extension}`. In practice, no built-in preset today emits hooks; extensions are the only source. Even so, the grammar reserves the preset layer for forward compatibility. + +Runtime bindings under `.specify/extensions.yml` that name an extension or command no installed extension has declared do **not** synthesize a phantom row — the inventory is manifest-driven, and orphan bindings only influence the `registered` flag on rows that already exist. + +### Sort order + +Hooks appear after all `command` / `template` / `script` rows in `artifact list --json`. Within the hook block, rows are sorted primarily by `eventName` (alphabetical) and secondarily by the winner's `priority` (ascending). Two rows in the same event at the same priority preserve their original insertion order — matching the runtime's stable-sort tiebreak in `HookExecutor.get_hooks_for_event`. + ## JSON Errors On failure, nothing is written to stdout. A single-key JSON envelope is written to stderr and the process exits with code `1`: ```json -{ "error": "unknown artifact command:nope" } +{ "error": "unknown artifact hook:before_specify:absent.cmd" } ``` | Message | Cause | | --------------------------------------------------- | ---------------------------------------------------------------- | | `not a Spec Kit project: no .specify/ directory found` | Run outside an initialized project | -| `unknown artifact ` | No artifact matches the requested name (and kind, when given) | +| `unknown artifact ` | No artifact matches the requested name (and kind, when given) — same envelope for unknown hooks (`hook:{event}:{command}`) | | `ambiguous artifact : matches kinds [...]` | The bare name matches more than one kind — re-run with `--kind` | | `artifact resolution failed` | The preset/extension registries could not be read, or artifact content could not be composed | -Exit code `2` is reserved for usage errors — a missing `--json` flag or an invalid `--kind` value — and emits a plain-text message on stderr rather than a JSON envelope. +Exit code `2` is reserved for usage errors — a missing `--json` flag or an invalid `--kind` value (accepted: `command`, `template`, `script`, `hook`) — and emits a plain-text message on stderr rather than a JSON envelope. diff --git a/src/specify_cli/artifacts/__init__.py b/src/specify_cli/artifacts/__init__.py index f32bdb854e..8b1ea84869 100644 --- a/src/specify_cli/artifacts/__init__.py +++ b/src/specify_cli/artifacts/__init__.py @@ -22,6 +22,7 @@ from .._identifier import ( PROJECT_OVERRIDE_LAYER, IdentifierComponentError, + derive_hook_id, derive_public_id, is_dotted_command_name, layer_kind_from_lookup_id, @@ -33,10 +34,17 @@ # Public data classes # --------------------------------------------------------------------------- -ArtifactKind = Literal["command", "template", "script"] +ArtifactKind = Literal["command", "template", "script", "hook"] LayerName = Literal["project", "preset", "extension"] Strategy = Literal["replace", "wrap", "prepend", "append"] +# Kinds whose logical name fits the ``(kind, name)`` candidate tuple grammar +# used by ``_iter_pack_candidates``. Hooks use ``{event}:{command}`` as their +# logical name — the embedded ``:`` breaks that grammar — so they flow +# through a separate iterator/stack builder pipeline. See +# ``_iter_hook_contributions`` and ``_build_hook_stack``. +_NAMED_ARTIFACT_KINDS: frozenset[str] = frozenset({"command", "template", "script"}) + @dataclass(frozen=True) class Artifact: @@ -96,6 +104,80 @@ def to_json_dict(self) -> dict[str, Any]: } +@dataclass(frozen=True) +class HookArtifact: + """One row in the flat inventory for a hook contribution. + + A hook row is keyed by the ``(eventName, targetCommand)`` pair. The + top-level ``optional`` and ``priority`` scalars reflect the contributor + marked ``active: true`` on the composition stack — the priority-sorted + winner the runtime will actually execute. ``registered`` reflects the + project's ``.specify/extensions.yml`` binding state, matching the + runtime's own execution decision. + """ + + id: str + name: str + kind: Literal["hook"] + description: str + eventName: str + targetCommand: str + optional: bool + priority: int + registered: bool + + def to_json_dict(self) -> dict[str, Any]: + return { + "id": self.id, + "name": self.name, + "kind": self.kind, + "description": self.description, + "eventName": self.eventName, + "targetCommand": self.targetCommand, + "optional": self.optional, + "priority": self.priority, + "registered": self.registered, + } + + +@dataclass(frozen=True) +class HookStackEntry: + """One entry inside the ``stack`` array on a hook row. + + Hook stack entries mirror the shape of :class:`StackLayer` for the fields + common to every artifact kind (``id``, ``layer``, ``sourceId``, + ``strategy``, ``active``, ``lookupId``) and add ``priority`` and + ``optional`` — the two per-contributor scalars that vary across the stack + and drive the runtime's active-winner selection. ``strategy`` is fixed + to ``"replace"`` because the runtime does not implement a composable hook + strategy vocabulary; the field is present for shape parity with the other + kinds. Hooks are always attributed to a manifest-declared contributor + (``preset`` or ``extension``), so ``layer``, ``sourceId``, and ``lookupId`` + are never ``None``. + """ + + id: str + layer: LayerName + sourceId: str + strategy: Literal["replace"] + active: bool + lookupId: str + priority: int + optional: bool + + def to_json_dict(self) -> dict[str, Any]: + return { + "id": self.id, + "layer": self.layer, + "sourceId": self.sourceId, + "strategy": self.strategy, + "active": self.active, + "lookupId": self.lookupId, + "priority": self.priority, + "optional": self.optional, + } + + # --------------------------------------------------------------------------- # Exceptions — pinned error strings (see artifact-error contract regex) # --------------------------------------------------------------------------- @@ -328,6 +410,138 @@ class ``PresetManager.list_installed()`` and ``specify preset list`` use — return pack_id +def _iter_hook_contributions( + project_root: Path, +) -> Iterable[tuple[int, dict[str, Any]]]: + """Yield ``(insertion_index, contribution)`` pairs for every declared hook. + + Walks every installed extension (and, forward-compatibly, every installed + preset — though :class:`PresetManifest` today does not emit ``kind:"hook"`` + entries) in the same order used by + :meth:`PresetResolver.iter_extensions_by_priority` / + :meth:`PresetResolver.iter_presets_by_priority`, and yields each + ``ExtensionManifest.iter_contributions()`` entry whose ``kind`` is + ``"hook"``. The insertion index is a running counter across the whole + walk; two contributors that share the same explicit ``priority`` on the + same ``(event, command)`` pair are ordered by this index — matching the + runtime's stable-sort tiebreak (see + ``HookExecutor.get_hooks_for_event``). + + Contributions with missing/empty ``eventName`` or ``command`` fields are + silently skipped — the manifest validator has already surfaced those. + """ + from ..extensions import ExtensionManager, ExtensionManifest, ValidationError + from ..presets import PresetManager, PresetResolver # lazy: avoids circular import + + try: + resolver = PresetResolver(project_root) + except OSError: + return + + counter = 0 + + # Presets are walked first for forward compatibility with a future + # ``PresetManifest.iter_contributions()`` that emits hooks. Today none + # do, so this loop yields nothing — but the ordering ensures that if a + # preset ever declares a hook it participates in the same insertion-index + # tiebreak as extensions. + preset_manager = PresetManager(project_root) + for pack_id, _metadata in resolver.iter_presets_by_priority(): + manifest = preset_manager.get_pack(pack_id) + if manifest is None: + continue + for contribution in manifest.iter_contributions(): + if contribution.get("kind") != "hook": + continue + if not contribution.get("eventName") or not contribution.get("command"): + continue + counter += 1 + yield counter, contribution + + ext_manager = ExtensionManager(project_root) + for _priority, ext_id, metadata in resolver.iter_extensions_by_priority(): + ext_dir = resolver.extensions_dir / ext_id + if metadata is not None: + manifest = ext_manager.get_extension(ext_id) + else: + manifest_path = ext_dir / "extension.yml" + manifest = None + if manifest_path.is_file(): + try: + manifest = ExtensionManifest(manifest_path) + except (ValidationError, OSError, TypeError, AttributeError): + manifest = None + if manifest is None: + continue + for contribution in manifest.iter_contributions(): + if contribution.get("kind") != "hook": + continue + if not contribution.get("eventName") or not contribution.get("command"): + continue + counter += 1 + yield counter, contribution + + +def _hook_logical_name(event_name: str, command: str) -> str: + """Return the ``{event}:{command}`` logical name used for hook rows.""" + return f"{event_name}:{command}" + + +def _hook_public_id(event_name: str, command: str) -> str: + """Return the shorthand ``hook:{event}:{command}`` public identifier.""" + return f"hook:{event_name}:{command}" + + +def _build_hook_stack( + grouped: list[tuple[int, dict[str, Any]]], +) -> list[HookStackEntry]: + """Build the composition stack for a single ``(event, command)`` group. + + ``grouped`` is the subset of ``_iter_hook_contributions`` output that + shares one ``(eventName, command)`` pair, in original insertion order. + Contributors are re-sorted by ``(priority, insertion_index)`` — Python's + stable sort combined with the ascending secondary key preserves the same + "priority ascending, ties break by insertion order" behavior the runtime + uses (see ``HookExecutor.get_hooks_for_event``). The first entry after + the sort is marked ``active: true``. + """ + from ..extensions import DEFAULT_HOOK_PRIORITY, normalize_priority + + def _sort_key(item: tuple[int, dict[str, Any]]) -> tuple[int, int]: + idx, contribution = item + priority = normalize_priority( + contribution.get("priority"), DEFAULT_HOOK_PRIORITY + ) + return (priority, idx) + + ordered = sorted(grouped, key=_sort_key) + entries: list[HookStackEntry] = [] + for position, (_idx, contribution) in enumerate(ordered): + layer = contribution.get("layer", "extension") + source_id = contribution.get("sourceId", "") + lookup_id = contribution.get("id", "") + priority = normalize_priority( + contribution.get("priority"), DEFAULT_HOOK_PRIORITY + ) + optional = bool(contribution.get("optional", True)) + entries.append( + HookStackEntry( + id=_hook_public_id( + str(contribution.get("eventName", "")), + str(contribution.get("command", "")), + ), + layer=layer, # type: ignore[arg-type] + sourceId=str(source_id), + strategy="replace", + active=(position == 0), + lookupId=str(lookup_id), + priority=priority, + optional=optional, + ) + ) + return entries + + def _build_stack( project_root: Path, kind: ArtifactKind, @@ -514,19 +728,43 @@ def _resolve_kind_hint(name: str, kind: ArtifactKind | None) -> tuple[str, Artif Returns ``(bare_name, resolved_kind)``. When ``name`` uses the ``kind:name`` grammar and ``kind`` is also set explicitly, the two must agree — a mismatch is treated as an unknown artifact. + + Hook shorthand is ``hook:{eventName}:{command}``; the "bare name" after + the leading ``hook:`` still contains the ``event:command`` colon and is + the logical row name used by :class:`HookArtifact`. That name is + intentionally exempt from :func:`validate_component` — the colon-free + grammar the other three kinds enforce does not apply to hooks (see + :func:`_validate_artifact_name`). """ if ":" in name: prefix, _, bare = name.partition(":") - if prefix in ("command", "template", "script"): + if prefix in _NAMED_ARTIFACT_KINDS: resolved: ArtifactKind = prefix # type: ignore[assignment] if kind is not None and kind != resolved: raise ArtifactNotFoundError(name) return bare, resolved + if prefix == "hook": + if kind is not None and kind != "hook": + raise ArtifactNotFoundError(name) + return bare, "hook" return name, kind def _validate_artifact_name(name: str, kind: ArtifactKind) -> str: - """Validate the structural identifier component constraints for ``name``.""" + """Validate the structural identifier component constraints for ``name``. + + Hook logical names are ``{eventName}:{command}`` — the embedded ``:`` is + intentional and part of the round-trip key, so + :func:`validate_component` is bypassed for hook kind. Empty hook names + still raise :class:`ArtifactNotFoundError`. + """ + if kind == "hook": + if not isinstance(name, str) or not name or ":" not in name: + raise ArtifactNotFoundError(name) + event, _, command = name.partition(":") + if not event or not command: + raise ArtifactNotFoundError(name) + return name try: return validate_component(name, f"{kind} name") except IdentifierComponentError as exc: @@ -572,7 +810,14 @@ def list_artifacts(self) -> list[Artifact]: return artifacts def list_artifacts_with_stack(self) -> list[dict[str, Any]]: - """Return list rows enriched with each artifact's full composition stack.""" + """Return list rows enriched with each artifact's full composition stack. + + Ordered so all command/template/script rows appear first (sorted by + the existing ``kind`` order and then by name), followed by hook rows + sorted primarily by ``eventName`` alphabetical and secondarily by the + winner's ``priority`` — matching the runtime's execution order for + hooks that share an event (see FR-016). + """ artifacts, layers_cache = self._collect_inventory() rows: list[dict[str, Any]] = [] for artifact in artifacts: @@ -585,6 +830,15 @@ def list_artifacts_with_stack(self) -> list[dict[str, Any]]: row = artifact.to_json_dict() row["stack"] = [layer.to_json_dict() for layer in stack] rows.append(row) + + # Validate project + registries once. The hook pipeline reuses the + # same failure envelopes as the command/template/script pipeline. + hook_rows, hook_stack_cache = self._collect_hook_inventory() + for hook in hook_rows: + stack_entries = hook_stack_cache.get((hook.eventName, hook.targetCommand), []) + row = hook.to_json_dict() + row["stack"] = [entry.to_json_dict() for entry in stack_entries] + rows.append(row) return rows # ------------------------------------------------------------------ info @@ -598,7 +852,9 @@ def get_artifact_info( Argument resolution: * ``name`` accepts the ``kind:name`` grammar as shorthand; when both - the shorthand and ``kind`` are supplied they must agree. + the shorthand and ``kind`` are supplied they must agree. Hook + shorthand is ``hook:{eventName}:{command}`` — the embedded + ``event:command`` colon is part of the hook logical name. * When neither the shorthand nor ``kind`` narrows the search and more than one kind matches ``name``, raises :class:`AmbiguousArtifactError`. @@ -606,6 +862,9 @@ def get_artifact_info( """ bare, resolved_kind = _resolve_kind_hint(name, kind) + if resolved_kind == "hook": + return self._get_hook_info(bare, original_argument=name) + # Project and registry validation happens once, inside # ``_collect_inventory`` below — the same chokepoint ``list_artifacts`` # uses — so both public methods fail closed identically instead of @@ -617,11 +876,21 @@ def get_artifact_info( for artifact in inventory if artifact.name == bare ] + # Also probe the hook inventory so a bare name that unambiguously + # matches only a hook logical name (``event:command``) still + # resolves — and so a name that matches BOTH a named kind and a + # hook surfaces the ambiguity envelope. + hook_rows, _hook_stack_cache = self._collect_hook_inventory() + hook_match = any(row.name == bare for row in hook_rows) + if hook_match: + matches.append(("hook", bare)) if not matches: raise ArtifactNotFoundError(name) - if len(matches) > 1: + if len({m[0] for m in matches}) > 1: raise AmbiguousArtifactError(bare, [k for k, _ in matches]) resolved_kind = matches[0][0] + if resolved_kind == "hook": + return self._get_hook_info(bare, original_argument=name) validated_name = _validate_artifact_name(bare, resolved_kind) artifact = next( @@ -651,6 +920,27 @@ def get_artifact_info( "stack": [layer.to_json_dict() for layer in stack], } + def _get_hook_info(self, bare_name: str, original_argument: str) -> dict[str, Any]: + """Return the full JSON-ready dict for a hook artifact info lookup. + + ``bare_name`` is the logical hook name (``{eventName}:{command}``) + after any ``hook:`` prefix has been stripped by + :func:`_resolve_kind_hint`. ``original_argument`` is preserved so the + error message on an unknown hook mirrors what the caller passed in. + """ + _validate_artifact_name(bare_name, "hook") + event_name, _, command = bare_name.partition(":") + hook_rows, stack_cache = self._collect_hook_inventory() + for row in hook_rows: + if row.eventName == event_name and row.targetCommand == command: + stack_entries = stack_cache.get((event_name, command), []) + if not stack_entries: # pragma: no cover — invariant + raise ArtifactNotFoundError(original_argument) + payload = row.to_json_dict() + payload["stack"] = [entry.to_json_dict() for entry in stack_entries] + return payload + raise ArtifactNotFoundError(original_argument) + # -------------------------------------------------------------- internals def _collect_inventory( self, @@ -715,6 +1005,105 @@ def _has_any_replace_layer(layers: list[dict[str, Any]]) -> bool: kind_order = {"command": 0, "template": 1, "script": 2} return sorted(artifacts, key=lambda a: (kind_order[a.kind], a.name)), layers_cache + def _collect_hook_inventory( + self, + ) -> tuple[ + list[HookArtifact], + dict[tuple[str, str], list[HookStackEntry]], + ]: + """Return the hook inventory plus the per-pair composition stacks. + + The list is sorted primarily by ``eventName`` alphabetical and + secondarily by the winner's ``priority`` — matching FR-016. Ties + between winners at the same event and priority preserve first-yield + insertion order via a stable sort. + + The second return value maps each ``(eventName, targetCommand)`` pair + to its full :class:`HookStackEntry` list so callers do not need to + rebuild the stack for a subsequent info lookup. + + Registry validation raises :class:`ArtifactResolutionError` if the + extension registry is corrupt; a structurally-invalid + ``.specify/extensions.yml`` is normalized to an empty bindings map + by :meth:`HookExecutor.get_project_config` and produces + ``registered: false`` for every declared hook without raising. + """ + _validate_project(self.project_root) + _validate_extension_registry(self.project_root) + _validate_preset_registry(self.project_root) + + from ..extensions import DEFAULT_HOOK_PRIORITY, HookExecutor, normalize_priority + + grouped: dict[tuple[str, str], list[tuple[int, dict[str, Any]]]] = {} + for idx, contribution in _iter_hook_contributions(self.project_root): + event_name = str(contribution.get("eventName", "")) + command = str(contribution.get("command", "")) + grouped.setdefault((event_name, command), []).append((idx, contribution)) + + hook_executor = HookExecutor(self.project_root) + + rows: list[HookArtifact] = [] + stack_cache: dict[tuple[str, str], list[HookStackEntry]] = {} + + for (event_name, command), contributions in grouped.items(): + stack_entries = _build_hook_stack(contributions) + stack_cache[(event_name, command)] = stack_entries + if not stack_entries: # pragma: no cover — invariant + continue + + # The description precedence follows the same "highest-priority + # non-empty" rule the other kinds use (FR-015): walk contributors + # in stack order (already priority-sorted) and take the first + # non-empty description. + description = "" + ordered_contributions = [ + contribution + for _idx, contribution in sorted( + contributions, + key=lambda item: ( + normalize_priority( + item[1].get("priority"), DEFAULT_HOOK_PRIORITY + ), + item[0], + ), + ) + ] + for contribution in ordered_contributions: + candidate = contribution.get("description", "") + if isinstance(candidate, str) and candidate: + description = candidate + break + + # Top-level ``optional`` / ``priority`` mirror the active winner + # (FR-017); ``registered`` is true when ANY contributor in the + # stack has a matching, non-disabled binding entry (Q1 answer B). + winner = stack_entries[0] + registered = any( + hook_executor.is_hook_registered( + event_name=event_name, + extension_id=entry.sourceId, + command=command, + ) + for entry in stack_entries + ) + + rows.append( + HookArtifact( + id=_hook_public_id(event_name, command), + name=_hook_logical_name(event_name, command), + kind="hook", + description=description, + eventName=event_name, + targetCommand=command, + optional=winner.optional, + priority=winner.priority, + registered=registered, + ) + ) + + rows.sort(key=lambda row: (row.eventName, row.priority)) + return rows, stack_cache + def _iter_candidate_artifacts( self, resolver: Any, @@ -781,13 +1170,20 @@ def _iter_pack_candidates( manifest: Any, pack_dir: Path, ) -> Iterable[tuple[ArtifactKind, str]]: - """Yield manifest-declared and convention-based candidate names.""" + """Yield manifest-declared and convention-based candidate names. + + Hook contributions are intentionally not yielded here: their logical + name (``{event}:{command}``) contains a ``:`` that would break the + ``(kind, name)`` candidate tuple grammar shared with the resolver. + Hooks are surfaced through the parallel :func:`_iter_hook_contributions` + pipeline instead — see :meth:`ArtifactCatalog._collect_hook_inventory`. + """ if manifest is not None: for contribution in manifest.iter_contributions(): kind = contribution.get("kind") name = contribution.get("name") if ( - kind in ("command", "template", "script") + kind in _NAMED_ARTIFACT_KINDS and isinstance(name, str) and name and ":" not in name @@ -1046,6 +1442,8 @@ def _iter_convention_contributions( "ArtifactKind", "ArtifactNotFoundError", "ArtifactResolutionError", + "HookArtifact", + "HookStackEntry", "LayerName", "NotASpecKitProjectError", "StackLayer", diff --git a/src/specify_cli/artifacts/_commands.py b/src/specify_cli/artifacts/_commands.py index 2c19b6166b..0e90368c07 100644 --- a/src/specify_cli/artifacts/_commands.py +++ b/src/specify_cli/artifacts/_commands.py @@ -35,7 +35,7 @@ artifact_app = typer.Typer( name="artifact", - help="Introspect commands, templates, and scripts SpecKit exposes.", + help="Introspect commands, templates, scripts, and hooks SpecKit exposes.", no_args_is_help=True, ) @@ -100,7 +100,7 @@ def artifact_list( help="Emit the inventory as a JSON array on stdout.", ), ) -> None: - """List every command, template, and script SpecKit exposes.""" + """List every command, template, script, and hook SpecKit exposes.""" _require_json_flag(json_flag) try: root = _resolve_project_root() @@ -128,7 +128,7 @@ def artifact_info( kind: Optional[str] = typer.Option( None, "--kind", - help="Narrow the lookup to one artifact family (command/template/script).", + help="Narrow the lookup to one artifact family (command/template/script/hook).", ), ) -> None: """Show one artifact and its full composition stack.""" @@ -136,9 +136,9 @@ def artifact_info( resolved_kind: Optional[ArtifactKind] = None if kind is not None: - if kind not in ("command", "template", "script"): + if kind not in ("command", "template", "script", "hook"): print( - f"invalid --kind {kind!r}: expected one of command, template, script", + f"invalid --kind {kind!r}: expected one of command, template, script, hook", file=sys.stderr, ) raise typer.Exit(code=2) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index af370b05f7..8ffacca717 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -5247,6 +5247,43 @@ def get_hooks_for_event(self, event_name: str) -> List[Dict[str, Any]]: key=lambda h: normalize_priority(h.get("priority"), DEFAULT_HOOK_PRIORITY), ) + def is_hook_registered( + self, + event_name: str, + extension_id: str, + command: str, + ) -> bool: + """Return whether a declared hook is currently registered to run. + + A hook contribution is "registered" when the project's + ``.specify/extensions.yml`` binding array for ``event_name`` contains + an entry that (a) names the owning contributor via ``extension`` and + (b) is not explicitly disabled (``enabled: false``). The entry's + ``command`` must either match the declared target ``command`` or be + missing/empty — matching the runtime's own execution decision (see + :meth:`enable_hooks` / :meth:`disable_hooks`, which do not + distinguish per-command entries). + + A structurally invalid ``.specify/extensions.yml`` is normalized to + an empty ``hooks`` map by :meth:`get_project_config` — this method + never raises for a malformed registry; it simply returns ``False``. + """ + config = self.get_project_config() + bindings = config.get("hooks", {}).get(event_name, []) + if not isinstance(bindings, list): + return False + for entry in bindings: + if not isinstance(entry, dict): + continue + if entry.get("extension") != extension_id: + continue + if entry.get("enabled", True) is False: + continue + binding_command = entry.get("command") + if not binding_command or binding_command == command: + return True + return False + 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_artifact_command.py b/tests/test_artifact_command.py index 44b5d24ba9..e6d335128f 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -1295,5 +1295,597 @@ def test_falls_back_to_pack_id_without_manifest_file(self, tmp_path: Path): # --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- +# Hook artifact tests — spec #4343 (see specs/001-hook-artifacts/) +# --------------------------------------------------------------------------- + + +def _install_extension_with_hooks( + project_root: Path, + extension_id: str, + hooks: dict, + *, + description: str = "Test extension", + priority: int = 10, + enabled: bool = True, +) -> Path: + """Create a registered extension whose manifest declares the given hook events.""" + ext_dir = project_root / ".specify" / "extensions" / extension_id + ext_dir.mkdir(parents=True, exist_ok=True) + manifest = { + "schema_version": "1.0", + "extension": { + "id": extension_id, + "name": extension_id, + "version": "1.0.0", + "description": description, + "author": "test", + "repository": "https://example.com", + "license": "MIT", + }, + "requires": {"speckit_version": ">=0.2.0"}, + "provides": {}, + "hooks": hooks, + } + (ext_dir / "extension.yml").write_text( + yaml.safe_dump(manifest), encoding="utf-8" + ) + ExtensionRegistry(project_root / ".specify" / "extensions").add( + extension_id, + {"version": "1.0.0", "enabled": enabled, "priority": priority}, + ) + return ext_dir + + +def _write_hook_binding( + project_root: Path, + event_name: str, + entries: list[dict], +) -> Path: + """Write ``.specify/extensions.yml`` binding a hook to the given extensions. + + Each ``entries`` dict must at minimum contain ``extension`` and + ``command`` — the schema :meth:`HookExecutor.register_hooks` writes. + """ + config_path = project_root / ".specify" / "extensions.yml" + payload = { + "installed": [], + "settings": {"auto_execute_hooks": True}, + "hooks": {event_name: entries}, + } + config_path.parent.mkdir(parents=True, exist_ok=True) + config_path.write_text(yaml.safe_dump(payload), encoding="utf-8") + return config_path + + +class TestHookInventorySurfacing: + """US1 — hooks appear in ``list_artifacts_with_stack`` alongside other kinds.""" + + def test_no_hooks_when_no_extensions_installed(self, spec_kit_project: Path): + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + assert all(row.get("kind") != "hook" for row in rows) + + def test_declared_hook_appears_as_row(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={ + "before_specify": [ + { + "command": "speckit.compliance.pre-check", + "description": "Compliance pre-check hook", + } + ] + }, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_rows = [row for row in rows if row["kind"] == "hook"] + assert len(hook_rows) == 1 + row = hook_rows[0] + assert row["id"] == "hook:before_specify:speckit.compliance.pre-check" + assert row["name"] == "before_specify:speckit.compliance.pre-check" + assert row["eventName"] == "before_specify" + assert row["targetCommand"] == "speckit.compliance.pre-check" + assert row["description"] == "Compliance pre-check hook" + + def test_stack_entry_has_hook_shape(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={ + "before_specify": [ + {"command": "speckit.compliance.pre-check", "priority": 5, "optional": False} + ] + }, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_rows = [row for row in rows if row["kind"] == "hook"] + assert len(hook_rows[0]["stack"]) == 1 + entry = hook_rows[0]["stack"][0] + assert entry["layer"] == "extension" + assert entry["sourceId"] == "compliance" + assert entry["strategy"] == "replace" + assert entry["active"] is True + assert entry["priority"] == 5 + assert entry["optional"] is False + assert entry["lookupId"] == ( + "extension:compliance:hook:before_specify:speckit.compliance.pre-check" + ) + + def test_hook_lookupid_matches_derive_hook_id(self, spec_kit_project: Path): + from specify_cli._identifier import derive_hook_id + + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={ + "before_specify": [ + {"command": "speckit.compliance.pre-check"} + ] + }, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + entry = [r for r in rows if r["kind"] == "hook"][0]["stack"][0] + expected = derive_hook_id( + "extension", + "compliance", + "before_specify", + "speckit.compliance.pre-check", + ) + assert entry["lookupId"] == expected + + def test_multiple_extensions_same_event_same_command_produce_one_row( + self, spec_kit_project: Path + ): + _install_extension_with_hooks( + spec_kit_project, + "ext-a", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 10}]}, + ) + _install_extension_with_hooks( + spec_kit_project, + "ext-b", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 5}]}, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_rows = [row for row in rows if row["kind"] == "hook"] + assert len(hook_rows) == 1 + assert len(hook_rows[0]["stack"]) == 2 + + def test_higher_precedence_winner_selected_by_priority( + self, spec_kit_project: Path + ): + _install_extension_with_hooks( + spec_kit_project, + "ext-a", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 10, "optional": True}]}, + ) + _install_extension_with_hooks( + spec_kit_project, + "ext-b", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 3, "optional": False}]}, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_row = [row for row in rows if row["kind"] == "hook"][0] + # Winner is ext-b (priority 3, lower = higher precedence). + assert hook_row["priority"] == 3 + assert hook_row["optional"] is False + assert hook_row["stack"][0]["sourceId"] == "ext-b" + assert hook_row["stack"][0]["active"] is True + assert hook_row["stack"][1]["sourceId"] == "ext-a" + assert hook_row["stack"][1]["active"] is False + + def test_stable_insertion_tiebreak_on_equal_priorities( + self, spec_kit_project: Path + ): + _install_extension_with_hooks( + spec_kit_project, + "ext-a", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 5}]}, + priority=5, + ) + _install_extension_with_hooks( + spec_kit_project, + "ext-b", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 5}]}, + priority=10, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_row = [row for row in rows if row["kind"] == "hook"][0] + # Higher-precedence extension (lower priority in resolver ordering) + # emits first — that becomes the insertion-order winner on tie. + assert hook_row["stack"][0]["sourceId"] == "ext-a" + + +class TestHookInfoShorthand: + """US2 — ``hook:{event}:{command}`` round-trips through artifact info.""" + + def test_shorthand_round_trip(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={ + "before_specify": [ + {"command": "speckit.compliance.pre-check", "description": "Guard"} + ] + }, + ) + payload = ArtifactCatalog(spec_kit_project).get_artifact_info( + "hook:before_specify:speckit.compliance.pre-check" + ) + assert payload["kind"] == "hook" + assert payload["id"] == "hook:before_specify:speckit.compliance.pre-check" + assert payload["eventName"] == "before_specify" + assert payload["targetCommand"] == "speckit.compliance.pre-check" + assert payload["description"] == "Guard" + + def test_unknown_hook_shorthand_raises_not_found(self, spec_kit_project: Path): + catalog = ArtifactCatalog(spec_kit_project) + with pytest.raises(ArtifactNotFoundError) as excinfo: + catalog.get_artifact_info("hook:nope:speckit.absent") + assert "unknown artifact" in excinfo.value.message + + def test_explicit_kind_flag_agrees(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "ext", + hooks={"after_plan": [{"command": "cmd.x"}]}, + ) + payload = ArtifactCatalog(spec_kit_project).get_artifact_info( + "after_plan:cmd.x", kind="hook" + ) + assert payload["eventName"] == "after_plan" + assert payload["targetCommand"] == "cmd.x" + + def test_bare_hook_name_resolves(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "ext", + hooks={"before_specify": [{"command": "unique.hook.only"}]}, + ) + payload = ArtifactCatalog(spec_kit_project).get_artifact_info( + "before_specify:unique.hook.only" + ) + assert payload["kind"] == "hook" + + +class TestHookRegisteredFlag: + """US3 — ``registered`` reflects ``.specify/extensions.yml`` bindings.""" + + def test_declared_but_not_bound_is_false(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={"before_specify": [{"command": "speckit.compliance.pre-check"}]}, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_row = [row for row in rows if row["kind"] == "hook"][0] + assert hook_row["registered"] is False + + def test_bound_and_enabled_is_true(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={"before_specify": [{"command": "speckit.compliance.pre-check"}]}, + ) + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + { + "extension": "compliance", + "command": "speckit.compliance.pre-check", + "enabled": True, + } + ], + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_row = [row for row in rows if row["kind"] == "hook"][0] + assert hook_row["registered"] is True + + def test_bound_but_disabled_is_false(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={"before_specify": [{"command": "speckit.compliance.pre-check"}]}, + ) + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + { + "extension": "compliance", + "command": "speckit.compliance.pre-check", + "enabled": False, + } + ], + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_row = [row for row in rows if row["kind"] == "hook"][0] + assert hook_row["registered"] is False + + def test_binding_without_command_matches(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={"before_specify": [{"command": "speckit.compliance.pre-check"}]}, + ) + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[{"extension": "compliance", "enabled": True}], + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_row = [row for row in rows if row["kind"] == "hook"][0] + assert hook_row["registered"] is True + + def test_orphan_binding_does_not_create_row(self, spec_kit_project: Path): + """A binding naming an uninstalled extension MUST NOT synthesize a row.""" + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[{"extension": "ghost", "command": "ghost.cmd", "enabled": True}], + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_rows = [row for row in rows if row["kind"] == "hook"] + assert hook_rows == [] + + def test_malformed_extensions_yml_degrades_to_registered_false( + self, spec_kit_project: Path + ): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={"before_specify": [{"command": "speckit.compliance.pre-check"}]}, + ) + (spec_kit_project / ".specify" / "extensions.yml").write_text( + "this is not: valid: yaml: [\n", encoding="utf-8" + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_rows = [row for row in rows if row["kind"] == "hook"] + assert hook_rows[0]["registered"] is False + + +class TestHookPerContributorFields: + """US4 — per-contributor priority and optional visible on each stack entry.""" + + def test_per_entry_priority_and_optional(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "ext-a", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 10, "optional": True}]}, + ) + _install_extension_with_hooks( + spec_kit_project, + "ext-b", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 3, "optional": False}]}, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + stack = [row for row in rows if row["kind"] == "hook"][0]["stack"] + by_source = {entry["sourceId"]: entry for entry in stack} + assert by_source["ext-a"]["priority"] == 10 + assert by_source["ext-a"]["optional"] is True + assert by_source["ext-b"]["priority"] == 3 + assert by_source["ext-b"]["optional"] is False + + def test_every_stack_entry_uses_replace_strategy(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "ext-a", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 10}]}, + ) + _install_extension_with_hooks( + spec_kit_project, + "ext-b", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 5}]}, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + stack = [row for row in rows if row["kind"] == "hook"][0]["stack"] + assert all(entry["strategy"] == "replace" for entry in stack) + + +class TestHookLayerInvariants: + """US5 — hooks never appear on a built-in ``core`` layer.""" + + def test_no_hooks_from_core_tier(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "ext", + hooks={"before_specify": [{"command": "cmd.x"}]}, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + for row in rows: + if row["kind"] != "hook": + continue + for entry in row["stack"]: + assert entry["layer"] in ("preset", "extension"), ( + "hook stack entries must never carry a built-in layer" + ) + assert entry["sourceId"], "hook stack entries must have a sourceId" + assert entry["lookupId"], "hook stack entries must have a lookupId" + + +class TestHookSortOrder: + """FR-016 — sort order matches runtime execution ordering.""" + + def test_hooks_sorted_by_event_then_priority(self, spec_kit_project: Path): + _install_extension_with_hooks( + spec_kit_project, + "ext", + hooks={ + "before_specify": [ + {"command": "cmd.z", "priority": 20}, + {"command": "cmd.a", "priority": 5}, + ], + "after_plan": [{"command": "cmd.x", "priority": 15}], + }, + ) + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_rows = [row for row in rows if row["kind"] == "hook"] + # after_plan sorts before before_specify alphabetically. + assert hook_rows[0]["eventName"] == "after_plan" + # Within before_specify, cmd.a (priority 5) sorts before cmd.z (priority 20). + assert hook_rows[1]["targetCommand"] == "cmd.a" + assert hook_rows[2]["targetCommand"] == "cmd.z" + + +class TestHookCliIntegration: + """CLI wrapper — ``specify artifact list --json`` and ``info --json`` for hooks.""" + + def test_list_includes_hook_via_cli(self, spec_kit_project: Path, monkeypatch): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={ + "before_specify": [ + {"command": "speckit.compliance.pre-check", "description": "Guard"} + ] + }, + ) + monkeypatch.chdir(spec_kit_project) + runner = CliRunner() + result = runner.invoke(app, ["artifact", "list", "--json"]) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + hook_rows = [row for row in payload if row.get("kind") == "hook"] + assert len(hook_rows) == 1 + assert hook_rows[0]["id"] == "hook:before_specify:speckit.compliance.pre-check" + + def test_info_shorthand_via_cli(self, spec_kit_project: Path, monkeypatch): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={"before_specify": [{"command": "speckit.compliance.pre-check"}]}, + ) + monkeypatch.chdir(spec_kit_project) + runner = CliRunner() + result = runner.invoke( + app, + [ + "artifact", + "info", + "hook:before_specify:speckit.compliance.pre-check", + "--json", + ], + ) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + assert payload["kind"] == "hook" + assert payload["eventName"] == "before_specify" + assert payload["targetCommand"] == "speckit.compliance.pre-check" + + def test_kind_hook_accepted_by_cli(self, spec_kit_project: Path, monkeypatch): + _install_extension_with_hooks( + spec_kit_project, + "ext", + hooks={"before_specify": [{"command": "cmd.x"}]}, + ) + monkeypatch.chdir(spec_kit_project) + runner = CliRunner() + result = runner.invoke( + app, + [ + "artifact", + "info", + "before_specify:cmd.x", + "--kind", + "hook", + "--json", + ], + ) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + assert payload["kind"] == "hook" + + def test_unknown_hook_via_cli_returns_error_envelope( + self, spec_kit_project: Path, monkeypatch + ): + monkeypatch.chdir(spec_kit_project) + runner = CliRunner() + result = runner.invoke( + app, + ["artifact", "info", "hook:nope:absent.cmd", "--json"], + catch_exceptions=False, + ) + assert result.exit_code == 1 + assert result.stdout == "" + assert ERROR_REGEX.match(json.loads(result.stderr)["error"]) + + +class TestNoRegressionExistingKinds: + """Confirm hook rollout is additive-only — command/template/script rows are unchanged.""" + + def test_existing_kind_row_shape_unchanged(self, spec_kit_project: Path): + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + for row in rows: + if row.get("kind") == "hook": + continue + # Existing kinds must NOT gain hook-only fields. + assert "eventName" not in row + assert "targetCommand" not in row + assert "registered" not in row + # Stack entries for existing kinds must retain their original shape. + for entry in row.get("stack", []): + assert "presetId" in entry + assert "presetName" in entry + assert "hidden" in entry + assert "manifestPath" in entry + + +class TestIsHookRegisteredHelper: + """HookExecutor.is_hook_registered — behavioral truth table.""" + + def test_no_config_returns_false(self, spec_kit_project: Path): + from specify_cli.extensions import HookExecutor + + assert ( + HookExecutor(spec_kit_project).is_hook_registered( + event_name="before_specify", + extension_id="whatever", + command="cmd.x", + ) + is False + ) + + def test_matching_binding_returns_true(self, spec_kit_project: Path): + from specify_cli.extensions import HookExecutor + + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + {"extension": "ext-a", "command": "cmd.x", "enabled": True} + ], + ) + executor = HookExecutor(spec_kit_project) + assert executor.is_hook_registered("before_specify", "ext-a", "cmd.x") is True + + def test_disabled_binding_returns_false(self, spec_kit_project: Path): + from specify_cli.extensions import HookExecutor + + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + {"extension": "ext-a", "command": "cmd.x", "enabled": False} + ], + ) + executor = HookExecutor(spec_kit_project) + assert executor.is_hook_registered("before_specify", "ext-a", "cmd.x") is False + + def test_command_mismatch_returns_false(self, spec_kit_project: Path): + from specify_cli.extensions import HookExecutor + + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + {"extension": "ext-a", "command": "cmd.y", "enabled": True} + ], + ) + executor = HookExecutor(spec_kit_project) + assert executor.is_hook_registered("before_specify", "ext-a", "cmd.x") is False + + def test_module_imports(): assert ArtifactCatalog is not None From 4aa36bfb661e5fc9b0c8b938602ccaadc935bbcd Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 9 Sep 2026 14:08:32 -0500 Subject: [PATCH 2/9] chore: drop unused derive_hook_id import The hook stack builder consumes the manifest-emitted `id` from `EnhancedManifest.iter_contributions()` directly, so the top-level `derive_hook_id` import is unused. Assisted-by: GitHub Copilot (model: Claude Opus 4.7, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- src/specify_cli/artifacts/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/specify_cli/artifacts/__init__.py b/src/specify_cli/artifacts/__init__.py index 8b1ea84869..191dcb5b6b 100644 --- a/src/specify_cli/artifacts/__init__.py +++ b/src/specify_cli/artifacts/__init__.py @@ -22,7 +22,6 @@ from .._identifier import ( PROJECT_OVERRIDE_LAYER, IdentifierComponentError, - derive_hook_id, derive_public_id, is_dotted_command_name, layer_kind_from_lookup_id, From 8aa634acbe03aee5ac5c481ec579d746b3562b76 Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 9 Sep 2026 14:18:30 -0500 Subject: [PATCH 3/9] fix: model hook artifacts as additive execution Reuse HookExecutor.get_hooks_for_event() so hook artifact stacks reflect the runtime's enabled bindings instead of synthesizing a single priority winner. Duplicate declarations from different extensions remain active and execute additively in priority order. Remove row-level priority and optional fields because no single contributor owns those values. Keep them per stack entry, mark each contributor active independently, and report top-level registered when any contributor is active. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- docs/reference/artifacts.md | 38 ++++--- src/specify_cli/artifacts/__init__.py | 92 ++++++++-------- src/specify_cli/extensions/__init__.py | 37 ------- tests/test_artifact_command.py | 140 +++++++++++++------------ 4 files changed, 144 insertions(+), 163 deletions(-) diff --git a/docs/reference/artifacts.md b/docs/reference/artifacts.md index 0cf11932fc..ef909322f4 100644 --- a/docs/reference/artifacts.md +++ b/docs/reference/artifacts.md @@ -71,7 +71,7 @@ Prints the full inventory of every visible artifact — one row per `(kind, name | `description` | Description from the highest-precedence layer that declares one, else `""` | | `stack` | Composition stack for this artifact, using the same row shape as `artifact info` | -Hook rows carry additional top-level scalar fields that mirror the priority-sorted winner of the composition stack — see [Hook artifacts](#hook-artifacts) below. +Hook rows carry additional fields describing their event, command, and runtime registration state — see [Hook artifacts](#hook-artifacts) below. Built-in artifacts always appear, even when nothing overrides them. Descriptions come from the highest-priority layer that has one — a preset or project override that hides a built-in command reports its own description, not the hidden built-in text. Skills (`.github/skills/**/SKILL.md`) are excluded: they are integration-specific output, not a shipped asset family. @@ -158,19 +158,27 @@ Hook rows extend the shape above with a few fields that only apply to hooks. A h "description": "Compliance pre-check guard", "eventName": "before_specify", "targetCommand": "speckit.compliance.pre-check", - "optional": false, - "priority": 5, "registered": true, "stack": [ { "id": "hook:before_specify:speckit.compliance.pre-check", "layer": "extension", - "sourceId": "compliance", - "strategy": "replace", + "sourceId": "compliance-fast", + "strategy": "additive", "active": true, - "lookupId": "extension:compliance:hook:before_specify:speckit.compliance.pre-check", + "lookupId": "extension:compliance-fast:hook:before_specify:speckit.compliance.pre-check", "priority": 5, "optional": false + }, + { + "id": "hook:before_specify:speckit.compliance.pre-check", + "layer": "extension", + "sourceId": "compliance-audit", + "strategy": "additive", + "active": true, + "lookupId": "extension:compliance-audit:hook:before_specify:speckit.compliance.pre-check", + "priority": 10, + "optional": true } ] } @@ -182,30 +190,28 @@ Hook rows extend the shape above with a few fields that only apply to hooks. A h | --------------- | ----------------------------------------------------------------------------------------------- | | `eventName` | The event whose fires trigger this hook (`before_specify`, `after_plan`, …) | | `targetCommand` | The command the hook proposes to run when the event fires | -| `optional` | Mirrors the active winner's `optional` scalar — the value the runtime will actually see | -| `priority` | Mirrors the active winner's `priority` scalar | -| `registered` | `true` when a matching `.specify/extensions.yml` binding exists and is not `enabled: false` | +| `registered` | `true` when at least one matching `.specify/extensions.yml` binding is enabled | -`optional` and `priority` on the row always agree with the entry marked `active: true` on the stack — they are the values the runtime will actually execute for this `(eventName, targetCommand)` pair. Per-contributor `priority` / `optional` remain visible on every stack entry so callers can audit why one contributor won. +There are no row-level `optional` or `priority` fields because hooks do not have a single winner. Those values remain on each stack entry, where they describe that contributor's runtime binding. ### Hook stack entries -Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` (all of which are meaningless for hooks) and add per-contributor `priority` and `optional`. `strategy` is always `"replace"` — the runtime has no composable hook-strategy vocabulary today, so the field is present for shape parity but carries no semantics beyond "this hook overrides earlier hooks in the same slot". +Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` (all of which are meaningless for hooks) and add per-contributor `priority` and `optional`. Hooks execute additively across extensions: priority determines execution order, but it does not suppress a lower-priority declaration. The stack mirrors that behavior by retaining every contributor in runtime order. | Field | Description | | ----------- | -------------------------------------------------------------------------------------------- | | `id` | The row-shorthand `hook:{eventName}:{targetCommand}`, identical on every entry | | `layer` | Always `preset` or `extension` (never `null`, never `project`, never the built-in tier) | | `sourceId` | The contributing pack's manifest id | -| `strategy` | Always `"replace"` — see note above | -| `active` | `true` only on the priority-sorted winner (index `0`) | +| `strategy` | Always `"additive"` because enabled contributors all execute | +| `active` | Whether this contributor has a matching enabled runtime binding; multiple entries may be `true` | | `lookupId` | The manifest identifier: `{layer}:{sourceId}:hook:{eventName}:{targetCommand}` | -| `priority` | Per-contributor priority (ascending = higher precedence; falls back to the runtime default) | +| `priority` | Per-contributor priority (ascending = earlier execution; falls back to the runtime default) | | `optional` | Per-contributor optional flag | ### `registered` semantics -`registered` reflects the project's runtime binding state under `.specify/extensions.yml` and MUST match the runtime's own execution decision. It is `true` when at least one entry in the event's binding array (a) names one of the row's contributing sources via `extension` and (b) is not explicitly `enabled: false`. A binding entry with a matching `extension` but no `command` field counts as a wildcard — same rule the runtime's `enable_hooks` / `disable_hooks` apply. +`registered` reflects the project's runtime binding state under `.specify/extensions.yml` and MUST match the runtime's own execution decision. Each stack entry is independently `active` when an entry in the event's binding array (a) names that contributor via `extension`, (b) matches the command or omits it, and (c) is not explicitly `enabled: false`. Top-level `registered` is `true` when any stack entry is active. This mirrors the runtime: `HookExecutor.get_hooks_for_event` returns every enabled entry, sorted by priority. A declared hook whose contributors have **no** matching binding entry still appears in the inventory with `registered: false`. This is intentional: `artifact list --json` describes what an extension declares, and `registered` tells you whether the runtime will actually invoke it. A structurally invalid `.specify/extensions.yml` (parse error, wrong top-level type, missing `hooks:` key) is silently normalized to an empty bindings map — every declared hook then reports `registered: false` and no error is raised to callers. @@ -217,7 +223,7 @@ Runtime bindings under `.specify/extensions.yml` that name an extension or comma ### Sort order -Hooks appear after all `command` / `template` / `script` rows in `artifact list --json`. Within the hook block, rows are sorted primarily by `eventName` (alphabetical) and secondarily by the winner's `priority` (ascending). Two rows in the same event at the same priority preserve their original insertion order — matching the runtime's stable-sort tiebreak in `HookExecutor.get_hooks_for_event`. +Hooks appear after all `command` / `template` / `script` rows in `artifact list --json`. Within the hook block, rows are sorted primarily by `eventName` (alphabetical) and secondarily by the first stack entry's execution priority (ascending). Stack entries themselves use the runtime order: priority ascending, with original insertion order preserved for ties. ## JSON Errors diff --git a/src/specify_cli/artifacts/__init__.py b/src/specify_cli/artifacts/__init__.py index 191dcb5b6b..0c92a30ed0 100644 --- a/src/specify_cli/artifacts/__init__.py +++ b/src/specify_cli/artifacts/__init__.py @@ -108,11 +108,9 @@ class HookArtifact: """One row in the flat inventory for a hook contribution. A hook row is keyed by the ``(eventName, targetCommand)`` pair. The - top-level ``optional`` and ``priority`` scalars reflect the contributor - marked ``active: true`` on the composition stack — the priority-sorted - winner the runtime will actually execute. ``registered`` reflects the - project's ``.specify/extensions.yml`` binding state, matching the - runtime's own execution decision. + stack preserves every contributor in runtime execution order. + ``registered`` is true when any contributor has an enabled binding in + the project's ``.specify/extensions.yml``. """ id: str @@ -121,8 +119,6 @@ class HookArtifact: description: str eventName: str targetCommand: str - optional: bool - priority: int registered: bool def to_json_dict(self) -> dict[str, Any]: @@ -133,8 +129,6 @@ def to_json_dict(self) -> dict[str, Any]: "description": self.description, "eventName": self.eventName, "targetCommand": self.targetCommand, - "optional": self.optional, - "priority": self.priority, "registered": self.registered, } @@ -147,10 +141,10 @@ class HookStackEntry: common to every artifact kind (``id``, ``layer``, ``sourceId``, ``strategy``, ``active``, ``lookupId``) and add ``priority`` and ``optional`` — the two per-contributor scalars that vary across the stack - and drive the runtime's active-winner selection. ``strategy`` is fixed - to ``"replace"`` because the runtime does not implement a composable hook - strategy vocabulary; the field is present for shape parity with the other - kinds. Hooks are always attributed to a manifest-declared contributor + and determine the runtime's execution order. ``strategy`` is fixed to + ``"additive"`` because enabled hooks from different extensions all run; + priority orders them but does not select a winner. Hooks are always + attributed to a manifest-declared contributor (``preset`` or ``extension``), so ``layer``, ``sourceId``, and ``lookupId`` are never ``None``. """ @@ -158,7 +152,7 @@ class HookStackEntry: id: str layer: LayerName sourceId: str - strategy: Literal["replace"] + strategy: Literal["additive"] active: bool lookupId: str priority: int @@ -493,6 +487,7 @@ def _hook_public_id(event_name: str, command: str) -> str: def _build_hook_stack( grouped: list[tuple[int, dict[str, Any]]], + enabled_bindings: list[dict[str, Any]], ) -> list[HookStackEntry]: """Build the composition stack for a single ``(event, command)`` group. @@ -501,8 +496,9 @@ def _build_hook_stack( Contributors are re-sorted by ``(priority, insertion_index)`` — Python's stable sort combined with the ascending secondary key preserves the same "priority ascending, ties break by insertion order" behavior the runtime - uses (see ``HookExecutor.get_hooks_for_event``). The first entry after - the sort is marked ``active: true``. + uses (see ``HookExecutor.get_hooks_for_event``). Each entry's ``active`` + flag independently reflects whether that contributor's binding is enabled; + multiple entries can therefore be active and execute. """ from ..extensions import DEFAULT_HOOK_PRIORITY, normalize_priority @@ -515,24 +511,31 @@ def _sort_key(item: tuple[int, dict[str, Any]]) -> tuple[int, int]: ordered = sorted(grouped, key=_sort_key) entries: list[HookStackEntry] = [] - for position, (_idx, contribution) in enumerate(ordered): + for _idx, contribution in ordered: layer = contribution.get("layer", "extension") - source_id = contribution.get("sourceId", "") + source_id = str(contribution.get("sourceId", "")) lookup_id = contribution.get("id", "") + event_name = str(contribution.get("eventName", "")) + command = str(contribution.get("command", "")) priority = normalize_priority( contribution.get("priority"), DEFAULT_HOOK_PRIORITY ) optional = bool(contribution.get("optional", True)) + active = any( + binding.get("extension") == source_id + and ( + not binding.get("command") + or binding.get("command") == command + ) + for binding in enabled_bindings + ) entries.append( HookStackEntry( - id=_hook_public_id( - str(contribution.get("eventName", "")), - str(contribution.get("command", "")), - ), + id=_hook_public_id(event_name, command), layer=layer, # type: ignore[arg-type] - sourceId=str(source_id), - strategy="replace", - active=(position == 0), + sourceId=source_id, + strategy="additive", + active=active, lookupId=str(lookup_id), priority=priority, optional=optional, @@ -814,8 +817,7 @@ def list_artifacts_with_stack(self) -> list[dict[str, Any]]: Ordered so all command/template/script rows appear first (sorted by the existing ``kind`` order and then by name), followed by hook rows sorted primarily by ``eventName`` alphabetical and secondarily by the - winner's ``priority`` — matching the runtime's execution order for - hooks that share an event (see FR-016). + first stack entry's execution priority. """ artifacts, layers_cache = self._collect_inventory() rows: list[dict[str, Any]] = [] @@ -1013,8 +1015,8 @@ def _collect_hook_inventory( """Return the hook inventory plus the per-pair composition stacks. The list is sorted primarily by ``eventName`` alphabetical and - secondarily by the winner's ``priority`` — matching FR-016. Ties - between winners at the same event and priority preserve first-yield + secondarily by the first hook's execution priority. Ties + at the same event and priority preserve first-yield insertion order via a stable sort. The second return value maps each ``(eventName, targetCommand)`` pair @@ -1043,9 +1045,15 @@ def _collect_hook_inventory( rows: list[HookArtifact] = [] stack_cache: dict[tuple[str, str], list[HookStackEntry]] = {} + enabled_hooks_by_event: dict[str, list[dict[str, Any]]] = {} for (event_name, command), contributions in grouped.items(): - stack_entries = _build_hook_stack(contributions) + if event_name not in enabled_hooks_by_event: + enabled_hooks_by_event[event_name] = hook_executor.get_hooks_for_event( + event_name + ) + enabled_bindings = enabled_hooks_by_event[event_name] + stack_entries = _build_hook_stack(contributions, enabled_bindings) stack_cache[(event_name, command)] = stack_entries if not stack_entries: # pragma: no cover — invariant continue @@ -1073,18 +1081,9 @@ def _collect_hook_inventory( description = candidate break - # Top-level ``optional`` / ``priority`` mirror the active winner - # (FR-017); ``registered`` is true when ANY contributor in the - # stack has a matching, non-disabled binding entry (Q1 answer B). - winner = stack_entries[0] - registered = any( - hook_executor.is_hook_registered( - event_name=event_name, - extension_id=entry.sourceId, - command=command, - ) - for entry in stack_entries - ) + # Hooks execute additively. Registration is therefore the OR of + # the independently enabled contributors, not a winner property. + registered = any(entry.active for entry in stack_entries) rows.append( HookArtifact( @@ -1094,13 +1093,16 @@ def _collect_hook_inventory( description=description, eventName=event_name, targetCommand=command, - optional=winner.optional, - priority=winner.priority, registered=registered, ) ) - rows.sort(key=lambda row: (row.eventName, row.priority)) + rows.sort( + key=lambda row: ( + row.eventName, + stack_cache[(row.eventName, row.targetCommand)][0].priority, + ) + ) return rows, stack_cache def _iter_candidate_artifacts( diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 8ffacca717..af370b05f7 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -5247,43 +5247,6 @@ def get_hooks_for_event(self, event_name: str) -> List[Dict[str, Any]]: key=lambda h: normalize_priority(h.get("priority"), DEFAULT_HOOK_PRIORITY), ) - def is_hook_registered( - self, - event_name: str, - extension_id: str, - command: str, - ) -> bool: - """Return whether a declared hook is currently registered to run. - - A hook contribution is "registered" when the project's - ``.specify/extensions.yml`` binding array for ``event_name`` contains - an entry that (a) names the owning contributor via ``extension`` and - (b) is not explicitly disabled (``enabled: false``). The entry's - ``command`` must either match the declared target ``command`` or be - missing/empty — matching the runtime's own execution decision (see - :meth:`enable_hooks` / :meth:`disable_hooks`, which do not - distinguish per-command entries). - - A structurally invalid ``.specify/extensions.yml`` is normalized to - an empty ``hooks`` map by :meth:`get_project_config` — this method - never raises for a malformed registry; it simply returns ``False``. - """ - config = self.get_project_config() - bindings = config.get("hooks", {}).get(event_name, []) - if not isinstance(bindings, list): - return False - for entry in bindings: - if not isinstance(entry, dict): - continue - if entry.get("extension") != extension_id: - continue - if entry.get("enabled", True) is False: - continue - binding_command = entry.get("command") - if not binding_command or binding_command == command: - return True - return False - 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_artifact_command.py b/tests/test_artifact_command.py index e6d335128f..7a27ff0987 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -1404,8 +1404,8 @@ def test_stack_entry_has_hook_shape(self, spec_kit_project: Path): entry = hook_rows[0]["stack"][0] assert entry["layer"] == "extension" assert entry["sourceId"] == "compliance" - assert entry["strategy"] == "replace" - assert entry["active"] is True + assert entry["strategy"] == "additive" + assert entry["active"] is False assert entry["priority"] == 5 assert entry["optional"] is False assert entry["lookupId"] == ( @@ -1452,7 +1452,7 @@ def test_multiple_extensions_same_event_same_command_produce_one_row( assert len(hook_rows) == 1 assert len(hook_rows[0]["stack"]) == 2 - def test_higher_precedence_winner_selected_by_priority( + def test_priority_orders_additive_execution( self, spec_kit_project: Path ): _install_extension_with_hooks( @@ -1465,15 +1465,22 @@ def test_higher_precedence_winner_selected_by_priority( "ext-b", hooks={"before_specify": [{"command": "shared.cmd", "priority": 3, "optional": False}]}, ) + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + {"extension": "ext-a", "command": "shared.cmd", "enabled": True}, + {"extension": "ext-b", "command": "shared.cmd", "enabled": True}, + ], + ) rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() hook_row = [row for row in rows if row["kind"] == "hook"][0] - # Winner is ext-b (priority 3, lower = higher precedence). - assert hook_row["priority"] == 3 - assert hook_row["optional"] is False + assert "priority" not in hook_row + assert "optional" not in hook_row assert hook_row["stack"][0]["sourceId"] == "ext-b" assert hook_row["stack"][0]["active"] is True assert hook_row["stack"][1]["sourceId"] == "ext-a" - assert hook_row["stack"][1]["active"] is False + assert hook_row["stack"][1]["active"] is True def test_stable_insertion_tiebreak_on_equal_priorities( self, spec_kit_project: Path @@ -1493,7 +1500,7 @@ def test_stable_insertion_tiebreak_on_equal_priorities( rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() hook_row = [row for row in rows if row["kind"] == "hook"][0] # Higher-precedence extension (lower priority in resolver ordering) - # emits first — that becomes the insertion-order winner on tie. + # emits first, matching the runtime's stable-sort tie behavior. assert hook_row["stack"][0]["sourceId"] == "ext-a" @@ -1645,6 +1652,64 @@ def test_malformed_extensions_yml_degrades_to_registered_false( hook_rows = [row for row in rows if row["kind"] == "hook"] assert hook_rows[0]["registered"] is False + def test_each_contributor_active_flag_matches_its_binding( + self, spec_kit_project: Path + ): + _install_extension_with_hooks( + spec_kit_project, + "ext-a", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 10}]}, + ) + _install_extension_with_hooks( + spec_kit_project, + "ext-b", + hooks={"before_specify": [{"command": "shared.cmd", "priority": 3}]}, + ) + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + {"extension": "ext-a", "command": "shared.cmd", "enabled": False}, + {"extension": "ext-b", "command": "shared.cmd", "enabled": True}, + ], + ) + + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_row = [row for row in rows if row["kind"] == "hook"][0] + by_source = {entry["sourceId"]: entry for entry in hook_row["stack"]} + + assert by_source["ext-a"]["active"] is False + assert by_source["ext-b"]["active"] is True + assert hook_row["registered"] is True + + def test_all_contributors_disabled_means_unregistered( + self, spec_kit_project: Path + ): + _install_extension_with_hooks( + spec_kit_project, + "ext-a", + hooks={"before_specify": [{"command": "shared.cmd"}]}, + ) + _install_extension_with_hooks( + spec_kit_project, + "ext-b", + hooks={"before_specify": [{"command": "shared.cmd"}]}, + ) + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + {"extension": "ext-a", "command": "shared.cmd", "enabled": False}, + {"extension": "ext-b", "command": "shared.cmd", "enabled": False}, + ], + ) + + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_row = [row for row in rows if row["kind"] == "hook"][0] + + assert all(entry["active"] is False for entry in hook_row["stack"]) + assert hook_row["registered"] is False + class TestHookPerContributorFields: """US4 — per-contributor priority and optional visible on each stack entry.""" @@ -1668,7 +1733,7 @@ def test_per_entry_priority_and_optional(self, spec_kit_project: Path): assert by_source["ext-b"]["priority"] == 3 assert by_source["ext-b"]["optional"] is False - def test_every_stack_entry_uses_replace_strategy(self, spec_kit_project: Path): + def test_every_stack_entry_uses_additive_strategy(self, spec_kit_project: Path): _install_extension_with_hooks( spec_kit_project, "ext-a", @@ -1681,7 +1746,7 @@ def test_every_stack_entry_uses_replace_strategy(self, spec_kit_project: Path): ) rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() stack = [row for row in rows if row["kind"] == "hook"][0]["stack"] - assert all(entry["strategy"] == "replace" for entry in stack) + assert all(entry["strategy"] == "additive" for entry in stack) class TestHookLayerInvariants: @@ -1832,60 +1897,5 @@ def test_existing_kind_row_shape_unchanged(self, spec_kit_project: Path): assert "manifestPath" in entry -class TestIsHookRegisteredHelper: - """HookExecutor.is_hook_registered — behavioral truth table.""" - - def test_no_config_returns_false(self, spec_kit_project: Path): - from specify_cli.extensions import HookExecutor - - assert ( - HookExecutor(spec_kit_project).is_hook_registered( - event_name="before_specify", - extension_id="whatever", - command="cmd.x", - ) - is False - ) - - def test_matching_binding_returns_true(self, spec_kit_project: Path): - from specify_cli.extensions import HookExecutor - - _write_hook_binding( - spec_kit_project, - "before_specify", - entries=[ - {"extension": "ext-a", "command": "cmd.x", "enabled": True} - ], - ) - executor = HookExecutor(spec_kit_project) - assert executor.is_hook_registered("before_specify", "ext-a", "cmd.x") is True - - def test_disabled_binding_returns_false(self, spec_kit_project: Path): - from specify_cli.extensions import HookExecutor - - _write_hook_binding( - spec_kit_project, - "before_specify", - entries=[ - {"extension": "ext-a", "command": "cmd.x", "enabled": False} - ], - ) - executor = HookExecutor(spec_kit_project) - assert executor.is_hook_registered("before_specify", "ext-a", "cmd.x") is False - - def test_command_mismatch_returns_false(self, spec_kit_project: Path): - from specify_cli.extensions import HookExecutor - - _write_hook_binding( - spec_kit_project, - "before_specify", - entries=[ - {"extension": "ext-a", "command": "cmd.y", "enabled": True} - ], - ) - executor = HookExecutor(spec_kit_project) - assert executor.is_hook_registered("before_specify", "ext-a", "cmd.x") is False - - def test_module_imports(): assert ArtifactCatalog is not None From 753735d53b0d8111cebb359f4dbb1e162c33b7d3 Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 9 Sep 2026 15:04:12 -0500 Subject: [PATCH 4/9] test: pin hook active registration semantics Define hook stack entries as active exactly when their declaration matches an enabled registration returned by HookExecutor.get_hooks_for_event(). Document that priority winners and event-time condition evaluation do not affect this flag. Add one focused command-matching regression test and extend existing registration tests with active-state assertions without duplicating their setup. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- docs/reference/artifacts.md | 4 ++- src/specify_cli/artifacts/__init__.py | 7 +++-- tests/test_artifact_command.py | 44 ++++++++++++++++++++++++++- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/docs/reference/artifacts.md b/docs/reference/artifacts.md index ef909322f4..8a6638ec64 100644 --- a/docs/reference/artifacts.md +++ b/docs/reference/artifacts.md @@ -204,7 +204,7 @@ Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` ( | `layer` | Always `preset` or `extension` (never `null`, never `project`, never the built-in tier) | | `sourceId` | The contributing pack's manifest id | | `strategy` | Always `"additive"` because enabled contributors all execute | -| `active` | Whether this contributor has a matching enabled runtime binding; multiple entries may be `true` | +| `active` | `true` exactly when this declaration matches an enabled runtime registration returned by `HookExecutor.get_hooks_for_event()`; multiple entries may be `true` | | `lookupId` | The manifest identifier: `{layer}:{sourceId}:hook:{eventName}:{targetCommand}` | | `priority` | Per-contributor priority (ascending = earlier execution; falls back to the runtime default) | | `optional` | Per-contributor optional flag | @@ -213,6 +213,8 @@ Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` ( `registered` reflects the project's runtime binding state under `.specify/extensions.yml` and MUST match the runtime's own execution decision. Each stack entry is independently `active` when an entry in the event's binding array (a) names that contributor via `extension`, (b) matches the command or omits it, and (c) is not explicitly `enabled: false`. Top-level `registered` is `true` when any stack entry is active. This mirrors the runtime: `HookExecutor.get_hooks_for_event` returns every enabled entry, sorted by priority. +`active` describes registration state only. It does not identify a priority winner or evaluate the hook's optional event-time `condition`; condition filtering happens later in `HookExecutor.check_hooks_for_event()`. + A declared hook whose contributors have **no** matching binding entry still appears in the inventory with `registered: false`. This is intentional: `artifact list --json` describes what an extension declares, and `registered` tells you whether the runtime will actually invoke it. A structurally invalid `.specify/extensions.yml` (parse error, wrong top-level type, missing `hooks:` key) is silently normalized to an empty bindings map — every declared hook then reports `registered: false` and no error is raised to callers. ### Layer invariant diff --git a/src/specify_cli/artifacts/__init__.py b/src/specify_cli/artifacts/__init__.py index 0c92a30ed0..f84175571c 100644 --- a/src/specify_cli/artifacts/__init__.py +++ b/src/specify_cli/artifacts/__init__.py @@ -496,9 +496,10 @@ def _build_hook_stack( Contributors are re-sorted by ``(priority, insertion_index)`` — Python's stable sort combined with the ascending secondary key preserves the same "priority ascending, ties break by insertion order" behavior the runtime - uses (see ``HookExecutor.get_hooks_for_event``). Each entry's ``active`` - flag independently reflects whether that contributor's binding is enabled; - multiple entries can therefore be active and execute. + uses (see ``HookExecutor.get_hooks_for_event``). Each entry is ``active`` + exactly when its declaration matches an enabled runtime registration + returned by that method. Multiple entries can therefore be active and + execute. Event-time condition evaluation does not affect this flag. """ from ..extensions import DEFAULT_HOOK_PRIORITY, normalize_priority diff --git a/tests/test_artifact_command.py b/tests/test_artifact_command.py index 7a27ff0987..20f7286ae3 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -1568,8 +1568,11 @@ def test_declared_but_not_bound_is_false(self, spec_kit_project: Path): rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() hook_row = [row for row in rows if row["kind"] == "hook"][0] assert hook_row["registered"] is False + assert hook_row["stack"][0]["active"] is False - def test_bound_and_enabled_is_true(self, spec_kit_project: Path): + def test_enabled_binding_is_active_regardless_of_condition( + self, spec_kit_project: Path + ): _install_extension_with_hooks( spec_kit_project, "compliance", @@ -1583,12 +1586,14 @@ def test_bound_and_enabled_is_true(self, spec_kit_project: Path): "extension": "compliance", "command": "speckit.compliance.pre-check", "enabled": True, + "condition": "env.NEVER_SET is set", } ], ) rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() hook_row = [row for row in rows if row["kind"] == "hook"][0] assert hook_row["registered"] is True + assert hook_row["stack"][0]["active"] is True def test_bound_but_disabled_is_false(self, spec_kit_project: Path): _install_extension_with_hooks( @@ -1610,6 +1615,7 @@ def test_bound_but_disabled_is_false(self, spec_kit_project: Path): rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() hook_row = [row for row in rows if row["kind"] == "hook"][0] assert hook_row["registered"] is False + assert hook_row["stack"][0]["active"] is False def test_binding_without_command_matches(self, spec_kit_project: Path): _install_extension_with_hooks( @@ -1625,6 +1631,42 @@ def test_binding_without_command_matches(self, spec_kit_project: Path): rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() hook_row = [row for row in rows if row["kind"] == "hook"][0] assert hook_row["registered"] is True + assert hook_row["stack"][0]["active"] is True + + def test_binding_matches_only_its_declared_command( + self, spec_kit_project: Path + ): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={ + "before_specify": [ + {"command": "speckit.compliance.pre-check"}, + {"command": "speckit.compliance.audit"}, + ] + }, + ) + _write_hook_binding( + spec_kit_project, + "before_specify", + entries=[ + { + "extension": "compliance", + "command": "speckit.compliance.pre-check", + "enabled": True, + } + ], + ) + + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_rows = { + row["targetCommand"]: row for row in rows if row["kind"] == "hook" + } + + assert ( + hook_rows["speckit.compliance.pre-check"]["stack"][0]["active"] is True + ) + assert hook_rows["speckit.compliance.audit"]["stack"][0]["active"] is False def test_orphan_binding_does_not_create_row(self, spec_kit_project: Path): """A binding naming an uninstalled extension MUST NOT synthesize a row.""" From 678431dee51c7528c82bd14a1456b054c43166a4 Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 9 Sep 2026 15:37:11 -0500 Subject: [PATCH 5/9] docs: clarify hook declaration and runtime state Document that hook priority and optionality come from the contributing manifest while active state comes from HookExecutor.get_hooks_for_event(). Clarify that this split follows Spec Kit's existing hook registration behavior and does not attempt to solve later configuration drift. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- docs/reference/artifacts.md | 12 +++++++----- src/specify_cli/artifacts/__init__.py | 25 ++++++++++++++----------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/docs/reference/artifacts.md b/docs/reference/artifacts.md index 8a6638ec64..69b9eaec42 100644 --- a/docs/reference/artifacts.md +++ b/docs/reference/artifacts.md @@ -192,11 +192,11 @@ Hook rows extend the shape above with a few fields that only apply to hooks. A h | `targetCommand` | The command the hook proposes to run when the event fires | | `registered` | `true` when at least one matching `.specify/extensions.yml` binding is enabled | -There are no row-level `optional` or `priority` fields because hooks do not have a single winner. Those values remain on each stack entry, where they describe that contributor's runtime binding. +There are no row-level `optional` or `priority` fields because hooks do not have a single winner. Those values remain on each stack entry, where they describe that contributor's manifest declaration. ### Hook stack entries -Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` (all of which are meaningless for hooks) and add per-contributor `priority` and `optional`. Hooks execute additively across extensions: priority determines execution order, but it does not suppress a lower-priority declaration. The stack mirrors that behavior by retaining every contributor in runtime order. +Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` (all of which are meaningless for hooks) and add per-contributor `priority` and `optional`. Hooks execute additively across extensions, so priority never suppresses another declaration. The stack retains every contributor in manifest-declared priority order. | Field | Description | | ----------- | -------------------------------------------------------------------------------------------- | @@ -206,8 +206,8 @@ Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` ( | `strategy` | Always `"additive"` because enabled contributors all execute | | `active` | `true` exactly when this declaration matches an enabled runtime registration returned by `HookExecutor.get_hooks_for_event()`; multiple entries may be `true` | | `lookupId` | The manifest identifier: `{layer}:{sourceId}:hook:{eventName}:{targetCommand}` | -| `priority` | Per-contributor priority (ascending = earlier execution; falls back to the runtime default) | -| `optional` | Per-contributor optional flag | +| `priority` | Priority declared by the contributing manifest (ascending values are registered to run earlier by default) | +| `optional` | Optional flag declared by the contributing manifest | ### `registered` semantics @@ -215,6 +215,8 @@ Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` ( `active` describes registration state only. It does not identify a priority winner or evaluate the hook's optional event-time `condition`; condition filtering happens later in `HookExecutor.check_hooks_for_event()`. +This division is consistent with Spec Kit's existing hook model: the installed extension manifest declares the hook and its defaults, while `.specify/extensions.yml` records the project's registered and enabled runtime state. Registration normally copies `priority` and `optional` from the manifest, but this artifact view does not attempt to reconcile later manual drift between those files; that broader registration concern is outside this command. + A declared hook whose contributors have **no** matching binding entry still appears in the inventory with `registered: false`. This is intentional: `artifact list --json` describes what an extension declares, and `registered` tells you whether the runtime will actually invoke it. A structurally invalid `.specify/extensions.yml` (parse error, wrong top-level type, missing `hooks:` key) is silently normalized to an empty bindings map — every declared hook then reports `registered: false` and no error is raised to callers. ### Layer invariant @@ -225,7 +227,7 @@ Runtime bindings under `.specify/extensions.yml` that name an extension or comma ### Sort order -Hooks appear after all `command` / `template` / `script` rows in `artifact list --json`. Within the hook block, rows are sorted primarily by `eventName` (alphabetical) and secondarily by the first stack entry's execution priority (ascending). Stack entries themselves use the runtime order: priority ascending, with original insertion order preserved for ties. +Hooks appear after all `command` / `template` / `script` rows in `artifact list --json`. Within the hook block, rows are sorted primarily by `eventName` (alphabetical) and secondarily by the first stack entry's manifest-declared priority (ascending). Stack entries use declared priority ascending, with original insertion order preserved for ties. ## JSON Errors diff --git a/src/specify_cli/artifacts/__init__.py b/src/specify_cli/artifacts/__init__.py index f84175571c..c3f3501209 100644 --- a/src/specify_cli/artifacts/__init__.py +++ b/src/specify_cli/artifacts/__init__.py @@ -108,9 +108,11 @@ class HookArtifact: """One row in the flat inventory for a hook contribution. A hook row is keyed by the ``(eventName, targetCommand)`` pair. The - stack preserves every contributor in runtime execution order. + stack preserves every contributor in declared priority order. ``registered`` is true when any contributor has an enabled binding in - the project's ``.specify/extensions.yml``. + the project's ``.specify/extensions.yml``. This follows Spec Kit's + existing split: manifests describe hook contributions, while + ``HookExecutor.get_hooks_for_event`` supplies current activation state. """ id: str @@ -493,13 +495,14 @@ def _build_hook_stack( ``grouped`` is the subset of ``_iter_hook_contributions`` output that shares one ``(eventName, command)`` pair, in original insertion order. - Contributors are re-sorted by ``(priority, insertion_index)`` — Python's - stable sort combined with the ascending secondary key preserves the same - "priority ascending, ties break by insertion order" behavior the runtime - uses (see ``HookExecutor.get_hooks_for_event``). Each entry is ``active`` - exactly when its declaration matches an enabled runtime registration - returned by that method. Multiple entries can therefore be active and - execute. Event-time condition evaluation does not affect this flag. + Contributors are sorted by their manifest-declared + ``(priority, insertion_index)``. Registration normally copies those + values into ``.specify/extensions.yml``, so this matches Spec Kit's + standard hook ordering unless project runtime configuration has later + diverged from the declaration. Each entry is ``active`` exactly when its + declaration matches an enabled runtime registration returned by + ``HookExecutor.get_hooks_for_event``. Multiple entries can therefore be + active. Event-time condition evaluation does not affect this flag. """ from ..extensions import DEFAULT_HOOK_PRIORITY, normalize_priority @@ -818,7 +821,7 @@ def list_artifacts_with_stack(self) -> list[dict[str, Any]]: Ordered so all command/template/script rows appear first (sorted by the existing ``kind`` order and then by name), followed by hook rows sorted primarily by ``eventName`` alphabetical and secondarily by the - first stack entry's execution priority. + first stack entry's manifest-declared priority. """ artifacts, layers_cache = self._collect_inventory() rows: list[dict[str, Any]] = [] @@ -1016,7 +1019,7 @@ def _collect_hook_inventory( """Return the hook inventory plus the per-pair composition stacks. The list is sorted primarily by ``eventName`` alphabetical and - secondarily by the first hook's execution priority. Ties + secondarily by the first hook's manifest-declared priority. Ties at the same event and priority preserve first-yield insertion order via a stable sort. From 4cafa5f21400c19ca53ccba6550f25f8b1348fed Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 9 Sep 2026 15:40:45 -0500 Subject: [PATCH 6/9] fix: preserve hook artifact error contract Let hook resolver failures reach the collection boundary and translate OSError and PresetError to the existing ArtifactResolutionError, matching the named-artifact inventory path. This prevents resolver failures from appearing as unknown hooks or escaping the catalog contract. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- src/specify_cli/artifacts/__init__.py | 28 ++++++++++++++++----------- tests/test_artifact_command.py | 26 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/artifacts/__init__.py b/src/specify_cli/artifacts/__init__.py index c3f3501209..98985e6acb 100644 --- a/src/specify_cli/artifacts/__init__.py +++ b/src/specify_cli/artifacts/__init__.py @@ -428,10 +428,7 @@ def _iter_hook_contributions( from ..extensions import ExtensionManager, ExtensionManifest, ValidationError from ..presets import PresetManager, PresetResolver # lazy: avoids circular import - try: - resolver = PresetResolver(project_root) - except OSError: - return + resolver = PresetResolver(project_root) counter = 0 @@ -1038,12 +1035,18 @@ def _collect_hook_inventory( _validate_preset_registry(self.project_root) from ..extensions import DEFAULT_HOOK_PRIORITY, HookExecutor, normalize_priority + from ..presets import PresetError grouped: dict[tuple[str, str], list[tuple[int, dict[str, Any]]]] = {} - for idx, contribution in _iter_hook_contributions(self.project_root): - event_name = str(contribution.get("eventName", "")) - command = str(contribution.get("command", "")) - grouped.setdefault((event_name, command), []).append((idx, contribution)) + try: + for idx, contribution in _iter_hook_contributions(self.project_root): + event_name = str(contribution.get("eventName", "")) + command = str(contribution.get("command", "")) + grouped.setdefault((event_name, command), []).append( + (idx, contribution) + ) + except (OSError, PresetError) as exc: + raise ArtifactResolutionError() from exc hook_executor = HookExecutor(self.project_root) @@ -1053,9 +1056,12 @@ def _collect_hook_inventory( for (event_name, command), contributions in grouped.items(): if event_name not in enabled_hooks_by_event: - enabled_hooks_by_event[event_name] = hook_executor.get_hooks_for_event( - event_name - ) + try: + enabled_hooks_by_event[event_name] = ( + hook_executor.get_hooks_for_event(event_name) + ) + except (OSError, PresetError) as exc: + raise ArtifactResolutionError() from exc enabled_bindings = enabled_hooks_by_event[event_name] stack_entries = _build_hook_stack(contributions, enabled_bindings) stack_cache[(event_name, command)] = stack_entries diff --git a/tests/test_artifact_command.py b/tests/test_artifact_command.py index 20f7286ae3..8e36fae9f4 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -1753,6 +1753,32 @@ def test_all_contributors_disabled_means_unregistered( assert hook_row["registered"] is False +class TestHookResolutionErrors: + @pytest.mark.parametrize("failure_point", ["construction", "iteration"]) + def test_resolver_oserror_uses_artifact_resolution_error( + self, + spec_kit_project: Path, + monkeypatch: pytest.MonkeyPatch, + failure_point: str, + ): + from specify_cli.presets import PresetResolver + + def raise_oserror(*_args, **_kwargs): + raise OSError("simulated resolver failure") + + if failure_point == "construction": + monkeypatch.setattr(PresetResolver, "__init__", raise_oserror) + else: + monkeypatch.setattr( + PresetResolver, "iter_extensions_by_priority", raise_oserror + ) + + with pytest.raises(ArtifactResolutionError): + ArtifactCatalog(spec_kit_project).get_artifact_info( + "hook:before_specify:missing.cmd" + ) + + class TestHookPerContributorFields: """US4 — per-contributor priority and optional visible on each stack entry.""" From 91bfb84af2d9f14993defeb1ae7126a8a8708ed1 Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 9 Sep 2026 15:48:59 -0500 Subject: [PATCH 7/9] fix: preserve common hook stack fields Keep presetId, presetName, hidden, and manifestPath on hook stack entries so hooks follow the existing artifact stack contract. Reuse the shared manifest path and preset display-name helpers, with hidden fixed false for additive hooks. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- docs/reference/artifacts.md | 14 ++++++- src/specify_cli/artifacts/__init__.py | 58 +++++++++++++++++++++------ tests/test_artifact_command.py | 7 ++++ 3 files changed, 66 insertions(+), 13 deletions(-) diff --git a/docs/reference/artifacts.md b/docs/reference/artifacts.md index 69b9eaec42..68efb55711 100644 --- a/docs/reference/artifacts.md +++ b/docs/reference/artifacts.md @@ -164,8 +164,12 @@ Hook rows extend the shape above with a few fields that only apply to hooks. A h "id": "hook:before_specify:speckit.compliance.pre-check", "layer": "extension", "sourceId": "compliance-fast", + "presetId": null, + "presetName": null, "strategy": "additive", "active": true, + "hidden": false, + "manifestPath": ".specify/extensions/compliance-fast/extension.yml", "lookupId": "extension:compliance-fast:hook:before_specify:speckit.compliance.pre-check", "priority": 5, "optional": false @@ -174,8 +178,12 @@ Hook rows extend the shape above with a few fields that only apply to hooks. A h "id": "hook:before_specify:speckit.compliance.pre-check", "layer": "extension", "sourceId": "compliance-audit", + "presetId": null, + "presetName": null, "strategy": "additive", "active": true, + "hidden": false, + "manifestPath": ".specify/extensions/compliance-audit/extension.yml", "lookupId": "extension:compliance-audit:hook:before_specify:speckit.compliance.pre-check", "priority": 10, "optional": true @@ -196,15 +204,19 @@ There are no row-level `optional` or `priority` fields because hooks do not have ### Hook stack entries -Hook stack entries drop `presetId`, `presetName`, `hidden`, and `manifestPath` (all of which are meaningless for hooks) and add per-contributor `priority` and `optional`. Hooks execute additively across extensions, so priority never suppresses another declaration. The stack retains every contributor in manifest-declared priority order. +Hook stack entries retain the common stack fields and add per-contributor `priority` and `optional`. Hooks execute additively across extensions, so priority never suppresses another declaration and `hidden` is always `false`. Extension entries use `null` for `presetId` and `presetName`; a future preset hook would populate them consistently with other preset contributions. `manifestPath` points to the manifest that declares the hook. | Field | Description | | ----------- | -------------------------------------------------------------------------------------------- | | `id` | The row-shorthand `hook:{eventName}:{targetCommand}`, identical on every entry | | `layer` | Always `preset` or `extension` (never `null`, never `project`, never the built-in tier) | | `sourceId` | The contributing pack's manifest id | +| `presetId` | Installed preset id for a preset declaration, otherwise `null` | +| `presetName` | Preset display name for a preset declaration, otherwise `null` | | `strategy` | Always `"additive"` because enabled contributors all execute | | `active` | `true` exactly when this declaration matches an enabled runtime registration returned by `HookExecutor.get_hooks_for_event()`; multiple entries may be `true` | +| `hidden` | Always `false`; additive hook declarations do not hide one another | +| `manifestPath` | Project-relative path to the manifest declaring the hook | | `lookupId` | The manifest identifier: `{layer}:{sourceId}:hook:{eventName}:{targetCommand}` | | `priority` | Priority declared by the contributing manifest (ascending values are registered to run earlier by default) | | `optional` | Optional flag declared by the contributing manifest | diff --git a/src/specify_cli/artifacts/__init__.py b/src/specify_cli/artifacts/__init__.py index 98985e6acb..19e392f2b5 100644 --- a/src/specify_cli/artifacts/__init__.py +++ b/src/specify_cli/artifacts/__init__.py @@ -139,23 +139,25 @@ def to_json_dict(self) -> dict[str, Any]: class HookStackEntry: """One entry inside the ``stack`` array on a hook row. - Hook stack entries mirror the shape of :class:`StackLayer` for the fields - common to every artifact kind (``id``, ``layer``, ``sourceId``, - ``strategy``, ``active``, ``lookupId``) and add ``priority`` and - ``optional`` — the two per-contributor scalars that vary across the stack - and determine the runtime's execution order. ``strategy`` is fixed to + Hook stack entries preserve the common :class:`StackLayer` fields and add + ``priority`` and ``optional`` — the two per-contributor scalars that vary + across hook declarations. ``strategy`` is fixed to ``"additive"`` because enabled hooks from different extensions all run; - priority orders them but does not select a winner. Hooks are always - attributed to a manifest-declared contributor - (``preset`` or ``extension``), so ``layer``, ``sourceId``, and ``lookupId`` - are never ``None``. + priority orders them but does not select a winner. ``hidden`` is therefore + always false. Hooks are always attributed to a manifest-declared + contributor (``preset`` or ``extension``), so ``layer``, ``sourceId``, + ``manifestPath``, and ``lookupId`` are never ``None``. """ id: str layer: LayerName sourceId: str + presetId: str | None + presetName: str | None strategy: Literal["additive"] active: bool + hidden: bool + manifestPath: str lookupId: str priority: int optional: bool @@ -165,8 +167,12 @@ def to_json_dict(self) -> dict[str, Any]: "id": self.id, "layer": self.layer, "sourceId": self.sourceId, + "presetId": self.presetId, + "presetName": self.presetName, "strategy": self.strategy, "active": self.active, + "hidden": self.hidden, + "manifestPath": self.manifestPath, "lookupId": self.lookupId, "priority": self.priority, "optional": self.optional, @@ -448,7 +454,13 @@ def _iter_hook_contributions( if not contribution.get("eventName") or not contribution.get("command"): continue counter += 1 - yield counter, contribution + contribution_with_provenance = dict(contribution) + contribution_with_provenance["lookupId"] = contribution.get("id") + contribution_with_provenance["preset_id"] = pack_id + contribution_with_provenance["pack_dir"] = ( + resolver.presets_dir / pack_id + ) + yield counter, contribution_with_provenance ext_manager = ExtensionManager(project_root) for _priority, ext_id, metadata in resolver.iter_extensions_by_priority(): @@ -471,7 +483,11 @@ def _iter_hook_contributions( if not contribution.get("eventName") or not contribution.get("command"): continue counter += 1 - yield counter, contribution + contribution_with_provenance = dict(contribution) + contribution_with_provenance["lookupId"] = contribution.get("id") + contribution_with_provenance["extension_id"] = ext_id + contribution_with_provenance["extension_dir"] = ext_dir + yield counter, contribution_with_provenance def _hook_logical_name(event_name: str, command: str) -> str: @@ -485,6 +501,7 @@ def _hook_public_id(event_name: str, command: str) -> str: def _build_hook_stack( + project_root: Path, grouped: list[tuple[int, dict[str, Any]]], enabled_bindings: list[dict[str, Any]], ) -> list[HookStackEntry]: @@ -518,6 +535,17 @@ def _sort_key(item: tuple[int, dict[str, Any]]) -> tuple[int, int]: lookup_id = contribution.get("id", "") event_name = str(contribution.get("eventName", "")) command = str(contribution.get("command", "")) + preset_id: str | None = None + preset_name: str | None = None + if layer == "preset": + raw_preset_id = contribution.get("preset_id") + pack_dir = contribution.get("pack_dir") + if isinstance(raw_preset_id, str) and isinstance(pack_dir, Path): + preset_id = raw_preset_id + preset_name = _preset_display_name(pack_dir, preset_id) + manifest_path = _derive_manifest_path(contribution, project_root) + if manifest_path is None: # pragma: no cover — validated manifest invariant + raise ArtifactResolutionError() priority = normalize_priority( contribution.get("priority"), DEFAULT_HOOK_PRIORITY ) @@ -535,8 +563,12 @@ def _sort_key(item: tuple[int, dict[str, Any]]) -> tuple[int, int]: id=_hook_public_id(event_name, command), layer=layer, # type: ignore[arg-type] sourceId=source_id, + presetId=preset_id, + presetName=preset_name, strategy="additive", active=active, + hidden=False, + manifestPath=manifest_path, lookupId=str(lookup_id), priority=priority, optional=optional, @@ -1063,7 +1095,9 @@ def _collect_hook_inventory( except (OSError, PresetError) as exc: raise ArtifactResolutionError() from exc enabled_bindings = enabled_hooks_by_event[event_name] - stack_entries = _build_hook_stack(contributions, enabled_bindings) + stack_entries = _build_hook_stack( + self.project_root, contributions, enabled_bindings + ) stack_cache[(event_name, command)] = stack_entries if not stack_entries: # pragma: no cover — invariant continue diff --git a/tests/test_artifact_command.py b/tests/test_artifact_command.py index 8e36fae9f4..7d4dd1d9fb 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -1404,8 +1404,15 @@ def test_stack_entry_has_hook_shape(self, spec_kit_project: Path): entry = hook_rows[0]["stack"][0] assert entry["layer"] == "extension" assert entry["sourceId"] == "compliance" + assert entry["presetId"] is None + assert entry["presetName"] is None assert entry["strategy"] == "additive" assert entry["active"] is False + assert entry["hidden"] is False + assert ( + entry["manifestPath"] + == ".specify/extensions/compliance/extension.yml" + ) assert entry["priority"] == 5 assert entry["optional"] is False assert entry["lookupId"] == ( From 7ffee23bf5bf89d31ba915cbd86080aa3b3a4421 Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 9 Sep 2026 16:15:13 -0500 Subject: [PATCH 8/9] docs: resolve remaining hook review notes Clarify the separate hook ordering rule, correct the eventName field description, and document that test hook bindings may omit command for the supported wildcard shape. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- docs/reference/artifacts.md | 4 ++-- tests/test_artifact_command.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/reference/artifacts.md b/docs/reference/artifacts.md index 68efb55711..0ffb4eb10c 100644 --- a/docs/reference/artifacts.md +++ b/docs/reference/artifacts.md @@ -16,7 +16,7 @@ specify artifact list --json | -------- | -------------------------------------------------------- | | `--json` | Required. Emit the inventory as a JSON array on stdout. | -Prints the full inventory of every visible artifact — one row per `(kind, name)` pair, including its composition `stack` — sorted by kind (`command`, then `template`, then `script`, then `hook`) and then by name. +Prints the full inventory of every visible artifact — one row per `(kind, name)` pair, including its composition `stack`. Command, template, and script rows are sorted by kind and then by name. Hook rows follow those three kinds and use the hook-specific ordering described in [Sort order](#sort-order). ```json [ @@ -196,7 +196,7 @@ Hook rows extend the shape above with a few fields that only apply to hooks. A h | Field | Description | | --------------- | ----------------------------------------------------------------------------------------------- | -| `eventName` | The event whose fires trigger this hook (`before_specify`, `after_plan`, …) | +| `eventName` | The event whose occurrence triggers this hook (`before_specify`, `after_plan`, …) | | `targetCommand` | The command the hook proposes to run when the event fires | | `registered` | `true` when at least one matching `.specify/extensions.yml` binding is enabled | diff --git a/tests/test_artifact_command.py b/tests/test_artifact_command.py index 7d4dd1d9fb..39a5c3c9a0 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -1344,8 +1344,9 @@ def _write_hook_binding( ) -> Path: """Write ``.specify/extensions.yml`` binding a hook to the given extensions. - Each ``entries`` dict must at minimum contain ``extension`` and - ``command`` — the schema :meth:`HookExecutor.register_hooks` writes. + Each ``entries`` dict must contain ``extension``. ``command`` is normally + present in entries written by :meth:`HookExecutor.register_hooks`, but may + be omitted to exercise the supported extension/event wildcard binding. """ config_path = project_root / ".specify" / "extensions.yml" payload = { From 863f0dc48ab649da95aefe77211a592c26ed75f7 Mon Sep 17 00:00:00 2001 From: Nicole Haugen Date: Wed, 9 Sep 2026 17:20:09 -0500 Subject: [PATCH 9/9] fix: align hook config error semantics Use the existing tolerant hook configuration loader for artifact registration state and document unreadable runtime config as unregistered rather than a resolution failure. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 667230a1-e9fa-4500-a57f-c1c482be2507 --- docs/reference/artifacts.md | 4 ++-- src/specify_cli/artifacts/__init__.py | 16 +++++++--------- tests/test_artifact_command.py | 23 +++++++++++++++++++++++ 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/docs/reference/artifacts.md b/docs/reference/artifacts.md index dc980dec5c..c1d351874a 100644 --- a/docs/reference/artifacts.md +++ b/docs/reference/artifacts.md @@ -234,7 +234,7 @@ Hook stack entries retain the common stack fields and add per-contributor `prior This division is consistent with Spec Kit's existing hook model: the installed extension manifest declares the hook and its defaults, while `.specify/extensions.yml` records the project's registered and enabled runtime state. Registration normally copies `priority` and `optional` from the manifest, but this artifact view does not attempt to reconcile later manual drift between those files; that broader registration concern is outside this command. -A declared hook whose contributors have **no** matching binding entry still appears in the inventory with `registered: false`. This is intentional: `artifact list --json` describes what an extension declares, and `registered` tells you whether the runtime will actually invoke it. A structurally invalid `.specify/extensions.yml` (parse error, wrong top-level type, missing `hooks:` key) is silently normalized to an empty bindings map — every declared hook then reports `registered: false` and no error is raised to callers. +A declared hook whose contributors have **no** matching binding entry still appears in the inventory with `registered: false`. This is intentional: `artifact list --json` describes what an extension declares, and `registered` tells you whether the runtime will actually invoke it. Consistent with Spec Kit's existing hook runtime, an invalid or unreadable `.specify/extensions.yml` is normalized to an empty bindings map — every declared hook then reports `registered: false` and no error is raised to callers. ### Layer invariant @@ -259,6 +259,6 @@ On failure, nothing is written to stdout. A single-key JSON envelope is written | `not a Spec Kit project: no .specify/ directory found` | Run outside an initialized project | | `unknown artifact ` | No artifact matches the requested name (and kind, when given) — same envelope for unknown hooks (`hook:{event}:{command}`) | | `ambiguous artifact : matches kinds [...]` | The bare name matches more than one kind — re-run with `--kind` | -| `artifact resolution failed` | The extension registry could not be read, or an error prevented the artifact layer stack from being collected | +| `artifact resolution failed` | The extension registry could not be read, or an error prevented manifest contributions or artifact layers from being collected. Runtime hook configuration uses the tolerant behavior described above. | Exit code `2` is reserved for usage errors — a missing `--json` flag or an invalid `--kind` value (accepted: `command`, `template`, `script`, `hook`) — and emits a plain-text message on stderr rather than a JSON envelope. diff --git a/src/specify_cli/artifacts/__init__.py b/src/specify_cli/artifacts/__init__.py index 48bb8f49e7..5da0dc5a1e 100644 --- a/src/specify_cli/artifacts/__init__.py +++ b/src/specify_cli/artifacts/__init__.py @@ -1242,9 +1242,10 @@ def _collect_hook_inventory( rebuild the stack for a subsequent info lookup. Registry validation raises :class:`ArtifactResolutionError` if the - extension registry is corrupt; a structurally-invalid - ``.specify/extensions.yml`` is normalized to an empty bindings map - by :meth:`HookExecutor.get_project_config` and produces + extension registry is corrupt. Consistent with the existing hook + runtime, an invalid or unreadable ``.specify/extensions.yml`` is + normalized to an empty bindings map by + :meth:`HookExecutor.get_project_config` and produces ``registered: false`` for every declared hook without raising. """ _validate_project(self.project_root) @@ -1272,12 +1273,9 @@ def _collect_hook_inventory( for (event_name, command), contributions in grouped.items(): if event_name not in enabled_hooks_by_event: - try: - enabled_hooks_by_event[event_name] = ( - hook_executor.get_hooks_for_event(event_name) - ) - except (OSError, PresetError) as exc: - raise ArtifactResolutionError() from exc + enabled_hooks_by_event[event_name] = hook_executor.get_hooks_for_event( + event_name + ) enabled_bindings = enabled_hooks_by_event[event_name] stack_entries = _build_hook_stack( self.project_root, contributions, enabled_bindings diff --git a/tests/test_artifact_command.py b/tests/test_artifact_command.py index d3ee40a275..7d2e2b46ba 100644 --- a/tests/test_artifact_command.py +++ b/tests/test_artifact_command.py @@ -1890,6 +1890,29 @@ def test_malformed_extensions_yml_degrades_to_registered_false( hook_rows = [row for row in rows if row["kind"] == "hook"] assert hook_rows[0]["registered"] is False + def test_unreadable_extensions_yml_degrades_to_registered_false( + self, spec_kit_project: Path, monkeypatch: pytest.MonkeyPatch + ): + _install_extension_with_hooks( + spec_kit_project, + "compliance", + hooks={"before_specify": [{"command": "speckit.compliance.pre-check"}]}, + ) + config_path = spec_kit_project / ".specify" / "extensions.yml" + config_path.write_text("hooks: {}\n", encoding="utf-8") + original_read_text = Path.read_text + + def read_text(path: Path, *args, **kwargs): + if path == config_path: + raise OSError("simulated unreadable runtime config") + return original_read_text(path, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", read_text) + + rows = ArtifactCatalog(spec_kit_project).list_artifacts_with_stack() + hook_rows = [row for row in rows if row["kind"] == "hook"] + assert hook_rows[0]["registered"] is False + def test_each_contributor_active_flag_matches_its_binding( self, spec_kit_project: Path ):