feat(wfctl): lockfile dep tracking + always-track plain installs (workflow#771)

cmd/wfctl/plugin_deps.go

@@ -108,7 +108,27 @@ func installPluginReqDirect(pluginDir, registryCfgPath string, req config.Plugin
 		}
 	}
 
-	return installPluginFromManifest(pluginDir, pluginName, manifest, req.Verify, false)
+	if err := installPluginFromManifest(pluginDir, pluginName, manifest, req.Verify, false); err != nil {
+		return err
+	}
+
+	// Track parent in lockfile (workflow#771 Task 5). Closes the asymmetry
+	// where --from-config dep installs were tracked via Task 4 but parent
+	// installs via this path were not.
+	// Cycle-3 fix per reviewer CYC2-I1: use the function's NORMALIZED pluginName
+	// (already computed at line 87 via normalizePluginName(rawName)), NOT raw
+	// req.Name. installPluginFromManifest installs at pluginDir/<normalized>/<normalized>;
+	// raw req.Name "workflow-plugin-auth" would hash-miss at pluginDir/workflow-plugin-auth/...
+	// and produce an asymmetric lockfile key vs runPluginInstall's writes.
+	binaryPath := filepath.Join(pluginDir, pluginName, pluginName)
+	checksum := ""
+	if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil {
+		checksum = cs
+	} else {
+		fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr)
+	}
+	updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, "", checksum)
+	return nil
 }
 
 // runPluginDeps lists dependencies for a plugin without installing them.
@@ -269,6 +289,26 @@ func resolveDependencies(
 			return fmt.Errorf("install dependency %q of %q: %w", dep.Name, pluginName, err)
 		}
 		resolved[dep.Name] = depManifest.Version
+
+		// Track dep in lockfile (workflow#771 Task 4). The chokepoint guard
+		// inside updateLockfileWithChecksum (Task 1) suppresses writes when
+		// running under an outer-frame installer (installFromLockfile etc.).
+		// Cycle-4 reviewer I1 Option-(b): use raw dep.Name for ALL three sites
+		// (install dir, hash path, lockfile key) — matches the un-normalized
+		// install above (`installPluginFromManifest(pluginDir, dep.Name, ...)`).
+		// Normalizing only the lockfile-side without changing install-side
+		// produces hash-MISS warnings + empty checksums for long-form dep names.
+		// Parent (Task 5) keeps normalize because runPluginInstall:257 normalizes
+		// install-side too — symmetric for that path. Asymmetric across Task 4 vs 5
+		// is a pre-existing convention difference, not regressed by this PR.
+		depBinaryPath := filepath.Join(pluginDir, dep.Name, dep.Name)
+		depChecksum := ""
+		if cs, hashErr := hashFileSHA256(depBinaryPath); hashErr == nil {
+			depChecksum = cs
+		} else {
+			fmt.Fprintf(os.Stderr, "warning: could not hash dep binary %s: %v (lockfile will have no checksum)\n", depBinaryPath, hashErr)
+		}
+		updateLockfileWithChecksum(dep.Name, depManifest.Version, depManifest.Repository, "", depChecksum)
 	}
 	return nil
 }

cmd/wfctl/plugin_deps_test.go

@@ -7,7 +7,11 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
+	"time"
+
+	"github.com/GoCodeAlone/workflow/config"
 )
 
 // TestCompareSemverConstraints verifies semver comparison used in version constraint checks.
@@ -341,6 +345,193 @@ func TestPluginInstall_ResolveDependencies(t *testing.T) {
 	}
 }
 
