fix(plugin): repair three --from-config install bugs#457
Merged
Conversation
…ame collision, stale static registry) Bug 1 — double-v URL: pinManifestToVersion now normalises both versions by stripping a leading 'v' before comparison, so manifest version '0.6.1' and requested '@v0.6.1' are recognised as equal (no-op). When versions truly differ the fallback URL replacement uses the bare numeric forms, preventing 'v0.5.0' from becoming 'vv0.6.1'. Bug 2 — name collision: MultiRegistry.FetchManifest now tries the full original name first across all sources before falling back to the 'workflow-plugin-' stripped short name. This stops 'workflow-plugin-auth' resolving to the unrelated builtin 'auth' entry. installFromWorkflowConfig also normalises req.Name when computing the installDir skip-check so the already-installed detection matches the actual on-disk location. Bug 3 — dead static registry: DefaultRegistryConfig promotes the GitHub raw source to priority 0 (primary) and demotes the GitHub Pages mirror to priority 100 (fallback). The Pages site was returning 404, forcing every lookup through the fallback path that exacerbated Bug 2. Tests: added TestPinManifestToVersion_VPrefixMismatchSameVersion, TestPinManifestToVersion_CrossPrefixPin, TestMultiRegistryFetchOriginalNameFirst, TestMultiRegistryFetchNormalizedFallback, TestInstallFromConfig_NormalizedSkipCheck; updated registry config assertions and plugin_auth_test install-dir to match new normalised behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes multiple issues in wfctl plugin install --from-config by improving registry defaults, version pin URL rewriting, and registry name resolution behavior, along with targeted tests to prevent regressions.
Changes:
- Make the GitHub raw-content registry the default primary source, with GitHub Pages as a lower-priority mirror.
- Make
pinManifestToVersionv-prefix-tolerant to avoid corrupting GitHub release URLs (double-v). - Adjust multi-registry manifest lookup order (original name first, normalized fallback) and improve skip-check behavior for normalized install dirs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/wfctl/registry_config.go | Switch default registry ordering to GitHub primary + static mirror fallback. |
| cmd/wfctl/plugin_install.go | Make version pinning tolerant of v prefix mismatches and avoid double-v URL rewrites. |
| cmd/wfctl/multi_registry.go | Change manifest lookup order to prefer original name before normalized fallback. |
| cmd/wfctl/plugin_deps.go | Normalize plugin name when checking the on-disk install directory (skip check). |
| cmd/wfctl/plugin_version_pin_test.go | Add tests covering v-prefix mismatch and version pin URL rewrite behavior. |
| cmd/wfctl/plugin_from_config_test.go | Add test ensuring from-config skip-check uses normalized install dir. |
| cmd/wfctl/plugin_auth_test.go | Update test fixture to reflect normalized on-disk plugin install name. |
| cmd/wfctl/multi_registry_test.go | Update default registry expectations and add tests for original-name-first lookup. |
Comment on lines
67
to
86
| func (m *MultiRegistry) FetchManifest(name string) (*RegistryManifest, string, error) { | ||
| normalized := normalizePluginName(name) | ||
|
|
||
| // Try normalized name first across all sources. | ||
| // Try the original name first across all sources. | ||
| var lastErr error | ||
| for _, src := range m.sources { | ||
| manifest, err := src.FetchManifest(normalized) | ||
| manifest, err := src.FetchManifest(name) | ||
| if err == nil { | ||
| return manifest, src.Name(), nil | ||
| } | ||
| lastErr = err | ||
| } | ||
|
|
||
| // If normalized differs from original, try original name as fallback. | ||
| // If the normalized name differs from the original, retry with the short name. | ||
| // This allows users to omit the "workflow-plugin-" prefix (e.g. "auth" finds | ||
| // "workflow-plugin-auth" when no plugin named "auth" exists in any source). | ||
| if normalized != name { | ||
| for _, src := range m.sources { | ||
| manifest, err := src.FetchManifest(name) | ||
| manifest, err := src.FetchManifest(normalized) | ||
| if err == nil { |
There was a problem hiding this comment.
FetchManifest now prefers the original (possibly prefixed) name, but wfctl plugin install normalizes rawName before calling FetchManifest (plugin_install.go sets pluginName := normalizePluginName(rawName)), so installs triggered by --from-config will still look up auth even when the config explicitly requests workflow-plugin-auth — reintroducing the name-collision bug this change is meant to fix. Consider changing the install call site to pass the unnormalized name to FetchManifest (while still normalizing only for the on-disk install dir), or adjust the API so FetchManifest receives both original+normalized forms.
Comment on lines
+81
to
+82
| // This allows users to omit the "workflow-plugin-" prefix (e.g. "auth" finds | ||
| // "workflow-plugin-auth" when no plugin named "auth" exists in any source). |
There was a problem hiding this comment.
The fallback comment is misleading: this code only retries with the normalized short name when the caller provided a prefixed name (i.e., normalized != name). It does not allow an unprefixed name like auth to resolve a registry entry named workflow-plugin-auth. Update the comment to reflect the actual behavior to avoid confusion for future changes.
Suggested change
| // This allows users to omit the "workflow-plugin-" prefix (e.g. "auth" finds | |
| // "workflow-plugin-auth" when no plugin named "auth" exists in any source). | |
| // This only applies when the caller provided the prefixed form (for example, | |
| // "workflow-plugin-auth"), allowing a second lookup for "auth" if the prefixed | |
| // name was not found in any source. |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…kup engages plugin_install.go and plugin_deps.go (runPluginDeps) were normalizing rawName → pluginName before calling mr.FetchManifest, so "workflow-plugin-auth" arrived at MultiRegistry as "auth" and the original-name-first fix from the previous commit never fired. Fix: pass rawName directly to mr.FetchManifest; keep pluginName (normalized) for the on-disk install-directory path only. This lets MultiRegistry try "workflow-plugin-auth" in the registry before falling back to "auth", preventing the collision with the builtin auth module. Also tighten the comment in multi_registry.go line ~82 (Copilot review) to accurately describe the short-name fallback direction. New test: TestRunPluginInstall_FullNameNotNormalizedBeforeLookup — httptest server exposes both /plugins/workflow-plugin-auth/manifest.json (external v0.1.2, has downloads) and /plugins/auth/manifest.json (builtin v0.3.51, no downloads). The test asserts that installing "workflow-plugin-auth@v0.1.2" hits the external manifest and installs v0.1.2, not the builtin. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed both Copilot review comments: Comment 1 — regression (normalization before FetchManifest): Confirmed correct. Comment 2 — misleading comment (multi_registry.go ~line 82): Updated the comment from "auth finds workflow-plugin-auth" to accurately describe that the short-name fallback lets callers resolve a registry entry named All tests pass ( |
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
Fixes three bugs in `wfctl plugin install --from-config` that were exposed by BMW PR #147:
Why
BMW PR #147 replaced curl-based plugin installs in deploy.yml with `wfctl plugin install --from-config app.yaml`. The BMW Deploy run subsequently failed at the plugin install step because of the three bugs above. Without these fixes, the manifest-based install path is unusable.
Test plan
🤖 Generated with Claude Code