fix(plugin): repair three --from-config install bugs

cmd/wfctl/multi_registry.go

@@ -57,25 +57,33 @@ 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 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(name)
+			manifest, err := src.FetchManifest(normalized)
 			if err == nil {
 				return manifest, src.Name(), nil
 			}

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{

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 <pluginDir>/payments. The skip check also uses the normalized name.
+	fakeInstalledPlugin(t, pluginDir, "payments", "1.0.0")
 
 	content := `
 requires:

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 <pluginDir>/auth, not
+		// <pluginDir>/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
@@ -101,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 {

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 <pluginDir>/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 <pluginDir>/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")

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].
@@ -649,22 +653,39 @@ func installFromLocal(srcDir, pluginDir string) error {
 // /releases/download/<new>/<filename>. 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