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

[spxrogers]

Summary

Closes #233. Seventh PR in the #226 code-quality series (after #240, #244, #247, #249, #252, #253).

marketplace add re-slots the freshly fetched cache from the URL-derived slug directory to the marketplace's declared name — the name it records in marketplaces/<name>.toml and targets.json, and the only name any later lookup derives the cache directory from. Both failure arms of that move were discarded while the registration proceeded, so a failed move produced ✅ added marketplace … plus a registration whose cache nothing could find, and the next plugin add <id>@<name> reported marketplace "<name>" not found in cache; run: agentsync marketplace add <url> — advice that repeats the same failure.

The move is now reslotMarketplaceCache. A symlink at the source is refused before anything moves (no fetcher leaves one, and the plain rename would install it into an empty slot as the registered cache). Then it is a plain rename first, which is the whole job when nothing is at the destination and, on case-insensitive filesystems, for a case-only alias of the same directory. When a different entry stands at the destination while the fetched tree is still at the source (decided by Lstat, because Linux reports an exists error for an occupied destination and Windows reports access denied, and because a symlink at the destination is a link to unlink rather than the tree under another name), the destination is replaced with the package's swapDir, so a re-add refreshes the cache instead of silently keeping the stale one: a rename onto a different existing directory fails, which is why every re-add of a marketplace whose declared name differs from its slug (typically every github:owner/repo source) kept the stale cache, orphaned the fresh tree under the slug, and, for a git source, advanced head_sha anyway — a plugin published between two adds stayed invisible. That second symptom was found while planning and reproduced at the base commit; the CHANGELOG bullet describes both. swapDir itself — shared with the cache swap plugin upgrade performs — now renames the old tree aside, renames the new one into place, and discards the aside only then; if the new tree cannot be moved in, the old one is put back, and if even that fails the error says where the old tree remains (the shape extractSubdir in internal/marketplace documents as the fix for a cache it once destroyed). The aside is defined once (marketplace.CacheAsideSuffix, <dir>..old, a name no sanitized cache key can be), and every cache-root scan skips it (marketplace.IsCacheAside), so an interrupted replace never surfaces a stale copy as a marketplace of its own. A move that fails leaves the destination as it was, fails the add before the TOML or the state entry is written, and discards the fetched tree; the error says nothing was written and to fix the cause and re-run. import <agent>:plugin registers marketplaces through the same function and now warns and skips instead of registering a phantom. Success-path behaviour is unchanged: a two-binary transcript comparison of a first marketplace add is byte-identical.

Review provenance. The execution spec was reviewed before execution by a fresh reviewer who replicated both symptoms at the base commit and found three issues, all folded in: the failure path left a plugin-installable orphan under the slug (now discarded); the error wording misdescribed a re-add; two rows of the break-verify table understated which tests a mutation fails. Two pre-existing findings from that review are filed separately: #258 (a test helper leaks an unterminated prompt onto the test binary's stderr) and #259 (the issue's "related consolidation" plus orphan-cache cleanup, the slug-collision hazard, and — from this loop — the stale .golangci.yml comment on plugin_poll.go's exclusion, which this PR does not touch).

