fix(cli): marketplace add fails instead of registering an unfindable cache (#233)

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 <id>@<name>` failed with `marketplace "<name>" not found in cache;
+  run: agentsync marketplace add <url>` — 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/<name>.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 <agent>: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

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 `<dir>..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`.
 
 ---
 

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 <agent>: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))
+	}
+}

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/<name>.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 {