feat: expose hook contributions via specify artifact

[nicolehaugen]

Summary

• expose hook contributions through specify artifact list --json and specify artifact info --json
• group declarations by hook:{eventName}:{targetCommand} and preserve deterministic per-contributor lookupId values
• model duplicate declarations as additive execution, matching the existing HookExecutor runtime
• project runtime registration state into registered and per-entry active without hiding declared-but-unregistered hooks
• preserve the common artifact stack provenance and existing registry/resolver error contracts
• document and test hook shorthand, activation, ordering, provenance, error behavior, and existing-kind compatibility

Hook JSON contract

Hook artifacts contain one stack entry per declaring contributor. Each entry uses strategy: "additive"; multiple entries may be active: true because Spec Kit executes every enabled hook returned by HookExecutor.get_hooks_for_event(). Per-entry priority and optional come from the declaring manifest. Top-level registered is true when any stack entry is active.

Runtime registration lookup deliberately follows Spec Kit's existing tolerant hook configuration behavior: invalid or unreadable .specify/extensions.yml content is normalized to an empty registration map, leaving declared hooks visible with registered: false.

Manifest handling also follows the existing artifact inventory: an individual preset or extension manifest that cannot be parsed or validated is omitted and diagnosed through the preset or extension inspection surfaces. Extension-registry and resolver failures that prevent artifact-layer collection retain the existing artifact resolution failed envelope.

This replaces the issue's original winner/replacement proposal after implementation review confirmed that a single active winner would contradict existing Spec Kit hook behavior. The contract in #4343 has been revised accordingly.

Identifier scope

Merged #4305 defines lookupId as artifact-stack provenance derived privately by the artifact package; it explicitly defers a public cross-surface contribution-ID contract to #4210. This PR follows that existing boundary for hooks through the artifact-private derive_hook_lookup_id() helper. Hook event and command components are UTF-8 percent-encoded so existing valid values containing identifier delimiters remain round-trippable.

This PR does not add contribution IDs to preset or extension manifest/info APIs and therefore does not claim parity with an independent manifest contribution surface. When #4210 introduces that shared surface, specify artifact should consume its shared hook identifier rather than reconstructing one privately. Exact equality between hook lookupId values and public preset/extension contribution IDs is deferred to #4210.

Dependency

Builds on the artifact introspection surface merged in #4305.

Closes #4343

---

Updated on behalf of @nicolehaugen by GitHub Copilot (model: GPT-5.6 Sol, autonomous).

[Copilot]

Pull request overview

Adds hook contributions to the artifact introspection API alongside commands, templates, and scripts.

Changes:

• Adds hook inventory, stack, lookup, ordering, and registration metadata.
• Adds runtime registration helper and extensive tests.
• Documents hook artifact fields and semantics.

Show a summary per file

File	Description
src/specify_cli/artifacts/__init__.py	Implements hook artifact collection and serialization.
src/specify_cli/artifacts/_commands.py	Accepts hooks in CLI help and kind validation.
src/specify_cli/extensions/__init__.py	Adds hook registration lookup.
tests/test_artifact_command.py	Covers hook inventory and CLI behavior.
docs/reference/artifacts.md	Documents hook artifacts.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (1)

docs/reference/artifacts.md:183

• Correct the grammatical error in this field description.

| `eventName`     | The event whose fires trigger this hook (`before_specify`, `after_plan`, …)                     |

• Files reviewed: 5/5 changed files
• Comments generated: 7
• Review effort level: Balanced

[mnriem]

Please address Copilot feedback and fix test & lint errrors

[Copilot]

Copilot review overview

🔵 Needs a closer look

Hook activity and registration metadata can disagree with runtime behavior, and required stack provenance is missing.

Review tier: Balanced
Findings: 5 · 1

Pre-existing issues (6)

