feat(wizard): consume native CLI hook artifacts and remove legacy hook parser by nicolehaugen · Pull Request #28 · github/spec-kit-copilot

nicolehaugen · 2026-09-10T22:40:56Z

Summary

This PR migrates the Spec Kit Wizard canvas extension off legacy manual YAML hook parsing (hooks.mjs, .specify/extensions.yml, and extension.yml manifest scanning) and onto native kind: "hook" artifacts supplied by specify artifact list --json.

Key Changes

Removed Legacy Hook Parser:
- Deleted plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/hooks.mjs.
- Removed js-yaml runtime dependency import from composition loader.
Native CLI Hook Artifact Support:
- Updated shapeArtifact in artifact-cli.mjs to accept kind: "hook".
- Synthesizes compatibility fields (event, targetCommand, registered, hookBindings, hookBinding) for UI renderers (resolveHooksForCommand).
- Extended accumulateProvidesCounts to count native kind: "hook" artifacts towards extension/preset hook summaries (item.provides.hooks).
- Added applyNativeHookAttributions to attach inline hooks[] arrays onto parent phase command artifacts (commands/speckit.<phase>) directly in memory.
Test Suite Updates:
- Updated artifact-cli.test.mjs to supply native kind: "hook" artifacts in test fixtures instead of writing mock manifest files to disk.
- All 25 unit tests across 5 test suites pass cleanly.

Stacked PR Context

Stacked on top of nicolehaugen-lookupid-wizard-simplification-d12 (PR Consume deterministic lookupId to simplify wizard provider resolution #21).

Copilot

Copilot wasn't able to review any files in this pull request.

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

…tifacts Remove hooks.mjs and update artifact-cli.mjs to consume native kind: 'hook' artifacts from specify artifact list --json. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

This reverts commit 5e020ad.

Copilot

Copilot review overview

🟡 Changes recommended

Runtime normalization corrupts native hook targets and metadata, while the unrelated product rename conflicts with documented branding.

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

Review tier: Balanced
Findings: 1 · 2

New issues introduced by this change (3)

Severity	Finding
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Keep hook IDs aligned with their target commands
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Preserve additive strategy through composition application
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Carry optionality onto inline hook attributions

…guess normalizeHookArtifactsInComposition() derived a hook's own command from its id by stripping a 'commands/' prefix, but native hook ids are shaped as 'hooks/<event>:<target>' (cliIdToWizardId), so the prefix never matched and the comparison always failed. This forced every native hook through the commandByProvider fallback, which silently rewrote the hook's id/targetCommand to whichever command from that provider was indexed first -- wrong for any provider with 2+ commands. Use the artifact's own authoritative targetCommand field (set at shape time from the CLI) instead, falling back to the old id-derived guess only when that field is absent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

… allowlist artifact-cli.mjs shapes native hook layers with strategy "additive" (hooks stack alongside a command rather than replace/wrap/prepend/ append it), and its own VALID_STRATEGIES set already includes it. But composition-apply.mjs's normalizeArtifact() kept a separate, stale allowlist without "additive", so runFastComposition() -> applyComposition() silently coerced every native hook layer to "replace" on the way into the cached/UI composition, contradicting what the CLI actually reported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…ntry applyNativeHookAttributions() builds a synthesized `hooks[]` entry on the parent command artifact for display on the phase card, but omitted `optional` even though shapeArtifact() already computed it (as `layer.optional`) for the standalone hook artifact's `hookBindings`. resolveHooksForCommand() processes the inline entry first and de-duplicates the standalone binding on the same key, so an optional hook's `optional: true` was silently dropped and it rendered as Required on the phase card, even though the Composition/Artifacts tab (which reads the standalone hookBindings directly) showed it correctly as Optional. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🔵 Needs a closer look

Native hook IDs and multi-contributor stacks are not handled correctly by several existing UI and attribution paths.

Review tier: Balanced
Findings: 1

Pre-existing issues (1)

Severity	Finding
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Keep hook IDs aligned with their target commands View comment

Issues resolved since last review (2)

Severity	Finding
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Carry optionality onto inline hook attributions View resolved comment
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Preserve additive strategy through composition application View resolved comment

Previously missed findings (3)

In code that hasn't changed since last review

plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:124

This creates one binding per stack layer, but ui/composition.js:163-184 treats hookBindings.length as an event count and applies that full weight to every layer. For a native row shared by two providers, each provider is consequently reported as contributing two hooks instead of one. Update the contribution calculation to associate/count each binding with its matching provider.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:216
accumulateProvidesCounts now computes hooks for both preset and extension layers, but this assignment remains inside the extension-only branch. A preset that contributes a native hook therefore omits provides.hooks, contrary to the stated extension/preset summary support. Populate the hook count for preset summaries as well.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:234
Native hook stacks are additive, so multiple entries may be active at once. Selecting only the first active layer records just one provider in the parent command's hooks[]; other enabled declarations for the same event/target disappear from phase attribution and lose their own optional/registration state. Emit one inline entry per stack layer and derive registered from that layer's active flag.

nicolehaugen · 2026-09-11T03:04:21Z

Re: the "previously missed finding" that native hook stacks with 2+ active layers only surface one contributor in the parent command's inline hooks[] (via applyNativeHookAttributions in artifact-cli.mjs) — deliberately scoping this out of the current PR rather than fixing it now.

Why: a hook artifact only gets 2+ stack layers when two different extensions declare a hook with the exact same (event, targetCommand) pair. In this repo's model:

Two extensions defining the same command is already rejected/deduped.
Two extensions independently choosing the same target command name is unlikely, since target commands are namespaced per-extension (speckit.<extension>.<command>) and normally owned by the extension that defines them.
The only realistic path is extension B intentionally referencing extension A's existing command from its own hook declaration (piggybacking on another extension's action) — legitimate per the data model, but not something we've observed in any real extension here or elsewhere.

