Move wizard composition to specify artifact CLI (blocked by spec-kit#4305) - #18
Draft
nicolehaugen wants to merge 40 commits into
Draft
Move wizard composition to specify artifact CLI (blocked by spec-kit#4305)#18nicolehaugen wants to merge 40 commits into
nicolehaugen wants to merge 40 commits into
Conversation
Complements artifact-cli.test.mjs (fixture round-trip) with three real-shape guards: * Live-CLI test invokes real `specify artifact list/info` on a scaffolded workspace and asserts wizard-contract fields (id/kind/stack, layer vocabulary, exactly-one-active). Skips when `specify` isn't on PATH. * Fixture-drift tests replay a committed snapshot of real CLI output and assert the field set the shape mapper reads is present. Guards against silent CLI shape changes without needing the binary in CI. * fixtures/README.md documents regeneration. Full suite: 211/211 pass.
On a warm cache the server's `bootAsync` reaches `phase: ready` before
the browser's first paint. The old flow relied on JS to (a) populate the
overlay content and (b) hide `main.app-body`, then almost immediately
flipped the overlay to `is-hidden` in the same microtask cycle. The
browser composited populate + hide into one frame and the user saw a
blank body flip straight to the loaded app with no boot indicator.
Three-part fix so the overlay is guaranteed to paint:
* Static markup in `index.html` — pre-render the overlay panel with
title + subtitle so it is visible from the very first paint, before
any module fetch/parse.
* CSS-level `main.app-body { visibility: hidden }` — no longer
depends on JS running to keep the app body hidden underneath.
* JS-side minimum visible time (`MIN_OVERLAY_MS = 450`) — even when
the state fetch resolves in a single frame, the hide is deferred via
`setTimeout` so the overlay stays up long enough to register.
Boot's `hydrateCatalogs` used to walk preset → extension → bundle serially, and each hydrator walked its 2–3 source URLs serially inside `hydrateFromCatalogSources`. That's ~8 GitHub GETs strictly serial on a cold cache, plus 3 sequential `specify <kind> list` shell-outs, for what is entirely disjoint state. Two changes: * Run `hydratePresetsForSources` / `hydrateExtensionsForSources` / `hydrateBundlesForSources` via `Promise.all` — they touch independent cache slices. * Inside `hydrateFromCatalogSources`, `Promise.all` the per-source `fetchCatalogJson` calls before folding into the items array. Order of items is preserved because we still iterate the resolved array in source order. All 211 tests pass.
This reverts commit bb5edf2.
The catalog boot step called specify artifact info once per artifact via �xecFileSync in a serial loop. With a real workspace stack (~70 artifacts across 4 presets + 1 extension), that's ~70 shell-outs, each one blocking the Node event loop for its full duration. Impact: /api/state and SSE could not be answered during boot, so the UI sat on 'Loading catalogs' for the full 108s wall time of the loop, even though the HTTP server was up. From the user's perspective the wizard 'hung'. Fix: - Swap the default runner to a promisified execFile so each shell-out yields the event loop instead of hard-blocking it. - Fan the info-per-id calls out with Promise.all — safe now that spawn is non-blocking. - Await the runner return so injected sync test runners (which return a Buffer/string) still work unchanged. Measured on the current workspace (68 artifacts): before: 108s serial sync, HTTP frozen throughout after: 13s parallel async, HTTP responsive throughout (~8x faster). All 211 tests still pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d44fd06-6323-4654-8a80-b756561ef669
The catalog boot step was fragile in three separate ways beyond the
sync exec loop already fixed in the artifact-info fan-out:
1. fetchCatalogJson had NO timeout. A stalled socket (slow DNS, TCP
loss, CDN outage) would block the fetch forever and freeze boot on
'Loading catalogs' with no recovery path. Added AbortSignal.timeout
(15s per fetch).
2. specifyRun had NO timeout either. A wedged CLI (uv resolver stuck,
PATH resolution hang) had the same failure mode. Added a 20s kill
timer; on expiry we resolve with the partial stdout (callers already
tolerate empty output).
3. hydrateCatalogs awaited the three groups (presets, extensions,
bundles) serially, and hydrateFromCatalogSources awaited each source
inside a group serially. Both are independent I/O — swapped for
Promise.all at each level so total time is bounded by the slowest
single call, not the sum.
Measured on the current workspace (7 catalogs, 68 artifacts):
before: 108s serial sync, HTTP frozen throughout
after: ~14s parallel async, HTTP responsive throughout (~8x faster,
and now failure-bounded instead of unbounded)
All 211 tests still pass.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2d44fd06-6323-4654-8a80-b756561ef669
Boot was making 68 sequential `specify artifact info` shell-outs per artifact to reconstruct composition stacks — the flow that regressed catalog-loading to 27-60s on Windows. spec-kit PR #4305 makes `artifact list --json` return the full per-row stack, so the wizard needs exactly ONE shell-out to build composition.
Wizard-side changes:
- `buildCompositionFromCli` now consumes a single `list --json` call and feeds rows directly through `shapeArtifact`. Presets/extensions summaries are folded from artifact stacks (accepted edge case: a preset that contributes zero currently-active artifacts won't appear — living with it until upstream ships `preset list --json`).
- Removed `specifyArtifactInfo` — dead post-refactor. Don't reintroduce a per-artifact fan-out on the boot critical path.
- Deleted `project-scanner.mjs::scanComposition` and its `.specify/{presets,extensions}.json` reads. Neither file is written by any CLI version; the code was dead. Removed the now-unused `readBoundedJson` import.
- Governing principle: CLI is the source of truth for composition. No direct fs reads of `.registry`/`.yml` from the wizard, ever.
Test-side changes:
- Unit fixtures flattened: list rows now embed `stack` (no separate `info` map). `fakeRunner` simplified to only handle `list`.
- Fixture-drift test rewritten to require `stack` on list rows. Skips gracefully when the on-disk snapshot is pre-#4305 (regen once upstream ships).
- Live-CLI test skips when the installed CLI's `list --json` doesn't yet emit `stack` — same rationale.
- Deleted obsolete `live-cli-info.json` fixture; updated README.
- Deleted `scanWorkspace drops malformed composition entries` test — it exercised the deleted `scanComposition` path.
208 tests: 207 pass, 1 skip (drift, until fixture regen).
Real `specify artifact list --json` output now carries per-row `stack` (spec-kit#4305 has landed). Fixture-drift and live-CLI tests are now actively guarding (208/208 pass, 0 skips) instead of skipping under the pre-#4305 detector.
Boot overlay previously bundled two unrelated phases into one 'catalog' step: remote catalog JSON fetches (~150ms parallel) AND the composition CLI build (specify artifact list --json, ~2s cold). When boot felt slow, you couldn't tell which side was blocked. Split them so each shows independently in the overlay: - `catalog` (Loading catalogs) \u2014 hydratePresets/Extensions/Bundles, remote HTTPS + `specify <group> list` per group, all parallel. - `composition` (Building composition) \u2014 single `specify artifact list --json` call + shape mapping. Now a hang on either side is visible at a glance without log scraping.
Comments now describe current behavior only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d44fd06-6323-4654-8a80-b756561ef669
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d44fd06-6323-4654-8a80-b756561ef669
- project-scanner.mjs: rewrite composition-block comment to describe current behavior only; drop 'used to look at' language and the fabricated AGENTS.md citation. - artifact-cli.mjs: drop the phantom 'AGENTS.md: CLI is the source of truth for composition.' line from the doc header. - pipeline-fast-path.mjs: rename 'LLM Stage 2' → 'LLM path' (there was no Stage 1) and 'Fast path' → 'Deterministic path' to match the actual code. - ui/index.html, ui/boot.js: drop '- Dev' suffix from the wizard title in all four spots to prepare for check-in. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d44fd06-6323-4654-8a80-b756561ef669
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Moves wizard composition from filesystem inspection to the forthcoming specify artifact list --json API. This remains blocked by spec-kit#4305 and its CLI release.
Changes:
- Adds CLI-backed composition mapping, hook enrichment, and pipeline fast-path logic.
- Separates catalog/composition boot phases and parallelizes bounded catalog hydration.
- Updates boot UX, dependencies, and unit tests while removing the legacy assembler.
Show a summary per file
| File | Description |
|---|---|
ui/styles/boot.css |
Hides app content during boot. |
ui/index.html |
Adds initial boot markup. |
ui/boot.js |
Adds composition progress and minimum display time. |
test/state-and-scanner.test.mjs |
Removes obsolete scanner test. |
test/composition.test.mjs |
Removes legacy composition tests. |
test/boot-progress.test.mjs |
Covers the new boot step. |
test/artifact-cli.test.mjs |
Tests CLI composition mapping. |
project-scanner.mjs |
Removes filesystem composition scanning. |
package.json |
Updates js-yaml. |
package-lock.json |
Locks updated dependency. |
extension.mjs |
Splits catalog and composition boot work. |
composition/pipeline-fast-path.mjs |
Adds deterministic pipeline selection. |
composition/hooks.mjs |
Extracts hook metadata. |
composition/collect.mjs |
Removes legacy filesystem collector. |
composition/assembler.mjs |
Removes legacy assembler. |
composition/artifact-cli.mjs |
Adds CLI-backed composition source. |
catalog/sources.mjs |
Adds fetch timeouts. |
catalog/shared.mjs |
Parallelizes hydration and bounds CLI calls. |
canvas-runtime/composition-apply.mjs |
Integrates CLI composition and fast path. |
canvas-runtime/boot-progress.mjs |
Registers composition boot progress. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Files not reviewed (1)
- plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/package-lock.json: Generated file
- Files reviewed: 19/20 changed files
- Comments generated: 4
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/package-lock.json: Generated file
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:105
projectis a valid CLI layer, but the wizard renderers only recognizecore,preset, andextension. Preserving it here causes an active project override to be shown as Core/unchanged (for example,artifactPillOriginfalls through tocoreand contributor rows omit it). Add project-origin handling across the composition UI before accepting this layer.
return {
// CLI `null` layer = built-in; wizard code expects "core".
layer: layer.layer == null ? "core" : layer.layer,
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/composition-apply.mjs:302
dispatchKindPromptstill checks!fast.stage2Needed, so this renamed return field is never consumed. On every successful refreshstage2Neededisundefined, making the caller return early even whenpipelineFastPathis false; novel commands and stack directives therefore never reachinferPipeline. Update that caller to branch onfast.pipelineFastPathas part of this rename.
return { ok: true, reason, pipelineFastPath: fastPath.canSynthesize };
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:108
- The upstream CLI contract sets
presetId/presetNameto null for extension layers and identifies the extension throughsourceId. Passing those fields through unchanged breaks this file's own extension folds and ownership checks (accumulateProvidesCounts,activeExtensionIds, and hook-command suppression), all of which key extensions bypresetId; uncatalogued extensions disappear and hook commands can be duplicated. Add the wizard compatibility alias when normalizing extension rows.
return {
// CLI `null` layer = built-in; wizard code expects "core".
layer: layer.layer == null ? "core" : layer.layer,
presetId: layer.presetId ?? null,
presetName: layer.presetName ?? null,
sourceId: layer.sourceId ?? null,
- Files reviewed: 19/20 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/package-lock.json: Generated file
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:432
- The new CLI-backed path's hook enrichment is untested: the replacement tests cover core/preset mapping and pipeline decisions, while the deleted suite contained the only assertions for inline hook attribution, standalone hook artifacts, registration flags, and hook-command suppression. Add a temporary extension manifest plus
extensions.ymlfixture and verify this output throughbuildCompositionFromCli.
const { extensionHookInfo, hooksMap } = await collectHookMetadata(
workspaceRoot,
activeExtensionIds,
);
const artifacts = applyHookAttributions(artifactsRaw, extensionHookInfo, hooksMap);
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/composition-apply.mjs:302
dispatchKindPromptstill checks!fast.stage2Neededatcanvas-runtime/dispatch.mjs:100. Because this result now omits that property, the condition is true for every successful build, includingpipelineFastPath: false, so Refresh never falls through toinferPipelinefor novel commands or stack directives. Update that caller to branch onfast.pipelineFastPathand remove its stale Stage 2 naming.
return { ok: true, reason, pipelineFastPath: fastPath.canSynthesize };
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/extension.mjs:219
- A composition failure calls
tracker.fail, but this unconditionalready()immediately overwritesboot.phasewith"ready".ui/boot.js:112then hides the overlay, and generic composition failures are not rendered throughdepsError, so an unsupported CLI produces empty composition without the error the PR description promises to surface. Preserve the failed phase or surface a persistent in-app error before marking boot ready.
tracker.ready();
- Files reviewed: 19/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The composition refresh flow still used the retired stage2Needed contract, incorrectly bypassing LLM pipeline inference. This aligns the runtime on pipelineFastPath while retaining LLM inference as the non-fast fallback.
Fast-path contract
Rename the deterministic decision helper to computePipelineFastPath.
Return pipelineFastPath: true only when inferredPipeline can be synthesized.
Refresh dispatch
Bypass LLM inference only for a successful deterministic pipeline:
if (fast?.ok && fast.pipelineFastPath) {
return { kind, fastComposition: true };
}
Fall through to LLM inference for novel commands, stack directives, or missing canonical anchors.
Terminology and coverage
Replace obsolete Stage 1/2 naming with “pipeline fast path” and “LLM inference.”
Update tests for deterministic synthesis and LLM fallback behavior.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Keep hook artifacts unchanged when their existing binding already identifies the artifact command, before applying the legacy provider-based fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Project overrides, provider ordering, and layer version metadata are not preserved correctly.
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
 |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/composition-apply.mjs — This provider-based rewrite ignores an already authoritative hook target. For an extension that… View comment |
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:105
- The upstream artifact contract includes
layer: "project", and this mapper passes it through, but the wizard still handles only core/preset/extension ownership. For example,contributorPart()returns the “unchanged” rendering for project-owned artifacts andlayerOwnerName()falls through to preset fields, so an active project override is displayed as if no customization exists. Add explicit project-layer labels/contributor rendering (and a contract-shaped test) before accepting these rows.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:114 - The CLI stack schema has no
versionfield, and the mapper does not reattach one, whilerenderStackLayer()displays versions exclusively fromlayer.version(ui/composition.js:381-383). The deleted assembler populated this from each provider manifest, so all preset/extension version badges in artifact stacks now disappear even when the catalog or extension manifest already supplied the version. Enrich mapped layers from the provider summaries/manifests, or resolve the version by provider id in the renderer, and cover the rendered stack contract.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:182 - The summary now inherits catalog-file order, not the CLI precedence recorded in each active item's
cliOrder. BothorderedCompositionPresets()andorderedCompositionExtensions()render these arrays verbatim (ui/state.js:113-130), so tied/priority-ordered providers can appear in the wrong order in Layers and provider sections. Sort active cached items by numericcliOrder(with stable input-order fallback) before constructing this map.
Seed composition provider summaries from active cached items ordered by cliOrder, while keeping unordered providers stable at the end. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Clarify that project layers remain in upstream stacks for fidelity while contributor ownership intentionally covers only Core, presets, and extensions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Stop rendering provider versions on individual artifact stack rows and simplify the corresponding three-column layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Extension metadata/count regressions and unsafe timeout handling remain, while the required upstream CLI change is still blocked.
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/composition-apply.mjs — This provider-based rewrite ignores an already authoritative hook target. For an extension that… View resolved comment |
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/catalog/shared.mjs:60
- Returning partial stdout on timeout can turn an incomplete
specify preset/extension listinto a valid-looking partial inventory. The parsers accept complete entries from that prefix, so later installed items are silently marked inactive and the catalog can offer to install them again. Treat a timed-out command as failed rather than consuming truncated output.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:455 - This summarizes the post-attribution array, after extension commands declared as hooks have been removed and replaced by
kind: "hook"rows. Consequently a manifest that provides one command and registers it as a hook reportsprovides.commands: 0(the new audit fixture has exactly this shape), whereas the provider summary should still count the CLI's command contribution. FoldartifactsRawhere; hook counts are already supplied separately byextensionHookInfo.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:440 - Extension layers never receive their display name. The upstream contract leaves
presetNamenull for extensions, so the changed stack/contributor renderers fall back tosourceId(for example,audit) even thoughextensionHookInfohas already loadedAudit Extension. Enrich the layers before attribution so the CLI migration preserves the prior human-readable ownership labels.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/composition-apply.mjs:319
- The requested API migration is still split across inverse flags: this adds
pipelineFastPath, but also retainsstage2Neededsolely becausecanvas-runtime/dispatch.mjs:108still consumes the legacy name. Update that caller to branch onfast.pipelineFastPathand remove this compatibility field so the two booleans cannot drift.
pipelineFastPath: fastPath.canSynthesize,
stage2Needed: !fastPath.canSynthesize,
Use the extension manifest's nested identity block for display metadata while retaining top-level fallbacks, and align the existing hook fixture with the real schema. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Reject partial provider inventories on timeout, count CLI command contributions before hook reclassification, preserve command provenance on hook artifacts, and complete the pipelineFastPath API migration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Extension manifest lookup, uncatalogued provider ordering, and CLI output limits can produce incomplete or failed compositions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs — sourceId is not a safe extension-directory name. The upstream artifact contract derives it from… |
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs — Providers absent from the hardcoded catalog sources are appended in counts insertion order, which… |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:32
- This new sole-source CLI call inherits
execFile's 1 MiB defaultmaxBuffer. Becauseartifact list --jsonnow returns every full stack and description, a sufficiently large composition is terminated withERR_CHILD_PROCESS_STDIO_MAXBUFFER, making boot report an empty composition even though the CLI succeeded. Set an explicit payload limit appropriate for the inventory or use a streaming child-process path with an intentional cap.
Carry CLI-reported manifest paths into hook metadata collection while retaining sourceId as provider identity, workspace containment, and the legacy path fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Remove the artifact adapter's partial cliOrder sort so this PR preserves the wizard's existing provider ordering until the CLI exposes a complete authoritative order. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Clarify that artifact stacks are authoritative per artifact while global provider precedence remains outside the wizard's responsibility. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Clarify that composition loading intentionally uses a bounded one-shot read and rejects oversized payloads rather than parsing partial JSON. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Project override layers are misrepresented as Core, and runtime remains blocked on the unreleased upstream CLI contract.
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs — Providers absent from the hardcoded catalog sources are appended in counts insertion order, which… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs — sourceId is not a safe extension-directory name. The upstream artifact contract derives it from… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:108
- The upstream contract also emits
layer: "project", so passing non-null layers through introduces project overrides into the UI. Downstream origin logic only recognizes preset/extension and otherwise falls back to Core (ui/composition.js:120-129), and the layer label map has no project entry, so an active project override is presented as Core/default. Add explicit project-layer labeling/origin handling and a contract-shaped project override test.
Document that supported providers come from the wizard catalogs, provider summaries preserve payload order, and applied precedence belongs to each artifact stack. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
CLI rows can lose valid templates and extension display names, while the upstream dependency remains unmerged.
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs — Providers absent from the hardcoded catalog sources are appended in counts insertion order, which… View resolved comment |
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/canvas-runtime/composition-apply.mjs:304
- Switching to authoritative CLI rows exposes a legacy normalization bug:
buildCompositionFromClican return both a command and a template with the same name, butapplyCompositionstill removes the template innormalizeHookArtifactsInComposition. The upstream artifact contract explicitly preserves both kind-qualified rows, so mixed-kind overrides lose their template from cached composition. Remove that old command/template de-duplication and retain both artifacts.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/artifact-cli.mjs:447 - CLI extension layers carry
sourceIdbut no display name. AlthoughextensionHookInfonow contains the manifest name, it is never attached to the original stack layers, sorenderStackLayerandlayerOwnerNamefall back to an ID such asqualityinstead ofQuality Extension. Enrich those layers before applying hook attribution.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/hooks.mjs:5 - The PR description says composition moves off direct filesystem inspection and that one artifact-list call provides everything needed for hooks, but this module still parses every extension manifest plus
.specify/extensions.yml. Either move hook metadata into the CLI contract or narrow the PR description to command/template/script composition and disclose this remaining filesystem dependency.
Remove the legacy command/template de-duplication now that the CLI emits authoritative kind-qualified rows. Document temporary hook enrichment and intentional extension ID labels. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf041796-b05b-4307-bef6-e6e54a544beb
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Valid list-form hooks are dropped and omitted optional flags are incorrectly treated as required.
Review tier: Balanced
Findings: None
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/hooks.mjs:138
- Extension manifests allow an event to contain a list of hook mappings, but spreading an array here creates numeric properties and no
command; the filter then drops every hook for that event. Flatten array-valued event configurations so each declared command receives its own attribution.
plugins/spec-kit-copilot-wizard/extensions/speckit-wizard-canvas/composition/hooks.mjs:146 - The extension manifest contract defaults an omitted
optionalfield totrue. Coercing it with!!instead marks valid hooks that omit the field as required, so the Composition UI incorrectly says they run unconditionally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
Blocked by github/spec-kit#4305.
Do not merge until #4305 lands and a
specify-clirelease containingspecify artifact list --jsonis published to PyPI. Once that ships, bumpthe version floor note in
skills/speckit-cli-setup/SKILL.mdand un-draft.Replaces #17 — same commits, but branched directly in
github/spec-kit-copilotoffmaininstead of coming from a fork.Summary
Move the wizard's Composition tab off direct filesystem inspection for
commands, templates, and scripts and onto a single
specify artifact list --jsoncall. The CLI returns one row per artifactcarrying the full composition
stack: [...]. Until the CLI exposes nativehook metadata, hook attribution remains a temporary wizard-owned enrichment
from installed extension manifests and
.specify/extensions.yml.Depends on
specify artifactcommand spec-kit#4305 — addsspecify artifact(list/info) with deterministiccontribution IDs and per-row stack. This PR does not work at runtime
without it.
What changes
composition/artifact-cli.mjs(new source of command/template/scriptcomposition stacks): calls
specify artifact list --jsononce at boot, mapsrows into the wizard's artifact / preset / extension shapes, temporarily
enriches hook attribution from installed extension metadata, and produces
the composition summary.
project-scanner.mjs: no longer reads.specify/{presets,extensions}.jsonfor composition. Starts empty and lets
overlayCachedCompositionapply theCLI-derived data after the scan.
composition/pipeline-fast-path.mjs: decides between the deterministicpipeline (canonical spine +
replace-only overrides) and the LLM pipeline(
prompts/composition.mjs::inferPipeline) needed when an extension adds anon-canonical command or uses
wrap/prepend/append.tracker steps so the user sees which phase is running.
test/artifact-cli.integration.test.mjs)and its fixture — that layer belongs in the spec-kit repo alongside the CLI
it exercises. Unit tests here inject a fake runner so CI is green regardless
of which
specify-cliversion is installed.Verification
239/239 unit tests pass (unit tests inject a fake
specifyrunner).Playwright DOM-diff matrix — captured the wizard side panel from a main-branch
plugin variant vs. this branch across five scenarios:
copilot-sub-agentspresetpirate-full-presetagent-contextextension onlyEach scenario snapshots five surfaces (Composition → Commands / Templates /
Scripts / Hooks, plus top-level Phases). 25/25 pairs are byte-identical
after normalizing internal
[ref=...]handles. No user-visible regressions.Runtime behavior when the CLI is too old
Right now the wizard will surface an error and empty composition if the
installed
specify-clilacksartifact list --json. That's acceptable whilethis PR is draft; when we un-draft, the floor version bump in
speckit-cli-setupguarantees users get a compatible CLI.