fix: address all 9 Copilot review comments on PR #331

- registry_source.go: switch GitHubRegistrySource to use registryHTTPClient
  (timeout-configured) for both ListPlugins and FetchManifest; update comment
- autofetch.go: scan for AutoFetch=true entries before exec.LookPath to avoid
  misleading startup warning when no plugins need auto-fetch
- autofetch.go: add internal autoFetchPlugin helper that accepts *slog.Logger
  and emits structured log entries when a logger is available; public
  AutoFetchPlugin delegates to it; AutoFetchDeclaredPlugins passes its logger
- autofetch_test.go: rename TestAutoFetchPlugin_CorrectArgs to
  TestAutoFetchPlugin_SkipsWhenExists to match what the test actually asserts
- integrity.go: replace os.ReadFile + sha256.Sum256 with streaming
  os.Open + io.Copy into sha256.New() to keep memory bounded for large binaries
- integrity_test.go: add t.Skip guard in UnreadableLockfile test when the file
  is actually readable (Windows / root environments)
- pipeline_step_workflow_call.go: use resolved workflowName (not s.workflow)
  in async return payload and sync error message for consistency
- docs/PLUGIN_AUTHORING.md: clarify the distinction between plugin.json name
  (short, used by engine) and the registry/provider manifest name
  (workflow-plugin- prefixed), and which is used for dependency resolution
- config/config.go: MergeApplicationConfig now merges Plugins.External from
  each referenced workflow file into the combined config, deduplicated by name

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: intel352

cmd/wfctl/registry_source.go

@@ -64,7 +64,7 @@ func (g *GitHubRegistrySource) ListPlugins() ([]string, error) {
 		req.Header.Set("Authorization", "Bearer "+token)
 	}
 
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := registryHTTPClient.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("list registry plugins from %s: %w", g.name, err)
 	}
@@ -90,7 +90,11 @@ func (g *GitHubRegistrySource) FetchManifest(name string) (*RegistryManifest, er
 		"https://raw.githubusercontent.com/%s/%s/%s/plugins/%s/manifest.json",
 		g.owner, g.repo, g.branch, name,
 	)
-	resp, err := http.Get(url) //nolint:gosec // URL constructed from configured registry
+	req, err := http.NewRequest(http.MethodGet, url, nil) //nolint:gosec // URL constructed from configured registry
+	if err != nil {
+		return nil, fmt.Errorf("build request: %w", err)
+	}
+	resp, err := registryHTTPClient.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("fetch manifest for %q from %s: %w", name, g.name, err)
 	}
@@ -226,7 +230,8 @@ func (s *StaticRegistrySource) ListPlugins() ([]string, error) {
 	return names, nil
 }
 
-// registryHTTPClient is used for all registry HTTP requests with a reasonable timeout.
+// registryHTTPClient is used for all registry HTTP requests (both GitHub and static
+// sources) with a reasonable timeout to avoid hangs on network issues.
 var registryHTTPClient = &http.Client{Timeout: 30 * time.Second}
 
 // fetch performs an HTTP GET with optional auth token.

config/config.go

@@ -419,6 +419,25 @@ func MergeApplicationConfig(appCfg *ApplicationConfig) (*WorkflowConfig, error)
 		}
 
 		combined.Modules = append(combined.Modules, wfCfg.Modules...)
