From c2ad5b08406e0964e2530b8e08f6a426062fc5ef Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 22 Apr 2026 02:57:05 -0400 Subject: [PATCH 1/2] fix(plugin): repair three --from-config install bugs (double-v URL, name collision, stale static registry) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/wfctl/multi_registry.go | 19 +++-- cmd/wfctl/multi_registry_test.go | 112 +++++++++++++++++++++------ cmd/wfctl/plugin_auth_test.go | 6 +- cmd/wfctl/plugin_deps.go | 8 +- cmd/wfctl/plugin_from_config_test.go | 22 ++++++ cmd/wfctl/plugin_install.go | 31 ++++++-- cmd/wfctl/plugin_version_pin_test.go | 78 +++++++++++++++++++ cmd/wfctl/registry_config.go | 19 +++-- 8 files changed, 247 insertions(+), 48 deletions(-) diff --git a/cmd/wfctl/multi_registry.go b/cmd/wfctl/multi_registry.go index d637234a..04428fc3 100644 --- a/cmd/wfctl/multi_registry.go +++ b/cmd/wfctl/multi_registry.go @@ -57,25 +57,32 @@ func normalizePluginName(name string) string { } // FetchManifest tries each source in priority order, returning the first successful result. -// It first tries the normalized name (stripping "workflow-plugin-" prefix); if the -// normalized name differs from the original, it also tries the original name as a fallback. +// It first tries the original name across all sources; if the original name differs from +// its normalized form (after stripping the "workflow-plugin-" prefix) and no source +// matched the original, it retries with the normalized name as a fallback. +// +// Trying the original name first prevents name collisions where both "auth" (a builtin +// module plugin) and "workflow-plugin-auth" (an external plugin) exist in the registry — +// the caller's intent is respected rather than conflating the two. 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 { return manifest, src.Name(), nil } diff --git a/cmd/wfctl/multi_registry_test.go b/cmd/wfctl/multi_registry_test.go index ec1a7e7e..646d87c8 100644 --- a/cmd/wfctl/multi_registry_test.go +++ b/cmd/wfctl/multi_registry_test.go @@ -106,36 +106,36 @@ func TestDefaultRegistryConfig(t *testing.T) { if len(cfg.Registries) != 2 { t.Fatalf("expected 2 registries, got %d", len(cfg.Registries)) } - // Primary: static registry + // Primary: github registry (raw.githubusercontent.com — reliable and always current). r := cfg.Registries[0] if r.Name != "default" { t.Errorf("name: got %q, want %q", r.Name, "default") } - if r.Type != "static" { - t.Errorf("type: got %q, want %q", r.Type, "static") + if r.Type != "github" { + t.Errorf("type: got %q, want %q", r.Type, "github") } - if r.URL == "" { - t.Error("expected non-empty URL for static registry") + if r.Owner != registryOwner { + t.Errorf("owner: got %q, want %q", r.Owner, registryOwner) + } + if r.Repo != registryRepo { + t.Errorf("repo: got %q, want %q", r.Repo, registryRepo) + } + if r.Branch != registryBranch { + t.Errorf("branch: got %q, want %q", r.Branch, registryBranch) } if r.Priority != 0 { t.Errorf("priority: got %d, want 0", r.Priority) } - // Fallback: github registry + // Secondary fallback: static mirror (GitHub Pages CDN — lower priority). fb := cfg.Registries[1] - if fb.Name != "github-fallback" { - t.Errorf("fallback name: got %q, want %q", fb.Name, "github-fallback") - } - if fb.Type != "github" { - t.Errorf("fallback type: got %q, want %q", fb.Type, "github") + if fb.Name != "static-mirror" { + t.Errorf("fallback name: got %q, want %q", fb.Name, "static-mirror") } - if fb.Owner != registryOwner { - t.Errorf("fallback owner: got %q, want %q", fb.Owner, registryOwner) + if fb.Type != "static" { + t.Errorf("fallback type: got %q, want %q", fb.Type, "static") } - if fb.Repo != registryRepo { - t.Errorf("fallback repo: got %q, want %q", fb.Repo, registryRepo) - } - if fb.Branch != registryBranch { - t.Errorf("fallback branch: got %q, want %q", fb.Branch, registryBranch) + if fb.URL == "" { + t.Error("expected non-empty URL for static-mirror registry") } if fb.Priority != 100 { t.Errorf("fallback priority: got %d, want 100", fb.Priority) @@ -187,13 +187,18 @@ func TestLoadRegistryConfigDefault(t *testing.T) { // Test DefaultRegistryConfig directly. cfg := DefaultRegistryConfig() if len(cfg.Registries) != 2 { - t.Fatalf("expected 2 registries (static + github fallback), got %d", len(cfg.Registries)) + t.Fatalf("expected 2 registries (github primary + static mirror), got %d", len(cfg.Registries)) } - if cfg.Registries[0].Type != "static" { - t.Errorf("first registry type: got %q, want %q", cfg.Registries[0].Type, "static") + // Primary must be the github source. + if cfg.Registries[0].Type != "github" { + t.Errorf("first registry type: got %q, want %q", cfg.Registries[0].Type, "github") } - if cfg.Registries[1].Owner != registryOwner { - t.Errorf("fallback owner: got %q, want %q", cfg.Registries[1].Owner, registryOwner) + if cfg.Registries[0].Owner != registryOwner { + t.Errorf("primary owner: got %q, want %q", cfg.Registries[0].Owner, registryOwner) + } + // Secondary must be the static mirror. + if cfg.Registries[1].Type != "static" { + t.Errorf("second registry type: got %q, want %q", cfg.Registries[1].Type, "static") } } @@ -212,7 +217,11 @@ func TestLoadRegistryConfigFallback(t *testing.T) { t.Fatalf("LoadRegistryConfig: %v", err) } if len(cfg.Registries) != 2 { - t.Fatalf("expected 2 registries (default fallback), got %d", len(cfg.Registries)) + t.Fatalf("expected 2 registries (github primary + static mirror), got %d", len(cfg.Registries)) + } + // Primary must be the github source (reliable raw.githubusercontent.com access). + if cfg.Registries[0].Type != "github" { + t.Errorf("first registry type: got %q, want %q (github should be primary)", cfg.Registries[0].Type, "github") } } @@ -362,6 +371,61 @@ func TestMultiRegistryFetchPriority(t *testing.T) { } } +// TestMultiRegistryFetchOriginalNameFirst verifies that FetchManifest tries the +// full original name before the normalized short name. This prevents "workflow-plugin-auth" +// from colliding with a builtin "auth" plugin: if both exist in the registry, +// the full-name entry is returned rather than the builtin. +func TestMultiRegistryFetchOriginalNameFirst(t *testing.T) { + // srcA has both "auth" (builtin, wrong one) and "workflow-plugin-auth" (correct). + srcA := &mockRegistrySource{ + name: "registry", + manifests: map[string]*RegistryManifest{ + "auth": {Name: "auth", Version: "0.3.51", Description: "builtin auth module"}, + "workflow-plugin-auth": {Name: "workflow-plugin-auth", Version: "0.1.2", Description: "external auth plugin"}, + }, + } + + mr := NewMultiRegistryFromSources(srcA) + + // Requesting "workflow-plugin-auth" should return the external plugin, NOT the builtin. + manifest, source, err := mr.FetchManifest("workflow-plugin-auth") + if err != nil { + t.Fatalf("FetchManifest: %v", err) + } + _ = source + if manifest.Version != "0.1.2" { + t.Errorf("version: got %q, want %q (should return workflow-plugin-auth, not builtin auth)", + manifest.Version, "0.1.2") + } + if manifest.Name != "workflow-plugin-auth" { + t.Errorf("name: got %q, want %q", manifest.Name, "workflow-plugin-auth") + } +} + +// TestMultiRegistryFetchNormalizedFallback verifies that when the full name is not +// found in any source, the normalized short name is used as a fallback. This allows +// users to omit the "workflow-plugin-" prefix in their config. +func TestMultiRegistryFetchNormalizedFallback(t *testing.T) { + // srcA only has the short name "auth" (no full-name entry). + srcA := &mockRegistrySource{ + name: "registry", + manifests: map[string]*RegistryManifest{ + "auth": {Name: "auth", Version: "1.0.0"}, + }, + } + + mr := NewMultiRegistryFromSources(srcA) + + // "workflow-plugin-auth" not found under full name → falls back to "auth". + manifest, _, err := mr.FetchManifest("workflow-plugin-auth") + if err != nil { + t.Fatalf("FetchManifest: %v", err) + } + if manifest.Version != "1.0.0" { + t.Errorf("version: got %q, want %q", manifest.Version, "1.0.0") + } +} + func TestMultiRegistryFetchFallback(t *testing.T) { // Source A errors for "unique-plugin", source B has it. srcA := &mockRegistrySource{ diff --git a/cmd/wfctl/plugin_auth_test.go b/cmd/wfctl/plugin_auth_test.go index fe921844..431c92a6 100644 --- a/cmd/wfctl/plugin_auth_test.go +++ b/cmd/wfctl/plugin_auth_test.go @@ -52,8 +52,10 @@ func TestInstallFromConfig_WithAuth_SkipsInstalledPrivate(t *testing.T) { dir := t.TempDir() pluginDir := filepath.Join(dir, "plugins") - // Pre-install the private plugin. - fakeInstalledPlugin(t, pluginDir, "workflow-plugin-payments", "1.0.0") + // Pre-install the private plugin using the normalized name ("payments"), since + // runPluginInstall normalizes "workflow-plugin-payments" → "payments" and + // installs to /payments. The skip check also uses the normalized name. + fakeInstalledPlugin(t, pluginDir, "payments", "1.0.0") content := ` requires: diff --git a/cmd/wfctl/plugin_deps.go b/cmd/wfctl/plugin_deps.go index 848dd471..cf49eb7e 100644 --- a/cmd/wfctl/plugin_deps.go +++ b/cmd/wfctl/plugin_deps.go @@ -25,7 +25,13 @@ func installFromWorkflowConfig(workflowCfgPath, pluginDir, registryCfgPath strin var failed []string for _, req := range cfg.Requires.Plugins { - installDir := filepath.Join(pluginDir, req.Name) + // Normalize the name before checking the install directory so the skip check + // matches the actual install location. runPluginInstall normalizes names via + // normalizePluginName (stripping "workflow-plugin-" prefix), so + // "workflow-plugin-auth" is installed at /auth, not + // /workflow-plugin-auth. + normalizedName := normalizePluginName(req.Name) + installDir := filepath.Join(pluginDir, normalizedName) if ver := readInstalledVersion(installDir); ver != "" && ver != "unknown" { fmt.Fprintf(os.Stderr, "%s v%s already installed, skipping.\n", req.Name, ver) continue diff --git a/cmd/wfctl/plugin_from_config_test.go b/cmd/wfctl/plugin_from_config_test.go index 1647c61d..fbc9dcd7 100644 --- a/cmd/wfctl/plugin_from_config_test.go +++ b/cmd/wfctl/plugin_from_config_test.go @@ -90,6 +90,28 @@ func TestInstallFromConfig_FlagWired(t *testing.T) { } } +// TestInstallFromConfig_NormalizedSkipCheck verifies that when requires.plugins +// lists "workflow-plugin-auth", the already-installed check uses the normalized +// name ("auth") so it correctly detects the plugin as installed — rather than +// looking in /workflow-plugin-auth and always thinking it needs install. +func TestInstallFromConfig_NormalizedSkipCheck(t *testing.T) { + dir := t.TempDir() + pluginDir := filepath.Join(dir, "plugins") + + // runPluginInstall normalizes "workflow-plugin-auth" → "auth" and installs + // to /auth. Pre-create that directory so the skip check fires. + fakeInstalledPlugin(t, pluginDir, "auth", "0.1.2") + + cfgPath := writeWorkflowWithPlugins(t, dir, []struct{ name, version string }{ + {"workflow-plugin-auth", "0.1.2"}, + }) + + // Should skip without error (no network call needed). + if err := installFromWorkflowConfig(cfgPath, pluginDir, ""); err != nil { + t.Fatalf("installFromWorkflowConfig: %v", err) + } +} + func TestInstallFromConfig_MissingConfigFile(t *testing.T) { dir := t.TempDir() pluginDir := filepath.Join(dir, "plugins") diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index 86152116..525d1875 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -649,22 +649,39 @@ func installFromLocal(srcDir, pluginDir string) error { // /releases/download//. SHA256 checksums are cleared since they are // only valid for the original version's assets. // -// If requestedVersion matches manifest.Version, this is a no-op. +// Version comparison is v-prefix-tolerant: "v0.6.1" and "0.6.1" are treated as +// the same version, so passing @v0.6.1 when the registry manifest has "0.6.1" is +// a no-op rather than a rewrite that would corrupt download URLs. +// +// If requestedVersion matches manifest.Version (after v-normalization), this is a no-op. func pinManifestToVersion(manifest *RegistryManifest, requestedVersion string) { - if requestedVersion == manifest.Version { - return + // Normalize both versions by stripping the leading "v" for comparison. + // This prevents treating "0.6.1" and "v0.6.1" as different versions, which + // would corrupt download URLs by producing "vv0.6.1" via the fallback replacement. + normalizedOld := strings.TrimPrefix(manifest.Version, "v") + normalizedNew := strings.TrimPrefix(requestedVersion, "v") + if normalizedOld == normalizedNew { + return // same version, v-prefix convention mismatch only } oldVersion := manifest.Version manifest.Version = requestedVersion for i := range manifest.Downloads { url := manifest.Downloads[i].URL - // Replace the release tag in the GitHub releases download path. + // 1. Try replacing the exact manifest version string in the GitHub releases path. rewritten := strings.ReplaceAll(url, "/releases/download/"+oldVersion+"/", "/releases/download/"+requestedVersion+"/") - // If the version string also appears in the filename, rewrite that too. - if rewritten == url && oldVersion != "" { - rewritten = strings.ReplaceAll(url, oldVersion, requestedVersion) + // 2. If no match, try v-normalized replacement. This handles the common case + // where the manifest stores "0.6.1" but the GitHub release tag is "v0.6.1". + if rewritten == url { + rewritten = strings.ReplaceAll(url, + "/releases/download/v"+normalizedOld+"/", + "/releases/download/v"+normalizedNew+"/") + } + // 3. Fallback: replace the bare version number anywhere in the URL, using + // normalized (no-v) forms so we don't double-up the "v" prefix. + if rewritten == url && normalizedOld != "" { + rewritten = strings.ReplaceAll(url, normalizedOld, normalizedNew) } manifest.Downloads[i].URL = rewritten manifest.Downloads[i].SHA256 = "" // checksums are for the old version's assets diff --git a/cmd/wfctl/plugin_version_pin_test.go b/cmd/wfctl/plugin_version_pin_test.go index cd35ddd9..89880d8d 100644 --- a/cmd/wfctl/plugin_version_pin_test.go +++ b/cmd/wfctl/plugin_version_pin_test.go @@ -12,6 +12,84 @@ import ( "testing" ) +// TestPinManifestToVersion_VPrefixMismatchSameVersion verifies that when the +// registry manifest stores a version without a "v" prefix (e.g. "0.6.1") but the +// user requests the same version with a "v" prefix (e.g. "@v0.6.1"), pinManifestToVersion +// treats them as equal and makes no changes — preventing the double-v bug where +// the fallback replacement would turn ".../v0.6.1/..." into ".../vv0.6.1/...". +func TestPinManifestToVersion_VPrefixMismatchSameVersion(t *testing.T) { + origURL := "https://github.com/owner/repo/releases/download/v0.6.1/plugin-linux-amd64.tar.gz" + manifest := &RegistryManifest{ + Name: "auth", + Version: "0.6.1", // registry stores without v prefix + Downloads: []PluginDownload{ + {OS: "linux", Arch: "amd64", URL: origURL, SHA256: "checksum"}, + }, + } + + // User passes @v0.6.1 — same version, just different prefix convention. + pinManifestToVersion(manifest, "v0.6.1") + + // Version should NOT have been changed (they're the same version). + // More importantly: URL must not contain "vv0.6.1". + if strings.Contains(manifest.Downloads[0].URL, "vv0.6.1") { + t.Errorf("double-v bug: URL contains %q: %s", "vv0.6.1", manifest.Downloads[0].URL) + } + if manifest.Downloads[0].URL != origURL { + t.Errorf("URL should be unchanged for same version: got %q, want %q", + manifest.Downloads[0].URL, origURL) + } + // SHA256 should NOT be cleared because no rewrite happened. + if manifest.Downloads[0].SHA256 == "" { + t.Error("SHA256 should not be cleared when version is unchanged") + } +} + +// TestPinManifestToVersion_CrossPrefixPin verifies that when the manifest stores +// a version without "v" (e.g. "0.5.0") and the URL has "v0.5.0", pinning to a +// new version (e.g. "v0.6.1") correctly rewrites the URL to "v0.6.1" without +// introducing a double-v or losing the prefix. +func TestPinManifestToVersion_CrossPrefixPin(t *testing.T) { + manifest := &RegistryManifest{ + Name: "auth", + Version: "0.5.0", // registry stores without v prefix + Downloads: []PluginDownload{ + { + OS: "linux", + Arch: "amd64", + // URL uses the standard GitHub release tag format (with v prefix). + URL: "https://github.com/owner/repo/releases/download/v0.5.0/auth-linux-amd64.tar.gz", + SHA256: "oldchecksum", + }, + { + OS: "darwin", + Arch: "arm64", + URL: "https://github.com/owner/repo/releases/download/v0.5.0/auth-darwin-arm64.tar.gz", + }, + }, + } + + pinManifestToVersion(manifest, "v0.6.1") + + if manifest.Version != "v0.6.1" { + t.Errorf("manifest.Version: got %q, want %q", manifest.Version, "v0.6.1") + } + for i, dl := range manifest.Downloads { + if strings.Contains(dl.URL, "vv0.6.1") { + t.Errorf("download[%d]: double-v bug in URL: %s", i, dl.URL) + } + if !strings.Contains(dl.URL, "v0.6.1") { + t.Errorf("download[%d]: URL should contain v0.6.1: %s", i, dl.URL) + } + if strings.Contains(dl.URL, "0.5.0") { + t.Errorf("download[%d]: URL still contains old version 0.5.0: %s", i, dl.URL) + } + if dl.SHA256 != "" { + t.Errorf("download[%d]: SHA256 should be cleared after version pin, got %q", i, dl.SHA256) + } + } +} + // TestPinManifestToVersion_URLRewritten verifies that pinManifestToVersion // replaces the old version string in download URLs and updates manifest.Version. func TestPinManifestToVersion_URLRewritten(t *testing.T) { diff --git a/cmd/wfctl/registry_config.go b/cmd/wfctl/registry_config.go index 3d78fc85..7c05d613 100644 --- a/cmd/wfctl/registry_config.go +++ b/cmd/wfctl/registry_config.go @@ -26,23 +26,26 @@ type RegistrySourceConfig struct { Token string `yaml:"token" json:"token"` // Auth token (optional) } -// DefaultRegistryConfig returns the built-in config with a static GitHub Pages -// primary registry and a GitHub API fallback. +// DefaultRegistryConfig returns the built-in registry config. +// The GitHub raw source is the primary registry (priority 0) because it serves +// manifest.json files directly from the repo and is always up to date. +// The static GitHub Pages source is a secondary fallback (priority 100) for +// organisations that mirror the registry to a CDN for lower-latency reads. func DefaultRegistryConfig() *RegistryConfig { return &RegistryConfig{ Registries: []RegistrySourceConfig{ { Name: "default", - Type: "static", - URL: "https://gocodealone.github.io/workflow-registry/v1", - Priority: 0, - }, - { - Name: "github-fallback", Type: "github", Owner: registryOwner, Repo: registryRepo, Branch: registryBranch, + Priority: 0, + }, + { + Name: "static-mirror", + Type: "static", + URL: "https://gocodealone.github.io/workflow-registry/v1", Priority: 100, }, }, From f1f6a5a1faa17f2c87ceec34f75e39a91a75329b Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 22 Apr 2026 03:15:29 -0400 Subject: [PATCH 2/2] fix(plugin): pass rawName to FetchManifest so original-name-first lookup engages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/wfctl/multi_registry.go | 7 +- cmd/wfctl/plugin_deps.go | 6 +- cmd/wfctl/plugin_install.go | 8 +- cmd/wfctl/plugin_version_pin_test.go | 111 +++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 7 deletions(-) diff --git a/cmd/wfctl/multi_registry.go b/cmd/wfctl/multi_registry.go index 04428fc3..91be4bc9 100644 --- a/cmd/wfctl/multi_registry.go +++ b/cmd/wfctl/multi_registry.go @@ -77,9 +77,10 @@ func (m *MultiRegistry) FetchManifest(name string) (*RegistryManifest, string, e lastErr = err } - // 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 the original name was not found and the normalized short name differs, + // retry with the short name. This lets callers omit the "workflow-plugin-" + // prefix (e.g. passing "auth" resolves to the registry entry named "auth" + // when no entry named "auth" exists under the full original name). if normalized != name { for _, src := range m.sources { manifest, err := src.FetchManifest(normalized) diff --git a/cmd/wfctl/plugin_deps.go b/cmd/wfctl/plugin_deps.go index cf49eb7e..09a98f31 100644 --- a/cmd/wfctl/plugin_deps.go +++ b/cmd/wfctl/plugin_deps.go @@ -107,9 +107,11 @@ func runPluginDeps(args []string) error { } mr := NewMultiRegistry(cfg) - manifest, _, err := mr.FetchManifest(pluginName) + // Pass rawName (original, un-normalized) to FetchManifest so the original- + // name-first lookup in MultiRegistry can engage before the short-name fallback. + manifest, _, err := mr.FetchManifest(rawName) if err != nil { - return fmt.Errorf("fetch manifest for %q: %w", pluginName, err) + return fmt.Errorf("fetch manifest for %q: %w", rawName, err) } if len(manifest.Dependencies) == 0 { diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index 525d1875..c5c28558 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -142,8 +142,12 @@ func runPluginInstall(args []string) error { mr = NewMultiRegistry(cfg) } - fmt.Fprintf(os.Stderr, "Fetching manifest for %q...\n", pluginName) - manifest, sourceName, registryErr := mr.FetchManifest(pluginName) + // Pass rawName (the original, un-normalized name) to FetchManifest so that + // "workflow-plugin-auth" is tried first in the registry before falling back + // to the normalized short name "auth". pluginName (normalized) is used only + // for the on-disk install directory path. + fmt.Fprintf(os.Stderr, "Fetching manifest for %q...\n", rawName) + manifest, sourceName, registryErr := mr.FetchManifest(rawName) if registryErr != nil { // Registry lookup failed. Try GitHub direct install if input looks like owner/repo[@version]. diff --git a/cmd/wfctl/plugin_version_pin_test.go b/cmd/wfctl/plugin_version_pin_test.go index 89880d8d..a360ab15 100644 --- a/cmd/wfctl/plugin_version_pin_test.go +++ b/cmd/wfctl/plugin_version_pin_test.go @@ -406,3 +406,114 @@ func TestRunPluginInstall_VersionPinNotFound(t *testing.T) { t.Errorf("error should mention requested version v99.99.99, got: %v", err) } } + +// TestRunPluginInstall_FullNameNotNormalizedBeforeLookup verifies that when +// runPluginInstall is given "workflow-plugin-auth@v0.1.2" the full name +// "workflow-plugin-auth" is sent to the registry rather than the normalized +// "auth". This prevents collisions where a builtin "auth" entry would be +// returned instead of the external workflow-plugin-auth plugin. +// +// The test server exposes two manifest paths: +// - /plugins/workflow-plugin-auth/manifest.json → external auth v0.1.2 +// - /plugins/auth/manifest.json → builtin auth v0.3.51 (wrong one) +// +// Only the external v0.1.2 tarball exists; hitting the builtin path would +// fail with "no download for linux/amd64" because it has no Downloads entry. +func TestRunPluginInstall_FullNameNotNormalizedBeforeLookup(t *testing.T) { + const externalVersion = "v0.1.2" + const builtinVersion = "v0.3.51" + + binaryContent := []byte("#!/bin/sh\necho auth\n") + // Tarball uses the normalized name "auth" as the binary name (as GoReleaser would). + tarball := buildPluginTarGz(t, "auth", binaryContent, minimalPluginJSON("auth", externalVersion)) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/plugins/workflow-plugin-auth/manifest.json": + // The correct external plugin. + m := RegistryManifest{ + Name: "workflow-plugin-auth", + Version: externalVersion, + Author: "tester", + Description: "external auth plugin", + Type: "external", + Tier: "community", + License: "MIT", + Downloads: []PluginDownload{ + { + OS: runtime.GOOS, + Arch: runtime.GOARCH, + URL: "http://" + r.Host + "/releases/download/" + externalVersion + "/auth.tar.gz", + }, + }, + } + data, _ := json.Marshal(m) + w.Header().Set("Content-Type", "application/json") + w.Write(data) //nolint:errcheck + + case "/plugins/auth/manifest.json": + // The builtin plugin — no Downloads, so installing it would fail. + m := RegistryManifest{ + Name: "auth", + Version: builtinVersion, + Author: "engine", + Description: "builtin auth module", + Type: "builtin", + Tier: "core", + License: "MIT", + // No Downloads — installing this would return "no download for OS/arch". + } + data, _ := json.Marshal(m) + w.Header().Set("Content-Type", "application/json") + w.Write(data) //nolint:errcheck + + case "/releases/download/" + externalVersion + "/auth.tar.gz": + w.WriteHeader(http.StatusOK) + w.Write(tarball) //nolint:errcheck + + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + cfgDir := t.TempDir() + regCfg := "registries:\n - name: test\n type: static\n url: " + srv.URL + "\n priority: 0\n" + regCfgPath := filepath.Join(cfgDir, "registry.yaml") + if err := os.WriteFile(regCfgPath, []byte(regCfg), 0600); err != nil { + t.Fatalf("write registry config: %v", err) + } + + origWD, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + if err := os.Chdir(t.TempDir()); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { os.Chdir(origWD) }) //nolint:errcheck + + pluginsDir := t.TempDir() + if err := runPluginInstall([]string{ + "--config", regCfgPath, + "--plugin-dir", pluginsDir, + "workflow-plugin-auth@" + externalVersion, + }); err != nil { + t.Fatalf("runPluginInstall workflow-plugin-auth: %v", err) + } + + // Plugin should be installed under the normalized name "auth". + pjPath := filepath.Join(pluginsDir, "auth", "plugin.json") + data, err := os.ReadFile(pjPath) + if err != nil { + t.Fatalf("read plugin.json: %v — did the builtin path get hit instead?", err) + } + var pj installedPluginJSON + if err := json.Unmarshal(data, &pj); err != nil { + t.Fatalf("parse plugin.json: %v", err) + } + if pj.Version != externalVersion { + t.Errorf("installed version: got %q, want %q (builtin %q would indicate name-collision bug)", + pj.Version, externalVersion, builtinVersion) + } +}