Severity	Finding
	src/​specify_cli/​artifacts/​__init__.py — is_hook_registered() reloads and parses .specify/extensions.yml on every call, and this loop… View comment
	src/​specify_cli/​artifacts/​__init__.py — The issue's proposed hook JSON keeps the common stack fields (presetId, presetName, hidden,… View comment
	src/​specify_cli/​artifacts/​__init__.py — Hook collection does not preserve the artifact command's error contract. An OSError while… View comment
	src/​specify_cli/​extensions/​__init__.py — This differs from the runtime filter in get_hooks_for_event, which treats any falsy enabled… View comment
	src/​specify_cli/​artifacts/​__init__.py — Duplicate declarations are not winners at runtime: HookExecutor.register_hooks preserves entries… View comment
	docs/​reference/​artifacts.md — This says every kind is sorted by name, but the new hook implementation sorts hooks by event and… View comment

Issues resolved since last review (1)

Severity	Finding
	src/​specify_cli/​artifacts/​__init__.py — This import is never referenced in the module. The repository runs Ruff over src, so this… View resolved comment

Suppressed comments (7)

Previously missed (2) — in code that hasn't changed since the last review.

docs/reference/artifacts.md:183

• Correct the grammar in this field description.
  tests/test_artifact_command.py:1349
• This contract excludes the wildcard binding shape used below by test_binding_without_command_matches. State that command is optional so the helper documentation matches the supported input.

src/specify_cli/extensions/init.py:5280

• This check disagrees with the runtime filter in get_hooks_for_event(), which excludes every falsey enabled value. For example, enabled: null is not executed at runtime but is reported as registered: true here. Use the same truthiness rule so registered reflects execution state.

            if entry.get("enabled", True) is False:

src/specify_cli/artifacts/init.py:536

• The runtime does not select a single winner for duplicate (event, command) declarations: register_hooks() retains one binding per extension, and get_hooks_for_event() returns all enabled bindings in priority order. Marking only the first contributor active (and projecting only its optional/priority) therefore reports later hooks as inactive even though check_hooks_for_event() will expose them for execution. Either add matching runtime deduplication or model every executable contributor without a single-winner claim.

                layer=layer,  # type: ignore[arg-type]
                sourceId=str(source_id),
                strategy="replace",
                active=(position == 0),
                lookupId=str(lookup_id),

src/specify_cli/artifacts/init.py:1086

