From 2ec7bd769cc08f87df5afc2268531b57b0b35a79 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:27:45 +0000 Subject: [PATCH 1/6] fix(cli): marketplace add fails instead of registering an unfindable cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `addMarketplaceSource` fetches into a URL-derived slug directory and re-slots the cache under the marketplace's declared name — the name it writes into marketplaces/.toml and the state record, and the only name any later lookup derives the cache directory from. Both arms of that re-slot dropped their errors, so a failed move still produced a success line and a registration whose cache nothing could find; the next plugin install reported `marketplace "x" not found in cache; run: agentsync marketplace add ` — advice that repeats the identical failure. The move is now a named helper that returns every failure, and the add stops before writing the TOML or the state entry, discarding the fetched tree so nothing is half-registered and no unregistered copy is left under the slug for a bare-id `plugin add` to find. The helper also replaces an existing destination (via the package's swapDir) rather than failing on it: os.Rename onto a non-empty directory fails with ENOTEMPTY, which meant EVERY re-add of a marketplace whose declared name differs from its slug silently kept the stale cache, orphaned the freshly fetched tree under the slug, and moved head_sha on anyway. Tests: the helper's four arms, a failed add registering nothing and leaving no cache behind, two end-to-end adds — first add slotted under the registered name, re-add refreshing it with no orphan left behind — and `import claude:plugin` warning and skipping a marketplace whose re-slot fails instead of registering a phantom. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 24 ++++ internal/cli/import_plugin_reslot_test.go | 45 ++++++ internal/cli/marketplace.go | 52 ++++++- .../cli/marketplace_reslot_internal_test.go | 133 ++++++++++++++++++ internal/cli/marketplace_reslot_test.go | 89 ++++++++++++ 5 files changed, 338 insertions(+), 5 deletions(-) create mode 100644 internal/cli/import_plugin_reslot_test.go create mode 100644 internal/cli/marketplace_reslot_internal_test.go create mode 100644 internal/cli/marketplace_reslot_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dfd59b3..4b19fd7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,30 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Fixed +- **`marketplace add` no longer registers a marketplace whose cache it failed to + put in place** ([#233](https://github.com/spxrogers/agentsync/issues/233)). + When a marketplace's declared name differs from the name derived from its URL + (typically every `github:owner/repo` source), the fetched cache is moved under + the declared name — the name the TOML and the state record use, and the only + name any later lookup derives the cache directory from. That move's failures + were discarded while the registration went ahead, so `marketplace add` printed + `✅ added marketplace …`, `marketplace list` showed it, and the very next + `plugin add @` failed with `marketplace "" not found in cache; + run: agentsync marketplace add ` — advice that repeated the failure. + The move now reports its failures and the add stops before registering + anything. It also **replaces** an existing cache instead of failing on it, + fixing the routine case: re-adding an already-registered marketplace hit the + same swallowed failure every time (a rename onto a non-empty directory), so + the cache was never refreshed — a plugin published since the first add stayed + invisible while the recorded `head_sha` moved on — and a duplicate copy + accumulated under the URL-derived name. A failed add now also discards its + fetched tree, so nothing is left behind that a bare-id `plugin add` could pick + up as an unregistered marketplace; and when two sources declare the same name, + the later add now replaces the earlier one's cache along with the + `marketplaces/.toml` and state record it already overwrote. + `import :plugin` registers marketplaces through the same code and now + warns and skips instead of registering a phantom. + - **A symlinked destination under `AGENTSYNC_ALLOW_SYMLINK_DEST=1` is no longer reported as permanently drifted** ([#229](https://github.com/spxrogers/agentsync/issues/229)). In the diff --git a/internal/cli/import_plugin_reslot_test.go b/internal/cli/import_plugin_reslot_test.go new file mode 100644 index 00000000..9f341238 --- /dev/null +++ b/internal/cli/import_plugin_reslot_test.go @@ -0,0 +1,45 @@ +package cli_test + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestImportPlugin_SkipsMarketplaceWhoseReslotFails pins the import half of +// #233: `import :plugin` registers a native marketplace through the same +// addMarketplaceSource as `marketplace add`, so a cache that cannot be +// re-slotted under the declared name must warn and skip that marketplace — +// never register a phantom whose plugin installs then fail. The forcing +// function is the same hostile 300-character declared name (one path segment, +// over every Linux filesystem's 255-byte cap), so the move fails anywhere this +// runs; the assertions name the re-slot error and check that nothing — TOML, +// state, fetch cache — is left behind. +func TestImportPlugin_SkipsMarketplaceWhoseReslotFails(t *testing.T) { + tmp, env := importTestEnv(t) + longName := strings.Repeat("n", 300) + mpDir := writeMarketplaceFixture(t, filepath.Join(t.TempDir(), "hostile-mp"), longName) + writeClaudeSettings(t, tmp, directoryMarketplaceSettings("hostile", mpDir, "demo")) + + out, err := runCLI(t, env, "import", "claude:plugin") + if err != nil { + t.Fatalf("import must warn and skip the marketplace, not fail: %v\n%s", err, out) + } + for _, want := range []string{"skipping marketplace", "register marketplace", "move marketplace cache"} { + if !strings.Contains(out, want) { + t.Fatalf("import must say which marketplace it skipped and why (missing %q); got:\n%s", want, out) + } + } + home := filepath.Join(tmp, ".agentsync") + if entries, rerr := os.ReadDir(filepath.Join(home, "marketplaces")); rerr == nil && len(entries) != 0 { + t.Errorf("a marketplace whose cache cannot be re-slotted must not be registered; marketplaces/ holds %d file(s)", len(entries)) + } + // importTestEnv's `agent add` already wrote targets.json, so check its content. + if st, rerr := os.ReadFile(filepath.Join(home, ".state", "targets.json")); rerr == nil && strings.Contains(string(st), longName) { + t.Errorf("a skipped marketplace must record no state entry; targets.json names it:\n%s", st) + } + if entries, rerr := os.ReadDir(filepath.Join(home, ".state", "cache", "marketplaces")); rerr == nil && len(entries) != 0 { + t.Errorf("a skipped marketplace must leave no fetch cache behind; cache root holds %d entr(y/ies)", len(entries)) + } +} diff --git a/internal/cli/marketplace.go b/internal/cli/marketplace.go index 2aab5ee8..14e0ecee 100644 --- a/internal/cli/marketplace.go +++ b/internal/cli/marketplace.go @@ -130,13 +130,27 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa } } - // If slug derived from URL differs from declared name, re-cache under declared name. + // The URL-derived slug and the declared name differ (the common case for a + // git marketplace): re-slot the fetched cache under the declared name. Every + // later lookup derives the cache dir from the name this function goes on to + // record, so the two MUST agree — a re-slot that fails and is IGNORED + // registers a marketplace whose cache nothing can find, and the miss reports + // "marketplace %q not found in cache; run: agentsync marketplace add " + // for a marketplace the user just added, with a remedy that repeats the same + // failure (#233). Fail the add instead: the TOML and the state record are + // written below, so an early return leaves nothing half-registered. Both + // names are sanitizeSlug-clean, so marketplaceCacheDir maps them to distinct + // sibling directories whenever they differ. if mpName != slug { newCacheDir := marketplaceCacheDir(home, mpName) - if newCacheDir != cacheDir { - if err := os.MkdirAll(filepath.Dir(newCacheDir), 0o755); err == nil { - _ = os.Rename(cacheDir, newCacheDir) //nolint:forbidigo // re-slots the marketplace cache under .state/cache, not a native destination - } + if err := reslotMarketplaceCache(cacheDir, newCacheDir); err != nil { + // Discard the fetched tree rather than leave it under the slug: no + // record points at that directory, `marketplace remove` cannot reach + // it, and searchAllMarketplaces would still offer it to a bare-id + // `plugin add` as an unregistered marketplace. A re-run re-fetches. + _ = os.RemoveAll(cacheDir) //nolint:forbidigo // discards the marketplace fetch cache under .state/cache, not a native destination + return "", "", fmt.Errorf("register marketplace %q: %w; nothing was written to marketplaces/ "+ + "or the state record — fix the cause above and re-run", mpName, err) } } @@ -210,6 +224,34 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa return mpName, result.HeadSHA, nil } +// reslotMarketplaceCache moves a freshly-fetched marketplace cache from the +// URL-derived slug directory to the one named by the marketplace's declared +// name. Both are single segments under .state/cache/marketplaces, so this is a +// same-directory rename. +// +// An EXISTING destination is replaced, not merged (swapDir): it is a stale tree +// from an earlier add of this marketplace — or of another one declaring the same +// name, whose marketplaces/.toml and state record this add overwrites +// regardless, so the cache must follow. os.Rename onto a non-empty directory +// fails with ENOTEMPTY, which is why a re-add used to keep the STALE cache and +// orphan the freshly fetched tree under the slug while reporting success (#233). +// +// Every failure is returned. The destination is removed before the rename, so a +// failure after that leaves this marketplace with no cache at all — the add +// says so, naming both paths, and a re-run re-fetches and completes (unlike the +// swallowed failure, which no re-run could repair). +func reslotMarketplaceCache(from, to string) error { + // Belt and braces: the fetch just created `from` under this same parent, so + // the only way the parent is missing here is a concurrent removal. + if err := os.MkdirAll(filepath.Dir(to), 0o755); err != nil { + return fmt.Errorf("prepare marketplace cache dir %s: %w", filepath.Dir(to), err) + } + if err := swapDir(from, to); err != nil { + return fmt.Errorf("move marketplace cache %s → %s: %w", from, to, err) + } + return nil +} + // ---- remove ----------------------------------------------------------------- func newMarketplaceRemoveCmd() *cobra.Command { diff --git a/internal/cli/marketplace_reslot_internal_test.go b/internal/cli/marketplace_reslot_internal_test.go new file mode 100644 index 00000000..c7693a30 --- /dev/null +++ b/internal/cli/marketplace_reslot_internal_test.go @@ -0,0 +1,133 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spxrogers/agentsync/internal/marketplace" +) + +// TestReslotMarketplaceCache covers the move that puts a freshly-fetched cache +// under the marketplace's DECLARED name (#233): that name is what every later +// lookup derives the cache dir from, so a move that does not happen — or happens +// only partially — must never be reported as success. The stale-destination case +// is the routine one: os.Rename onto a non-empty directory fails with ENOTEMPTY, +// so before the fix EVERY re-add kept the STALE tree and orphaned the fresh one. +func TestReslotMarketplaceCache(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T, root string) (from, to string) + wantErr string // substring; "" means the move must succeed + }{ + { + name: "moves the fetched tree under the declared name", + setup: func(t *testing.T, root string) (string, string) { + from := filepath.Join(root, "slug") + mustWrite(t, filepath.Join(from, "marker.txt"), "fresh") + return from, filepath.Join(root, "declared") + }, + }, + { + name: "replaces a stale cache left by an earlier add", + setup: func(t *testing.T, root string) (string, string) { + from := filepath.Join(root, "slug") + mustWrite(t, filepath.Join(from, "marker.txt"), "fresh") + to := filepath.Join(root, "declared") + mustWrite(t, filepath.Join(to, "marker.txt"), "stale") + mustWrite(t, filepath.Join(to, "gone.txt"), "stale") + return from, to + }, + }, + { + name: "propagates a failure to prepare the cache root", + setup: func(t *testing.T, root string) (string, string) { + from := filepath.Join(root, "slug") + mustWrite(t, filepath.Join(from, "marker.txt"), "fresh") + // A regular file where the cache root should be: MkdirAll fails. + notADir := filepath.Join(root, "not-a-dir") + mustWrite(t, notADir, "x") + return from, filepath.Join(notADir, "declared") + }, + wantErr: "prepare marketplace cache dir", + }, + { + name: "propagates a failure to move the tree", + setup: func(t *testing.T, root string) (string, string) { + // Nothing at `from`: the rename cannot succeed. + return filepath.Join(root, "slug"), filepath.Join(root, "declared") + }, + wantErr: "move marketplace cache", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + from, to := tc.setup(t, root) + + err := reslotMarketplaceCache(from, to) + + if tc.wantErr != "" { + if err == nil { + t.Fatalf("re-slot must report the failure, not swallow it; got nil error") + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error must name the failing step %q; got: %v", tc.wantErr, err) + } + return + } + if err != nil { + t.Fatalf("re-slot: %v", err) + } + got, rerr := os.ReadFile(filepath.Join(to, "marker.txt")) + if rerr != nil { + t.Fatalf("read the re-slotted cache: %v", rerr) + } + if string(got) != "fresh" { + t.Errorf("declared-name cache must hold the FRESHLY fetched tree; marker.txt = %q, want %q", got, "fresh") + } + if _, err := os.Lstat(filepath.Join(to, "gone.txt")); err == nil { + t.Errorf("a stale cache must be replaced, not merged: gone.txt survived at %s", to) + } + if _, err := os.Lstat(from); !os.IsNotExist(err) { + t.Errorf("the slug directory must not survive the move (it would be an orphan cache): %s (err=%v)", from, err) + } + }) + } +} + +// TestAddMarketplaceSource_ReslotFailureRegistersNothing pins the whole-command +// half of #233: when the cache cannot be re-slotted under the declared name, +// `marketplace add` must FAIL rather than write marketplaces/.toml and a +// state entry pointing at a cache that is not there. The forcing function is a +// hostile marketplace.json — a 300-character declared name is one path segment +// and every Linux filesystem caps a name at 255 bytes, so the move fails with +// ENAMETOOLONG anywhere this runs. Before the fix the failure was swallowed and +// surfaced later as a different error from the TOML write, which is why the +// assertion names the re-slot error rather than accepting "some error". +func TestAddMarketplaceSource_ReslotFailureRegistersNothing(t *testing.T) { + home := t.TempDir() + fixture := filepath.Join(t.TempDir(), "fixture-mp") + longName := strings.Repeat("n", 300) + mustWrite(t, filepath.Join(fixture, ".claude-plugin", "marketplace.json"), + `{"name": "`+longName+`", "owner": {"name": "x"}, "plugins": []}`) + + _, _, err := addMarketplaceSource(home, marketplace.Source{Relative: fixture}, fixture, func(string, ...any) {}) + if err == nil { + t.Fatalf("add must fail when the cache cannot be re-slotted under the declared name; got nil error") + } + if !strings.Contains(err.Error(), "move marketplace cache") { + t.Fatalf("the failure must be reported as the cache move it is; got: %v", err) + } + if entries, rerr := os.ReadDir(filepath.Join(home, "marketplaces")); rerr == nil && len(entries) != 0 { + t.Errorf("a failed add must register nothing; marketplaces/ holds %d file(s)", len(entries)) + } + if _, serr := os.Stat(filepath.Join(home, ".state", "targets.json")); serr == nil { + t.Errorf("a failed add must record no state entry; %s exists", filepath.Join(home, ".state", "targets.json")) + } + if entries, rerr := os.ReadDir(filepath.Join(home, ".state", "cache", "marketplaces")); rerr == nil && len(entries) != 0 { + t.Errorf("a failed add must leave no fetch cache behind (searchAllMarketplaces would offer it to a bare-id plugin add as an unregistered marketplace); cache root holds %d entr(y/ies)", len(entries)) + } +} diff --git a/internal/cli/marketplace_reslot_test.go b/internal/cli/marketplace_reslot_test.go new file mode 100644 index 00000000..cf7effdf --- /dev/null +++ b/internal/cli/marketplace_reslot_test.go @@ -0,0 +1,89 @@ +package cli_test + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// marketplaceCacheNames lists the marketplace cache directories under home. +func marketplaceCacheNames(t *testing.T, tmp string) []string { + t.Helper() + entries, err := os.ReadDir(filepath.Join(tmp, ".agentsync", ".state", "cache", "marketplaces")) + if err != nil { + t.Fatalf("read marketplace cache root: %v", err) + } + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + return names +} + +// TestMarketplaceAdd_CacheIsSlottedUnderTheRegisteredName pins the invariant +// behind #233: `marketplace add` registers the marketplace under its DECLARED +// name (marketplaces/.toml + the state key), and every later lookup — +// resolveMarketplaceEntry, plugin install/upgrade, the poll index — derives the +// cache directory from that same name. So the cache must end up under the +// declared name and NOWHERE else; the slug directory the fetch lands in is +// scratch. A leftover is not cosmetic: searchAllMarketplaces scans every +// directory under the cache root for a bare-id `plugin add`, so an orphan is a +// second, unregistered copy of the marketplace. +func TestMarketplaceAdd_CacheIsSlottedUnderTheRegisteredName(t *testing.T) { + tmp := t.TempDir() + env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp, "HOME": tmp, "NO_COLOR": "1"} + // The fixture path slugs to "…-fixture-mp", which differs from the declared + // name — the re-slot is exercised, as it is for any real git marketplace. + fixture := writeMarketplaceFixture(t, filepath.Join(tmp, "fixture-mp"), "test-mp") + mustRun(t, env, "init") + mustRun(t, env, "marketplace", "add", fixture) + + if got := marketplaceCacheNames(t, tmp); len(got) != 1 || got[0] != "test-mp" { + t.Fatalf("the cache must live under the declared name and nothing else; cache dirs = %v, want [test-mp]", got) + } + if _, err := os.Stat(filepath.Join(tmp, ".agentsync", "marketplaces", "test-mp.toml")); err != nil { + t.Fatalf("marketplaces/test-mp.toml must be registered under the same name: %v", err) + } + st, err := os.ReadFile(filepath.Join(tmp, ".agentsync", ".state", "targets.json")) + if err != nil { + t.Fatalf("read state: %v", err) + } + if !strings.Contains(string(st), `"test-mp"`) { + t.Fatalf("the state record must key the marketplace by the same name; got:\n%s", st) + } +} + +// TestMarketplaceAdd_ReAddRefreshesTheCache is the regression for the silent +// half of #233. A re-add re-fetches into the slug directory and re-slots it, but +// os.Rename onto the ALREADY-POPULATED declared-name directory fails with +// ENOTEMPTY. With that discarded, `marketplace add` printed success and wrote a +// fresh head_sha while the cache it points at kept the OLD tree (the fresh one +// orphaned under the slug), so a plugin published since the first add stayed +// invisible forever. +func TestMarketplaceAdd_ReAddRefreshesTheCache(t *testing.T) { + tmp := t.TempDir() + env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp, "HOME": tmp, "NO_COLOR": "1"} + fixture := writeMarketplaceFixture(t, filepath.Join(tmp, "fixture-mp"), "test-mp") + mustRun(t, env, "init") + mustRun(t, env, "marketplace", "add", fixture) + + // Upstream publishes a plugin, then the user re-adds the same source. + const republished = `{"name": "test-mp", "owner": {"name": "x"}, "plugins": [{"name": "demo", "source": "./plugins/demo"}]}` + mpJSON := filepath.Join(fixture, ".claude-plugin", "marketplace.json") + if err := os.WriteFile(mpJSON, []byte(republished), 0o644); err != nil { + t.Fatal(err) + } + mustRun(t, env, "marketplace", "add", fixture) + + cached, err := os.ReadFile(filepath.Join(tmp, ".agentsync", ".state", "cache", "marketplaces", "test-mp", ".claude-plugin", "marketplace.json")) + if err != nil { + t.Fatalf("read the re-slotted cache: %v", err) + } + if !strings.Contains(string(cached), `"demo"`) { + t.Fatalf("a re-add must refresh the cache it registers, not keep the stale tree; cached marketplace.json:\n%s", cached) + } + if got := marketplaceCacheNames(t, tmp); len(got) != 1 || got[0] != "test-mp" { + t.Fatalf("a re-add must leave no orphan slug cache behind; cache dirs = %v, want [test-mp]", got) + } +} From 241a74dff25f2a99f0bb4d6ad239c152ece60099 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 17:42:22 +0000 Subject: [PATCH 2/6] fix(cli): close review-loop round 1 on #260 (#233) Round 1 (four lenses on 2ec7bd7): two lenses found the same defect and three converged on the error text. - On a case-insensitive filesystem (macOS, Windows) a slug and a declared name that differ only in case name ONE directory, so the remove-then- rename deleted the freshly fetched tree and then failed, where the old plain rename had succeeded as a case-only rename. reslotMarketplaceCache is now rename-first: os.Rename does the whole job when nothing is at the destination and, on such filesystems, for a case-only alias of the same directory; it reports EEXIST for a different existing directory and fails without touching the destination otherwise. Only EEXIST goes on to the replace, and never when the destination turns out to be the source itself. A missing source therefore no longer costs a marketplace the cache it already has. - The add's error no longer claims nothing changed: a cache already under that name may be gone in the one destructive window left (a rename that fails after the destination was removed), and the message says so. The helper's doc and the call-site comment describe the new shape; the comment's "distinct sibling directories whenever they differ" claim, false on case-folding filesystems, is gone. - Tests: two table arms pin the missing-source and same-directory cases; TestMarketplaceAdd_SameDeclaredNameReplacesTheEarlierCache pins the one intentional behaviour change the CHANGELOG describes; the import test moves beside the other import tests. The CHANGELOG scopes the head_sha clause to git sources. Four mutations each fail exactly their target test: dropping the same- file guard, replacing first, removing the EEXIST gate, and replacing with a plain rename. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 5 +- internal/cli/import_plugin_reslot_test.go | 45 ---------------- internal/cli/import_plugin_test.go | 37 +++++++++++++ internal/cli/marketplace.go | 53 ++++++++++++++----- .../cli/marketplace_reslot_internal_test.go | 38 ++++++++++++- internal/cli/marketplace_reslot_test.go | 34 ++++++++++++ 6 files changed, 151 insertions(+), 61 deletions(-) delete mode 100644 internal/cli/import_plugin_reslot_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b19fd7b..eba3b177 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,8 +26,9 @@ source layout, CLI surface, and state schema are stabilizing but may still chang fixing the routine case: re-adding an already-registered marketplace hit the same swallowed failure every time (a rename onto a non-empty directory), so the cache was never refreshed — a plugin published since the first add stayed - invisible while the recorded `head_sha` moved on — and a duplicate copy - accumulated under the URL-derived name. A failed add now also discards its + invisible while, for a git source, the recorded `head_sha` moved on — and a + duplicate copy accumulated under the URL-derived name. A failed add now also + discards its fetched tree, so nothing is left behind that a bare-id `plugin add` could pick up as an unregistered marketplace; and when two sources declare the same name, the later add now replaces the earlier one's cache along with the diff --git a/internal/cli/import_plugin_reslot_test.go b/internal/cli/import_plugin_reslot_test.go deleted file mode 100644 index 9f341238..00000000 --- a/internal/cli/import_plugin_reslot_test.go +++ /dev/null @@ -1,45 +0,0 @@ -package cli_test - -import ( - "os" - "path/filepath" - "strings" - "testing" -) - -// TestImportPlugin_SkipsMarketplaceWhoseReslotFails pins the import half of -// #233: `import :plugin` registers a native marketplace through the same -// addMarketplaceSource as `marketplace add`, so a cache that cannot be -// re-slotted under the declared name must warn and skip that marketplace — -// never register a phantom whose plugin installs then fail. The forcing -// function is the same hostile 300-character declared name (one path segment, -// over every Linux filesystem's 255-byte cap), so the move fails anywhere this -// runs; the assertions name the re-slot error and check that nothing — TOML, -// state, fetch cache — is left behind. -func TestImportPlugin_SkipsMarketplaceWhoseReslotFails(t *testing.T) { - tmp, env := importTestEnv(t) - longName := strings.Repeat("n", 300) - mpDir := writeMarketplaceFixture(t, filepath.Join(t.TempDir(), "hostile-mp"), longName) - writeClaudeSettings(t, tmp, directoryMarketplaceSettings("hostile", mpDir, "demo")) - - out, err := runCLI(t, env, "import", "claude:plugin") - if err != nil { - t.Fatalf("import must warn and skip the marketplace, not fail: %v\n%s", err, out) - } - for _, want := range []string{"skipping marketplace", "register marketplace", "move marketplace cache"} { - if !strings.Contains(out, want) { - t.Fatalf("import must say which marketplace it skipped and why (missing %q); got:\n%s", want, out) - } - } - home := filepath.Join(tmp, ".agentsync") - if entries, rerr := os.ReadDir(filepath.Join(home, "marketplaces")); rerr == nil && len(entries) != 0 { - t.Errorf("a marketplace whose cache cannot be re-slotted must not be registered; marketplaces/ holds %d file(s)", len(entries)) - } - // importTestEnv's `agent add` already wrote targets.json, so check its content. - if st, rerr := os.ReadFile(filepath.Join(home, ".state", "targets.json")); rerr == nil && strings.Contains(string(st), longName) { - t.Errorf("a skipped marketplace must record no state entry; targets.json names it:\n%s", st) - } - if entries, rerr := os.ReadDir(filepath.Join(home, ".state", "cache", "marketplaces")); rerr == nil && len(entries) != 0 { - t.Errorf("a skipped marketplace must leave no fetch cache behind; cache root holds %d entr(y/ies)", len(entries)) - } -} diff --git a/internal/cli/import_plugin_test.go b/internal/cli/import_plugin_test.go index 9d14d614..43e72cda 100644 --- a/internal/cli/import_plugin_test.go +++ b/internal/cli/import_plugin_test.go @@ -286,3 +286,40 @@ func TestImport_FullAgentIncludesPlugins(t *testing.T) { } } } + +// TestImportPlugin_SkipsMarketplaceWhoseReslotFails pins the import half of +// #233: `import :plugin` registers a native marketplace through the same +// addMarketplaceSource as `marketplace add`, so a cache that cannot be +// re-slotted under the declared name must warn and skip that marketplace — +// never register a phantom whose plugin installs then fail. The forcing +// function is the same hostile 300-character declared name (one path segment, +// over every Linux filesystem's 255-byte cap), so the move fails anywhere this +// runs; the assertions name the re-slot error and check that nothing — TOML, +// state, fetch cache — is left behind. +func TestImportPlugin_SkipsMarketplaceWhoseReslotFails(t *testing.T) { + tmp, env := importTestEnv(t) + longName := strings.Repeat("n", 300) + mpDir := writeMarketplaceFixture(t, filepath.Join(t.TempDir(), "hostile-mp"), longName) + writeClaudeSettings(t, tmp, directoryMarketplaceSettings("hostile", mpDir, "demo")) + + out, err := runCLI(t, env, "import", "claude:plugin") + if err != nil { + t.Fatalf("import must warn and skip the marketplace, not fail: %v\n%s", err, out) + } + for _, want := range []string{"skipping marketplace", "register marketplace", "move marketplace cache"} { + if !strings.Contains(out, want) { + t.Fatalf("import must say which marketplace it skipped and why (missing %q); got:\n%s", want, out) + } + } + home := filepath.Join(tmp, ".agentsync") + if entries, rerr := os.ReadDir(filepath.Join(home, "marketplaces")); rerr == nil && len(entries) != 0 { + t.Errorf("a marketplace whose cache cannot be re-slotted must not be registered; marketplaces/ holds %d file(s)", len(entries)) + } + // importTestEnv's `agent add` already wrote targets.json, so check its content. + if st, rerr := os.ReadFile(filepath.Join(home, ".state", "targets.json")); rerr == nil && strings.Contains(string(st), longName) { + t.Errorf("a skipped marketplace must record no state entry; targets.json names it:\n%s", st) + } + if entries, rerr := os.ReadDir(filepath.Join(home, ".state", "cache", "marketplaces")); rerr == nil && len(entries) != 0 { + t.Errorf("a skipped marketplace must leave no fetch cache behind; cache root holds %d entr(y/ies)", len(entries)) + } +} diff --git a/internal/cli/marketplace.go b/internal/cli/marketplace.go index 14e0ecee..97e7e43c 100644 --- a/internal/cli/marketplace.go +++ b/internal/cli/marketplace.go @@ -2,7 +2,9 @@ package cli import ( "encoding/json" + "errors" "fmt" + "io/fs" "os" "path/filepath" "sort" @@ -133,14 +135,12 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // The URL-derived slug and the declared name differ (the common case for a // git marketplace): re-slot the fetched cache under the declared name. Every // later lookup derives the cache dir from the name this function goes on to - // record, so the two MUST agree — a re-slot that fails and is IGNORED - // registers a marketplace whose cache nothing can find, and the miss reports - // "marketplace %q not found in cache; run: agentsync marketplace add " - // for a marketplace the user just added, with a remedy that repeats the same - // failure (#233). Fail the add instead: the TOML and the state record are - // written below, so an early return leaves nothing half-registered. Both - // names are sanitizeSlug-clean, so marketplaceCacheDir maps them to distinct - // sibling directories whenever they differ. + // record, so the two MUST agree; a re-slot that fails and is ignored + // registers a marketplace whose cache nothing can find (#233). Fail the add + // instead: the TOML and the state record are written below, so an early + // return leaves nothing half-registered. Both names are sanitizeSlug-clean + // (one path segment each); reslotMarketplaceCache handles the filesystems + // that fold their case. if mpName != slug { newCacheDir := marketplaceCacheDir(home, mpName) if err := reslotMarketplaceCache(cacheDir, newCacheDir); err != nil { @@ -150,7 +150,8 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // `plugin add` as an unregistered marketplace. A re-run re-fetches. _ = os.RemoveAll(cacheDir) //nolint:forbidigo // discards the marketplace fetch cache under .state/cache, not a native destination return "", "", fmt.Errorf("register marketplace %q: %w; nothing was written to marketplaces/ "+ - "or the state record — fix the cause above and re-run", mpName, err) + "or the state record, but a cache already under that name may be gone — fix the cause above "+ + "and re-run to fetch it again", mpName, err) } } @@ -236,16 +237,42 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // fails with ENOTEMPTY, which is why a re-add used to keep the STALE cache and // orphan the freshly fetched tree under the slug while reporting success (#233). // -// Every failure is returned. The destination is removed before the rename, so a -// failure after that leaves this marketplace with no cache at all — the add -// says so, naming both paths, and a re-run re-fetches and completes (unlike the -// swallowed failure, which no re-run could repair). +// The move is a plain os.Rename first. That is the whole job when nothing is at +// the destination, and on a case-insensitive filesystem (macOS, Windows) also +// when the destination is this very tree under another spelling — a slug and a +// declared name that differ only in case — because os.Rename accepts a case-only +// rename of the same directory. It reports EEXIST when a DIFFERENT directory is +// already there, and anything else (a missing source, permissions) before the +// destination has been touched; only the EEXIST case goes on to the replace, and +// never when the destination turns out to be the source itself. +// +// Every failure is returned. The one destructive window left is a rename that +// fails after swapDir has removed the destination (a same-parent rename with an +// existing source: I/O or permission failures only): this marketplace then has +// no cache under either name, the add's error says a cache may be gone, and a +// re-run re-fetches and completes (unlike the swallowed failure, which no re-run +// could repair). func reslotMarketplaceCache(from, to string) error { // Belt and braces: the fetch just created `from` under this same parent, so // the only way the parent is missing here is a concurrent removal. if err := os.MkdirAll(filepath.Dir(to), 0o755); err != nil { return fmt.Errorf("prepare marketplace cache dir %s: %w", filepath.Dir(to), err) } + err := os.Rename(from, to) //nolint:forbidigo // moves the marketplace fetch cache under .state/cache, not a native destination + if err == nil { + return nil + } + if !errors.Is(err, fs.ErrExist) { + return fmt.Errorf("move marketplace cache %s → %s: %w", from, to, err) + } + // Never replace the destination with itself: identical paths, or an alias + // the rename above did not resolve, would have swapDir delete the fresh tree + // and then fail to rename what is gone. The tree is already where it belongs. + if fromInfo, ferr := os.Stat(from); ferr == nil { + if toInfo, terr := os.Stat(to); terr == nil && os.SameFile(fromInfo, toInfo) { + return nil + } + } if err := swapDir(from, to); err != nil { return fmt.Errorf("move marketplace cache %s → %s: %w", from, to, err) } diff --git a/internal/cli/marketplace_reslot_internal_test.go b/internal/cli/marketplace_reslot_internal_test.go index c7693a30..c6951a9d 100644 --- a/internal/cli/marketplace_reslot_internal_test.go +++ b/internal/cli/marketplace_reslot_internal_test.go @@ -15,11 +15,17 @@ import ( // only partially — must never be reported as success. The stale-destination case // is the routine one: os.Rename onto a non-empty directory fails with ENOTEMPTY, // so before the fix EVERY re-add kept the STALE tree and orphaned the fresh one. +// Replacing that destination must never cost a marketplace a cache it already +// has: not when the source is missing, and not when the destination IS the +// source under another spelling (a case-insensitive filesystem). func TestReslotMarketplaceCache(t *testing.T) { tests := []struct { name string setup func(t *testing.T, root string) (from, to string) wantErr string // substring; "" means the move must succeed + // after runs once the error has been checked, for arms whose contract + // is about what the failure did NOT touch. + after func(t *testing.T, from, to string) }{ { name: "moves the fetched tree under the declared name", @@ -60,6 +66,33 @@ func TestReslotMarketplaceCache(t *testing.T) { }, wantErr: "move marketplace cache", }, + { + name: "leaves an existing destination alone when there is nothing to move", + setup: func(t *testing.T, root string) (string, string) { + to := filepath.Join(root, "declared") + mustWrite(t, filepath.Join(to, "stale.txt"), "stale") + return filepath.Join(root, "slug"), to + }, + wantErr: "move marketplace cache", + after: func(t *testing.T, _, to string) { + if _, err := os.Stat(filepath.Join(to, "stale.txt")); err != nil { + t.Errorf("a missing source must not cost the marketplace its existing cache; stale.txt under %s: %v", to, err) + } + }, + }, + { + // On a case-insensitive filesystem (macOS, Windows) a slug and a + // declared name that differ only in case are ONE directory; this + // Linux-only suite stands that in with identical paths. A replace + // here would remove the fresh tree and then fail to rename what is + // gone; the move must adopt the spelling and keep the tree. + name: "adopts the declared spelling when the destination already names the fetched tree", + setup: func(t *testing.T, root string) (string, string) { + from := filepath.Join(root, "slug") + mustWrite(t, filepath.Join(from, "marker.txt"), "fresh") + return from, from + }, + }, } for _, tc := range tests { @@ -76,6 +109,9 @@ func TestReslotMarketplaceCache(t *testing.T) { if !strings.Contains(err.Error(), tc.wantErr) { t.Fatalf("error must name the failing step %q; got: %v", tc.wantErr, err) } + if tc.after != nil { + tc.after(t, from, to) + } return } if err != nil { @@ -91,7 +127,7 @@ func TestReslotMarketplaceCache(t *testing.T) { if _, err := os.Lstat(filepath.Join(to, "gone.txt")); err == nil { t.Errorf("a stale cache must be replaced, not merged: gone.txt survived at %s", to) } - if _, err := os.Lstat(from); !os.IsNotExist(err) { + if _, err := os.Lstat(from); from != to && !os.IsNotExist(err) { t.Errorf("the slug directory must not survive the move (it would be an orphan cache): %s (err=%v)", from, err) } }) diff --git a/internal/cli/marketplace_reslot_test.go b/internal/cli/marketplace_reslot_test.go index cf7effdf..9c1f11d9 100644 --- a/internal/cli/marketplace_reslot_test.go +++ b/internal/cli/marketplace_reslot_test.go @@ -87,3 +87,37 @@ func TestMarketplaceAdd_ReAddRefreshesTheCache(t *testing.T) { t.Fatalf("a re-add must leave no orphan slug cache behind; cache dirs = %v, want [test-mp]", got) } } + +// TestMarketplaceAdd_SameDeclaredNameReplacesTheEarlierCache pins the one +// behaviour change the fix makes on purpose. When two different sources declare +// the same name, marketplaces/.toml and the state record were already +// last-writer-wins; the cache now follows them instead of keeping the first +// source's tree (os.Rename onto it failed with ENOTEMPTY, silently), so the +// registration and the cache describe the same source again. +func TestMarketplaceAdd_SameDeclaredNameReplacesTheEarlierCache(t *testing.T) { + tmp := t.TempDir() + env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp, "HOME": tmp, "NO_COLOR": "1"} + first := writeMarketplaceFixture(t, filepath.Join(tmp, "first-mp"), "shared") + second := writeMarketplaceFixture(t, filepath.Join(tmp, "second-mp"), "shared") + // Distinct plugin lists tell the two trees apart once cached. + for dir, plugin := range map[string]string{first: "alpha", second: "beta"} { + body := `{"name": "shared", "owner": {"name": "x"}, "plugins": [{"name": "` + plugin + `", "source": "./plugins/` + plugin + `"}]}` + if err := os.WriteFile(filepath.Join(dir, ".claude-plugin", "marketplace.json"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + mustRun(t, env, "init") + mustRun(t, env, "marketplace", "add", first) + mustRun(t, env, "marketplace", "add", second) + + cached, err := os.ReadFile(filepath.Join(tmp, ".agentsync", ".state", "cache", "marketplaces", "shared", ".claude-plugin", "marketplace.json")) + if err != nil { + t.Fatalf("read the cache under the shared name: %v", err) + } + if !strings.Contains(string(cached), `"beta"`) || strings.Contains(string(cached), `"alpha"`) { + t.Fatalf("the later add must replace the earlier source's cache along with its record; cached marketplace.json:\n%s", cached) + } + if got := marketplaceCacheNames(t, tmp); len(got) != 1 || got[0] != "shared" { + t.Fatalf("no orphan may remain under either slug; cache dirs = %v, want [shared]", got) + } +} From 99288e78aeed42bb76015e7d73aca88d69250b52 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 17:45:27 +0000 Subject: [PATCH 3/6] fix(cli): gate the cache replace on stat, not on the rename's errno (#233) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 follow-up on #260. The replace step ran only when the failed rename reported EEXIST. That is Linux's answer for an occupied destination; Go's os.Rename on Windows has no directory pre-check and MoveFileEx onto an existing directory reports access denied, which maps to ErrPermission — so a re-add on Windows would have failed cleanly instead of refreshing the cache. The decision is now made by stat: after a failed rename, replace only when both the source and the destination exist and are different files; if either stat fails the failure is genuine and the destination is left as it was; the same file under two spellings is already in place. Same tests; the guard and gate mutations each fail exactly their arm. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- internal/cli/marketplace.go | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/internal/cli/marketplace.go b/internal/cli/marketplace.go index 97e7e43c..42a606b8 100644 --- a/internal/cli/marketplace.go +++ b/internal/cli/marketplace.go @@ -2,9 +2,7 @@ package cli import ( "encoding/json" - "errors" "fmt" - "io/fs" "os" "path/filepath" "sort" @@ -240,11 +238,13 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // The move is a plain os.Rename first. That is the whole job when nothing is at // the destination, and on a case-insensitive filesystem (macOS, Windows) also // when the destination is this very tree under another spelling — a slug and a -// declared name that differ only in case — because os.Rename accepts a case-only -// rename of the same directory. It reports EEXIST when a DIFFERENT directory is -// already there, and anything else (a missing source, permissions) before the -// destination has been touched; only the EEXIST case goes on to the replace, and -// never when the destination turns out to be the source itself. +// declared name that differ only in case — because a case-only rename of the +// same directory is accepted. A rename that fails has touched nothing; it goes +// on to the replace only when a DIFFERENT directory is standing at the +// destination while the source is still there. That is decided by stat, not by +// the error: Linux reports EEXIST for an occupied destination, Windows reports +// access denied, and a missing source or a permission failure must never be +// answered by removing the cache the marketplace already has. // // Every failure is returned. The one destructive window left is a rename that // fails after swapDir has removed the destination (a same-parent rename with an @@ -262,16 +262,18 @@ func reslotMarketplaceCache(from, to string) error { if err == nil { return nil } - if !errors.Is(err, fs.ErrExist) { + fromInfo, ferr := os.Stat(from) + toInfo, terr := os.Stat(to) + if ferr != nil || terr != nil { + // No source to move, or nothing standing in the way: the failure is + // genuine, and the destination is left exactly as it was. return fmt.Errorf("move marketplace cache %s → %s: %w", from, to, err) } - // Never replace the destination with itself: identical paths, or an alias - // the rename above did not resolve, would have swapDir delete the fresh tree - // and then fail to rename what is gone. The tree is already where it belongs. - if fromInfo, ferr := os.Stat(from); ferr == nil { - if toInfo, terr := os.Stat(to); terr == nil && os.SameFile(fromInfo, toInfo) { - return nil - } + if os.SameFile(fromInfo, toInfo) { + // Identical paths, or an alias the rename above did not resolve: the + // tree is already where it belongs. Replacing it would delete the fresh + // tree and then fail to rename what is gone. + return nil } if err := swapDir(from, to); err != nil { return fmt.Errorf("move marketplace cache %s → %s: %w", from, to, err) From 00d448755ab911add86c2b8e2c014c64b159ef04 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 17:56:51 +0000 Subject: [PATCH 4/6] fix(cli): close review-loop round 2 on #260 (#233) Round 2 (four lenses on 99288e7): three lenses CLEAN with NITs, the adversarial lens with two findings about symlinks at the destination. - The gate decides with Lstat, not Stat. A symlink standing at the declared-name path is a link to unlink, not the tree under another name: with Stat, a planted link pointing at the source read as "already in place", the tree stayed under the slug behind the link, and a later `marketplace remove` would unlink the link alone and leave the slug tree as an orphan a bare-id `plugin add` could pick up. - Two table arms pin the symlink cases: replacing a symlinked destination unlinks the link and never touches its target (a resolve-then-remove would delete data outside the cache root with every other test green), and a link pointing at the source is replaced rather than mistaken for the tree. The plain missing-source arm is folded into the one that also checks the destination was left alone; the identical-paths arm is named for what it pins ("keeps the tree when the destination already is the fetched tree") rather than a second spelling Linux cannot produce. - Wording: Go refuses any directory destination before the syscall, so "a rename onto a non-empty directory" and ENOTEMPTY become "an existing directory" in the helper doc, both test files and the CHANGELOG; the add's advice is "fix the cause and re-run" (nothing is "above" on one line, least of all under import's prefix); the failure-path discard is marked best-effort; the CHANGELOG bullet is reflowed. Two mutations each fail exactly their arm: Stat instead of Lstat fails the link-to-source arm; resolving the destination's symlink before the replace fails the link-elsewhere arm. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 12 ++-- internal/cli/marketplace.go | 20 +++--- .../cli/marketplace_reslot_internal_test.go | 67 ++++++++++++++----- internal/cli/marketplace_reslot_test.go | 6 +- 4 files changed, 72 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eba3b177..1930d0b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,15 +24,15 @@ source layout, CLI surface, and state schema are stabilizing but may still chang The move now reports its failures and the add stops before registering anything. It also **replaces** an existing cache instead of failing on it, fixing the routine case: re-adding an already-registered marketplace hit the - same swallowed failure every time (a rename onto a non-empty directory), so + same swallowed failure every time (a rename onto an existing directory), so the cache was never refreshed — a plugin published since the first add stayed invisible while, for a git source, the recorded `head_sha` moved on — and a duplicate copy accumulated under the URL-derived name. A failed add now also - discards its - fetched tree, so nothing is left behind that a bare-id `plugin add` could pick - up as an unregistered marketplace; and when two sources declare the same name, - the later add now replaces the earlier one's cache along with the - `marketplaces/.toml` and state record it already overwrote. + discards its fetched tree, so nothing is left behind that a bare-id + `plugin add` could pick up as an unregistered marketplace; and when two + sources declare the same name, the later add now replaces the earlier one's + cache along with the `marketplaces/.toml` and state record it already + overwrote. `import :plugin` registers marketplaces through the same code and now warns and skips instead of registering a phantom. diff --git a/internal/cli/marketplace.go b/internal/cli/marketplace.go index 42a606b8..d56c5af3 100644 --- a/internal/cli/marketplace.go +++ b/internal/cli/marketplace.go @@ -145,10 +145,11 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // Discard the fetched tree rather than leave it under the slug: no // record points at that directory, `marketplace remove` cannot reach // it, and searchAllMarketplaces would still offer it to a bare-id - // `plugin add` as an unregistered marketplace. A re-run re-fetches. + // `plugin add` as an unregistered marketplace. Best-effort: a re-run + // re-fetches into whatever is left and re-slots it. _ = os.RemoveAll(cacheDir) //nolint:forbidigo // discards the marketplace fetch cache under .state/cache, not a native destination return "", "", fmt.Errorf("register marketplace %q: %w; nothing was written to marketplaces/ "+ - "or the state record, but a cache already under that name may be gone — fix the cause above "+ + "or the state record, but a cache already under that name may be gone — fix the cause "+ "and re-run to fetch it again", mpName, err) } } @@ -231,9 +232,10 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // An EXISTING destination is replaced, not merged (swapDir): it is a stale tree // from an earlier add of this marketplace — or of another one declaring the same // name, whose marketplaces/.toml and state record this add overwrites -// regardless, so the cache must follow. os.Rename onto a non-empty directory -// fails with ENOTEMPTY, which is why a re-add used to keep the STALE cache and -// orphan the freshly fetched tree under the slug while reporting success (#233). +// regardless, so the cache must follow. os.Rename onto an existing directory +// always fails (Go refuses a directory destination before the syscall), which +// is why a re-add used to keep the STALE cache and orphan the freshly fetched +// tree under the slug while reporting success (#233). // // The move is a plain os.Rename first. That is the whole job when nothing is at // the destination, and on a case-insensitive filesystem (macOS, Windows) also @@ -244,7 +246,9 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // destination while the source is still there. That is decided by stat, not by // the error: Linux reports EEXIST for an occupied destination, Windows reports // access denied, and a missing source or a permission failure must never be -// answered by removing the cache the marketplace already has. +// answered by removing the cache the marketplace already has. Lstat, not Stat: +// a symlink standing at the destination is a link to unlink (swapDir removes +// the link, never what it points at), not this tree under another name. // // Every failure is returned. The one destructive window left is a rename that // fails after swapDir has removed the destination (a same-parent rename with an @@ -262,8 +266,8 @@ func reslotMarketplaceCache(from, to string) error { if err == nil { return nil } - fromInfo, ferr := os.Stat(from) - toInfo, terr := os.Stat(to) + fromInfo, ferr := os.Lstat(from) + toInfo, terr := os.Lstat(to) if ferr != nil || terr != nil { // No source to move, or nothing standing in the way: the failure is // genuine, and the destination is left exactly as it was. diff --git a/internal/cli/marketplace_reslot_internal_test.go b/internal/cli/marketplace_reslot_internal_test.go index c6951a9d..60e122e1 100644 --- a/internal/cli/marketplace_reslot_internal_test.go +++ b/internal/cli/marketplace_reslot_internal_test.go @@ -13,8 +13,8 @@ import ( // under the marketplace's DECLARED name (#233): that name is what every later // lookup derives the cache dir from, so a move that does not happen — or happens // only partially — must never be reported as success. The stale-destination case -// is the routine one: os.Rename onto a non-empty directory fails with ENOTEMPTY, -// so before the fix EVERY re-add kept the STALE tree and orphaned the fresh one. +// is the routine one: os.Rename onto an existing directory always fails, so +// before the fix EVERY re-add kept the STALE tree and orphaned the fresh one. // Replacing that destination must never cost a marketplace a cache it already // has: not when the source is missing, and not when the destination IS the // source under another spelling (a case-insensitive filesystem). @@ -23,8 +23,8 @@ func TestReslotMarketplaceCache(t *testing.T) { name string setup func(t *testing.T, root string) (from, to string) wantErr string // substring; "" means the move must succeed - // after runs once the error has been checked, for arms whose contract - // is about what the failure did NOT touch. + // after runs once the outcome has been checked, for arms whose contract + // is also about what the move did NOT touch. after func(t *testing.T, from, to string) }{ { @@ -58,14 +58,6 @@ func TestReslotMarketplaceCache(t *testing.T) { }, wantErr: "prepare marketplace cache dir", }, - { - name: "propagates a failure to move the tree", - setup: func(t *testing.T, root string) (string, string) { - // Nothing at `from`: the rename cannot succeed. - return filepath.Join(root, "slug"), filepath.Join(root, "declared") - }, - wantErr: "move marketplace cache", - }, { name: "leaves an existing destination alone when there is nothing to move", setup: func(t *testing.T, root string) (string, string) { @@ -83,16 +75,56 @@ func TestReslotMarketplaceCache(t *testing.T) { { // On a case-insensitive filesystem (macOS, Windows) a slug and a // declared name that differ only in case are ONE directory; this - // Linux-only suite stands that in with identical paths. A replace - // here would remove the fresh tree and then fail to rename what is - // gone; the move must adopt the spelling and keep the tree. - name: "adopts the declared spelling when the destination already names the fetched tree", + // Linux-only suite stands that in with identical paths, which reach + // the same guard. A replace here would remove the fresh tree and then + // fail to rename what is gone; the tree must be left in place. + name: "keeps the tree when the destination already is the fetched tree", setup: func(t *testing.T, root string) (string, string) { from := filepath.Join(root, "slug") mustWrite(t, filepath.Join(from, "marker.txt"), "fresh") return from, from }, }, + { + // A symlink standing at the destination is a LINK to unlink, not the + // tree under another name, and replacing it must never reach through + // to whatever it points at. + name: "replaces a symlink at the destination without touching its target", + setup: func(t *testing.T, root string) (string, string) { + from := filepath.Join(root, "slug") + mustWrite(t, filepath.Join(from, "marker.txt"), "fresh") + mustWrite(t, filepath.Join(root, "elsewhere", "keep.txt"), "keep") + to := filepath.Join(root, "declared") + if err := os.Symlink(filepath.Join(root, "elsewhere"), to); err != nil { + t.Fatal(err) + } + return from, to + }, + after: func(t *testing.T, _, to string) { + if _, err := os.Stat(filepath.Join(filepath.Dir(to), "elsewhere", "keep.txt")); err != nil { + t.Errorf("replacing a symlinked destination must unlink the link, never its target: %v", err) + } + if fi, err := os.Lstat(to); err != nil || fi.Mode()&os.ModeSymlink != 0 { + t.Errorf("the destination must now be the real tree, not a link (err=%v)", err) + } + }, + }, + { + // A symlink at the destination that points at the SOURCE is what a + // following stat would mistake for "already in place": the tree must + // still move under the declared name, or the slug directory becomes + // an orphan behind a link that `marketplace remove` unlinks alone. + name: "replaces a symlink at the destination even when it points at the source", + setup: func(t *testing.T, root string) (string, string) { + from := filepath.Join(root, "slug") + mustWrite(t, filepath.Join(from, "marker.txt"), "fresh") + to := filepath.Join(root, "declared") + if err := os.Symlink(from, to); err != nil { + t.Fatal(err) + } + return from, to + }, + }, } for _, tc := range tests { @@ -130,6 +162,9 @@ func TestReslotMarketplaceCache(t *testing.T) { if _, err := os.Lstat(from); from != to && !os.IsNotExist(err) { t.Errorf("the slug directory must not survive the move (it would be an orphan cache): %s (err=%v)", from, err) } + if tc.after != nil { + tc.after(t, from, to) + } }) } } diff --git a/internal/cli/marketplace_reslot_test.go b/internal/cli/marketplace_reslot_test.go index 9c1f11d9..5a8d886f 100644 --- a/internal/cli/marketplace_reslot_test.go +++ b/internal/cli/marketplace_reslot_test.go @@ -56,8 +56,8 @@ func TestMarketplaceAdd_CacheIsSlottedUnderTheRegisteredName(t *testing.T) { // TestMarketplaceAdd_ReAddRefreshesTheCache is the regression for the silent // half of #233. A re-add re-fetches into the slug directory and re-slots it, but -// os.Rename onto the ALREADY-POPULATED declared-name directory fails with -// ENOTEMPTY. With that discarded, `marketplace add` printed success and wrote a +// os.Rename onto the EXISTING declared-name directory always fails. With that +// discarded, `marketplace add` printed success and wrote a // fresh head_sha while the cache it points at kept the OLD tree (the fresh one // orphaned under the slug), so a plugin published since the first add stayed // invisible forever. @@ -92,7 +92,7 @@ func TestMarketplaceAdd_ReAddRefreshesTheCache(t *testing.T) { // behaviour change the fix makes on purpose. When two different sources declare // the same name, marketplaces/.toml and the state record were already // last-writer-wins; the cache now follows them instead of keeping the first -// source's tree (os.Rename onto it failed with ENOTEMPTY, silently), so the +// source's tree (os.Rename onto it failed, silently), so the // registration and the cache describe the same source again. func TestMarketplaceAdd_SameDeclaredNameReplacesTheEarlierCache(t *testing.T) { tmp := t.TempDir() From 23e8398db7216b5fab5969a13098ed6b5250dfd9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 18:17:00 +0000 Subject: [PATCH 5/6] fix(cli): close review-loop round 3 on #260 (#233) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit swapDir, shared by the marketplace re-slot and the plugin-upgrade cache swap, removed the destination before renaming the source in, so an I/O failure between the two left the marketplace with no cache under either name — the pattern internal/marketplace's extractSubdir documents as a retired bug. It now renames the old tree aside, renames the new one into place, discards the aside only then, and rolls back when the second rename fails; the aside's name holds "..", which no sanitized cache key can, and a leftover from an interrupted swap is cleared first. The poll engine's "cache (old) and TOML (old) stay consistent" comment is true now, and the add's error no longer warns that a cache may be gone. reslotMarketplaceCache refuses a symlink standing at the source: no fetcher leaves one, and moving it would install a link (dangling, when it points at the destination) as the registered cache. The helper doc and the test header no longer say a rename onto an existing directory "always" fails — Go lets a case-only alias of the same directory through. Tests: TestSwapDir (replace, empty destination, rollback on a missing source, leftover aside cleared; no aside survives any arm); the stale-replace arm asserts no aside survives; a symlink-at-source arm. Break-verified: dropping the aside's final removal fails the stale-replace arm, two TestSwapDir arms and both re-add e2e tests; dropping the rollback fails exactly the missing-source TestSwapDir arm; dropping the leading clear fails exactly the leftover-aside arm; reverting swapDir to remove-then-rename fails the missing-source and leftover-aside arms; disabling the symlink-at-source guard fails exactly its arm. CHANGELOG notes the non-destructive replace. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 5 +- internal/cli/marketplace.go | 45 +++++----- .../cli/marketplace_reslot_internal_test.go | 42 ++++++++- internal/cli/marketplace_reslot_test.go | 13 ++- internal/cli/plugin_poll.go | 29 +++++-- internal/cli/plugin_poll_internal_test.go | 85 +++++++++++++++++++ 6 files changed, 184 insertions(+), 35 deletions(-) create mode 100644 internal/cli/plugin_poll_internal_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 1930d0b4..2a3ec97c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,7 +32,10 @@ source layout, CLI surface, and state schema are stabilizing but may still chang `plugin add` could pick up as an unregistered marketplace; and when two sources declare the same name, the later add now replaces the earlier one's cache along with the `marketplaces/.toml` and state record it already - overwrote. + overwrote. The replace itself — shared with the cache swap `plugin upgrade` + performs — keeps the old tree until the new one is in place, so a replace + that fails part-way leaves the marketplace or plugin the cache it had rather + than none. `import :plugin` registers marketplaces through the same code and now warns and skips instead of registering a phantom. diff --git a/internal/cli/marketplace.go b/internal/cli/marketplace.go index d56c5af3..76563afa 100644 --- a/internal/cli/marketplace.go +++ b/internal/cli/marketplace.go @@ -149,8 +149,7 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // re-fetches into whatever is left and re-slots it. _ = os.RemoveAll(cacheDir) //nolint:forbidigo // discards the marketplace fetch cache under .state/cache, not a native destination return "", "", fmt.Errorf("register marketplace %q: %w; nothing was written to marketplaces/ "+ - "or the state record, but a cache already under that name may be gone — fix the cause "+ - "and re-run to fetch it again", mpName, err) + "or the state record — fix the cause and re-run", mpName, err) } } @@ -232,30 +231,32 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // An EXISTING destination is replaced, not merged (swapDir): it is a stale tree // from an earlier add of this marketplace — or of another one declaring the same // name, whose marketplaces/.toml and state record this add overwrites -// regardless, so the cache must follow. os.Rename onto an existing directory -// always fails (Go refuses a directory destination before the syscall), which -// is why a re-add used to keep the STALE cache and orphan the freshly fetched -// tree under the slug while reporting success (#233). +// regardless, so the cache must follow. os.Rename onto a DIFFERENT existing +// directory fails (on Unix Go refuses it before the syscall, on Windows the +// syscall does; only a case-only alias of the same directory goes through), +// which is why a re-add used to keep the STALE cache and orphan the freshly +// fetched tree under the slug while reporting success (#233). // // The move is a plain os.Rename first. That is the whole job when nothing is at // the destination, and on a case-insensitive filesystem (macOS, Windows) also // when the destination is this very tree under another spelling — a slug and a // declared name that differ only in case — because a case-only rename of the // same directory is accepted. A rename that fails has touched nothing; it goes -// on to the replace only when a DIFFERENT directory is standing at the -// destination while the source is still there. That is decided by stat, not by -// the error: Linux reports EEXIST for an occupied destination, Windows reports -// access denied, and a missing source or a permission failure must never be -// answered by removing the cache the marketplace already has. Lstat, not Stat: -// a symlink standing at the destination is a link to unlink (swapDir removes -// the link, never what it points at), not this tree under another name. +// on to the replace only when a DIFFERENT entry (a directory or a symlink) is +// standing at the destination while the fetched tree is still at the source. +// That is decided by stat, not by the error: Linux reports EEXIST for an +// occupied destination, Windows reports access denied, and a missing source or +// a permission failure must never be answered by touching the cache the +// marketplace already has. Lstat, not Stat: a symlink standing at the +// destination is a link to unlink (swapDir removes the link, never what it +// points at), not this tree under another name; and a symlink standing at the +// SOURCE is not a fetched tree to move — no fetcher leaves one — so it is +// refused rather than installed as the cache. // -// Every failure is returned. The one destructive window left is a rename that -// fails after swapDir has removed the destination (a same-parent rename with an -// existing source: I/O or permission failures only): this marketplace then has -// no cache under either name, the add's error says a cache may be gone, and a -// re-run re-fetches and completes (unlike the swallowed failure, which no re-run -// could repair). +// Every failure is returned, and none costs the marketplace the cache it had: +// swapDir keeps the old tree until the fresh one is standing in its place, so a +// replace that fails part-way leaves the destination as it was and the source +// where it was, and a re-run re-fetches and completes. func reslotMarketplaceCache(from, to string) error { // Belt and braces: the fetch just created `from` under this same parent, so // the only way the parent is missing here is a concurrent removal. @@ -273,6 +274,12 @@ func reslotMarketplaceCache(from, to string) error { // genuine, and the destination is left exactly as it was. return fmt.Errorf("move marketplace cache %s → %s: %w", from, to, err) } + if fromInfo.Mode()&os.ModeSymlink != 0 { + // A link where the fetched tree should be: moving it would install a + // link (to anywhere, the destination itself included) as the cache, and + // the replace below would first unlink what the marketplace has. + return fmt.Errorf("move marketplace cache %s → %s: the source is a symlink, not a fetched tree", from, to) + } if os.SameFile(fromInfo, toInfo) { // Identical paths, or an alias the rename above did not resolve: the // tree is already where it belongs. Replacing it would delete the fresh diff --git a/internal/cli/marketplace_reslot_internal_test.go b/internal/cli/marketplace_reslot_internal_test.go index 60e122e1..051246fc 100644 --- a/internal/cli/marketplace_reslot_internal_test.go +++ b/internal/cli/marketplace_reslot_internal_test.go @@ -13,11 +13,12 @@ import ( // under the marketplace's DECLARED name (#233): that name is what every later // lookup derives the cache dir from, so a move that does not happen — or happens // only partially — must never be reported as success. The stale-destination case -// is the routine one: os.Rename onto an existing directory always fails, so +// is the routine one: os.Rename onto a different existing directory fails, so // before the fix EVERY re-add kept the STALE tree and orphaned the fresh one. // Replacing that destination must never cost a marketplace a cache it already -// has: not when the source is missing, and not when the destination IS the -// source under another spelling (a case-insensitive filesystem). +// has: not when the source is missing or is a link rather than a tree, and not +// when the destination IS the source under another spelling (a case-insensitive +// filesystem). func TestReslotMarketplaceCache(t *testing.T) { tests := []struct { name string @@ -45,6 +46,13 @@ func TestReslotMarketplaceCache(t *testing.T) { mustWrite(t, filepath.Join(to, "gone.txt"), "stale") return from, to }, + after: func(t *testing.T, _, to string) { + // The stale tree is renamed aside while the fresh one moves in and + // discarded only afterwards; a completed replace leaves no aside. + if _, err := os.Lstat(to + "..old"); !os.IsNotExist(err) { + t.Errorf("a completed replace must discard the tree it moved aside: %s..old (err=%v)", to, err) + } + }, }, { name: "propagates a failure to prepare the cache root", @@ -72,6 +80,34 @@ func TestReslotMarketplaceCache(t *testing.T) { } }, }, + { + // A link where the fetched tree should be is not a tree to move: no + // fetcher leaves one, and installing the link as the cache (a link to + // the destination itself would dangle once the stale tree is gone) + // must be refused with the destination untouched. + name: "refuses a symlink at the source rather than moving the link", + setup: func(t *testing.T, root string) (string, string) { + to := filepath.Join(root, "declared") + mustWrite(t, filepath.Join(to, "stale.txt"), "stale") + from := filepath.Join(root, "slug") + if err := os.Symlink(to, from); err != nil { + t.Fatal(err) + } + return from, to + }, + wantErr: "move marketplace cache", + after: func(t *testing.T, from, to string) { + if _, err := os.Stat(filepath.Join(to, "stale.txt")); err != nil { + t.Errorf("a refused move must leave the existing cache alone; stale.txt under %s: %v", to, err) + } + if fi, err := os.Lstat(to); err != nil || fi.Mode()&os.ModeSymlink != 0 { + t.Errorf("the destination must still be the real tree, not a link (err=%v)", err) + } + if fi, err := os.Lstat(from); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Errorf("the refused link must be left where it was (err=%v)", err) + } + }, + }, { // On a case-insensitive filesystem (macOS, Windows) a slug and a // declared name that differ only in case are ONE directory; this diff --git a/internal/cli/marketplace_reslot_test.go b/internal/cli/marketplace_reslot_test.go index 5a8d886f..3d265588 100644 --- a/internal/cli/marketplace_reslot_test.go +++ b/internal/cli/marketplace_reslot_test.go @@ -56,11 +56,10 @@ func TestMarketplaceAdd_CacheIsSlottedUnderTheRegisteredName(t *testing.T) { // TestMarketplaceAdd_ReAddRefreshesTheCache is the regression for the silent // half of #233. A re-add re-fetches into the slug directory and re-slots it, but -// os.Rename onto the EXISTING declared-name directory always fails. With that -// discarded, `marketplace add` printed success and wrote a -// fresh head_sha while the cache it points at kept the OLD tree (the fresh one -// orphaned under the slug), so a plugin published since the first add stayed -// invisible forever. +// os.Rename onto the EXISTING declared-name directory fails. With that failure +// discarded, `marketplace add` printed success and wrote a fresh head_sha while +// the cache it points at kept the OLD tree (the fresh one orphaned under the +// slug), so a plugin published since the first add stayed invisible forever. func TestMarketplaceAdd_ReAddRefreshesTheCache(t *testing.T) { tmp := t.TempDir() env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp, "HOME": tmp, "NO_COLOR": "1"} @@ -92,8 +91,8 @@ func TestMarketplaceAdd_ReAddRefreshesTheCache(t *testing.T) { // behaviour change the fix makes on purpose. When two different sources declare // the same name, marketplaces/.toml and the state record were already // last-writer-wins; the cache now follows them instead of keeping the first -// source's tree (os.Rename onto it failed, silently), so the -// registration and the cache describe the same source again. +// source's tree (os.Rename onto it failed, silently), so the registration and +// the cache describe the same source again. func TestMarketplaceAdd_SameDeclaredNameReplacesTheEarlierCache(t *testing.T) { tmp := t.TempDir() env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp, "HOME": tmp, "NO_COLOR": "1"} diff --git a/internal/cli/plugin_poll.go b/internal/cli/plugin_poll.go index 6f342cfe..6f5a6c3e 100644 --- a/internal/cli/plugin_poll.go +++ b/internal/cli/plugin_poll.go @@ -437,14 +437,33 @@ func applyPluginBump(home string, b marketplace.Bump, fetched map[string]map[str return nil } -// swapDir replaces dst with src by removing dst and renaming src into place. -// src and dst must be on the same filesystem (callers create src as a sibling -// of dst). After a successful swap src no longer exists. +// swapDir replaces dst with src: dst is renamed aside, src is renamed into +// place, and only then is the old tree discarded. src and dst must be on the +// same filesystem (callers create src as a sibling of dst). After a successful +// swap src no longer exists. +// +// The old tree survives until the new one is standing at dst. A rename that +// fails part-way is rolled back, so a marketplace or plugin never loses the +// cache it had to a replace that did not complete — the shape extractSubdir in +// internal/marketplace documents (RemoveAll-then-Rename destroyed the cache +// whenever the rename then failed). The aside is a sibling of dst whose name +// holds "..", which sanitizeCacheKey never lets into a cache key, so it can +// never be another plugin's or marketplace's cache; a leftover from an +// interrupted earlier swap is cleared first, as extractSubdir clears its own. func swapDir(src, dst string) error { - if err := os.RemoveAll(dst); err != nil { + aside := dst + "..old" + if err := os.RemoveAll(aside); err != nil { + return err + } + if err := os.Rename(dst, aside); err != nil && !os.IsNotExist(err) { return err } - return os.Rename(src, dst) + if err := os.Rename(src, dst); err != nil { + _ = os.Rename(aside, dst) // best-effort rollback; a no-op when dst was absent + return err + } + _ = os.RemoveAll(aside) // best-effort; the next swap clears a leftover + return nil } // filterSafeBumps partitions bumps for `plugin upgrade --all --lossless` into diff --git a/internal/cli/plugin_poll_internal_test.go b/internal/cli/plugin_poll_internal_test.go new file mode 100644 index 00000000..3eb30f90 --- /dev/null +++ b/internal/cli/plugin_poll_internal_test.go @@ -0,0 +1,85 @@ +package cli + +import ( + "os" + "path/filepath" + "testing" +) + +// TestSwapDir pins the replace both cache callers rely on — the plugin-upgrade +// cache swap and the marketplace re-slot (#233): the old tree must survive +// until the new one is standing in its place, so a swap that fails part-way +// leaves the cache the caller had rather than none, and a completed swap leaves +// neither the source nor the tree it moved aside. +func TestSwapDir(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T, src, dst string) + wantErr bool + // wantDst is the marker the destination must hold afterwards. + wantDst string + }{ + { + name: "replaces the old tree with the new one", + setup: func(t *testing.T, src, dst string) { + mustWrite(t, filepath.Join(src, "marker.txt"), "fresh") + mustWrite(t, filepath.Join(dst, "marker.txt"), "stale") + }, + wantDst: "fresh", + }, + { + name: "installs the new tree when nothing is at the destination", + setup: func(t *testing.T, src, _ string) { + mustWrite(t, filepath.Join(src, "marker.txt"), "fresh") + }, + wantDst: "fresh", + }, + { + // The forcing function is a missing source: the old tree has already + // been moved aside when the rename into place fails, and it must + // come back rather than stay aside or be discarded. + name: "keeps the old tree when the new one cannot be moved in", + setup: func(t *testing.T, _, dst string) { + mustWrite(t, filepath.Join(dst, "marker.txt"), "stale") + }, + wantErr: true, + wantDst: "stale", + }, + { + name: "clears a leftover aside from an interrupted earlier swap", + setup: func(t *testing.T, src, dst string) { + mustWrite(t, filepath.Join(src, "marker.txt"), "fresh") + mustWrite(t, filepath.Join(dst, "marker.txt"), "stale") + mustWrite(t, filepath.Join(dst+"..old", "marker.txt"), "older") + }, + wantDst: "fresh", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + src, dst := filepath.Join(root, "incoming"), filepath.Join(root, "cache") + tc.setup(t, src, dst) + + err := swapDir(src, dst) + + if (err != nil) != tc.wantErr { + t.Fatalf("swapDir error = %v, wantErr %v", err, tc.wantErr) + } + got, rerr := os.ReadFile(filepath.Join(dst, "marker.txt")) + if rerr != nil { + t.Fatalf("the destination must hold a tree afterwards: %v", rerr) + } + if string(got) != tc.wantDst { + t.Errorf("destination marker.txt = %q, want %q", got, tc.wantDst) + } + if _, lerr := os.Lstat(dst + "..old"); !os.IsNotExist(lerr) { + t.Errorf("no aside may survive the swap: %s..old (err=%v)", dst, lerr) + } + if _, lerr := os.Lstat(src); !tc.wantErr && !os.IsNotExist(lerr) { + t.Errorf("a completed swap must consume the source: %s (err=%v)", src, lerr) + } + }) + } +} From b768f49d5e35e240e76999d6751823d57991c777 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 18:34:30 +0000 Subject: [PATCH 6/6] fix(cli): close review-loop round 4 on #260 (#233) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3's symlink-at-source guard sat after the plain rename, which succeeds into an empty slot — the first-add path — and installed the link as the registered cache; the refusal now comes before anything moves, and an arm with an empty destination pins it. swapDir reported nothing when the rename that puts the old tree back failed too, leaving the only copy at the aside with an error naming neither; it now reports both failures and where the old tree remains, as extractSubdir does. The aside is defined once, in the marketplace package (CacheAsideSuffix, IsCacheAside), and every cache-root scan — searchAllMarketplaces, buildMarketplaceIndex and the direct scan in resolveInstalledEntry — skips it: an interrupted replace must not surface a stale copy as a marketplace of its own, nor let a removed plugin outlive its removal under the live marketplace's name. The three doc sites that claimed an unconditional restore say what holds. Tests: a cross-device forcing arm (source on /dev/shm; skips where it is not a separate, writable filesystem) makes the rename-in fail with a SURVIVING source and pins that both trees are kept — the one witness a swap that checks the source first and then removes the destination does not pass; TestSearchAllMarketplaces_SkipsCacheAsides; TestBuildMarketplaceIndex_SkipsCacheAsides (index and direct scan). Break-verified: deleting the guard fails both symlink-at-source arms; the round-3 placement fails exactly the empty-destination arm; the check-then-remove swap fails exactly the cross-device arm; each scan's skip and IsCacheAside fail exactly their tests. A failed restore is not forcible here (root; same parent), so that branch is pinned by review. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 9 ++-- docs/components.md | 8 ++-- internal/cli/marketplace.go | 28 ++++++----- .../cli/marketplace_reslot_internal_test.go | 43 ++++++++++++++++- internal/cli/plugin.go | 5 +- internal/cli/plugin_poll.go | 32 ++++++++----- internal/cli/plugin_poll_internal_test.go | 48 +++++++++++++++++-- internal/marketplace/cache_aside.go | 19 ++++++++ internal/marketplace/iss162_internal_test.go | 25 ++++++++++ internal/marketplace/loadprojected.go | 4 +- 10 files changed, 182 insertions(+), 39 deletions(-) create mode 100644 internal/marketplace/cache_aside.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a3ec97c..b127d7f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,9 +33,12 @@ source layout, CLI surface, and state schema are stabilizing but may still chang sources declare the same name, the later add now replaces the earlier one's cache along with the `marketplaces/.toml` and state record it already overwrote. The replace itself — shared with the cache swap `plugin upgrade` - performs — keeps the old tree until the new one is in place, so a replace - that fails part-way leaves the marketplace or plugin the cache it had rather - than none. + performs — keeps the old tree until the new one is in place and puts it back + when the new one cannot be moved in, so a replace that fails leaves the + marketplace or plugin the cache it had rather than none (and says where the + old tree sits should even that fail); the cache-root scans treat the aside + such a replace parks the old tree at as scratch, never as a marketplace of + its own. `import :plugin` registers marketplaces through the same code and now warns and skips instead of registering a phantom. diff --git a/docs/components.md b/docs/components.md index 4e793110..9c99c7ab 100644 --- a/docs/components.md +++ b/docs/components.md @@ -482,11 +482,13 @@ manifests into canonical components. provenance — including the providing plugin's `agents`/`native_agents` targeting, which travels with the component because the flattened canonical drops the association — so two plugins shipping one name cannot collide at a - destination path — see architecture.md § Plugin component namespacing). + destination path — see architecture.md § Plugin component namespacing); + `CacheAsideSuffix`/`IsCacheAside` (the `..old` sibling a cache replace + parks the old tree at, which every cache-root scan skips as scratch). - **Depends on:** source, log. - **Files:** `manifest.go`, `treehash.go` (the `tree:v1:` content hash), - `projection.go`, `loadprojected.go`, `fetcher.go`, `fetch_git.go`, - `fetch_npm.go`, `fetch_relative.go`, `update.go`. + `projection.go`, `loadprojected.go`, `cache_aside.go`, `fetcher.go`, + `fetch_git.go`, `fetch_npm.go`, `fetch_relative.go`, `update.go`. --- diff --git a/internal/cli/marketplace.go b/internal/cli/marketplace.go index 76563afa..412d6db0 100644 --- a/internal/cli/marketplace.go +++ b/internal/cli/marketplace.go @@ -249,20 +249,28 @@ func addMarketplaceSource(home string, src marketplace.Source, rawURL string, wa // a permission failure must never be answered by touching the cache the // marketplace already has. Lstat, not Stat: a symlink standing at the // destination is a link to unlink (swapDir removes the link, never what it -// points at), not this tree under another name; and a symlink standing at the -// SOURCE is not a fetched tree to move — no fetcher leaves one — so it is -// refused rather than installed as the cache. +// points at), not this tree under another name. A symlink standing at the +// SOURCE is not a fetched tree to move — no fetcher leaves one — and is refused +// before anything moves: the plain rename would otherwise install it, into an +// empty slot, as the registered cache. // -// Every failure is returned, and none costs the marketplace the cache it had: -// swapDir keeps the old tree until the fresh one is standing in its place, so a -// replace that fails part-way leaves the destination as it was and the source -// where it was, and a re-run re-fetches and completes. +// Every failure is returned. A replace that fails leaves the destination as it +// was and the source where it was (swapDir puts the old tree back, and names +// where it remains should even that fail), and a re-run re-fetches and +// completes. func reslotMarketplaceCache(from, to string) error { // Belt and braces: the fetch just created `from` under this same parent, so // the only way the parent is missing here is a concurrent removal. if err := os.MkdirAll(filepath.Dir(to), 0o755); err != nil { return fmt.Errorf("prepare marketplace cache dir %s: %w", filepath.Dir(to), err) } + // A link where the fetched tree should be: a rename would install it + // (pointing anywhere, the destination itself included) as the cache, and a + // replace would first unlink what the marketplace has. A missing source is + // left to the rename to report. + if fi, lerr := os.Lstat(from); lerr == nil && fi.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("move marketplace cache %s → %s: the source is a symlink, not a fetched tree", from, to) + } err := os.Rename(from, to) //nolint:forbidigo // moves the marketplace fetch cache under .state/cache, not a native destination if err == nil { return nil @@ -274,12 +282,6 @@ func reslotMarketplaceCache(from, to string) error { // genuine, and the destination is left exactly as it was. return fmt.Errorf("move marketplace cache %s → %s: %w", from, to, err) } - if fromInfo.Mode()&os.ModeSymlink != 0 { - // A link where the fetched tree should be: moving it would install a - // link (to anywhere, the destination itself included) as the cache, and - // the replace below would first unlink what the marketplace has. - return fmt.Errorf("move marketplace cache %s → %s: the source is a symlink, not a fetched tree", from, to) - } if os.SameFile(fromInfo, toInfo) { // Identical paths, or an alias the rename above did not resolve: the // tree is already where it belongs. Replacing it would delete the fresh diff --git a/internal/cli/marketplace_reslot_internal_test.go b/internal/cli/marketplace_reslot_internal_test.go index 051246fc..1cf1a505 100644 --- a/internal/cli/marketplace_reslot_internal_test.go +++ b/internal/cli/marketplace_reslot_internal_test.go @@ -49,8 +49,8 @@ func TestReslotMarketplaceCache(t *testing.T) { after: func(t *testing.T, _, to string) { // The stale tree is renamed aside while the fresh one moves in and // discarded only afterwards; a completed replace leaves no aside. - if _, err := os.Lstat(to + "..old"); !os.IsNotExist(err) { - t.Errorf("a completed replace must discard the tree it moved aside: %s..old (err=%v)", to, err) + if _, err := os.Lstat(to + marketplace.CacheAsideSuffix); !os.IsNotExist(err) { + t.Errorf("a completed replace must discard the tree it moved aside: %s%s (err=%v)", to, marketplace.CacheAsideSuffix, err) } }, }, @@ -108,6 +108,29 @@ func TestReslotMarketplaceCache(t *testing.T) { } }, }, + { + // The plain rename moves a link into an EMPTY slot as happily as a + // tree — the first-add path — so the refusal has to come before it, + // not only once a stale tree has made the rename fail. + name: "refuses a symlink at the source even when nothing is at the destination", + setup: func(t *testing.T, root string) (string, string) { + mustWrite(t, filepath.Join(root, "elsewhere", "keep.txt"), "keep") + from := filepath.Join(root, "slug") + if err := os.Symlink(filepath.Join(root, "elsewhere"), from); err != nil { + t.Fatal(err) + } + return from, filepath.Join(root, "declared") + }, + wantErr: "move marketplace cache", + after: func(t *testing.T, from, to string) { + if _, err := os.Lstat(to); !os.IsNotExist(err) { + t.Errorf("the link must not be installed as the cache: %s exists (err=%v)", to, err) + } + if fi, err := os.Lstat(from); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Errorf("the refused link must be left where it was (err=%v)", err) + } + }, + }, { // On a case-insensitive filesystem (macOS, Windows) a slug and a // declared name that differ only in case are ONE directory; this @@ -238,3 +261,19 @@ func TestAddMarketplaceSource_ReslotFailureRegistersNothing(t *testing.T) { t.Errorf("a failed add must leave no fetch cache behind (searchAllMarketplaces would offer it to a bare-id plugin add as an unregistered marketplace); cache root holds %d entr(y/ies)", len(entries)) } } + +// TestSearchAllMarketplaces_SkipsCacheAsides pins the other half of the aside +// contract. swapDir parks the old tree at ..old while a replace is in +// flight, and an interrupted replace leaves it there; a bare-id `plugin add` +// must never resolve against that copy — it would register the plugin under a +// name no cache directory can be derived from, the #233 shape again. +func TestSearchAllMarketplaces_SkipsCacheAsides(t *testing.T) { + home := t.TempDir() + aside := filepath.Join(home, ".state", "cache", "marketplaces", "shared"+marketplace.CacheAsideSuffix) + mustWrite(t, filepath.Join(aside, ".claude-plugin", "marketplace.json"), + `{"name": "shared", "owner": {"name": "x"}, "plugins": [{"name": "ghost", "source": "./ghost"}]}`) + + if _, _, via, err := searchAllMarketplaces(home, "ghost"); err == nil { + t.Fatalf("a cache aside must not be searched as a marketplace; ghost resolved via %q", via) + } +} diff --git a/internal/cli/plugin.go b/internal/cli/plugin.go index c61e14d3..01b9dcd7 100644 --- a/internal/cli/plugin.go +++ b/internal/cli/plugin.go @@ -891,7 +891,10 @@ func searchAllMarketplaces(home, pluginID string) ([]byte, marketplace.PluginEnt } for _, e := range entries { - if !e.IsDir() { + // The aside a cache replace parks the old tree at is a stale copy, not + // a marketplace: offered here, it would resolve a bare id under a name + // no cache directory can be derived from (the #233 shape again). + if !e.IsDir() || marketplace.IsCacheAside(e.Name()) { continue } mpJSONPath := filepath.Join(cacheRoot, e.Name(), ".claude-plugin", "marketplace.json") diff --git a/internal/cli/plugin_poll.go b/internal/cli/plugin_poll.go index 6f5a6c3e..b51d5c57 100644 --- a/internal/cli/plugin_poll.go +++ b/internal/cli/plugin_poll.go @@ -427,7 +427,8 @@ func applyPluginBump(home string, b marketplace.Bump, fetched map[string]map[str // TOML committed; swap the fetched cache into place. If the swap fails, // roll the TOML back so cache (old) and TOML (old) stay consistent rather - // than leaving a new-SHA TOML over an old cache. + // than leaving a new-SHA TOML over an old cache (swapDir puts the old cache + // back, and names where it remains should even that fail). if err := swapDir(tmpCache, cacheDir); err != nil { if prevTOML != nil { _ = iox.AtomicWrite(pluginPath, prevTOML, 0o644) @@ -442,16 +443,20 @@ func applyPluginBump(home string, b marketplace.Bump, fetched map[string]map[str // same filesystem (callers create src as a sibling of dst). After a successful // swap src no longer exists. // -// The old tree survives until the new one is standing at dst. A rename that -// fails part-way is rolled back, so a marketplace or plugin never loses the -// cache it had to a replace that did not complete — the shape extractSubdir in -// internal/marketplace documents (RemoveAll-then-Rename destroyed the cache -// whenever the rename then failed). The aside is a sibling of dst whose name -// holds "..", which sanitizeCacheKey never lets into a cache key, so it can -// never be another plugin's or marketplace's cache; a leftover from an -// interrupted earlier swap is cleared first, as extractSubdir clears its own. +// The old tree survives until the new one is standing at dst. When src cannot +// be moved in, the old tree is renamed back and the error returned — the shape +// extractSubdir in internal/marketplace documents (RemoveAll-then-Rename +// destroyed the cache whenever the rename then failed). Should that restore +// fail too, the error says so and names the aside, where the old tree remains. +// The aside is dst + marketplace.CacheAsideSuffix: a sibling whose name holds +// "..", which sanitizeCacheKey never lets into a cache key, so it can never be +// another plugin's or marketplace's cache, and which every cache-root scan +// treats as scratch (marketplace.IsCacheAside). A leftover from an interrupted +// earlier swap is cleared first, as extractSubdir clears its own — a fresh +// replacement is in hand by then — and discarding the aside after a completed +// swap is best-effort for the same reason. func swapDir(src, dst string) error { - aside := dst + "..old" + aside := dst + marketplace.CacheAsideSuffix if err := os.RemoveAll(aside); err != nil { return err } @@ -459,10 +464,13 @@ func swapDir(src, dst string) error { return err } if err := os.Rename(src, dst); err != nil { - _ = os.Rename(aside, dst) // best-effort rollback; a no-op when dst was absent + // Put the old tree back; there is none to put back when dst was absent. + if rerr := os.Rename(aside, dst); rerr != nil && !os.IsNotExist(rerr) { + return fmt.Errorf("%w; restoring the previous tree failed (%v), it remains at %s", err, rerr, aside) + } return err } - _ = os.RemoveAll(aside) // best-effort; the next swap clears a leftover + _ = os.RemoveAll(aside) return nil } diff --git a/internal/cli/plugin_poll_internal_test.go b/internal/cli/plugin_poll_internal_test.go index 3eb30f90..dd9ee8ec 100644 --- a/internal/cli/plugin_poll_internal_test.go +++ b/internal/cli/plugin_poll_internal_test.go @@ -4,6 +4,8 @@ import ( "os" "path/filepath" "testing" + + "github.com/spxrogers/agentsync/internal/marketplace" ) // TestSwapDir pins the replace both cache callers rely on — the plugin-upgrade @@ -50,7 +52,7 @@ func TestSwapDir(t *testing.T) { setup: func(t *testing.T, src, dst string) { mustWrite(t, filepath.Join(src, "marker.txt"), "fresh") mustWrite(t, filepath.Join(dst, "marker.txt"), "stale") - mustWrite(t, filepath.Join(dst+"..old", "marker.txt"), "older") + mustWrite(t, filepath.Join(dst+marketplace.CacheAsideSuffix, "marker.txt"), "older") }, wantDst: "fresh", }, @@ -74,8 +76,8 @@ func TestSwapDir(t *testing.T) { if string(got) != tc.wantDst { t.Errorf("destination marker.txt = %q, want %q", got, tc.wantDst) } - if _, lerr := os.Lstat(dst + "..old"); !os.IsNotExist(lerr) { - t.Errorf("no aside may survive the swap: %s..old (err=%v)", dst, lerr) + if _, lerr := os.Lstat(dst + marketplace.CacheAsideSuffix); !os.IsNotExist(lerr) { + t.Errorf("no aside may survive the swap: %s%s (err=%v)", dst, marketplace.CacheAsideSuffix, lerr) } if _, lerr := os.Lstat(src); !tc.wantErr && !os.IsNotExist(lerr) { t.Errorf("a completed swap must consume the source: %s (err=%v)", src, lerr) @@ -83,3 +85,43 @@ func TestSwapDir(t *testing.T) { }) } } + +// TestSwapDir_KeepsBothTreesWhenTheRenameInFails forces the failure the +// rollback exists for with a source the swap cannot move: a tree on another +// filesystem (/dev/shm is a tmpfs on Linux; the test skips where it is missing, +// unwritable, or on the same filesystem as the test's temp dir). Unlike a +// missing source, this tree survives the failed rename, so the arm also pins +// that the source is left where it was — and it is the one witness the closed +// window has: a swap that checks the source first and then removes the +// destination passes every other test in the package. +func TestSwapDir_KeepsBothTreesWhenTheRenameInFails(t *testing.T) { + shm, err := os.MkdirTemp("/dev/shm", "agentsync-swapdir-") + if err != nil { + t.Skipf("no writable /dev/shm to force a cross-device rename: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(shm) }) + root := t.TempDir() + probe := filepath.Join(shm, "probe") + mustWrite(t, filepath.Join(probe, "x"), "x") + if err := os.Rename(probe, filepath.Join(root, "probe")); err == nil { + t.Skip("/dev/shm and the test temp dir are one filesystem; a rename between them cannot fail") + } + src, dst := filepath.Join(shm, "incoming"), filepath.Join(root, "cache") + mustWrite(t, filepath.Join(src, "marker.txt"), "fresh") + mustWrite(t, filepath.Join(dst, "marker.txt"), "stale") + + err = swapDir(src, dst) + + if err == nil { + t.Fatal("a cross-device rename must fail the swap; got nil") + } + if got, rerr := os.ReadFile(filepath.Join(dst, "marker.txt")); rerr != nil || string(got) != "stale" { + t.Errorf("the old tree must be put back; marker.txt = %q (err=%v)", got, rerr) + } + if got, rerr := os.ReadFile(filepath.Join(src, "marker.txt")); rerr != nil || string(got) != "fresh" { + t.Errorf("the source must be left where it was; marker.txt = %q (err=%v)", got, rerr) + } + if _, lerr := os.Lstat(dst + marketplace.CacheAsideSuffix); !os.IsNotExist(lerr) { + t.Errorf("no aside may survive the swap (err=%v)", lerr) + } +} diff --git a/internal/marketplace/cache_aside.go b/internal/marketplace/cache_aside.go new file mode 100644 index 00000000..0b944ee0 --- /dev/null +++ b/internal/marketplace/cache_aside.go @@ -0,0 +1,19 @@ +package marketplace + +import "strings" + +// CacheAsideSuffix names the sibling a cache directory is parked at while it is +// being replaced: the cli's swapDir renames the old tree to ..old, moves +// the new tree in, and discards the aside only then — so a replace that fails +// can put the old tree back, and one that is interrupted leaves it there until +// the next replace of the same directory clears it. The name holds "..", which +// the cli's cache-key sanitizer never lets into a cache directory name, so an +// aside can never be a cache of its own. +const CacheAsideSuffix = "..old" + +// IsCacheAside reports whether a cache-root entry is the parked old tree of a +// replace in progress (or interrupted) rather than a cache: its name holds +// "..", which no cache directory name can. Every scan of a cache root skips +// such an entry — offered as a marketplace of its own, a stale copy would be +// resolvable under a name no cache directory can be derived from. +func IsCacheAside(name string) bool { return strings.Contains(name, "..") } diff --git a/internal/marketplace/iss162_internal_test.go b/internal/marketplace/iss162_internal_test.go index b3c64e8c..8151cabf 100644 --- a/internal/marketplace/iss162_internal_test.go +++ b/internal/marketplace/iss162_internal_test.go @@ -80,3 +80,28 @@ func TestResolveInstalledEntry_MemoizedScan(t *testing.T) { t.Fatalf("scan should miss after cache deletion, returning a bare entry; got desc=%q", got.Description) } } + +// TestBuildMarketplaceIndex_SkipsCacheAsides: the cli's cache replace parks the +// old tree at ..old until the new one is in place, and an interrupted +// replace leaves it there. Neither the index nor the direct scan may take +// entries from that copy — under the live marketplace's declared name, a +// plugin removed upstream would otherwise outlive its removal. +func TestBuildMarketplaceIndex_SkipsCacheAsides(t *testing.T) { + home := t.TempDir() + osfs := afero.NewOsFs() + writeMarketplaceCacheJSON(t, home, "a", + `{"name":"mpA","plugins":[{"name":"plug1"}]}`) + writeMarketplaceCacheJSON(t, home, "a"+CacheAsideSuffix, + `{"name":"mpA","plugins":[{"name":"ghost","description":"from the aside"}]}`) + + idx := buildMarketplaceIndex(osfs, home) + + if got := len(idx["mpA"]); got != 1 { + t.Fatalf("the aside's entries must not be indexed; idx[mpA] has %d entries: %v", got, idx["mpA"]) + } + for _, viaIdx := range []marketplaceIndex{idx, nil} { + if got := resolveInstalledEntry(osfs, home, "ghost", "mpA", viaIdx); got.Description == "from the aside" { + t.Errorf("an aside's entry resolved (index=%v): %+v", viaIdx != nil, got) + } + } +} diff --git a/internal/marketplace/loadprojected.go b/internal/marketplace/loadprojected.go index 502974ed..0965fe6c 100644 --- a/internal/marketplace/loadprojected.go +++ b/internal/marketplace/loadprojected.go @@ -529,7 +529,7 @@ func resolveInstalledEntry(fs afero.Fs, home, id, mpName string, idx marketplace return PluginEntry{Name: untrusted.Wrap(id)} } for _, d := range dirs { - if !d.IsDir() { + if !d.IsDir() || IsCacheAside(d.Name()) { continue } data, rerr := afero.ReadFile(fs, filepath.Join(cacheRoot, d.Name(), ".claude-plugin", "marketplace.json")) @@ -578,7 +578,7 @@ func buildMarketplaceIndex(fs afero.Fs, home string) marketplaceIndex { return idx } for _, d := range dirs { - if !d.IsDir() { + if !d.IsDir() || IsCacheAside(d.Name()) { continue } data, rerr := afero.ReadFile(fs, filepath.Join(cacheRoot, d.Name(), ".claude-plugin", "marketplace.json"))