None of the 6 hook artifacts currently produced by this repo have more than 1 stack layer, so this doesn't affect current behavior. Fixing it properly would touch both applyNativeHookAttributions() and the standalone fallback in pipeline/active-artifacts.mjs::resolveHooksForCommand(), which is a larger surface than the targeted one-line fixes already made in this PR, for a display-only gap (it never affects actual hook dispatch/execution) that requires an uncommon extension-authoring pattern to trigger.

Happy to revisit if/when a real extension combination actually exercises this.

- Note why preset hook counts are always zero (hooks are extension-only by design, per artifactOrigin()). - Note the additive-hook-stack limitation in applyNativeHookAttributions() where only the first active layer is captured for a command's inline hooks[]. - Note that the hookBindings-length weight used to tally per-provider and header hook counts in ui/composition.js is a local stand-in that should be replaced once hook counts are reported directly by the CLI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

Native hook IDs remain incompatible with command-ID-based UI lookups, causing incorrect hook rendering and command behavior.

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

Review tier: Balanced
Findings: 1 · 1

New issues introduced by this change (2)

Severity	Finding
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Key hook UI lookups by targetCommand
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Remove the stale legacy hook architecture note

Issues resolved since last review (1)

Severity	Finding
	`plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs` — Keep hook IDs aligned with their target commands View resolved comment

+        shaped.event = event;
+        shaped.targetCommand = targetCommand;
+        shaped.registered = cliArtifact.registered ?? true;


+// Native CLI hook artifacts are consumed directly without manual extension
+// manifest parsing.


Hook artifacts' own id (hooks/<event>:<target>) no longer matches their target command's id (commands/<target>) after the native hook artifact migration. This broke two lookups: - composition-artifacts.js: hookByTargetId and the hook source-path/label resolution keyed off the hook's own id, so command rows never picked up their hook attribution chip and the hook target link could not resolve. - phase-runtime.js: hookBindingsByCommandId keyed off the hook's own id, so the command card never found its hook binding, and the 'items' filter fell back to rendering hook artifacts as their own standalone cards (duplicating the command with a malformed hooks/... id/label). Every hook artifact's targetCommand is guaranteed to resolve to an existing command artifact, so the 'hook with no command' fallback in phase-runtime.js is removed entirely rather than patched - items is now strictly commands, and hooks only ever surface via their command's binding. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI balanced review requested due to automatic review settings September 10, 2026 22:40

wip: initial hook artifact rework base

db52943

nicolehaugen changed the base branch from main to nicolehaugen-lookupid-wizard-simplification-d12 September 10, 2026 22:41

Copilot started reviewing on behalf of nicolehaugen September 10, 2026 22:41 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

feat: replace legacy manual YAML hook parsing with native CLI hook ar…

328a182

…tifacts Remove hooks.mjs and update artifact-cli.mjs to consume native kind: 'hook' artifacts from specify artifact list --json. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

nicolehaugen changed the title ~~Consume native hook artifacts for wizard composition~~ feat(wizard): consume native CLI hook artifacts and remove legacy hook parser Sep 10, 2026

chore(wizard): update title to Spec Kit Wizard (Hook Artifacts)

5e020ad

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

nicolehaugen requested a balanced review from Copilot September 11, 2026 00:09

Copilot started reviewing on behalf of nicolehaugen September 11, 2026 00:10 View session

Revert "chore(wizard): update title to Spec Kit Wizard (Hook Artifacts)"

e51c364

This reverts commit 5e020ad.

Copilot AI reviewed Sep 11, 2026

View reviewed changes

nicolehaugen and others added 3 commits September 10, 2026 19:23

nicolehaugen marked this pull request as ready for review September 11, 2026 01:09

nicolehaugen requested a review from mnriem as a code owner September 11, 2026 01:09

nicolehaugen requested a balanced review from Copilot September 11, 2026 01:09

Copilot started reviewing on behalf of nicolehaugen September 11, 2026 01:09 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

nicolehaugen requested a balanced review from Copilot September 11, 2026 03:37

Copilot started reviewing on behalf of nicolehaugen September 11, 2026 03:38 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Navigation Menu

feat(wizard): consume native CLI hook artifacts and remove legacy hook parser - #28

feat(wizard): consume native CLI hook artifacts and remove legacy hook parser#28
nicolehaugen wants to merge 9 commits into
nicolehaugen-lookupid-wizard-simplification-d12from
nicolehaugen-hook-rework-pr-4348

nicolehaugen commented Sep 10, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

nicolehaugen commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Native CLI hook artifacts are consumed directly without manual extension
		// manifest parsing.

Conversation

nicolehaugen commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Changes

Stacked PR Context

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

nicolehaugen commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nicolehaugen commented Sep 10, 2026 •

edited

Loading