Consume deterministic lookupId to simplify wizard provider resolution - #21
Open
nicolehaugen wants to merge 13 commits into
Open
Conversation
…allback; add bucketing tests
…okAttributions migration
…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
There was a problem hiding this comment.
Pull request overview
Simplifies wizard provider resolution by consuming deterministic composition-layer lookupId values.
Changes:
- Adds shared lookup-ID parsing and layer lookup helpers.
- Propagates lookup IDs into phases and provider contribution logic.
- Adds focused unit and integration coverage.
Show a summary per file
| File | Description |
|---|---|
ui/phase-runtime.js |
Integrates lookup IDs into phase and command resolution. |
ui/phase-card.js |
Adds core-phase lookup provenance. |
ui/lookup-id.mjs |
Implements lookup-ID helpers. |
ui/composition.js |
Uses lookup IDs for provider contributions. |
test/server-integration.test.mjs |
Covers legacy command-source fallback. |
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: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>
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).