diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dfd59b3..b127d7f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,37 @@ 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 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. The replace itself — shared with the cache swap `plugin upgrade` + 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. + - **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/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/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 2aab5ee8..412d6db0 100644 --- a/internal/cli/marketplace.go +++ b/internal/cli/marketplace.go @@ -130,13 +130,26 @@ 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 (#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 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. 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 — fix the cause and re-run", mpName, err) } } @@ -210,6 +223,77 @@ 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 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 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. 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. 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 + } + 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. + return fmt.Errorf("move marketplace cache %s → %s: %w", from, to, err) + } + 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) + } + 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..1cf1a505 --- /dev/null +++ b/internal/cli/marketplace_reslot_internal_test.go @@ -0,0 +1,279 @@ +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 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 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 + setup func(t *testing.T, root string) (from, to string) + wantErr string // substring; "" means the move must succeed + // 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) + }{ + { + 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 + }, + 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 + 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) + } + }, + }, + { + 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: "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) + } + }, + }, + { + // 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) + } + }, + }, + { + // 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 + // 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 { + 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) + } + if tc.after != nil { + tc.after(t, from, to) + } + 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); 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) + } + }) + } +} + +// 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)) + } +} + +// 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/marketplace_reslot_test.go b/internal/cli/marketplace_reslot_test.go new file mode 100644 index 00000000..3d265588 --- /dev/null +++ b/internal/cli/marketplace_reslot_test.go @@ -0,0 +1,122 @@ +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 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"} + 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) + } +} + +// 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, 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) + } +} 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 6f342cfe..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) @@ -437,14 +438,40 @@ 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. 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 { - if err := os.RemoveAll(dst); err != nil { + aside := dst + marketplace.CacheAsideSuffix + 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 { + // 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) + 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..dd9ee8ec --- /dev/null +++ b/internal/cli/plugin_poll_internal_test.go @@ -0,0 +1,127 @@ +package cli + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spxrogers/agentsync/internal/marketplace" +) + +// 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+marketplace.CacheAsideSuffix, "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 + 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) + } + }) + } +} + +// 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"))