+
+		// Merge external plugin declarations — deduplicate by name (first definition wins).
+		if wfCfg.Plugins != nil && len(wfCfg.Plugins.External) > 0 {
+			if combined.Plugins == nil {
+				combined.Plugins = &PluginsConfig{}
+			}
+			existingPlugins := make(map[string]struct{}, len(combined.Plugins.External))
+			for _, ep := range combined.Plugins.External {
+				existingPlugins[ep.Name] = struct{}{}
+			}
+			for _, ep := range wfCfg.Plugins.External {
+				if _, exists := existingPlugins[ep.Name]; exists {
+					continue
+				}
+				combined.Plugins.External = append(combined.Plugins.External, ep)
+				existingPlugins[ep.Name] = struct{}{}
+			}
+		}
+
 		for k, v := range wfCfg.Workflows {
 			if existing, exists := combined.Workflows[k]; exists {
 				// If the existing value is nil (e.g. `http:` with no body in YAML),

docs/PLUGIN_AUTHORING.md

@@ -101,7 +101,17 @@ func (p *Provider) CreateModule(typeName, name string, config map[string]any) (s
 
 ## Plugin Manifest
 
-The `plugin.json` declares what your plugin provides. The name should match what you passed to `wfctl plugin init`:
+The `plugin.json` at the project root declares what your plugin provides. The `name`
+field **must match the short name** you passed to `wfctl plugin init` (e.g. `my-plugin`).
+This is the name used by the engine for plugin discovery, the `requires.plugins` dependency
+check, and `wfctl plugin install`.
+
+> **Note:** The scaffolded `internal/provider.go` returns a manifest with the name prefixed
+> as `workflow-plugin-<short-name>` (e.g. `workflow-plugin-my-plugin`). That longer form is
+> the canonical name used in the **public registry** (`workflow-registry`) and in release
+> artifact URLs. When referencing your plugin in a workflow config's `requires.plugins` or
+> `plugins.external`, use the same short name you put in `plugin.json` — the engine resolves
+> both forms automatically.
 
 ```json
 {

module/pipeline_step_workflow_call.go

@@ -139,7 +139,7 @@ func (s *WorkflowCallStep) Execute(ctx context.Context, pc *PipelineContext) (*S
 			defer cancel()
 			_, _ = target.Execute(asyncCtx, data) //nolint:errcheck
 		}(ctx, triggerData)
-		return &StepResult{Output: map[string]any{"workflow": s.workflow, "mode": "async", "dispatched": true}}, nil
+		return &StepResult{Output: map[string]any{"workflow": workflowName, "mode": "async", "dispatched": true}}, nil
 	}
 
 	// Sync mode: apply timeout and wait for result
@@ -148,7 +148,7 @@ func (s *WorkflowCallStep) Execute(ctx context.Context, pc *PipelineContext) (*S
 
 	childCtx, err := target.Execute(syncCtx, triggerData)
 	if err != nil {
-		return nil, fmt.Errorf("workflow_call step %q: workflow %q failed: %w", s.name, s.workflow, err)
+		return nil, fmt.Errorf("workflow_call step %q: workflow %q failed: %w", s.name, workflowName, err)
 	}
 
 	// Map outputs back to parent context

plugin/autofetch.go

@@ -13,21 +13,37 @@ import (
 // It shells out to wfctl for the actual download/install logic.
 // version is an optional semver constraint (e.g., ">=0.1.0" or "0.2.0").
 func AutoFetchPlugin(pluginName, version, pluginDir string) error {
+	return autoFetchPlugin(pluginName, version, pluginDir, nil)
+}
+
+// autoFetchPlugin is the internal implementation that accepts an optional structured
+// logger. When logger is non-nil, status messages are emitted via slog instead of
+// writing directly to stderr.
+func autoFetchPlugin(pluginName, version, pluginDir string, logger *slog.Logger) error {
 	// Check both pluginName and workflow-plugin-<pluginName> (or the short form
 	// if pluginName already has the "workflow-plugin-" prefix).
 	if isPluginInstalled(pluginName, pluginDir) {
 		return nil
 	}
 
-	fmt.Fprintf(os.Stderr, "[auto-fetch] Plugin %q not found locally, fetching from registry...\n", pluginName)
+	if logger != nil {
+		logger.Info("plugin not found locally, fetching from registry", "plugin", pluginName)
+	} else {
+		fmt.Fprintf(os.Stderr, "[auto-fetch] Plugin %q not found locally, fetching from registry...\n", pluginName)
+	}
 
 	// Build install argument with version if specified.
 	installArg := pluginName
 	if version != "" {
 		stripped, ok := stripVersionConstraint(version)
 		if !ok {
 			// Complex constraint (e.g. ">=0.1.0,<0.2.0") — install latest instead.
-			fmt.Fprintf(os.Stderr, "[auto-fetch] Version constraint %q is complex; installing latest version of %q\n", version, pluginName)
+			if logger != nil {
+				logger.Warn("version constraint is complex; installing latest version",
+					"plugin", pluginName, "constraint", version)
+			} else {
+				fmt.Fprintf(os.Stderr, "[auto-fetch] Version constraint %q is complex; installing latest version of %q\n", version, pluginName)
+			}
 			stripped = ""
 		}
 		if stripped != "" {
@@ -118,7 +134,20 @@ func AutoFetchDeclaredPlugins(decls []AutoFetchDecl, pluginDir string, logger *s
 		return
 	}
 
-	// Check wfctl availability once.
+	// Scan for at least one AutoFetch=true entry before checking wfctl availability.
+	// This avoids a misleading warning on startup when no plugins require auto-fetch.
+	hasAutoFetch := false
+	for _, d := range decls {
+		if d.AutoFetch {
+			hasAutoFetch = true
+			break
+		}
+	}
+	if !hasAutoFetch {
+		return
+	}
+
+	// Check wfctl availability once — only needed when auto-fetch is actually requested.
 	if _, err := exec.LookPath("wfctl"); err != nil {
 		if logger != nil {
 			logger.Warn("wfctl not found on PATH; skipping auto-fetch for declared plugins",
@@ -134,7 +163,7 @@ func AutoFetchDeclaredPlugins(decls []AutoFetchDecl, pluginDir string, logger *s
 		}
 		// Record whether the plugin was already present before fetching.
 		alreadyPresent := isPluginInstalled(d.Name, pluginDir)
-		if err := AutoFetchPlugin(d.Name, d.Version, pluginDir); err != nil {
+		if err := autoFetchPlugin(d.Name, d.Version, pluginDir, logger); err != nil {
 			if logger != nil {
 				logger.Warn("auto-fetch failed for plugin", "plugin", d.Name, "error", err)
 			}

plugin/autofetch_test.go

@@ -81,11 +81,9 @@ func TestStripVersionConstraint(t *testing.T) {
 	}
 }
 
-// TestAutoFetchPlugin_CorrectArgs verifies that AutoFetchPlugin constructs the
-// expected wfctl arguments. We do this by ensuring the function short-circuits
-// when the plugin is already installed (not executing wfctl), which confirms
-// the plugin.json check is evaluated before any exec.Command call.
-func TestAutoFetchPlugin_CorrectArgs(t *testing.T) {
+// TestAutoFetchPlugin_SkipsWhenExists verifies that AutoFetchPlugin returns nil
+// immediately when the plugin is already installed, without invoking wfctl.
+func TestAutoFetchPlugin_SkipsWhenExists(t *testing.T) {
 	dir := t.TempDir()
 	pluginDir := filepath.Join(dir, "plugins")
 	pluginName := "test-plugin"

plugin/integrity.go

@@ -4,6 +4,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"
@@ -49,13 +50,17 @@ func VerifyPluginIntegrity(pluginDir, pluginName string) error {
 	}
 
 	binaryPath := filepath.Join(pluginDir, pluginName, pluginName)
-	binaryData, err := os.ReadFile(binaryPath)
+	f, err := os.Open(binaryPath)
 	if err != nil {
 		return fmt.Errorf("read plugin binary %s: %w", binaryPath, err)
 	}
+	defer f.Close()
 
-	h := sha256.Sum256(binaryData)
-	got := hex.EncodeToString(h[:])
+	h := sha256.New()
+	if _, err := io.Copy(h, f); err != nil {
+		return fmt.Errorf("hash plugin binary %s: %w", binaryPath, err)
+	}
+	got := hex.EncodeToString(h.Sum(nil))
 	if !strings.EqualFold(got, entry.SHA256) {
 		return fmt.Errorf("plugin %q integrity check failed: binary checksum %s does not match lockfile %s", pluginName, got, entry.SHA256)
 	}

plugin/integrity_test.go

@@ -57,6 +57,13 @@ func TestVerifyPluginIntegrity_UnreadableLockfile(t *testing.T) {
 		t.Fatalf("write lockfile: %v", err)
 	}
 
+	// POSIX permission bits are not enforced on all platforms (e.g. Windows, or
+	// when running as root). Verify the file is actually unreadable before
+	// asserting fail-closed behaviour.
+	if _, err := os.ReadFile(p); err == nil {
+		t.Skip("lockfile is readable despite 0000 permissions (platform or root); skipping test")
+	}
+
 	err := VerifyPluginIntegrity(filepath.Join(dir, "plugins"), "my-plugin")
 	if err == nil {
 		t.Error("expected error when lockfile is unreadable, got nil (fail-open)")