+// TestResolveDependencies_TracksDepsInLockfile verifies workflow#771 Task 4:
+// when resolveDependencies installs a transitive dep, the lockfile receives an
+// entry for that dep (provided no outer-frame installer suppressed writes).
+func TestResolveDependencies_TracksDepsInLockfile(t *testing.T) {
+	pluginDir := t.TempDir()
+
+	origWD, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("getwd: %v", err)
+	}
+	cwd := t.TempDir()
+	if err := os.Chdir(cwd); err != nil {
+		t.Fatalf("chdir: %v", err)
+	}
+	t.Cleanup(func() { os.Chdir(origWD) }) //nolint:errcheck
+
+	// Seed an empty v1 lockfile so the chokepoint fan-out fires.
+	emptyLF := &config.WfctlLockfile{
+		Version:     1,
+		GeneratedAt: time.Now(),
+		Plugins:     map[string]config.WfctlLockPluginEntry{},
+	}
+	if err := config.SaveWfctlLockfile(wfctlLockPath, emptyLF); err != nil {
+		t.Fatal(err)
+	}
+
+	const depName = "depa"
+	binaryContent := []byte("#!/bin/sh\necho depa\n")
+	pjContent := minimalPluginJSON(depName, "0.5.0")
+	tarball := buildPluginTarGz(t, depName, binaryContent, pjContent)
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/plugins/" + depName + "/manifest.json":
+			manifest := RegistryManifest{
+				Name:        depName,
+				Version:     "0.5.0",
+				Repository:  "github.com/x/" + depName,
+				Author:      "tester",
+				Description: "depa",
+				Type:        "external",
+				Tier:        "community",
+				License:     "MIT",
+				Downloads: []PluginDownload{
+					{
+						OS:     runtime.GOOS,
+						Arch:   runtime.GOARCH,
+						URL:    "http://" + r.Host + "/download/" + depName + ".tar.gz",
+						SHA256: sha256Hex(tarball),
+					},
+				},
+			}
+			w.Header().Set("Content-Type", "application/json")
+			_ = json.NewEncoder(w).Encode(manifest)
+		case "/download/" + depName + ".tar.gz":
+			w.Header().Set("Content-Type", "application/octet-stream")
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write(tarball)
+		default:
+			http.NotFound(w, r)
+		}
+	}))
+	defer srv.Close()
+
+	cfgFile := writeTestRegistryConfig(t, srv.URL)
+
+	parentManifest := &RegistryManifest{
+		Name:    "parent",
+		Version: "1.0.0",
+		Dependencies: []PluginDependency{
+			{Name: depName, MinVersion: "0.1.0"},
+		},
+	}
+
+	resolved := make(map[string]string)
+	if err := resolveDependencies("parent", parentManifest, pluginDir, cfgFile, []string{}, resolved); err != nil {
+		t.Fatalf("resolveDependencies: %v", err)
+	}
+	if resolved[depName] != "0.5.0" {
+		t.Errorf("resolved[%s] = %q, want 0.5.0", depName, resolved[depName])
+	}
+
+	lf, err := config.LoadWfctlLockfile(wfctlLockPath)
+	if err != nil {
+		t.Fatalf("load lockfile: %v", err)
+	}
+	entry, ok := lf.Plugins[depName]
+	if !ok {
+		t.Fatalf("dep not tracked in lockfile; lf.Plugins=%#v", lf.Plugins)
+	}
+	if entry.Version != "0.5.0" {
+		t.Errorf("dep entry Version = %q, want 0.5.0", entry.Version)
+	}
+}
+
+// TestInstallPluginReqDirect_TracksParentInLockfile verifies workflow#771 Task 5:
+// installPluginReqDirect writes a lockfile entry for the parent plugin (closing
+// the asymmetry where --from-config dep installs were tracked via Task 4 but
+// parent installs via this path were not).
+func TestInstallPluginReqDirect_TracksParentInLockfile(t *testing.T) {
+	pluginDir := t.TempDir()
+
+	origWD, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("getwd: %v", err)
+	}
+	cwd := t.TempDir()
+	if err := os.Chdir(cwd); err != nil {
+		t.Fatalf("chdir: %v", err)
+	}
+	t.Cleanup(func() { os.Chdir(origWD) }) //nolint:errcheck
+
+	emptyLF := &config.WfctlLockfile{
+		Version:     1,
+		GeneratedAt: time.Now(),
+		Plugins:     map[string]config.WfctlLockPluginEntry{},
+	}
+	if err := config.SaveWfctlLockfile(wfctlLockPath, emptyLF); err != nil {
+		t.Fatal(err)
+	}
+
+	// Parent plugin uses long-form name to verify normalize-to-lockfile-key.
+	const parentLong = "workflow-plugin-fromcfg"
+	const parentShort = "fromcfg"
+	binaryContent := []byte("#!/bin/sh\necho fromcfg\n")
+	pjContent := minimalPluginJSON(parentShort, "1.5.0")
+	tarball := buildPluginTarGz(t, parentShort, binaryContent, pjContent)
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/plugins/" + parentLong + "/manifest.json":
+			manifest := RegistryManifest{
+				Name:        parentShort,
+				Version:     "1.5.0",
+				Repository:  "github.com/x/" + parentShort,
+				Author:      "tester",
+				Description: "fromcfg",
+				Type:        "external",
+				Tier:        "community",
+				License:     "MIT",
+				Downloads: []PluginDownload{
+					{
+						OS:     runtime.GOOS,
+						Arch:   runtime.GOARCH,
+						URL:    "http://" + r.Host + "/download/" + parentShort + ".tar.gz",
+						SHA256: sha256Hex(tarball),
+					},
+				},
+			}
+			w.Header().Set("Content-Type", "application/json")
+			_ = json.NewEncoder(w).Encode(manifest)
+		case "/download/" + parentShort + ".tar.gz":
+			w.Header().Set("Content-Type", "application/octet-stream")
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write(tarball)
+		default:
+			http.NotFound(w, r)
+		}
+	}))
+	defer srv.Close()
+
+	cfgFile := writeTestRegistryConfig(t, srv.URL)
+
+	req := config.PluginRequirement{
+		Name:    parentLong,
+		Version: "1.5.0",
+	}
+
+	if err := installPluginReqDirect(pluginDir, cfgFile, req); err != nil {
+		t.Fatalf("installPluginReqDirect: %v", err)
+	}
+
+	lf, err := config.LoadWfctlLockfile(wfctlLockPath)
+	if err != nil {
+		t.Fatalf("load lockfile: %v", err)
+	}
+	// installPluginReqDirect normalizes via normalizePluginName at line 87;
+	// the lockfile entry key is the NORMALIZED form ("fromcfg").
+	entry, ok := lf.Plugins[parentShort]
+	if !ok {
+		t.Fatalf("parent not tracked in lockfile (normalized key %q missing); lf.Plugins=%#v", parentShort, lf.Plugins)
+	}
+	if entry.Version != "1.5.0" {
+		t.Errorf("parent entry Version = %q, want 1.5.0", entry.Version)
+	}
+}
+
 // writeTestRegistryConfig writes a minimal registry YAML config to a temp file
 // pointing at the given static URL, and returns the file path.
 func writeTestRegistryConfig(t *testing.T, baseURL string) string {

cmd/wfctl/plugin_install.go

@@ -26,6 +26,18 @@ import (
 // defaultDataDir is the default location for installed plugin binaries.
 const defaultDataDir = "data/plugins"
 
+// installSkipLockfileUpdate suppresses ALL lockfile writes when set. Outer
+// installers (installFromLockfile / installFromWfctlLockfile) hold the
+// lockfile in memory and re-save it themselves; without this guard, inner
+// install paths' lockfile writes would be silently overwritten by the
+// outer re-save (workflow#771 cycle-5 chokepoint pattern).
+//
+// NOTE: package-level state. Tests touching this MUST NOT call t.Parallel() —
+// cross-test flag leakage would silently break lockfile invariants. See
+// design doc §"Top 3 doubts #2" for rationale on rejecting context.Context
+// threading.
+var installSkipLockfileUpdate bool
+
 func runPluginSearch(args []string) error {
 	fs := flag.NewFlagSet("plugin search", flag.ContinueOnError)
 	cfgPath := fs.String("config", "", "Registry config file path")
@@ -252,18 +264,18 @@ func runPluginInstall(args []string) error {
 		return err
 	}
 
-	// Update .wfctl-lock.yaml lockfile if name@version was provided.
-	if _, ver := parseNameVersion(nameArg); ver != "" {
-		pluginName = normalizePluginName(pluginName)
-		binaryChecksum := ""
-		binaryPath := filepath.Join(pluginDirVal, pluginName, pluginName)
-		if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil {
-			binaryChecksum = cs
-		} else {
-			fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr)
-		}
-		updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, sourceName, binaryChecksum)
+	// Update .wfctl-lock.yaml lockfile (workflow#771: always-track, gate removed).
+	// The chokepoint guard inside updateLockfileWithChecksum (Task 1) is responsible
+	// for suppressing writes during outer-frame installers.
+	pluginName = normalizePluginName(pluginName)
+	binaryChecksum := ""
+	binaryPath := filepath.Join(pluginDirVal, pluginName, pluginName)
+	if cs, hashErr := hashFileSHA256(binaryPath); hashErr == nil {
+		binaryChecksum = cs
+	} else {
+		fmt.Fprintf(os.Stderr, "warning: could not hash binary %s: %v (lockfile will have no checksum)\n", binaryPath, hashErr)
 	}
+	updateLockfileWithChecksum(pluginName, manifest.Version, manifest.Repository, sourceName, binaryChecksum)
 
 	return nil
 }