Consume deterministic lookupId to simplify wizard provider resolution - #20
Closed
nicolehaugen wants to merge 12 commits into
Conversation
…allback; add bucketing tests
…okAttributions migration
Contributor
There was a problem hiding this comment.
Pull request overview
Centralizes deterministic composition-layer lookup IDs for provider resolution and command source paths.
Changes:
- Adds lookup ID parsing and layer lookup helpers.
- Propagates lookup IDs through synthesized phases and provider calculations.
- Adds focused behavioral tests.
Show a summary per file
| File | Description |
|---|---|
ui/phase-runtime.js |
Migrates phase and source-path lookup logic. |
ui/phase-card.js |
Adds core phase lookup ID. |
ui/lookup-id.mjs |
Adds lookup ID helpers. |
ui/composition.js |
Resolves contribution providers from lookup IDs. |
test/phase-runtime-command-source-path.test.mjs |
Tests command path resolution. |
test/lookup-id.test.mjs |
Tests lookup ID helpers. |
test/compute-provider-contributions.test.mjs |
Tests provider bucketing. |
test/artifact-cli.test.mjs |
Adds behavioral layer lookup coverage. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+907
to
+908
| const parsedActive = parseLookupId(activeLayer?.lookupId); | ||
| const parsedPhase = parseLookupId(p.lookupId); |
…ommands
snapshot-builder.mjs's buildCommands forwards cmd.source ("preset:<id>")
onto snapshot.commands but never attaches a lookupId - that field only
exists on composition-artifact stack layers, a separate producer. The
lookupId-only fallback added in this PR made commandSourcePath return
null for real preset-only command tiles, breaking the on-disk source
link. Restore the string-parse fallback as a third tier after the
lookupId-based derivations, and add a scanner-to-buildStateSnapshot
integration test proving the real command shape resolves correctly.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 06f1c8e8-17cf-4754-ab7c-9f583bdb876f
Contributor
Author
|
Superseded by #21 — recreated as a same-repo PR (head branch pushed to github/spec-kit-copilot directly instead of a fork) so Copilot can work on it. Same commits, same base (nicolehaugen-wizard-composition-artifact-cli). |
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.
Summary
Consumes the deterministic
lookupIdfield carried on composition stack layers (preset:<presetId>:<kind>:<name>/extension:<extId>:<kind>:<name>/nullfor core) to replace ad-hoc string-parsing and multi-field OR fallbacks across the wizard with a single, well-tested parsing path.Stacked on this repo's wizard composition/artifact-cli PR — base ref is intentionally that branch, not
main.Changes (in migration order, each commit independently green)
ui/lookup-id.mjs— pureparseLookupId(str)andfindLayerByLookupId(compArtifact, lookupId)helpers.lookupActiveLayerintolookupActiveLayerForCommand(p)andlookupLayerByLookupId(lookupId); migratedcommandSourcePathto the new name.lookupIdadditively to phase objects at all phase-construction sites (core branch, extension branch,synthesizeCanonicalPhase).commandSourcePath's preset-path derivation from string-parsingp.sourcetoparseLookupId-based provider extraction (still preset-kind-only, matching prior behavior).computeProviderContributions's 4-way OR (presetId || extensionId || presetName || extensionName) toparseLookupId(layer.lookupId)?.providerId, falling back topresetId ?? extensionIdfor wizard-synthesized hook layers (which intentionally carrylookupId: null).test/artifact-cli.test.mjsto behavioral coverage.lookupActiveLayerdeprecation shim once its single caller was migrated.Out of scope (deliberately, tracked separately)
.specify/presets/<presetId>/commands/<commandName>.mdfilesystem-path construction incommandSourcePath.accumulateProvidesCounts/summarizeInstalled.applyHookAttributions, or synthesizing hooklookupIds client-side — hook layers keeplookupId: nulland rely on thepresetId ?? extensionIdfallback until the CLI exposes hook artifacts with real lookupIds and the wizard migrates offapplyHookAttributionsentirely.presetName/extensionNamedisplay fallbacks.p.sourcefield globally (kept as additive-legacy).Testing
npm test— 225/225 passing.test/lookup-id.test.mjs,test/phase-runtime-command-source-path.test.mjs,test/compute-provider-contributions.test.mjs, plus behavioral upgrades intest/artifact-cli.test.mjs. Each test targets a distinct code branch (reviewed for redundancy, one duplicate trimmed).