Review loop. Round 1 (four lenses on 2ec7bd7; closed by 241a74d and 99288e7): two lenses independently found that on a case-insensitive filesystem (macOS, Windows) a slug and a declared name differing only in case name one directory, so the remove-then-rename deleted the fresh fetch and then failed, where the base's plain rename had succeeded — the helper is now rename-first and never replaces a destination that is the source itself. Three lenses found the error text claimed nothing had changed while a rename failing after the destination's removal could leave an already-registered marketplace cache-less; a missing source no longer removes an existing destination at all. The same-name replacement the CHANGELOG describes is pinned by TestMarketplaceAdd_SameDeclaredNameReplacesTheEarlierCache; the import test moved beside its peers; the head_sha clause is scoped to git sources. A follow-up commit replaced an exists-errno gate with a stat-based one after checking Go's Windows rename path (MoveFileEx onto an existing directory reports access denied, not exists), so a Windows re-add refreshes rather than failing. Round 2 (on 99288e7; three lenses CLEAN; closed by 00d4487): the adversarial lens showed a symlink planted at the declared-name path pointing at the source read as "already in place" under Stat, leaving the tree under the slug behind a link that marketplace remove would unlink alone — the gate uses Lstat, and two table arms pin that a symlinked destination is unlinked without touching its target and is replaced even when it points at the source. Round 3 (on 00d4487; correctness, API-design and test-rigor lenses CLEAN; closed by 23e8398): the adversarial lens found that the remaining destructive window — swapDir 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, which the error text admitted — is exactly the pattern extractSubdir documents as a retired bug; swapDir became rename-aside with rollback, the error text dropped its "may be gone" clause, and TestSwapDir pins the replace, the rollback, and the clearing of a leftover aside. Three lenses converged on "always fails" being overstated for a rename (a case-only alias of the same directory goes through); overruled with a citation was the adversarial claim that the alias never reaches the kernel (os/file_unix.go lets newname != oldname && SameFile through; the correctness lens confirmed it, and the adversarial lens conceded it in round 4). Round 4 (on 23e8398; test-rigor CLEAN; closed by b768f49): the adversarial lens found round 3's symlink-at-source guard was dead on the common path — it sat after the plain rename, which succeeds into an empty slot and installs the link (the test-rigor lens measured the same; the correctness lens had reasoned only about the post-rename ordering) — the guard now runs before anything moves, and an empty-destination arm pins it. Three lenses converged on swapDir swallowing its rollback error (a double failure left the only copy at the aside, unnamed), and on a surviving aside being enumerated by the cache-root scans as a marketplace of its own; the rollback failure is now reported with the aside's path, and the aside is defined once in internal/marketplace and skipped by searchAllMarketplaces, buildMarketplaceIndex and the direct scan in resolveInstalledEntry. The test-rigor lens showed the closed window had no mutation-proof witness (a swap that checks the source first and then removes the destination passed the whole package); a cross-device forcing arm now kills that mutant. The "..old" literal became a constant. Declined with reason: a caller-level plugin upgrade swap-failure test (no injection seam; the unit test pins the contract and the TOML rollback has its own test); pinning the tolerance on the rename-aside step (not forcible as root); and from earlier rounds, dropping the MkdirAll failure arm, a build-tagged Windows test, an end-to-end failure-then-success test, and inlining the from != to carve-out. Deferred to #259: the stale .golangci.yml comment.

Type of change

• Bug fix
• New feature / enhancement
• Refactor (no behavior change)
• Docs
• Tests / CI / tooling

Test plan

New tests, no existing test changed:

