From 174526444b01f37335026dd396cbedf705154f8d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 13 May 2026 05:53:12 +0000 Subject: [PATCH 1/3] Initial plan From 1a88e47c093af1b341b37f55cd7f0ae080bf3d59 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 13 May 2026 06:11:33 +0000 Subject: [PATCH 2/3] fix: atomic plugin dir replacement to prevent stale binaries on upgrade Add preparePluginStagingDir/commitPluginStagingDir helpers that extract/copy into a sibling .installing directory and atomically rename it into place. This prevents stale executables after upgrades because: 1. The old directory is removed before the new one is moved in 2. ensurePluginBinary can no longer find the old binary in the staging dir Also add verifyInstalledVersion to confirm plugin.json version matches the manifest after a registry install. Tests added: - TestInstallPluginFromManifest_UpgradeReplacesStaleBinary - TestInstallFromURL_UpgradeReplacesStaleBinary - TestInstallFromLocal_UpgradeReplacesStaleBinary - TestInstallPluginFromManifest_StagingCleanedUpOnFailure Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/37d41012-d306-495a-af1f-9b948158f6e6 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --- cmd/wfctl/plugin_install.go | 121 +++++++++++--- cmd/wfctl/plugin_install_test.go | 267 ++++++++++++++++++++++++++++++- 2 files changed, 369 insertions(+), 19 deletions(-) diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index f0344692..1a9174ad 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -269,9 +269,16 @@ func installPluginFromManifest(dataDir, pluginName string, manifest *RegistryMan } destDir := filepath.Join(dataDir, pluginName) - if err := os.MkdirAll(destDir, 0750); err != nil { - return fmt.Errorf("create plugin dir %s: %w", destDir, err) + + // Prepare a staging directory alongside the final destination so that any + // existing installation is never mutated in place. This prevents stale + // binaries from surviving an upgrade when the tarball uses a differently- + // named executable (e.g. GoReleaser platform-suffix names). + stagingDir, cleanupStaging, err := preparePluginStagingDir(destDir) + if err != nil { + return err } + defer cleanupStaging() // no-op if commitPluginStagingDir succeeds (staging renamed away) fmt.Fprintf(os.Stderr, "Downloading %s...\n", dl.URL) data, err := downloadURL(dl.URL) @@ -307,7 +314,7 @@ func installPluginFromManifest(dataDir, pluginName string, manifest *RegistryMan // Emit install_verify hook after download and before extraction (opt-in via req.Verify). // Write tarball to disk so hook handlers can inspect it (e.g. sigstore cosign verify). if verify != nil { - tarballPath := filepath.Join(destDir, pluginName+".tar.gz") + tarballPath := filepath.Join(stagingDir, pluginName+".tar.gz") if writeErr := os.WriteFile(tarballPath, data, 0600); writeErr != nil { return fmt.Errorf("write tarball for verify hook: %w", writeErr) } @@ -318,30 +325,42 @@ func installPluginFromManifest(dataDir, pluginName string, manifest *RegistryMan } fmt.Fprintf(os.Stderr, "Extracting to %s...\n", destDir) - if err := extractTarGz(data, destDir); err != nil { + if err := extractTarGz(data, stagingDir); err != nil { return fmt.Errorf("extract plugin: %w", err) } // Ensure the plugin binary is named to match the plugin name so that // ExternalPluginManager.DiscoverPlugins() can find it (expects //). - if err := ensurePluginBinary(destDir, pluginName); err != nil { + if err := ensurePluginBinary(stagingDir, pluginName); err != nil { fmt.Fprintf(os.Stderr, "warning: could not normalize binary name: %v\n", err) } // Write plugin.json from the registry manifest. This keeps the installed // version metadata in sync with the manifest. If the tarball already // extracted a plugin.json, this overwrites it with the registry version. - pluginJSONPath := filepath.Join(destDir, "plugin.json") + pluginJSONPath := filepath.Join(stagingDir, "plugin.json") if writeErr := writeInstalledManifest(pluginJSONPath, manifest); writeErr != nil { fmt.Fprintf(os.Stderr, "warning: could not write plugin.json: %v\n", writeErr) } - // Verify the installed plugin.json is valid for ExternalPluginManager. + // Verify the staged plugin.json is valid for ExternalPluginManager. fmt.Fprintf(os.Stderr, "Verifying plugin manifest...\n") - if verifyErr := verifyInstalledPlugin(destDir, pluginName); verifyErr != nil { + if verifyErr := verifyInstalledPlugin(stagingDir, pluginName); verifyErr != nil { return fmt.Errorf("post-install verification failed: %w", verifyErr) } + // Verify the installed version matches what the manifest declares. This + // catches cases where plugin.json could not be written above. + if verifyErr := verifyInstalledVersion(stagingDir, manifest.Version); verifyErr != nil { + return fmt.Errorf("post-install version check failed: %w", verifyErr) + } + + // Atomically replace any previous installation with the validated staging + // directory. If this step fails the old installation is left intact. + if commitErr := commitPluginStagingDir(stagingDir, destDir); commitErr != nil { + return commitErr + } + // Strip any existing "v" prefix from the version before printing so that // manifests that store "v0.6.1" don't produce "Installed X vv0.6.1". fmt.Printf("Installed %s v%s to %s\n", manifest.Name, strings.TrimPrefix(manifest.Version, "v"), destDir) @@ -727,23 +746,33 @@ func installFromURL(rawURL, pluginDir, expectedSHA256 string, skipChecksum bool) pluginName := normalizePluginName(pj.Name) destDir := filepath.Join(pluginDir, pluginName) - if err := os.MkdirAll(destDir, 0750); err != nil { - return fmt.Errorf("create plugin dir: %w", err) + + // Prepare a staging directory alongside the final destination so that any + // existing installation is never mutated in place. + stagingDir, cleanupStaging, err := preparePluginStagingDir(destDir) + if err != nil { + return err } + defer cleanupStaging() - if err := extractTarGz(data, destDir); err != nil { + if err := extractTarGz(data, stagingDir); err != nil { return fmt.Errorf("extract to dest: %w", err) } - if err := ensurePluginBinary(destDir, pluginName); err != nil { + if err := ensurePluginBinary(stagingDir, pluginName); err != nil { return fmt.Errorf("could not normalize binary name: %w", err) } - // Validate the installed plugin (same checks as registry installs). - if verifyErr := verifyInstalledPlugin(destDir, pluginName); verifyErr != nil { + // Validate the staged plugin (same checks as registry installs). + if verifyErr := verifyInstalledPlugin(stagingDir, pluginName); verifyErr != nil { return fmt.Errorf("post-install verification failed: %w", verifyErr) } + // Atomically replace any previous installation. + if commitErr := commitPluginStagingDir(stagingDir, destDir); commitErr != nil { + return commitErr + } + // Hash the installed binary (not the archive) so that verifyInstalledChecksum matches. binaryPath := filepath.Join(destDir, pluginName) checksum, hashErr := hashFileSHA256(binaryPath) @@ -802,12 +831,17 @@ func installFromLocal(srcDir, pluginDir string) error { pluginName := normalizePluginName(pj.Name) destDir := filepath.Join(pluginDir, pluginName) - if err := os.MkdirAll(destDir, 0750); err != nil { - return fmt.Errorf("create plugin dir: %w", err) + + // Prepare a staging directory alongside the final destination so that any + // existing installation is never mutated in place. + stagingDir, cleanupStaging, err := preparePluginStagingDir(destDir) + if err != nil { + return err } + defer cleanupStaging() // Copy plugin.json - if err := copyFile(pjPath, filepath.Join(destDir, "plugin.json"), 0640); err != nil { + if err := copyFile(pjPath, filepath.Join(stagingDir, "plugin.json"), 0640); err != nil { return err } @@ -820,10 +854,15 @@ func installFromLocal(srcDir, pluginDir string) error { return fmt.Errorf("no plugin binary found in %s (tried %s and %s)", srcDir, pluginName, fullName) } } - if err := copyFile(srcBinary, filepath.Join(destDir, pluginName), 0750); err != nil { + if err := copyFile(srcBinary, filepath.Join(stagingDir, pluginName), 0750); err != nil { return err } + // Atomically replace any previous installation. + if commitErr := commitPluginStagingDir(stagingDir, destDir); commitErr != nil { + return commitErr + } + binaryChecksum, hashErr := hashFileSHA256(filepath.Join(destDir, pluginName)) if hashErr != nil { fmt.Fprintf(os.Stderr, "warning: could not compute binary checksum: %v\n", hashErr) @@ -1364,6 +1403,52 @@ func ensurePluginBinary(destDir, pluginName string) error { return os.Rename(filepath.Join(destDir, bestName), expectedPath) } +// preparePluginStagingDir removes any leftover staging directory and creates a +// fresh one alongside destDir (same filesystem). The caller must call +// commitPluginStagingDir on success. On failure the returned cleanup func +// removes the staging directory. +func preparePluginStagingDir(destDir string) (stagingDir string, cleanup func(), err error) { + stagingDir = destDir + ".installing" + if removeErr := os.RemoveAll(stagingDir); removeErr != nil { + return "", nil, fmt.Errorf("clean staging dir %s: %w", stagingDir, removeErr) + } + // Ensure the parent directory exists so MkdirAll only needs to create the + // staging leaf. + if mkErr := os.MkdirAll(filepath.Dir(destDir), 0750); mkErr != nil { + return "", nil, fmt.Errorf("create plugin base dir: %w", mkErr) + } + if mkErr := os.Mkdir(stagingDir, 0750); mkErr != nil { + return "", nil, fmt.Errorf("create staging dir %s: %w", stagingDir, mkErr) + } + return stagingDir, func() { os.RemoveAll(stagingDir) }, nil //nolint:errcheck +} + +// commitPluginStagingDir atomically replaces destDir with stagingDir. It +// removes the previous destDir content and renames the staging directory into +// its place. On success stagingDir no longer exists on disk; the deferred +// cleanup returned by preparePluginStagingDir becomes a harmless no-op. +func commitPluginStagingDir(stagingDir, destDir string) error { + if err := os.RemoveAll(destDir); err != nil { + return fmt.Errorf("remove existing plugin dir %s: %w", destDir, err) + } + if err := os.Rename(stagingDir, destDir); err != nil { + return fmt.Errorf("install plugin dir %s: %w", destDir, err) + } + return nil +} + +// verifyInstalledVersion checks that the plugin.json in dir declares the +// expected version. It normalises "v" prefixes before comparing, so "v1.0.8" +// and "1.0.8" are treated as equal. +func verifyInstalledVersion(dir, expectedVersion string) error { + installedVersion := readInstalledVersion(dir) + norm := func(v string) string { return strings.TrimPrefix(v, "v") } + if norm(installedVersion) != norm(expectedVersion) { + return fmt.Errorf("installed plugin.json version %q does not match expected %q", installedVersion, expectedVersion) + } + return nil +} + // verifyInstalledPlugin validates the installed plugin.json using the engine's // manifest loader and checks that the binary exists and is executable. func verifyInstalledPlugin(destDir, pluginName string) error { diff --git a/cmd/wfctl/plugin_install_test.go b/cmd/wfctl/plugin_install_test.go index 05525dcb..df259962 100644 --- a/cmd/wfctl/plugin_install_test.go +++ b/cmd/wfctl/plugin_install_test.go @@ -474,7 +474,272 @@ func makeTestManifest(name, downloadURL, sha256sum string) *RegistryManifest { } } -// ---- verifyChecksum error format ---- +// makeTestTarGzVersioned builds a minimal .tar.gz using the GoReleaser platform- +// suffix naming convention (e.g. myplugin-linux-amd64/myplugin-linux-amd64) so +// ensurePluginBinary must rename the binary. version and binaryContent allow +// callers to distinguish upgrades from fresh installs. +func makeTestTarGzVersioned(t *testing.T, pluginName, version string, binaryContent []byte) []byte { + t.Helper() + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gw) + + // Use the GoReleaser platform-suffix convention so ensurePluginBinary must + // rename the file (rather than finding it already correctly named). + topDir := pluginName + "-" + runtime.GOOS + "-" + runtime.GOARCH + binaryName := pluginName + "-" + runtime.GOOS + "-" + runtime.GOARCH + pjContent := fmt.Sprintf(`{"name":%q,"version":%q,"author":"test","description":"test plugin"}`, pluginName, version) + addTestTarFile(t, tw, topDir+"/plugin.json", 0640, []byte(pjContent)) + addTestTarFile(t, tw, topDir+"/"+binaryName, 0750, binaryContent) + + if err := tw.Close(); err != nil { + t.Fatalf("close tar: %v", err) + } + if err := gw.Close(); err != nil { + t.Fatalf("close gzip: %v", err) + } + return buf.Bytes() +} + +// makeTestManifestVersioned is like makeTestManifest but accepts an explicit version. +func makeTestManifestVersioned(name, version, downloadURL, sha256sum string) *RegistryManifest { + m := makeTestManifest(name, downloadURL, sha256sum) + m.Version = version + return m +} + +// ---- stale-binary upgrade tests (issue: wfctl plugin install reports upgraded +// plugin but leaves stale executable) ---- + +// TestInstallPluginFromManifest_UpgradeReplacesStaleBinary verifies that +// upgrading a plugin via registry manifest atomically replaces the previous +// installation. The old binary (from a GoReleaser platform-suffix tarball) must +// be gone after the upgrade; only the new binary should exist. +func TestInstallPluginFromManifest_UpgradeReplacesStaleBinary(t *testing.T) { + const pluginName = "myplugin" + + oldBinary := []byte("#!/bin/sh\necho v1.0.6\n") + newBinary := []byte("#!/bin/sh\necho v1.0.8\n") + + oldTar := makeTestTarGzVersioned(t, pluginName, "1.0.6", oldBinary) + newTar := makeTestTarGzVersioned(t, pluginName, "1.0.8", newBinary) + + var serveData []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(serveData) + })) + defer srv.Close() + + pluginDir := t.TempDir() + + // First install (v1.0.6). + serveData = oldTar + oldSum := sha256sum(oldTar) + if err := installPluginFromManifest(pluginDir, pluginName, + makeTestManifestVersioned(pluginName, "1.0.6", srv.URL+"/p.tar.gz", oldSum), nil, false); err != nil { + t.Fatalf("first install: %v", err) + } + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + gotOld, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v1 binary: %v", err) + } + if !bytes.Equal(gotOld, oldBinary) { + t.Fatalf("after v1 install, binary = %q, want %q", gotOld, oldBinary) + } + + // Upgrade to v1.0.8 — the new tarball uses the GoReleaser suffix name. + serveData = newTar + newSum := sha256sum(newTar) + if err := installPluginFromManifest(pluginDir, pluginName, + makeTestManifestVersioned(pluginName, "1.0.8", srv.URL+"/p.tar.gz", newSum), nil, false); err != nil { + t.Fatalf("upgrade install: %v", err) + } + + gotNew, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v2 binary: %v", err) + } + if bytes.Equal(gotNew, oldBinary) { + t.Fatal("binary still contains old content after upgrade — stale binary bug") + } + if !bytes.Equal(gotNew, newBinary) { + t.Fatalf("after upgrade, binary = %q, want %q", gotNew, newBinary) + } + + // Installed version in plugin.json must match v1.0.8. + if got := readInstalledVersion(filepath.Join(pluginDir, pluginName)); got != "1.0.8" { + t.Errorf("installed version = %q, want 1.0.8", got) + } + + // The old platform-suffix binary must NOT be present (no stale files). + staleName := pluginName + "-" + runtime.GOOS + "-" + runtime.GOARCH + if _, err := os.Stat(filepath.Join(pluginDir, pluginName, staleName)); !os.IsNotExist(err) { + t.Errorf("stale GoReleaser-named binary %q still present after upgrade", staleName) + } +} + +// TestInstallFromURL_UpgradeReplacesStaleBinary verifies the same invariant for +// the --url install path. +func TestInstallFromURL_UpgradeReplacesStaleBinary(t *testing.T) { + const pluginName = "urlplugin" + + oldBinary := []byte("#!/bin/sh\necho old-url\n") + newBinary := []byte("#!/bin/sh\necho new-url\n") + + oldTar := makeTestTarGzVersioned(t, pluginName, "2.0.0", oldBinary) + newTar := makeTestTarGzVersioned(t, pluginName, "3.0.0", newBinary) + + var serveData []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(serveData) + })) + defer srv.Close() + + pluginDir := t.TempDir() + + // First install. + serveData = oldTar + if err := installFromURL(srv.URL+"/p.tar.gz", pluginDir, sha256sum(oldTar), false); err != nil { + t.Fatalf("first install: %v", err) + } + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + gotOld, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v1 binary: %v", err) + } + if !bytes.Equal(gotOld, oldBinary) { + t.Fatalf("after first install, binary = %q, want %q", gotOld, oldBinary) + } + + // Upgrade. + serveData = newTar + if err := installFromURL(srv.URL+"/p.tar.gz", pluginDir, sha256sum(newTar), false); err != nil { + t.Fatalf("upgrade install: %v", err) + } + gotNew, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v2 binary: %v", err) + } + if bytes.Equal(gotNew, oldBinary) { + t.Fatal("binary still contains old content after URL upgrade — stale binary bug") + } + if !bytes.Equal(gotNew, newBinary) { + t.Fatalf("after upgrade, binary = %q, want %q", gotNew, newBinary) + } +} + +// TestInstallFromLocal_UpgradeReplacesStaleBinary verifies the same invariant +// for the --local install path. +func TestInstallFromLocal_UpgradeReplacesStaleBinary(t *testing.T) { + const pluginName = "localplugin" + + pluginDir := t.TempDir() + + makeLocalSrc := func(version string, binaryContent []byte) string { + src := t.TempDir() + pj := fmt.Sprintf(`{"name":%q,"version":%q,"author":"t","description":"t"}`, pluginName, version) + if err := os.WriteFile(filepath.Join(src, "plugin.json"), []byte(pj), 0640); err != nil { + t.Fatalf("write plugin.json: %v", err) + } + if err := os.WriteFile(filepath.Join(src, pluginName), binaryContent, 0750); err != nil { + t.Fatalf("write binary: %v", err) + } + return src + } + + oldBinary := []byte("#!/bin/sh\necho local-v1\n") + newBinary := []byte("#!/bin/sh\necho local-v2\n") + + // First install. + if err := installFromLocal(makeLocalSrc("1.0.0", oldBinary), pluginDir); err != nil { + t.Fatalf("first install: %v", err) + } + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + gotOld, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v1 binary: %v", err) + } + if !bytes.Equal(gotOld, oldBinary) { + t.Fatalf("after first install, binary = %q, want %q", gotOld, oldBinary) + } + + // Upgrade. + if err := installFromLocal(makeLocalSrc("2.0.0", newBinary), pluginDir); err != nil { + t.Fatalf("upgrade install: %v", err) + } + gotNew, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("read v2 binary: %v", err) + } + if bytes.Equal(gotNew, oldBinary) { + t.Fatal("binary still contains old content after local upgrade — stale binary bug") + } + if !bytes.Equal(gotNew, newBinary) { + t.Fatalf("after upgrade, binary = %q, want %q", gotNew, newBinary) + } +} + +// TestInstallPluginFromManifest_StagingCleanedUpOnFailure verifies that if the +// install fails (e.g. download error) the staging directory is removed and the +// original installation is left intact. +func TestInstallPluginFromManifest_StagingCleanedUpOnFailure(t *testing.T) { + const pluginName = "myplugin" + + oldBinary := []byte("#!/bin/sh\necho stable\n") + oldTar := makeTestTarGz(t, pluginName) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/old.tar.gz": + _, _ = w.Write(oldTar) + default: + http.Error(w, "not found", http.StatusNotFound) + } + })) + defer srv.Close() + + pluginDir := t.TempDir() + + // Install stable version first. + oldSum := sha256sum(oldTar) + if err := installPluginFromManifest(pluginDir, pluginName, + makeTestManifest(pluginName, srv.URL+"/old.tar.gz", oldSum), nil, false); err != nil { + t.Fatalf("first install: %v", err) + } + // Overwrite binary with sentinel so we can verify it's preserved. + binaryPath := filepath.Join(pluginDir, pluginName, pluginName) + if err := os.WriteFile(binaryPath, oldBinary, 0750); err != nil { + t.Fatalf("write sentinel: %v", err) + } + + // Attempt upgrade with a failing URL. + badManifest := makeTestManifest(pluginName, srv.URL+"/nonexistent.tar.gz", strings.Repeat("0", 64)) + _ = installPluginFromManifest(pluginDir, pluginName, badManifest, nil, false) // expected to fail + + // Staging directory must be cleaned up. + stagingDir := filepath.Join(pluginDir, pluginName+".installing") + if _, err := os.Stat(stagingDir); !os.IsNotExist(err) { + t.Errorf("staging dir %q was not cleaned up after failed install", stagingDir) + } + + // Original installation must be intact. + gotBinary, err := os.ReadFile(binaryPath) + if err != nil { + t.Fatalf("original binary missing after failed upgrade: %v", err) + } + if !bytes.Equal(gotBinary, oldBinary) { + t.Errorf("original binary modified by failed upgrade: got %q, want %q", gotBinary, oldBinary) + } +} + +// sha256sum is a test helper that returns the hex-encoded SHA256 of data. +func sha256sum(data []byte) string { + h := sha256.Sum256(data) + return hex.EncodeToString(h[:]) +} + + func TestVerifyChecksum_MismatchFormat(t *testing.T) { err := verifyChecksum([]byte("data"), strings.Repeat("0", 64)) From 09cee151c63fa1d448483bba6707494d26182ca2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 13 May 2026 22:46:01 +0000 Subject: [PATCH 3/3] fix: address review feedback on staged install atomicity and test quality - commitPluginStagingDir: rename existing dir to .uninstalling before moving staging in place; restore on failure, remove on success. This prevents the old install from being destroyed if the final rename fails. - installPluginFromManifest: make writeInstalledManifest failure a hard error instead of a warning so registry-only metadata (capabilities, CLI commands) cannot be silently dropped and an archive-supplied plugin.json cannot bypass verifyInstalledVersion. - TestInstallPluginFromManifest_StagingCleanedUpOnFailure: assert the failing install actually returns an error (was discarding it with _). - TestInstallFromURL_UpgradeReplacesStaleBinary, TestInstallFromLocal_UpgradeReplacesStaleBinary, TestInstallFromURL_WithExpectedSHA256_Correct, TestInstallFromURL_SkipChecksum_NonGitHub: add t.Chdir(t.TempDir()) so updateLockfileWithChecksum writes .wfctl-lock.yaml to a temp dir rather than the package checkout, preventing lockfile pollution and test interdependence. Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/6bc57919-fc52-426b-94ba-1545ffcd1537 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --- cmd/wfctl/plugin_install.go | 46 +++++++++++++++++++++++++++----- cmd/wfctl/plugin_install_test.go | 20 +++++++++++++- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index 1a9174ad..b16a3683 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -338,9 +338,12 @@ func installPluginFromManifest(dataDir, pluginName string, manifest *RegistryMan // Write plugin.json from the registry manifest. This keeps the installed // version metadata in sync with the manifest. If the tarball already // extracted a plugin.json, this overwrites it with the registry version. + // Failure is a hard error: continuing with an archive-supplied plugin.json + // could silently drop registry-only metadata (capabilities, CLI commands, + // build hooks) and would let the archive version bypass verifyInstalledVersion. pluginJSONPath := filepath.Join(stagingDir, "plugin.json") if writeErr := writeInstalledManifest(pluginJSONPath, manifest); writeErr != nil { - fmt.Fprintf(os.Stderr, "warning: could not write plugin.json: %v\n", writeErr) + return fmt.Errorf("write plugin.json: %w", writeErr) } // Verify the staged plugin.json is valid for ExternalPluginManager. @@ -1423,17 +1426,46 @@ func preparePluginStagingDir(destDir string) (stagingDir string, cleanup func(), return stagingDir, func() { os.RemoveAll(stagingDir) }, nil //nolint:errcheck } -// commitPluginStagingDir atomically replaces destDir with stagingDir. It -// removes the previous destDir content and renames the staging directory into -// its place. On success stagingDir no longer exists on disk; the deferred -// cleanup returned by preparePluginStagingDir becomes a harmless no-op. +// commitPluginStagingDir replaces destDir with stagingDir. To preserve the +// existing installation if the final rename fails, the old destDir is first +// renamed to a trash location on the same filesystem. Only after the new +// directory is successfully renamed into place is the trash removed. +// +// 1. Rename destDir → destDir+".uninstalling" (no-op if destDir absent) +// 2. Rename stagingDir → destDir +// 3. On step-2 failure: restore destDir+".uninstalling" → destDir +// 4. On step-2 success: remove destDir+".uninstalling" +// +// On success stagingDir no longer exists on disk; the deferred cleanup +// returned by preparePluginStagingDir becomes a harmless no-op. func commitPluginStagingDir(stagingDir, destDir string) error { - if err := os.RemoveAll(destDir); err != nil { - return fmt.Errorf("remove existing plugin dir %s: %w", destDir, err) + trashDir := destDir + ".uninstalling" + // Remove any leftover trash from a previous interrupted commit. + if err := os.RemoveAll(trashDir); err != nil { + return fmt.Errorf("clean trash dir %s: %w", trashDir, err) + } + + // Move the existing install out of the way before installing the new one. + // If destDir does not exist yet (first install) we skip this step. + hasExisting := false + if _, statErr := os.Stat(destDir); statErr == nil { + hasExisting = true + if err := os.Rename(destDir, trashDir); err != nil { + return fmt.Errorf("preserve existing plugin dir %s: %w", destDir, err) + } } + + // Move staging into the final location. if err := os.Rename(stagingDir, destDir); err != nil { + // Best-effort restore: move the old install back if we preserved it. + if hasExisting { + _ = os.Rename(trashDir, destDir) //nolint:errcheck + } return fmt.Errorf("install plugin dir %s: %w", destDir, err) } + + // New install is in place — remove the old one (best effort). + _ = os.RemoveAll(trashDir) //nolint:errcheck return nil } diff --git a/cmd/wfctl/plugin_install_test.go b/cmd/wfctl/plugin_install_test.go index df259962..16e686da 100644 --- a/cmd/wfctl/plugin_install_test.go +++ b/cmd/wfctl/plugin_install_test.go @@ -584,6 +584,10 @@ func TestInstallPluginFromManifest_UpgradeReplacesStaleBinary(t *testing.T) { func TestInstallFromURL_UpgradeReplacesStaleBinary(t *testing.T) { const pluginName = "urlplugin" + // Change cwd to a temp dir so updateLockfileWithChecksum writes + // .wfctl-lock.yaml there instead of the package checkout. + t.Chdir(t.TempDir()) + oldBinary := []byte("#!/bin/sh\necho old-url\n") newBinary := []byte("#!/bin/sh\necho new-url\n") @@ -634,6 +638,10 @@ func TestInstallFromURL_UpgradeReplacesStaleBinary(t *testing.T) { func TestInstallFromLocal_UpgradeReplacesStaleBinary(t *testing.T) { const pluginName = "localplugin" + // Change cwd to a temp dir so updateLockfileWithChecksum writes + // .wfctl-lock.yaml there instead of the package checkout. + t.Chdir(t.TempDir()) + pluginDir := t.TempDir() makeLocalSrc := func(version string, binaryContent []byte) string { @@ -715,7 +723,9 @@ func TestInstallPluginFromManifest_StagingCleanedUpOnFailure(t *testing.T) { // Attempt upgrade with a failing URL. badManifest := makeTestManifest(pluginName, srv.URL+"/nonexistent.tar.gz", strings.Repeat("0", 64)) - _ = installPluginFromManifest(pluginDir, pluginName, badManifest, nil, false) // expected to fail + if err := installPluginFromManifest(pluginDir, pluginName, badManifest, nil, false); err == nil { + t.Fatal("expected upgrade with non-existent URL to fail, but it succeeded") + } // Staging directory must be cleaned up. stagingDir := filepath.Join(pluginDir, pluginName+".installing") @@ -862,6 +872,10 @@ func TestInstallFromURL_NonGitHubNoSHAFails(t *testing.T) { } func TestInstallFromURL_WithExpectedSHA256_Correct(t *testing.T) { + // Change cwd to a temp dir so updateLockfileWithChecksum writes + // .wfctl-lock.yaml there instead of the package checkout. + t.Chdir(t.TempDir()) + archiveData := makeTestTarGz(t, "myplugin") h := sha256.Sum256(archiveData) sha := hex.EncodeToString(h[:]) @@ -895,6 +909,10 @@ func TestInstallFromURL_WithExpectedSHA256_Wrong(t *testing.T) { } func TestInstallFromURL_SkipChecksum_NonGitHub(t *testing.T) { + // Change cwd to a temp dir so updateLockfileWithChecksum writes + // .wfctl-lock.yaml there instead of the package checkout. + t.Chdir(t.TempDir()) + archiveData := makeTestTarGz(t, "myplugin") srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write(archiveData)