• is_hook_registered() calls get_project_config() internally, so this comprehension rereads and reparses the entire YAML file once per stack entry. Inventory generation becomes an N+1 I/O path and can approach quadratic work as hooks/bindings grow. Load the normalized config once and match all entries against that snapshot (or let the helper accept it).

            registered = any(
                hook_executor.is_hook_registered(
                    event_name=event_name,
                    extension_id=entry.sourceId,
                    command=command,
                )
                for entry in stack_entries

src/specify_cli/artifacts/init.py:165

• Issue #4343's proposed hook stack retains the common presetId, presetName, hidden, and manifestPath fields, but this new type drops all four. In particular, manifestPath is meaningful provenance for a manifest-declared hook, and omitting the preset fields makes the reserved preset layer unable to identify its installed pack. Preserve the established stack shape and use null only where a field is genuinely inapplicable.

    id: str
    layer: LayerName
    sourceId: str
    strategy: Literal["replace"]
    active: bool
    lookupId: str
    priority: int
    optional: bool

docs/reference/artifacts.md:19

• This says all kinds are sorted by name, but hook rows are actually sorted by event and winner priority, as the new Sort order section later explains. Document the separate hook ordering here to avoid a contradictory contract.

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.

[Copilot]

Copilot review overview

🟡 Changes recommended

The implementation diverges from the accepted JSON contract and contains registration matching and ordering defects.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 · 1 · 1

New issues introduced by this change (2)

Severity	Finding
	src/​specify_cli/​artifacts/​__init__.py — The hook JSON model here contradicts both this PR description and issue #4343's accepted contract.…
	src/​specify_cli/​artifacts/​__init__.py — This insertion index comes from resolver order, not runtime registration order.…

Pre-existing issues (1)

Severity	Finding
	docs/​reference/​artifacts.md — This says every kind is sorted by name, but the new hook implementation sorts hooks by event and… View comment

Issues resolved since last review (5)

Severity	Finding
	src/​specify_cli/​artifacts/​__init__.py — is_hook_registered() reloads and parses .specify/extensions.yml on every call, and this loop… View resolved comment
	src/​specify_cli/​artifacts/​__init__.py — The issue's proposed hook JSON keeps the common stack fields (presetId, presetName, hidden,… View resolved comment
	src/​specify_cli/​artifacts/​__init__.py — Hook collection does not preserve the artifact command's error contract. An OSError while… View resolved comment
	src/​specify_cli/​extensions/​__init__.py — This differs from the runtime filter in get_hooks_for_event, which treats any falsy enabled… View resolved comment
	src/​specify_cli/​artifacts/​__init__.py — Duplicate declarations are not winners at runtime: HookExecutor.register_hooks preserves entries… View resolved comment

Suppressed comments (5)

Previously missed (2) — in code that hasn't changed since the last review.

src/specify_cli/artifacts/init.py:778

• A valid bare hook whose event is named command, template, or script cannot resolve: this branch interprets command:foo as named-artifact shorthand even when kind="hook", then rejects the mismatch. Hook event names use the general identifier-component validator and do not reserve these words, while the new API documents every {eventName}:{targetCommand} as an accepted bare name. When the explicit kind is hook, preserve the whole input as the hook name unless it starts with hook:.
  docs/reference/artifacts.md:199
• Correct the grammatical error in this field description.

src/specify_cli/artifacts/init.py:445

• This preset branch is currently unreachable: PresetManifest.iter_contributions() only emits command, template, and script from provides.templates (src/specify_cli/presets/__init__.py:547-570). Therefore the PR does not actually surface preset-layer hooks or test them, despite claiming forward-compatible preset surfacing and issue #4343 requiring hooks from installed presets. Extend the preset schema/contribution API to emit hooks, or narrow the stated contract instead of retaining a permanently empty loop.

    # 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.

src/specify_cli/artifacts/init.py:559

• The wildcard test uses truthiness, so malformed bindings such as command: "", command: null, or command: 0 mark every declared command for this extension/event active. The stated rule only treats an omitted command key as a wildcard; distinguish absence from a present invalid value so malformed configuration degrades to unregistered rather than a false positive.

        active = any(
            binding.get("extension") == source_id
            and (
                not binding.get("command")
                or binding.get("command") == command
            )
            for binding in enabled_bindings

docs/reference/artifacts.md:19

• This says every kind is sorted by name, but hook rows are actually sorted by event and first-entry priority, as the new Sort order section later states. Clarify the split ordering so consumers do not rely on lexicographic hook-name order.

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.

[Copilot]

Copilot review overview

🟡 Changes recommended

Disabled hook declarations are omitted, and runtime-config I/O failures are silently treated as unregistered hooks.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 · 3

New issues introduced by this change (2)

Severity	Finding
	src/​specify_cli/​artifacts/​__init__.py — iter_extensions_by_priority() explicitly filters disabled registry entries…
	src/​specify_cli/​artifacts/​__init__.py — This catch cannot enforce the documented filesystem-error contract for .specify/extensions.yml:…

Pre-existing issues (2)

Severity	Finding
	src/​specify_cli/​artifacts/​__init__.py — The hook JSON model here contradicts both this PR description and issue #4343's accepted contract.… View comment
	src/​specify_cli/​artifacts/​__init__.py — This insertion index comes from resolver order, not runtime registration order.… View comment

Issues resolved since last review (1)

Severity	Finding
	docs/​reference/​artifacts.md — This says every kind is sorted by name, but the new hook implementation sorts hooks by event and… View resolved comment

[nicolehaugen]

Addressed the runtime-config error-contract feedback in commit \863f0dc4. Hook artifact registration now explicitly follows the existing tolerant \HookExecutor.get_project_config()\ behavior: invalid or unreadable .specify/extensions.yml\ is treated as an empty registration map, while manifest and extension-registry failures retain the artifact resolution error envelope. The ineffective caller-side exception translation was removed, the contract documentation and #4343 were aligned, and focused coverage now pins the unreadable-config behavior.\n\nPosted on behalf of @nicolela by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

Copilot review overview

🔵 Needs a closer look

Command-less bindings are incorrectly treated as wildcards, and manifest failures do not preserve the required error contract.

Review tier: Balanced
Findings: 1 · 3

Pre-existing issues (4)

Severity	Finding
	src/​specify_cli/​artifacts/​__init__.py — The hook JSON model here contradicts both this PR description and issue #4343's accepted contract.… View comment
	src/​specify_cli/​artifacts/​__init__.py — This catch cannot enforce the documented filesystem-error contract for .specify/extensions.yml:… View comment
	src/​specify_cli/​artifacts/​__init__.py — iter_extensions_by_priority() explicitly filters disabled registry entries… View comment
	src/​specify_cli/​artifacts/​__init__.py — This insertion index comes from resolver order, not runtime registration order.… View comment

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

src/specify_cli/artifacts/init.py:668

• This path silently drops a registered extension whose manifest fails validation because get_extension() converts ValidationError to None; the fallback branch below also suppresses validation and read errors. As a result, malformed/unreadable hook manifests produce an omitted row (or unknown artifact) instead of the artifact resolution failed envelope required by the PR and #4343 error contract. Distinguish an absent manifest from a parse/read failure and let the latter be wrapped as ArtifactResolutionError.

This issue also appears on line 752 of the same file.
tests/test_artifact_command.py:1816

• This test codifies an unsupported wildcard. Runtime registration always requires a command and skips entries without one; a manually malformed command-less entry is returned as a missing command, not expanded across the extension's declarations. Replace this with a regression asserting the declaration remains inactive.
  docs/reference/artifacts.md:231
• The documented command-omission wildcard does not exist in the hook runtime. register_hooks() skips entries without a command, and execution exposes such manually authored entries as a missing command rather than expanding them to every declared command. Document exact command matching so active describes a hook the runtime can actually invoke.

src/specify_cli/artifacts/init.py:756

• A binding with no command is a malformed runtime hook, not an event-wide wildcard: register_hooks() skips such entries, while execution renders them as <missing command>/None. Treating a missing command as a match marks every declaration from this extension/event active even though none of those commands would be executed. Require exact command equality here; the added wildcard test and documentation should be updated accordingly.

            binding.get("extension") == source_id
            and (
                not binding.get("command")
                or binding.get("command") == command
            )

[nicolehaugen]

Review round update through commit \10aa99bd: clarified that registry-disabled extensions are excluded by the standard resolver, documented hook stack order as deterministic declaration order rather than runtime execution order, aligned invalid-manifest handling with the existing artifact inventory, and corrected command-less runtime bindings so they no longer activate concrete hook declarations. The existing wildcard test was replaced rather than supplemented.\n\nPosted on behalf of @nicolela by GitHub Copilot (model: GPT-5.6 Sol).

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation matches the revised contract and includes comprehensive coverage.

Review tier: Balanced
Findings: 1 · 3

Pre-existing issues (4)

Severity	Finding
	src/​specify_cli/​artifacts/​__init__.py — The hook JSON model here contradicts both this PR description and issue #4343's accepted contract.… View comment
	src/​specify_cli/​artifacts/​__init__.py — This catch cannot enforce the documented filesystem-error contract for .specify/extensions.yml:… View comment
	src/​specify_cli/​artifacts/​__init__.py — iter_extensions_by_priority() explicitly filters disabled registry entries… View comment
	src/​specify_cli/​artifacts/​__init__.py — This insertion index comes from resolver order, not runtime registration order.… View comment

[Copilot]

Copilot review overview

🟡 Changes recommended

Valid colon-containing hook declarations currently cause artifact inventory to fail.

Get a fresh assessment by requesting another Copilot review.

Review tier: Balanced
Findings: 2 · 3

Open findings (5)

• Preserve valid colon-containing hook declarations · New
• The hook JSON model here contradicts both this PR description and issue #4343's accepted contract.…
• This catch cannot enforce the documented filesystem-error contract for .specify/extensions.yml:…
• iter_extensions_by_priority() explicitly filters disabled registry entries…
• This insertion index comes from resolver order, not runtime registration order.…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Reserved hook event names cannot round-trip through the documented name-plus-kind lookup form.

Review tier: Balanced
Findings: None

Resolved findings (1)

• Test omission of an invalid hook manifest

Previously missed findings (1)

In code that hasn't changed since last review

Disambiguate reserved hook event names

src/​specify_cli/​artifacts/​catalog.py:237

A valid hook whose event is command, template, script, or hook cannot round-trip via the documented row["name"] plus --kind hook form. For example, command:cmd.x is rejected as a mismatched command shorthand, while hook:cmd.x is stripped here and then fails hook-name parsing because only cmd.x remains. Extension manifests do not reserve these event names, so please disambiguate explicit hook lookups (or encode reserved event components) and add regression cases for them.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Malformed identifier coverage is missing, and the stack documentation retains contradictory winner semantics.

Review tier: Balanced
Findings: None

Previously missed findings (2)

In code that hasn't changed since last review

Qualify winner semantics for additive hook stacks

docs/​reference/​artifacts.md:142

This new additive value conflicts with the unqualified statement at line 132 that index 0 is the winning layer. Hook stacks can have multiple active entries and no winner, so readers can infer the wrong execution semantics before reaching the hook-specific section. Limit the winner statement to named artifacts and explicitly distinguish additive hook stacks.

Cover malformed hook identifier rejection

src/​specify_cli/​artifacts/​_identifiers.py:127

The new decoder has explicit failure behavior for malformed percent escapes and invalid UTF-8, but the added tests cover only successful decoding. Add negative artifact info cases such as hook:event:bad%escape and hook:event:%FF, asserting the stable unknown artifact envelope and empty stdout so these validation branches cannot regress.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The documented hook ID encoding rule does not accurately describe the implemented canonical encoding.

Review tier: Balanced
Findings: None

Previously missed findings (1)

In code that hasn't changed since last review

Describe the actual hook ID encoding rule

docs/​reference/​artifacts.md:167

The implementation uses urllib.parse.quote(..., safe=""), so it percent-encodes every character outside the URL unreserved set—not only characters needed to disambiguate colons. For example, the documented /skill... value becomes %2Fskill... even though / is not the delimiter. Consumers implementing the stated narrower rule would generate non-canonical IDs; document the full encoding rule instead.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Invalid Unicode hook components can escape the documented error handling with an uncaught exception.

Review tier: Balanced
Findings: None

Previously missed findings (1)

In code that hasn't changed since last review

Normalize invalid Unicode before building hook IDs

src/​specify_cli/​artifacts/​_identifiers.py:112

ExtensionManifest only requires hook commands to be truthy, so a YAML string containing a lone Unicode surrogate can reach this call. urllib.parse.quote() then raises UnicodeEncodeError, which bypasses the IdentifierComponentError handling in hook collection and makes artifact list/info fail outside the documented malformed-manifest behavior. Convert that encoding failure to IdentifierComponentError and add a regression case for an invalid Unicode event or command.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The private hook lookup-ID grammar does not satisfy the linked issue’s shared identifier parity requirement.

Review tier: Balanced
Findings: None

Previously missed findings (1)

In code that hasn't changed since last review

Align lookup IDs with the shared contribution contract

src/​specify_cli/​artifacts/​_identifiers.py:78

Issue #4343 requires each hook lookupId to equal the existing shared derive_hook_id(...) result and says no new identifier grammar is introduced. This instead creates an artifact-private derivation (including percent encoding), while the current tree has no shared derive_hook_id/manifest contribution surface; the added test therefore compares against the same private helper rather than proving cross-surface parity. Either land and consume the shared contribution-ID prerequisite, or revise #4343 and this PR's closing scope to explicitly defer that acceptance criterion.

[nicolehaugen]

The PR description has been updated to address the remaining reported issue:  Align lookup IDs with the shared contribution contractsrc/​specify_cli/​artifacts/​_identifiers.py:78 - this is so that the defined scope now matches what is implemented.

All tests also passed, so merging.

I've addressed your changes - all tests pass and I've ensured the changes for this are purely additive.