• TestReslotMarketplaceCache (nine arms: the move; a stale cache from an earlier add is replaced, not merged, and no aside survives; a failure to prepare the cache root, forced by a regular-file parent; a missing source with a populated destination leaves that destination alone; a symlink at the source is refused with the destination and the link left as they were, both with a populated destination and with an empty one; identical paths, the Linux stand-in for a case-only alias, keep the tree in place; a symlink at the destination is replaced without touching its target; a symlink at the destination pointing at the source is replaced rather than mistaken for the tree).
• TestSwapDir (four arms: an old tree is replaced; an empty destination is filled; a missing source leaves the old tree in place after the rollback; a leftover aside from an interrupted swap is cleared first — and no aside survives any arm) and TestSwapDir_KeepsBothTreesWhenTheRenameInFails (the source on /dev/shm, another filesystem, so the rename in fails with a surviving source; the old tree is put back, the source is left where it was, no aside survives; skips where /dev/shm is missing, unwritable, or the same filesystem as the temp dir — it ran here).
• TestSearchAllMarketplaces_SkipsCacheAsides and TestBuildMarketplaceIndex_SkipsCacheAsides (an aside carrying a marketplace.json is neither offered to a bare-id plugin add nor indexed, by the index or by the direct scan).
• TestAddMarketplaceSource_ReslotFailureRegistersNothing (a 300-character declared name forces ENAMETOOLONG; asserts the error names the move, and that marketplaces/, targets.json and the cache root are all untouched).
• End-to-end TestMarketplaceAdd_CacheIsSlottedUnderTheRegisteredName, TestMarketplaceAdd_ReAddRefreshesTheCache (re-adds a fixture after publishing a new plugin and asserts the cache carries it, with no orphan under the slug), and TestMarketplaceAdd_SameDeclaredNameReplacesTheEarlierCache (two sources declaring one name; the cache under it holds the second source's tree).
• TestImportPlugin_SkipsMarketplaceWhoseReslotFails (import claude:plugin exits 0, prints the skip warning naming the move, registers nothing).

Break-verifies (literal go test fail sets, each mutation asserted to land exactly once and to compile, files restored from a cp backup and sha-checked):

• Restoring the old swallowing block → TestAddMarketplaceSource_ReslotFailureRegistersNothing, TestMarketplaceAdd_ReAddRefreshesTheCache, TestImportPlugin_SkipsMarketplaceWhoseReslotFails.
• Ignoring MkdirAll's error → exactly the cache-root arm.
• Round 1: disabling the same-file guard → exactly the identical-paths arm; replacing first (no rename-first) → exactly the missing-source arm; removing the stat gate → exactly that arm again; a plain rename in the replace step → the stale-replace arm, TestMarketplaceAdd_ReAddRefreshesTheCache and TestMarketplaceAdd_SameDeclaredNameReplacesTheEarlierCache.
• Round 2: Stat instead of Lstat in the gate → exactly the link-to-source arm; resolving the destination's symlink before the replace → both symlink arms.
• Round 3: dropping the aside's final removal → the stale-replace arm, two TestSwapDir arms, and both re-add e2e tests (the aside shows up in the cache root); dropping the rollback → exactly the missing-source TestSwapDir arm; dropping the leading clear → exactly the leftover-aside arm; reverting swapDir to remove-then-rename → the missing-source and leftover-aside arms.
• Round 4: deleting the source-symlink guard → both symlink-at-source arms; the round-3 placement (guard after the rename) → exactly the empty-destination arm; a swap that checks the source first and then removes the destination → exactly TestSwapDir_KeepsBothTreesWhenTheRenameInFails; dropping the skip in searchAllMarketplaces → exactly its test; IsCacheAside always false → both skip tests; dropping the skip in either marketplace scan alone → exactly the index test. A failed restore inside swapDir is not forcible here (root; the aside shares the destination's parent), so that branch is pinned by review, not by a test.
• TestMarketplaceAdd_CacheIsSlottedUnderTheRegisteredName passes under every mutation, by design: it is the success-path guard.

Post-conditions on internal/cli/marketplace.go: //nolint:forbidigo 3 → 3 (the old rename's went; one arrived on the rename-first and the failure-path os.RemoveAll carries the other, replacing the two the base had on the block); .golangci.yml untouched (plugin_poll.go keeps its whole-file exclusion).

Gates on the head: go build, go vet, gofmt -l empty, gofumpt + go mod tidy rewrote nothing, AGENTSYNC_TEST_IN_CONTAINER=1 go test ./... (29 packages ok, zero -json failures), -race ./internal/cli/... ok, -tags=e2e ok, -tags=bdd ok, GOOS=windows go build ./... ok, GOTOOLCHAIN=go1.26.2 golangci-lint@v2.12.2 run ./... → 0 issues.

• just test-release is green (the release bar) — just is not installable in this session; every layer the recipe orchestrates was run directly as listed above, and CI's hermetic test-release job runs on the PR.
• just lint is clean — run directly with the pinned toolchain; go.mod/go.sum untouched.

Checklist

• Conventional commit messages with a scope (e.g. fix(secrets): …).
• Tests added/updated for the behavior changed.
• If this touches internal/secrets, internal/capture, or any
  source.Write* path, I've re-read the secret-handling invariants in
  CLAUDE.md / SECURITY.md and not weakened them. — Not applicable: the change is confined to the marketplace and plugin fetch caches under .state/cache; no dest→source path is involved.
• Docs updated if behavior, CLI surface, or capability coverage changed. — CHANGELOG.md under [Unreleased] / Fixed; docs/components.md lists the new cache_aside.go and its two exports under internal/marketplace. No docs/ or website sentence describes the cache layout, the slug-versus-declared-name distinction, the upgrade cache swap, or marketplace add's failure modes (checked), so nothing else changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG