From 786d9aec5e193da150d17ced052a339114cf7433 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 2 Oct 2026 12:55:18 +0200 Subject: [PATCH 1/3] ISSUE-450: Install plugins per scope and let the Claude CLI own enablement - Stop writing enabledPlugins; Claude Code ignored the bare-name key, and project settings now keep what the CLI wrote - Install by name@marketplace at the sync's scope (local in a project, user globally), recording only installs mcs made - Doctor reads the CLI's plugin list, once per run, listing the global scope from the home directory --- CLAUDE.md | 15 +- Sources/mcs/Core/ClaudeIntegration.swift | 44 +- Sources/mcs/Core/Constants.swift | 9 + Sources/mcs/Core/PluginRef.swift | 122 +++++ Sources/mcs/Doctor/CoreDoctorChecks.swift | 62 +-- Sources/mcs/Doctor/DerivedDoctorChecks.swift | 14 +- Sources/mcs/Doctor/DoctorRunner.swift | 5 +- Sources/mcs/Sync/ComponentExecutor.swift | 117 +++-- Sources/mcs/Sync/Configurator.swift | 117 ++--- Sources/mcs/Sync/ConfiguratorSupport.swift | 36 +- Sources/mcs/Sync/GlobalSyncStrategy.swift | 11 +- Sources/mcs/Sync/ProjectSyncStrategy.swift | 29 +- Sources/mcs/Sync/ResourceRefCounter.swift | 15 +- Sources/mcs/Sync/SyncScope.swift | 5 + Sources/mcs/Sync/SyncStrategy.swift | 3 +- Sources/mcs/TechPack/TechPack.swift | 2 +- Tests/MCSTests/ComponentExecutorTests.swift | 61 ++- .../CoreDoctorCheckSandboxTests.swift | 422 ++++-------------- .../DoctorRunnerIntegrationTests.swift | 51 +-- .../MCSTests/LifecycleIntegrationTests.swift | 114 ++++- Tests/MCSTests/PluginRefTests.swift | 24 + Tests/MCSTests/ProjectSyncTests.swift | 35 +- Tests/MCSTests/ResourceRefCounterTests.swift | 124 ----- Tests/MCSTests/TestHelpers.swift | 67 ++- 24 files changed, 808 insertions(+), 696 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4cf1f667..5d9de5f9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,13 +82,13 @@ mcs config set # Set a configuration value (true/false) - `Settings.swift` — Codable model for `settings.json` and `settings.local.json`, deep-merge - `Backup.swift` — timestamped backups for mixed-ownership files (CLAUDE.local.md), backup discovery and deletion - `GitignoreManager.swift` — global gitignore management, core entry list -- `ClaudeIntegration.swift` — `claude mcp add/remove` (with scope support), `claude plugin install/remove` +- `ClaudeIntegration.swift` — `claude mcp add/remove` (with scope support), `claude plugin install/uninstall/list` and `marketplace add/list` (scope + working directory, like MCP) - `ClaudePrerequisite.swift` — Claude Code CLI availability check with optional Homebrew auto-install - `Homebrew.swift` — brew detection, package install/uninstall, and `provides(_:)` — the one availability predicate shared by `ComponentExecutor` and `BrewPackageCheck` (PATH under `bareName(of:)`, falling back to `brew list`) - `FileHasher.swift` — SHA-256 file and directory hashing via CryptoKit (used by `PackTrustManager` and `ComponentExecutor`) - `FileLock.swift` — POSIX `flock()` process lock and `LockedCommand` protocol for mutually exclusive CLI commands - `PathContainment.swift` — centralized path-boundary checks and relative-path utilities (symlink-safe containment, traversal prevention) -- `PluginRef.swift` — parsed `name@repo` plugin references with marketplace resolution +- `PluginRef.swift` — parsed `name@repo` plugin references; resolves the `name@` id Claude Code keys plugins by (`@org/repo` needs a `marketplace list` lookup), plus the `plugin list` / `marketplace list` JSON models - `ProjectDetector.swift` — walk-up project root detection (`.git/` or `CLAUDE.local.md`) - `SettingsHasher.swift` — deterministic SHA-256 hashing of per-pack settings key-value pairs for drift detection - `ProjectState.swift` — per-project `.claude/.mcs-project` JSON state (configured packs, per-pack `PackArtifactRecord` with ownership tracking, version) @@ -148,13 +148,13 @@ mcs config set # Set a configuration value (true/false) - `SyncScope.swift` — pure data struct capturing path-level differences between project and global scopes - `SyncStrategy.swift` — protocol isolating scope-specific behavior (artifact installation, settings/CLAUDE composition, file artifact base); `removeFileArtifact` is a shared default that logs each actual deletion - `ProjectSyncStrategy.swift` — project-scope strategy (settings.local.json, CLAUDE.local.md, repo name resolution) -- `GlobalSyncStrategy.swift` — global-scope strategy (settings.json preservation, brew/plugin ownership, MCP scope override to "user") +- `GlobalSyncStrategy.swift` — global-scope strategy (settings.json preservation, brew ownership, plugins and MCP servers at "user" scope) - `ComponentExecutor.swift` — dispatches install actions (brew, MCP servers, plugins, gitignore, project-scoped file copy) - `CrossPackPromptResolver.swift` — deduplicates shared prompt keys across multiple packs (groups by key, executes once for shared `input`/`select` prompts) - `DestinationCollisionResolver.swift` — auto-namespaces `copyPackFile` destinations when multiple packs target the same `(destination, fileType)` pair - `ScopeReapplier.swift` — converges one scope onto its already-configured pack set (whole-scope skip when any configured pack cannot be produced); shared by `mcs update` and `mcs doctor --fix` - `PackUpdater.swift` — shared fetch → validate → trust cycle for updating a single git pack (used by `UpdatePack` and `UpdateCommand`) -- `ResourceRefCounter.swift` — two-tier reference counting (global artifacts + project index manifests) for safe removal of brew packages, plugins and gitignore entries; decoded state is cached per instance so one removal pass reads each state file once +- `ResourceRefCounter.swift` — two-tier reference counting (global artifacts + project index manifests) for safe removal of brew packages and gitignore entries; decoded state is cached per instance so one removal pass reads each state file once - `SyncDeltaSummary.swift` — computes add/remove/keep deltas between previous and selected pack sets and renders the review-changes summary shown before destructive sync operations ### Templates (`Sources/mcs/Templates/`) @@ -210,8 +210,9 @@ swiftlint --fix - **`mcs sync` is the primary command**: per-project multi-select of registered packs, fully idempotent convergence (add/remove/update), per-project artifact placement. `--global` flag handles global-scope install. `--pack` follows bootstrap's additive-by-default model: named packs are added/updated and every other configured pack is kept (a configured pack missing from the registry aborts the run); `--prune` opts into the authoritative set with a removal confirmation, `--yes` skips it - **`mcs bootstrap` is project-scoped, additive-by-default, opt-in-authoritative**: reads `./mcs.yaml` (fixed filename, cwd only) and drives the current project's pack set. Like `mcs sync`, its non-dry-run tail refreshes the shared user-level update-check hook in `~/.claude/settings.json` — that hook is a per-user preference, not per-project. Default is additive — declared packs are installed / updated, and packs already configured but absent from the file are preserved (a footer reports them so divergence stays visible). `--prune` makes `mcs.yaml` the exact desired set — extras get unconfigured through the same `Configurator.configure(confirmRemovals:)` gate `mcs sync --pack` uses, with `--yes` to skip the confirmation, and `--prune` also bypasses the unloadable-scope guard so a broken configured pack can be pruned rather than blocking the run. Composes existing primitives (`PackAdder`, `PackUpdater`) rather than reimplementing them. `PackRef.scope` in the schema is reserved for a future release; v1 rejects any value other than `"project"`. The additive default matches the pattern that Homebrew Bundle, Kubernetes, and npm settled on for declarative-file + external-state workflows: safe default, explicit opt-in for destructive action. - **Non-interactive prompt resolution**: when stdin is not a TTY, every prompt resolves as seeded/stored value → declared default (a `select` default must be an option; a `fileDetect` default must be a match, or with zero matches any non-empty value; otherwise an explicit `""` counts) → failure. `CrossPackPromptResolver.planValues` builds the key set and reuse partition that both sync (`resolveAllValues`) and the preflight (`resolveNonInteractively`) start from, and the preflight replays sync's stages in order — reused priors (per `Configurator.priorReuse`), shared prompts, then each pack for the keys still open, since a pack skips keys an earlier pack produced — so the two cannot disagree. `Configurator.configure` runs it against the post-removal priors before any removal or install and throws `PromptResolutionError` (every `pack: key`, never values, with a scope-specific remedy), otherwise merges its answers so no prompt reader runs. `--dry-run` only warns; `mcs update` skips a failing scope, finishes the rest, then exits non-zero. Two older prompts still read stdin off-TTY (tracked in #417) -- **Packs install whole**: a selected pack installs every component it declares, in declaration order (project sync still installs brew packages and plugins first). There is no per-component selection — `mcs.yaml` could not express one, so the same file would install different sets on different machines. `dependencies` / `isRequired` in a manifest are decoded only into `deprecatedKeys` for a `mcs pack validate` warning. Exclusions left in state by the removed `--customize` are announced by sync and dry-run, then installed and cleared (emptied, not removed — older releases require the key) by the first real sync; until a global sync clears them, `ScopeDuplicationCheck` refuses to remove a project copy -- **Per-project artifacts**: skills, hooks, commands, and `settings.local.json` go to `/.claude/`; only brew packages and plugins are global +- **Packs install whole**: a selected pack installs every component it declares, in declaration order (project sync still installs brew packages first). There is no per-component selection — `mcs.yaml` could not express one, so the same file would install different sets on different machines. `dependencies` / `isRequired` in a manifest are decoded only into `deprecatedKeys` for a `mcs pack validate` warning. Exclusions left in state by the removed `--customize` are announced by sync and dry-run, then installed and cleared (emptied, not removed — older releases require the key) by the first real sync; until a global sync clears them, `ScopeDuplicationCheck` refuses to remove a project copy +- **Per-project artifacts**: skills, hooks, commands, and `settings.local.json` go to `/.claude/`; only brew packages are global +- **Plugins install per scope, and the Claude CLI owns `enabledPlugins`**: project sync runs `claude plugin install @ -s local` from the project directory, global sync `-s user`; mcs never writes `enabledPlugins` (Claude Code ignores anything but the `name@marketplace` key) and project composition preserves what the CLI wrote to `settings.local.json`. A plugin is recorded only when mcs's install put it there, and removed from its own scope unless another pack in that scope claims it. `PluginCheck` reads `claude plugin list --json` - **MCP scope defaults to `local`**: per-user, per-project isolation via `claude mcp add -s local` (stored in `~/.claude.json` keyed by project path) - **Convergent sync**: `ProjectState` records per-pack `PackArtifactRecord` (MCP servers, files, template sections, hook commands, settings keys, settings hash, brew packages, plugins, gitignore entries, file hashes); re-running converges to desired state by diffing previous vs. selected packs. `mcs pack remove` discovers all scopes via `ProjectIndex` and runs the same `unconfigurePack()` convergence for each - **External pack protocol**: `TechPack` protocol with `ExternalPackAdapter` bridging YAML manifests (`techpack.yaml`) to the same install/doctor/sync flows @@ -223,5 +224,5 @@ swiftlint --fix - **Project awareness**: doctor detects project root (walk-up for `.git/`), resolves packs from `.claude/.mcs-project` before falling back to section marker inference, then to global manifest - **Local packs**: `mcs pack add /path` registers a pack read in-place — no git clone, no `mcs pack update`, no directory deletion on remove. Uses `isLocal: Bool?` on `PackEntry` (backward-compatible) and `commitSHA: "local"` sentinel. Trust verification is skipped since scripts change during development - **GitHub shorthand**: `mcs pack add user/repo` expands to `https://github.com/user/repo.git`. Filesystem paths are checked before shorthand regex to prevent ambiguity with relative paths like `org/pack` -- **Cross-project reference counting**: `ProjectIndex` (`~/.mcs/projects.yaml`) tracks which projects use which packs; `ResourceRefCounter` checks all scopes before removing shared brew packages, plugins or gitignore entries. Conservative by default — if state is unreadable, assume resource is still needed. MCP servers are project-independent (scoped via `-s local`) and skip ref counting. Gitignore entries need it because `GitignoreManager` resolves one file for the whole machine, so a pack in two scopes holds two claims on one line; `GitignoreManager.coreEntries` are owned by no pack and are protected from removal outright +- **Cross-project reference counting**: `ProjectIndex` (`~/.mcs/projects.yaml`) tracks which projects use which packs; `ResourceRefCounter` checks all scopes before removing shared brew packages or gitignore entries. Conservative by default — if state is unreadable, assume resource is still needed. MCP servers and plugins install per scope (`-s local` in a project) and skip cross-scope ref counting. Gitignore entries need it because `GitignoreManager` resolves one file for the whole machine, so a pack in two scopes holds two claims on one line; `GitignoreManager.coreEntries` are owned by no pack and are protected from removal outright - **Conditional copyPackFile namespacing**: `copyPackFile` destinations are installed flat by default — except `fileType: hook`, which `DestinationCollisionResolver` phase 0 namespaces unconditionally (whenever a filesystem context is present) so a pack can never overwrite a user's hand-written hook at a flat path. When two+ packs define the same `(destination, fileType)`, the resolver auto-namespaces: subdirectory prefix (`/`) for hooks/commands/agents/generic, or directory name suffix (`-`) for skills (which require flat one-level directories). First pack keeps the clean name; subsequent packs get namespaced. Skill renames emit a warning diff --git a/Sources/mcs/Core/ClaudeIntegration.swift b/Sources/mcs/Core/ClaudeIntegration.swift index 6c2c1a99..1c5e2598 100644 --- a/Sources/mcs/Core/ClaudeIntegration.swift +++ b/Sources/mcs/Core/ClaudeIntegration.swift @@ -10,10 +10,12 @@ protocol ClaudeCLI: Sendable { func mcpRemove(name: String, scope: String, workingDirectory: URL?) -> ShellResult @discardableResult func pluginMarketplaceAdd(repo: String) -> ShellResult + func pluginMarketplaceList() -> ShellResult + func pluginList(workingDirectory: URL?) -> ShellResult @discardableResult - func pluginInstall(ref: PluginRef) -> ShellResult + func pluginInstall(id: String, scope: String, workingDirectory: URL?) -> ShellResult @discardableResult - func pluginRemove(ref: PluginRef) -> ShellResult + func pluginRemove(id: String, scope: String, workingDirectory: URL?) -> ShellResult } /// Wrapper for the `claude` CLI to manage MCP servers and plugins. @@ -78,24 +80,44 @@ struct ClaudeIntegration: ClaudeCLI { ) } - /// Install a plugin (registers marketplace first). - @discardableResult - func pluginInstall(ref: PluginRef) -> ShellResult { - pluginMarketplaceAdd(repo: ref.marketplaceRepo) + /// `claude plugin marketplace list --json`. + func pluginMarketplaceList() -> ShellResult { + shell.run( + Constants.CLI.env, + arguments: [Constants.CLI.claudeCommand, "plugin", "marketplace", "list", "--json"], + additionalEnvironment: claudeEnv + ) + } - return shell.run( + /// `claude plugin list --json`. Run in a project, `enabled` reflects that project's settings. + func pluginList(workingDirectory: URL?) -> ShellResult { + shell.run( + Constants.CLI.env, + arguments: [Constants.CLI.claudeCommand, "plugin", "list", "--json"], + workingDirectory: workingDirectory?.path, + additionalEnvironment: claudeEnv + ) + } + + /// Install a plugin by its `name@marketplace` id. A `local` install belongs to the project + /// `workingDirectory` names, as with `mcpAdd`. + @discardableResult + func pluginInstall(id: String, scope: String, workingDirectory: URL?) -> ShellResult { + shell.run( Constants.CLI.env, - arguments: [Constants.CLI.claudeCommand, "plugin", "install", ref.bareName], + arguments: [Constants.CLI.claudeCommand, "plugin", "install", id, "-s", scope], + workingDirectory: workingDirectory?.path, additionalEnvironment: claudeEnv ) } - /// Remove a plugin. + /// Uninstall a plugin from one scope. See `pluginInstall` for `workingDirectory`. @discardableResult - func pluginRemove(ref: PluginRef) -> ShellResult { + func pluginRemove(id: String, scope: String, workingDirectory: URL?) -> ShellResult { shell.run( Constants.CLI.env, - arguments: [Constants.CLI.claudeCommand, "plugin", "remove", ref.bareName], + arguments: [Constants.CLI.claudeCommand, "plugin", "uninstall", id, "-s", scope], + workingDirectory: workingDirectory?.path, additionalEnvironment: claudeEnv ) } diff --git a/Sources/mcs/Core/Constants.swift b/Sources/mcs/Core/Constants.swift index 53ab7bdb..f9bf7508 100644 --- a/Sources/mcs/Core/Constants.swift +++ b/Sources/mcs/Core/Constants.swift @@ -164,6 +164,15 @@ enum Constants { // MARK: - Plugins + /// Claude Code plugin install scopes. + enum PluginScope { + /// Enabled only in the project the install ran in (`settings.local.json` there). + static let local = "local" + + /// Enabled in every project (`~/.claude/settings.json`). + static let user = "user" + } + enum Plugins { /// The official Anthropic plugin marketplace identifier. static let officialMarketplace = "claude-plugins-official" diff --git a/Sources/mcs/Core/PluginRef.swift b/Sources/mcs/Core/PluginRef.swift index 55641d3c..8d58c272 100644 --- a/Sources/mcs/Core/PluginRef.swift +++ b/Sources/mcs/Core/PluginRef.swift @@ -1,3 +1,5 @@ +import Foundation + /// Parsed representation of a plugin reference from `techpack.yaml`. /// /// Plugin names in manifests use the format `name@repo` where: @@ -15,6 +17,12 @@ struct PluginRef: Equatable { /// The original full string as declared in the manifest. let fullName: String + /// The marketplace's name as Claude Code keys it, when the reference itself says so. + /// + /// `nil` for an `@org/repo` reference: the name is declared by the marketplace, not derivable + /// from the repo, so it has to be looked up after `marketplace add`. + let marketplaceName: String? + /// Parse a plugin reference string. /// /// Accepted formats: @@ -29,14 +37,128 @@ struct PluginRef: Equatable { let repoToken = String(parts[1]) if repoToken.contains("/") { marketplaceRepo = repoToken + marketplaceName = repoToken == Constants.Plugins.officialMarketplaceRepo + ? Constants.Plugins.officialMarketplace : nil } else if repoToken == Constants.Plugins.officialMarketplace { marketplaceRepo = Constants.Plugins.officialMarketplaceRepo + marketplaceName = repoToken } else { marketplaceRepo = repoToken + marketplaceName = repoToken } } else { bareName = fullName marketplaceRepo = Constants.Plugins.officialMarketplaceRepo + marketplaceName = Constants.Plugins.officialMarketplace + } + } + + /// Whether `marketplaceRepo` names a repo `claude plugin marketplace add` can register. + var hasMarketplaceRepo: Bool { + marketplaceRepo.contains("/") + } + + /// The `name@marketplace` id Claude Code uses, resolved against the configured marketplaces. + func pluginID(in marketplaces: [PluginMarketplace]) -> String? { + if let marketplaceName { return "\(bareName)@\(marketplaceName)" } + let repo = marketplaceRepo.lowercased() + return marketplaces.first { $0.repo?.lowercased() == repo }.map { "\(bareName)@\($0.name)" } + } + + /// Whether `id` (`name@marketplace`) is this plugin. + func matches(id: String) -> Bool { + let parts = id.split(separator: "@", maxSplits: 1) + guard String(parts[0]) == bareName else { return false } + guard let marketplaceName, parts.count == 2 else { return true } + return String(parts[1]) == marketplaceName + } +} + +/// One entry of `claude plugin marketplace list --json`. +struct PluginMarketplace: Codable, Equatable { + let name: String + let repo: String? +} + +/// One entry of `claude plugin list --json`. +struct InstalledPlugin: Codable, Equatable { + let id: String + let scope: String + let enabled: Bool + /// The project a `local`- or `project`-scoped install belongs to. + let projectPath: String? + + /// Whether this install is the one a sync at `scope` (in `projectDirectory`, for `local`) owns. + func isInstall(atScope scope: String, projectDirectory: URL?) -> Bool { + guard self.scope == scope else { return false } + guard scope == Constants.PluginScope.local else { return true } + guard let projectDirectory, let projectPath else { return false } + return URL(fileURLWithPath: projectPath).resolvingSymlinksInPath().path + == projectDirectory.resolvingSymlinksInPath().path + } +} + +/// `claude plugin list --json`, run once per directory for the life of one sync or doctor run. +/// +/// Every plugin component and every `PluginCheck` needs the same listing, and each CLI call costs +/// a process start. Installs and removals made through it invalidate what they changed. +final class PluginListing: @unchecked Sendable { + struct Failure: Error, LocalizedError { + let errorDescription: String? + } + + let claudeCLI: any ClaudeCLI + private let lock = NSLock() + private var byDirectory: [String: Result<[InstalledPlugin], Failure>] = [:] + + init(claudeCLI: any ClaudeCLI) { + self.claudeCLI = claudeCLI + } + + /// Listed from `directory`, `enabled` is what that directory's project actually loads. + func plugins(in directory: URL?) -> Result<[InstalledPlugin], Failure> { + let key = directory?.path ?? "" + lock.lock() + defer { lock.unlock() } + if let cached = byDirectory[key] { return cached } + let listed = list(in: directory) + byDirectory[key] = listed + return listed + } + + func invalidate() { + lock.lock() + defer { lock.unlock() } + byDirectory.removeAll() + } + + private func list(in directory: URL?) -> Result<[InstalledPlugin], Failure> { + guard claudeCLI.isAvailable else { return .failure(Failure(errorDescription: "Claude Code CLI not found")) } + let result = claudeCLI.pluginList(workingDirectory: directory) + guard result.succeeded else { + return .failure(Failure(errorDescription: "could not list plugins: \(String(result.stderr.prefix(200)))")) + } + do { + return try .success(JSONDecoder().decode([InstalledPlugin].self, from: Data(result.stdout.utf8))) + } catch { + return .failure(Failure(errorDescription: "could not read the plugin list: \(error.localizedDescription)")) + } + } +} + +extension PackArtifactRecord { + func ownsPlugin(_ name: String) -> Bool { + let bareName = PluginRef(name).bareName + return plugins.contains { PluginRef($0).bareName == bareName } + } +} + +extension TechPack { + func declaresPlugin(_ name: String) -> Bool { + let bareName = PluginRef(name).bareName + return components.contains { component in + guard case let .plugin(declared) = component.installAction else { return false } + return PluginRef(declared).bareName == bareName } } } diff --git a/Sources/mcs/Doctor/CoreDoctorChecks.swift b/Sources/mcs/Doctor/CoreDoctorChecks.swift index c6d269f5..2e13f513 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -96,14 +96,11 @@ struct MCPServerCheck: DoctorCheck { struct PluginCheck: DoctorCheck { let pluginRef: PluginRef + /// Set for a project scope, where the plugin is installed `local` to this project. let projectRoot: URL? - let environment: Environment - - init(pluginRef: PluginRef, projectRoot: URL? = nil, environment: Environment = Environment()) { - self.pluginRef = pluginRef - self.projectRoot = projectRoot - self.environment = environment - } + let listing: PluginListing + /// Where the global scope lists from: anywhere else, a project's settings would decide `enabled`. + var homeDirectory = URL(fileURLWithPath: NSHomeDirectory()) var name: String { pluginRef.bareName @@ -114,48 +111,23 @@ struct PluginCheck: DoctorCheck { } func check() -> CheckResult { - var projectSettingsError: String? - - // Tier 1: Project-scoped settings.local.json - if let root = projectRoot { - let projectSettingsURL = root - .appendingPathComponent(Constants.FileNames.claudeDirectory) - .appendingPathComponent(Constants.FileNames.settingsLocal) - do { - let projectSettings = try Settings.load(from: projectSettingsURL) - if projectSettings.enabledPlugins?[pluginRef.bareName] == true { - return .pass("enabled (project)") - } - } catch { - // Corrupt project settings — fall through to global, but note for diagnostics - projectSettingsError = error.localizedDescription - } + let installs: [InstalledPlugin] + switch listing.plugins(in: projectRoot ?? homeDirectory) { + case let .success(plugins): + installs = plugins.filter { pluginRef.matches(id: $0.id) } + case let .failure(failure): + return .fail(failure.localizedDescription) } - // Tier 2: Global settings.json - let settingsURL = environment.claudeSettings - guard FileManager.default.fileExists(atPath: settingsURL.path) else { - if projectSettingsError != nil { - return .fail("settings.local.json is corrupt and settings.json not found") - } - return .fail("settings.json not found") - } - let settings: Settings - do { - settings = try Settings.load(from: settingsURL) - } catch { - return .fail("settings.json is invalid: \(error.localizedDescription)") - } - if settings.enabledPlugins?[pluginRef.bareName] == true { - if let errorDesc = projectSettingsError { - return .warn("enabled (global) — settings.local.json is unreadable: \(errorDesc)") - } - return .pass("enabled") + let expectedScope = projectRoot == nil ? Constants.PluginScope.user : Constants.PluginScope.local + if installs.contains(where: { $0.enabled && $0.isInstall(atScope: expectedScope, projectDirectory: projectRoot) }) { + return .pass(projectRoot == nil ? "enabled" : "enabled (project)") } - if projectSettingsError != nil { - return .fail("not enabled (settings.local.json is corrupt)") + // Not mcs's install, but the plugin does load here. + if let other = installs.first(where: \.enabled) { + return .pass("enabled (\(other.scope) scope)") } - return .fail("not enabled") + return .fail(installs.isEmpty ? "not installed" : "installed but disabled") } func fix() -> FixResult { diff --git a/Sources/mcs/Doctor/DerivedDoctorChecks.swift b/Sources/mcs/Doctor/DerivedDoctorChecks.swift index 4beb7c2d..2a31868a 100644 --- a/Sources/mcs/Doctor/DerivedDoctorChecks.swift +++ b/Sources/mcs/Doctor/DerivedDoctorChecks.swift @@ -6,13 +6,23 @@ extension ComponentDefinition { /// Auto-generates doctor check(s) from installAction. /// Returns nil for actions that have no mechanical verification /// (e.g. .shellCommand, .settingsMerge, .gitignoreEntries). - func deriveDoctorCheck(projectRoot: URL? = nil, environment: Environment = Environment()) -> (any DoctorCheck)? { + func deriveDoctorCheck( + projectRoot: URL? = nil, + environment: Environment = Environment(), + pluginListing: PluginListing? = nil + ) -> (any DoctorCheck)? { switch installAction { case let .mcpServer(config): return MCPServerCheck(name: displayName, serverName: config.name, projectRoot: projectRoot, environment: environment) case let .plugin(pluginName): - return PluginCheck(pluginRef: PluginRef(pluginName), projectRoot: projectRoot, environment: environment) + return PluginCheck( + pluginRef: PluginRef(pluginName), + projectRoot: projectRoot, + listing: pluginListing + ?? PluginListing(claudeCLI: ClaudeIntegration(shell: ShellRunner(environment: environment))), + homeDirectory: environment.homeDirectory + ) case let .brewInstall(package): return BrewPackageCheck( diff --git a/Sources/mcs/Doctor/DoctorRunner.swift b/Sources/mcs/Doctor/DoctorRunner.swift index 93b5f443..78dfba02 100644 --- a/Sources/mcs/Doctor/DoctorRunner.swift +++ b/Sources/mcs/Doctor/DoctorRunner.swift @@ -227,6 +227,7 @@ struct DoctorRunner { } // Layer 1+2: Derived + supplementary checks from installed components (per scope) + let pluginListing = PluginListing(claudeCLI: claudeCLI ?? ClaudeIntegration(shell: ShellRunner(environment: env))) for scope in scopes { allPackIDs.formUnion(scope.packIDs) @@ -239,7 +240,9 @@ struct DoctorRunner { // can repair them. Pack-authored checks can assert anything, so they never trigger one. for pack in scopePacks { for component in pack.components { - if let derived = component.deriveDoctorCheck(projectRoot: scope.effectiveProjectRoot, environment: env) { + if let derived = component.deriveDoctorCheck( + projectRoot: scope.effectiveProjectRoot, environment: env, pluginListing: pluginListing + ) { allChecks.append((check: derived, syncTarget: scope.syncTarget)) } allChecks += component.supplementaryChecks(scope.effectiveProjectRoot, env) diff --git a/Sources/mcs/Sync/ComponentExecutor.swift b/Sources/mcs/Sync/ComponentExecutor.swift index 7bdc5982..8b836c23 100644 --- a/Sources/mcs/Sync/ComponentExecutor.swift +++ b/Sources/mcs/Sync/ComponentExecutor.swift @@ -7,6 +7,8 @@ struct ComponentExecutor { let shell: any ShellRunning let claudeCLI: any ClaudeCLI var mcpWorkingDirectory: URL? + /// Shared across a run so each plugin doesn't pay for its own `plugin list`. + var pluginListing: PluginListing? // MARK: - Brew Packages @@ -54,15 +56,96 @@ struct ComponentExecutor { // MARK: - Plugins - /// Install a plugin via the Claude CLI. - func installPlugin(_ fullName: String) -> Bool { + enum PluginInstallOutcome: Equatable { + /// mcs ran the install, so it owns it. `userScopeCopy` is set when the plugin is also + /// enabled for every project, which a `local` install alone does not need. + case installed(id: String, userScopeCopy: Bool) + /// Already installed at this scope by something else; mcs takes no ownership. + case alreadyInstalled(id: String) + case failed + } + + /// Install a plugin at `scope`, in the scope's project for `local`. + func installPlugin(_ fullName: String, scope: String) -> PluginInstallOutcome { guard claudeCLI.isAvailable else { output.warn("Claude Code CLI not found, skipping plugin") + return .failed + } + let ref = PluginRef(fullName) + if ref.hasMarketplaceRepo { + claudeCLI.pluginMarketplaceAdd(repo: ref.marketplaceRepo) + } + guard let id = resolvePluginID(ref) else { + output.warn("Could not find the marketplace for plugin '\(fullName)' — is it added? Skipping") + return .failed + } + let listing = pluginListing ?? PluginListing(claudeCLI: claudeCLI) + let installed = installedPlugins(listing) + if installed.contains(where: { $0.id == id && $0.isInstall(atScope: scope, projectDirectory: mcpWorkingDirectory) }) { + return .alreadyInstalled(id: id) + } + let result = claudeCLI.pluginInstall(id: id, scope: scope, workingDirectory: mcpWorkingDirectory) + listing.invalidate() + guard result.succeeded else { + output.warn("Could not install plugin '\(id)': \(String(result.stderr.prefix(200)))") + return .failed + } + let userScopeCopy = scope != Constants.PluginScope.user + && installed.contains { $0.id == id && $0.scope == Constants.PluginScope.user && $0.enabled } + return .installed(id: id, userScopeCopy: userScopeCopy) + } + + /// Remove a plugin from `scope` via the Claude CLI and log the outcome. + /// Returns `true` when the plugin is no longer installed there, including when it already wasn't. + func removePlugin(_ fullName: String, scope: String) -> Bool { + guard claudeCLI.isAvailable else { + output.warn("Claude Code CLI not found, cannot remove plugin") return false } let ref = PluginRef(fullName) - let result = claudeCLI.pluginInstall(ref: ref) - return result.succeeded + // A marketplace removed since install can't be resolved; the CLI still matches a bare name. + let id = resolvePluginID(ref) ?? ref.bareName + let result = claudeCLI.pluginRemove(id: id, scope: scope, workingDirectory: mcpWorkingDirectory) + pluginListing?.invalidate() + // Reliable only because a `local` lookup runs in the scope's own project: from anywhere + // else the CLI would answer for a different project. + if result.stderr.contains(Constants.CLI.pluginNotFound) { + output.dimmed(" Plugin '\(ref.bareName)' is not installed — dropping it from tracking") + return true + } + guard result.succeeded else { + output.warn("Could not remove plugin '\(ref.bareName)': \(result.stderr)") + return false + } + output.dimmed(" Removed plugin: \(ref.bareName)") + return true + } + + private func resolvePluginID(_ ref: PluginRef) -> String? { + if ref.marketplaceName != nil { return ref.pluginID(in: []) } + let result = claudeCLI.pluginMarketplaceList() + guard result.succeeded else { + output.warn("Could not list plugin marketplaces: \(String(result.stderr.prefix(200)))") + return nil + } + do { + let marketplaces = try JSONDecoder().decode([PluginMarketplace].self, from: Data(result.stdout.utf8)) + return ref.pluginID(in: marketplaces) + } catch { + output.warn("Could not read the plugin marketplace list: \(error.localizedDescription)") + return nil + } + } + + /// An unreadable listing only costs an install that turns out to be a no-op, so it is not an error. + private func installedPlugins(_ listing: PluginListing) -> [InstalledPlugin] { + switch listing.plugins(in: mcpWorkingDirectory) { + case let .success(plugins): + return plugins + case let .failure(failure): + output.dimmed(" \(failure.localizedDescription) — installing anyway") + return [] + } } /// Uninstall a Homebrew package and log the outcome. @@ -87,28 +170,6 @@ struct ComponentExecutor { return true } - /// Remove a plugin via the Claude CLI and log the outcome. - /// Returns `true` when the plugin is no longer installed, including when it already wasn't. - func removePlugin(_ fullName: String) -> Bool { - guard claudeCLI.isAvailable else { - output.warn("Claude Code CLI not found, cannot remove plugin") - return false - } - let ref = PluginRef(fullName) - let result = claudeCLI.pluginRemove(ref: ref) - // Plugins are installed at user scope, so "not found" is reliable from any directory. - if result.stderr.contains(Constants.CLI.pluginNotFound) { - output.dimmed(" Plugin '\(ref.bareName)' is not installed — dropping it from tracking") - return true - } - guard result.succeeded else { - output.warn("Could not remove plugin '\(ref.bareName)': \(result.stderr)") - return false - } - output.dimmed(" Removed plugin: \(ref.bareName)") - return true - } - // MARK: - Gitignore /// Add entries to the global gitignore. @@ -394,7 +455,9 @@ struct ComponentExecutor { static func isAlreadyInstalled(_ component: ComponentDefinition) -> Bool { // Convergent actions: always re-run to pick up config changes switch component.installAction { - case .settingsMerge, .gitignoreEntries, .copyPackFile, .mcpServer: + // `.plugin`: `installPlugin` checks the scope it installs into itself, and a derived check + // here has no project, so it would look in the wrong place. + case .settingsMerge, .gitignoreEntries, .copyPackFile, .mcpServer, .plugin: return false default: break diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index 05fbdac4..c301e41c 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -13,6 +13,7 @@ struct Configurator { var registry: TechPackRegistry let strategy: any SyncStrategy let claudeCLI: any ClaudeCLI + private let pluginListing: PluginListing init( environment: Environment, @@ -28,6 +29,7 @@ struct Configurator { self.registry = registry self.strategy = strategy self.claudeCLI = claudeCLI ?? ClaudeIntegration(shell: shell) + pluginListing = PluginListing(claudeCLI: self.claudeCLI) } private var scope: SyncScope { @@ -422,7 +424,6 @@ struct Configurator { var remaining = artifacts var removedServers: Set = [] var removedBrewPackages: Set = [] - var removedPlugins: Set = [] // Remove MCS-owned brew packages and plugins (with reference counting) let excludeScope = refCountScope ?? scope.scopeIdentifier @@ -445,19 +446,11 @@ struct Configurator { } remaining.brewPackages.removeAll { removedBrewPackages.contains($0) } - for pluginName in artifacts.plugins { - let result = removePluginArtifact( - pluginName, exec: exec, refCounter: refCounter, - excludingScope: excludeScope, excludingPack: packID - ) - switch result { - case .removed, .stillNeeded: - removedPlugins.insert(pluginName) - case .failed: - output.warn(" Could not remove plugin '\(PluginRef(pluginName).bareName)' — re-run '\(retryHint)' to retry") - } + let unremovedPlugins = removeOwnedPlugins(artifacts.plugins, of: packID, exec: exec, state: state) + for pluginName in unremovedPlugins { + output.warn(" Could not remove plugin '\(PluginRef(pluginName).bareName)' — re-run '\(retryHint)' to retry") } - remaining.plugins.removeAll { removedPlugins.contains($0) } + remaining.plugins = remaining.plugins.filter { unremovedPlugins.contains($0) } // Remove MCP servers for server in artifacts.mcpServers @@ -909,10 +902,12 @@ struct Configurator { shell: shell, output: output ) + recordSharedPluginOwnership(pack, artifacts: &artifacts, state: state) reconcileStaleArtifacts( previousArtifacts: previousArtifacts, currentArtifacts: &artifacts, - packID: pack.identifier + packID: pack.identifier, + scopePacks: packs ) state.setArtifacts(artifacts, for: pack.identifier) state.recordPack(pack.identifier) @@ -959,6 +954,20 @@ struct Configurator { } } + /// A plugin another pack in this scope already installed counts as this pack's too, so the + /// last pack to leave the scope is the one that uninstalls it rather than nobody. + private func recordSharedPluginOwnership( + _ pack: any TechPack, artifacts: inout PackArtifactRecord, state: ProjectState + ) { + for component in pack.components { + guard case let .plugin(name) = component.installAction, !artifacts.plugins.contains(name) else { continue } + let ownedInScope = state.configuredPacks.contains { otherID in + otherID != pack.identifier && state.artifacts(for: otherID)?.ownsPlugin(name) == true + } + if ownedInScope { artifacts.recordPlugin(name) } + } + } + /// Remove artifacts that were tracked in the previous sync but are absent from the current one. /// /// After `installArtifacts()` produces a fresh `PackArtifactRecord`, this method diffs it @@ -971,7 +980,8 @@ struct Configurator { private func reconcileStaleArtifacts( previousArtifacts: PackArtifactRecord?, currentArtifacts: inout PackArtifactRecord, - packID: String + packID: String, + scopePacks: [any TechPack] ) { guard let previous = previousArtifacts else { return } @@ -1059,14 +1069,9 @@ struct Configurator { } } for name in stalePlugins { - let result = removePluginArtifact( - name, exec: exec, refCounter: refCounter, - excludingScope: scope.scopeIdentifier, excludingPack: packID - ) - switch result { - case .removed, .stillNeeded: - break - case .failed: + let claimants = scopePacks.filter { $0.identifier != packID } + .map { PluginClaimant(packID: $0.identifier, pack: $0, record: nil) } + if !removePluginArtifact(name, exec: exec, otherClaimants: claimants) { currentArtifacts.plugins.append(name) output.warn(" Could not remove stale plugin '\(PluginRef(name).bareName)' — will retry on next sync") } @@ -1194,29 +1199,39 @@ struct Configurator { return .failed } - /// Remove a plugin with reference counting. + /// Remove a plugin from this scope unless another pack in the same scope still claims it. /// - /// Logs the "Keeping" message when the plugin is still needed by another scope or pack. - /// Callers provide their own success/failure context messages. + /// Plugins install per scope (`local` in a project, `user` globally), so only packs sharing the + /// scope can share an install; other scopes hold installs of their own. + /// Returns the plugins that could not be removed and stay recorded. + private func removeOwnedPlugins( + _ plugins: [String], of packID: String, exec: ComponentExecutor, state: ProjectState + ) -> [String] { + let otherClaimants = state.configuredPacks.filter { $0 != packID }.map { + PluginClaimant(packID: $0, pack: registry.pack(for: $0), record: state.artifacts(for: $0)) + } + return plugins.filter { !removePluginArtifact($0, exec: exec, otherClaimants: otherClaimants) } + } + + private struct PluginClaimant { + let packID: String + let pack: (any TechPack)? + let record: PackArtifactRecord? + } + + /// Returns `false` only when an uninstall was attempted and failed; a plugin another pack in + /// the scope still claims is kept and counts as handled. private func removePluginArtifact( _ name: String, exec: ComponentExecutor, - refCounter: ResourceRefCounter, - excludingScope: String, - excludingPack: String - ) -> RefCountedRemovalResult { - if refCounter.isStillNeeded( - .plugin(name), - excludingScope: excludingScope, - excludingPack: excludingPack - ) { - output.dimmed(" Keeping plugin '\(PluginRef(name).bareName)' — still needed by another scope or pack") - return .stillNeeded - } - if exec.removePlugin(name) { - return .removed + otherClaimants: [PluginClaimant] + ) -> Bool { + // Conservative, as in `ResourceRefCounter`: an unloadable pack may still declare it. + if let claimant = otherClaimants.first(where: { $0.record?.ownsPlugin(name) == true || $0.pack?.declaresPlugin(name) ?? true }) { + output.dimmed(" Keeping plugin '\(PluginRef(name).bareName)' — still needed by \(claimant.packID)") + return true } - return .failed + return exec.removePlugin(name, scope: scope.pluginScope) } /// Remove a single gitignore entry with reference counting, absorbing the do/catch. @@ -1270,22 +1285,14 @@ struct Configurator { // MARK: - Global Dependencies - /// Auto-install brew packages and plugins (project scope only). + /// Auto-install brew packages (project scope only); plugins install per scope with the pack. private func autoInstallGlobalDependencies(_ pack: any TechPack) { let exec = makeExecutor() for component in pack.components { - guard !ComponentExecutor.isAlreadyInstalled(component) else { continue } - - switch component.installAction { - case let .brewInstall(package): - output.dimmed(" Installing \(component.displayName)...") - _ = exec.installBrewPackage(package) - case let .plugin(name): - output.dimmed(" Installing plugin \(component.displayName)...") - _ = exec.installPlugin(name) - default: - break - } + guard case let .brewInstall(package) = component.installAction, + !ComponentExecutor.isAlreadyInstalled(component) else { continue } + output.dimmed(" Installing \(component.displayName)...") + _ = exec.installBrewPackage(package) } } @@ -1294,7 +1301,7 @@ struct Configurator { private func makeExecutor() -> ComponentExecutor { ComponentExecutor( environment: environment, output: output, shell: shell, claudeCLI: claudeCLI, - mcpWorkingDirectory: scope.mcpWorkingDirectory + mcpWorkingDirectory: scope.mcpWorkingDirectory, pluginListing: pluginListing ) } } diff --git a/Sources/mcs/Sync/ConfiguratorSupport.swift b/Sources/mcs/Sync/ConfiguratorSupport.swift index f3d36302..ec01c5f5 100644 --- a/Sources/mcs/Sync/ConfiguratorSupport.swift +++ b/Sources/mcs/Sync/ConfiguratorSupport.swift @@ -366,7 +366,7 @@ enum ConfiguratorSupport { } // Pass 1: entries derived from component definitions. - for (pack, component) in packComponents { + for component in packComponents.map(\.component) { if let reg = component.hookRegistration, let command = component.hookCommand(pathPrefix: hookPathPrefix) { if settings.addHookEntry( @@ -386,17 +386,6 @@ enum ConfiguratorSupport { } } } - - if case let .plugin(name) = component.installAction { - let ref = PluginRef(name) - var plugins = settings.enabledPlugins ?? [:] - if plugins[ref.bareName] == nil { - plugins[ref.bareName] = true - } - settings.enabledPlugins = plugins - hasContent = true - contributedKeys[pack.identifier, default: []].append("enabledPlugins.\(ref.bareName)") - } } // Pass 2: pack-supplied settings files, merged on top of the derived entries. @@ -487,6 +476,29 @@ enum ConfiguratorSupport { return expanded } + /// Install a plugin at `scope` and record it only when this install is what put it there. + @discardableResult + static func installPlugin( + _ name: String, + component: ComponentDefinition, + scope: String, + executor: ComponentExecutor, + artifacts: inout PackArtifactRecord, + output: CLIOutput + ) -> ComponentExecutor.PluginInstallOutcome { + let outcome = executor.installPlugin(name, scope: scope) + switch outcome { + case .installed: + artifacts.recordPlugin(name) + output.success(" \(component.displayName) installed (scope: \(scope))") + case .alreadyInstalled: + output.dimmed(" \(component.displayName) already installed, skipping") + case .failed: + output.warn(" \(component.displayName) failed to install") + } + return outcome + } + /// Compute per-pack SHA-256 hashes of contributed settings values from the on-disk file. /// /// Reads the settings file once and hashes each pack's key-value pairs independently. diff --git a/Sources/mcs/Sync/GlobalSyncStrategy.swift b/Sources/mcs/Sync/GlobalSyncStrategy.swift index 656b5e63..24e19e09 100644 --- a/Sources/mcs/Sync/GlobalSyncStrategy.swift +++ b/Sources/mcs/Sync/GlobalSyncStrategy.swift @@ -92,13 +92,10 @@ struct GlobalSyncStrategy: SyncStrategy { } case let .plugin(name): - output.dimmed(" Installing plugin \(component.displayName)...") - if executor.installPlugin(name) { - artifacts.recordPlugin(name) - output.success(" \(component.displayName) installed") - } else { - output.warn(" \(component.displayName) failed to install") - } + ConfiguratorSupport.installPlugin( + name, component: component, scope: scope.pluginScope, + executor: executor, artifacts: &artifacts, output: output + ) case let .copyPackFile(source, destination, fileType): let result = executor.installCopyPackFile( diff --git a/Sources/mcs/Sync/ProjectSyncStrategy.swift b/Sources/mcs/Sync/ProjectSyncStrategy.swift index 00a0715c..2136664c 100644 --- a/Sources/mcs/Sync/ProjectSyncStrategy.swift +++ b/Sources/mcs/Sync/ProjectSyncStrategy.swift @@ -60,6 +60,7 @@ struct ProjectSyncStrategy: SyncStrategy { ) -> PackArtifactRecord { var artifacts = PackArtifactRecord() artifacts.fileHashesShippedOnly = true + artifacts.plugins = previousArtifacts?.plugins ?? [] for component in pack.components { if ComponentExecutor.isAlreadyInstalled(component) { @@ -102,7 +103,18 @@ struct ProjectSyncStrategy: SyncStrategy { artifacts.gitignoreEntries.append(contentsOf: entries) } - case .brewInstall, .plugin: + case let .plugin(name): + if case let .installed(id, userScopeCopy: true) = ConfiguratorSupport.installPlugin( + name, component: component, scope: scope.pluginScope, + executor: executor, artifacts: &artifacts, output: output + ), !globalScopeRecordsPlugin(name) { + output.info( + " '\(id)' is also installed for every project. If nothing outside the projects" + + " that declare it needs it, run: claude plugin uninstall \(id) -s user" + ) + } + + case .brewInstall: break case let .shellCommand(command, interactive): @@ -132,6 +144,17 @@ struct ProjectSyncStrategy: SyncStrategy { return artifacts } + /// Whether a global-scope pack owns a user-scope copy of the plugin, so the copy is not stray. + private func globalScopeRecordsPlugin(_ name: String) -> Bool { + do { + let globalState = try ProjectState(stateFile: environment.globalStateFile) + return globalState.configuredPacks.contains { globalState.artifacts(for: $0)?.ownsPlugin(name) == true } + } catch { + // Only decides whether a hint is shown, never whether anything is removed. + return false + } + } + // MARK: - Settings Composition func composeSettings( @@ -153,6 +176,8 @@ struct ProjectSyncStrategy: SyncStrategy { var existing = try Settings.load(from: scope.settingsPath) existing.removeKeys(allPreviousKeys) settings.extraJSON = existing.extraJSON + // Written by `claude plugin install -s local`, not by mcs. + settings.enabledPlugins = existing.enabledPlugins } catch { output.warn( "Could not parse \(scope.settingsPath.lastPathComponent): \(error.localizedDescription)" @@ -168,7 +193,7 @@ struct ProjectSyncStrategy: SyncStrategy { output: output ) - if hasContent || !settings.extraJSON.isEmpty { + if hasContent || !settings.extraJSON.isEmpty || !(settings.enabledPlugins ?? [:]).isEmpty { do { try settings.save(to: scope.settingsPath, dropKeys: dropKeys) if hasContent { diff --git a/Sources/mcs/Sync/ResourceRefCounter.swift b/Sources/mcs/Sync/ResourceRefCounter.swift index 3e952ee2..a6675d37 100644 --- a/Sources/mcs/Sync/ResourceRefCounter.swift +++ b/Sources/mcs/Sync/ResourceRefCounter.swift @@ -1,13 +1,13 @@ import Foundation -/// Determines whether a global resource (brew package, plugin, or gitignore entry) can be +/// Determines whether a global resource (brew package or gitignore entry) can be /// safely removed by checking all projects and the global scope for references. /// /// Uses a two-tier check: /// 1. Global-state artifact records (ownership) for other globally-configured packs /// 2. Project index → `.mcs-project` → pack manifest (declarations) for project-scoped packs /// -/// MCP servers are project-independent (scoped via `-s local`) and never need ref counting. +/// MCP servers and plugins install per scope, so they never need cross-scope ref counting. /// Gitignore entries do: `GitignoreManager` resolves one file for the whole machine, so a /// pack installed in two scopes holds two claims on a single physical line. struct ResourceRefCounter { @@ -31,13 +31,11 @@ struct ResourceRefCounter { enum Resource: Equatable { case brewPackage(String) - case plugin(String) case gitignoreEntry(String) var displayName: String { switch self { case let .brewPackage(name): "brew package '\(name)'" - case let .plugin(name): "plugin '\(PluginRef(name).bareName)'" case let .gitignoreEntry(entry): "gitignore entry '\(entry)'" } } @@ -49,7 +47,7 @@ struct ResourceRefCounter { /// mcs's own lines: a pack may declare one, but must not be able to remove it. var isProtected: Bool { switch self { - case .brewPackage, .plugin: false + case .brewPackage: false case let .gitignoreEntry(entry): GitignoreManager.coreEntries.contains(entry) } } @@ -126,11 +124,6 @@ struct ResourceRefCounter { switch resource { case let .brewPackage(name): if artifacts.brewPackages.contains(name) { return true } - case let .plugin(name): - let refBareName = PluginRef(name).bareName - if artifacts.plugins.contains(where: { PluginRef($0).bareName == refBareName }) { - return true - } case let .gitignoreEntry(entry): if artifacts.gitignoreEntries.contains(entry) { return true } } @@ -202,8 +195,6 @@ struct ResourceRefCounter { switch (resource, component.installAction) { case let (.brewPackage(name), .brewInstall(pkg)): if pkg == name { return true } - case let (.plugin(name), .plugin(pluginName)): - if PluginRef(pluginName).bareName == PluginRef(name).bareName { return true } // Exact match: `.gitignoreEntries` carries a literal payload that never goes // through placeholder substitution, unlike `MCPServerConfig.substituting`. case let (.gitignoreEntry(entry), .gitignoreEntries(entries)): diff --git a/Sources/mcs/Sync/SyncScope.swift b/Sources/mcs/Sync/SyncScope.swift index 2e302ade..0b64e126 100644 --- a/Sources/mcs/Sync/SyncScope.swift +++ b/Sources/mcs/Sync/SyncScope.swift @@ -33,6 +33,9 @@ struct SyncScope { /// `nil` = use the pack's declared scope (project default); `"user"` = global scope. let mcpScopeOverride: String? + /// Scope `claude plugin install/uninstall` targets: `local` (this project), `user` (global). + let pluginScope: String + /// Directory the `claude mcp` commands run in, which selects the project for `local` scope. /// Project: the project path, so a command started elsewhere still targets it; Global: `nil`. let mcpWorkingDirectory: URL? @@ -75,6 +78,7 @@ extension SyncScope { claudeFilePath: projectPath.appendingPathComponent(Constants.FileNames.claudeLocalMD), scopeIdentifier: projectPath.path, mcpScopeOverride: nil, + pluginScope: Constants.PluginScope.local, mcpWorkingDirectory: projectPath, includeTemplatesInScan: true, runConfigureProjectHooks: true, @@ -96,6 +100,7 @@ extension SyncScope { claudeFilePath: environment.globalClaudeMD, scopeIdentifier: ProjectIndex.globalSentinel, mcpScopeOverride: Constants.MCPScope.user, + pluginScope: Constants.PluginScope.user, mcpWorkingDirectory: nil, includeTemplatesInScan: false, runConfigureProjectHooks: false, diff --git a/Sources/mcs/Sync/SyncStrategy.swift b/Sources/mcs/Sync/SyncStrategy.swift index 39e0a4a6..c5532d40 100644 --- a/Sources/mcs/Sync/SyncStrategy.swift +++ b/Sources/mcs/Sync/SyncStrategy.swift @@ -183,8 +183,7 @@ extension SyncStrategy { return nil } if !plugins.isEmpty { - let suffix = scope.isGlobalScope ? "" : " (global)" - output.dimmed(" Plugins: \(plugins.joined(separator: ", "))\(suffix)") + output.dimmed(" Plugins: \(plugins.joined(separator: ", "))") } let templateSections = pack.templateSectionIdentifiers.map { "+\($0) section" } diff --git a/Sources/mcs/TechPack/TechPack.swift b/Sources/mcs/TechPack/TechPack.swift index 26da4592..67b821c4 100644 --- a/Sources/mcs/TechPack/TechPack.swift +++ b/Sources/mcs/TechPack/TechPack.swift @@ -100,7 +100,7 @@ extension DoctorCheck { } } -enum CheckResult { +enum CheckResult: Equatable { case pass(String) case fail(String) case warn(String) diff --git a/Tests/MCSTests/ComponentExecutorTests.swift b/Tests/MCSTests/ComponentExecutorTests.swift index a1d5e6da..d08b134e 100644 --- a/Tests/MCSTests/ComponentExecutorTests.swift +++ b/Tests/MCSTests/ComponentExecutorTests.swift @@ -16,16 +16,9 @@ struct ComponentExecutorTests { // MARK: - removePlugin private func removePlugin(returning result: ShellResult) -> Bool { - let env = Environment() let cli = MockClaudeCLI() cli.result = result - let exec = ComponentExecutor( - environment: env, - output: CLIOutput(colorsEnabled: false, interactiveStdin: false), - shell: ShellRunner(environment: env), - claudeCLI: cli - ) - return exec.removePlugin("lint@acme") + return makeExecutor(cli).removePlugin("lint@acme", scope: Constants.PluginScope.user) } @Test("removePlugin counts a plugin that is already uninstalled as removed") @@ -134,4 +127,56 @@ struct ComponentExecutorTests { let expectedHash = try FileHasher.sha256(of: installed) #expect(result.hashes.values.first == expectedHash) } + + // MARK: - installPlugin + + private func makeExecutor(_ cli: MockClaudeCLI, project: URL? = nil) -> ComponentExecutor { + let env = Environment() + return ComponentExecutor( + environment: env, + output: CLIOutput(colorsEnabled: false, interactiveStdin: false), + shell: ShellRunner(environment: env), + claudeCLI: cli, + mcpWorkingDirectory: project + ) + } + + @Test("installPlugin registers a repo marketplace and installs by its resolved id") + func installPluginResolvesRepoMarketplace() { + let cli = MockClaudeCLI() + cli.marketplaces = [PluginMarketplace(name: "hud", repo: "org/hud-repo")] + let project = URL(fileURLWithPath: "/tmp/mcs-exec-project") + + let outcome = makeExecutor(cli, project: project).installPlugin("lint@org/hud-repo", scope: Constants.PluginScope.local) + + #expect(outcome == .installed(id: "lint@hud", userScopeCopy: false)) + #expect(cli.marketplaceAddCalls == ["org/hud-repo"]) + #expect(cli.pluginInstallCalls == [MockClaudeCLI.PluginCall(id: "lint@hud", scope: "local", workingDirectory: project)]) + } + + @Test("installPlugin skips a plugin whose marketplace can't be found") + func installPluginUnresolvableMarketplace() { + let cli = MockClaudeCLI() + let outcome = makeExecutor(cli).installPlugin("lint@org/unknown", scope: Constants.PluginScope.user) + #expect(outcome == .failed) + #expect(cli.pluginInstallCalls.isEmpty) + } + + @Test("installPlugin takes no ownership of an install already at the target scope") + func installPluginAlreadyInstalled() { + let cli = MockClaudeCLI() + cli.installedPlugins = [InstalledPlugin(id: "lint@acme", scope: "user", enabled: true, projectPath: nil)] + let outcome = makeExecutor(cli).installPlugin("lint@acme", scope: Constants.PluginScope.user) + #expect(outcome == .alreadyInstalled(id: "lint@acme")) + #expect(cli.pluginInstallCalls.isEmpty) + } + + @Test("installPlugin reports a user-scope copy when installing locally") + func installPluginFlagsUserScopeCopy() { + let cli = MockClaudeCLI() + cli.installedPlugins = [InstalledPlugin(id: "lint@acme", scope: "user", enabled: true, projectPath: nil)] + let project = URL(fileURLWithPath: "/tmp/mcs-exec-project") + let outcome = makeExecutor(cli, project: project).installPlugin("lint@acme", scope: Constants.PluginScope.local) + #expect(outcome == .installed(id: "lint@acme", userScopeCopy: true)) + } } diff --git a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift index 14cbf0cc..d4307a5a 100644 --- a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift @@ -222,324 +222,123 @@ struct MCPServerCheckSandboxTests { } } -// MARK: - PluginCheck Sandbox Tests +// MARK: - PluginCheck Tests -struct PluginCheckSandboxTests { - @Test("pass when plugin is enabled in settings.json") - func passWhenEnabled() throws { - let home = try makeGlobalTmpDir(label: "plugin-pass") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let settings = """ - { - "enabledPlugins": { - "pr-review-toolkit": true - } - } - """ - try settings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) +struct PluginCheckTests { + private let project = URL(fileURLWithPath: "/tmp/mcs-plugin-check-project") - let check = PluginCheck(pluginRef: PluginRef("pr-review-toolkit"), environment: env) - let result = check.check() - guard case .pass = result else { - Issue.record("Expected .pass, got \(result)") - return - } + private func check( + _ name: String, projectRoot: URL?, installed: [InstalledPlugin] + ) -> CheckResult { + let cli = MockClaudeCLI() + cli.installedPlugins = installed + return PluginCheck(pluginRef: PluginRef(name), projectRoot: projectRoot, listing: PluginListing(claudeCLI: cli)).check() } - @Test("fail when plugin is not in enabledPlugins") - func failWhenNotEnabled() throws { - let home = try makeGlobalTmpDir(label: "plugin-fail") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let settings = """ - { - "enabledPlugins": { - "other-plugin": true - } - } - """ - try settings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) - - let check = PluginCheck(pluginRef: PluginRef("missing-plugin"), environment: env) - let result = check.check() - guard case .fail = result else { - Issue.record("Expected .fail, got \(result)") - return - } + @Test("Global scope passes for an enabled user-scope install") + func globalUserInstallPasses() { + let result = check("lint@acme", projectRoot: nil, installed: [ + InstalledPlugin(id: "lint@acme", scope: "user", enabled: true, projectPath: nil), + ]) + #expect(result == .pass("enabled")) } - @Test("fail when settings.json does not exist") - func failWhenNoSettings() throws { - let home = try makeGlobalTmpDir(label: "plugin-nosettings") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - // Don't create settings.json - - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), environment: env) - let result = check.check() - guard case .fail = result else { - Issue.record("Expected .fail, got \(result)") - return - } + @Test("Project scope passes for a local install in this project") + func projectLocalInstallPasses() { + let result = check("lint@acme", projectRoot: project, installed: [ + InstalledPlugin(id: "lint@acme", scope: "local", enabled: true, projectPath: project.path), + ]) + #expect(result == .pass("enabled (project)")) } - // MARK: - Project-scoped tests - - @Test("pass when plugin is enabled in project settings.local.json") - func passWhenEnabledInProjectSettings() throws { - let home = try makeGlobalTmpDir(label: "plugin-project-pass") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let projectRoot = home.appendingPathComponent("my-project") - let claudeDir = projectRoot.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) - - let projectSettings = """ - { - "enabledPlugins": { - "my-plugin": true - } - } - """ - try projectSettings.write( - to: claudeDir.appendingPathComponent("settings.local.json"), - atomically: true, encoding: .utf8 - ) - // No global settings.json - - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), projectRoot: projectRoot, environment: env) - let result = check.check() - guard case let .pass(msg) = result else { - Issue.record("Expected .pass, got \(result)") - return - } - #expect(msg == "enabled (project)") + @Test("A local install in another project does not count") + func otherProjectsLocalInstallFails() { + let result = check("lint@acme", projectRoot: project, installed: [ + InstalledPlugin(id: "lint@acme", scope: "local", enabled: true, projectPath: "/tmp/elsewhere"), + ]) + #expect(result == .fail("installed but disabled")) } - @Test("pass via global fallback when plugin not in project settings") - func passWhenEnabledGloballyButNotInProject() throws { - let home = try makeGlobalTmpDir(label: "plugin-global-fallback") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let projectRoot = home.appendingPathComponent("my-project") - let claudeDir = projectRoot.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) - - // Project settings without the target plugin - let projectSettings = """ - { - "enabledPlugins": { - "other-plugin": true - } - } - """ - try projectSettings.write( - to: claudeDir.appendingPathComponent("settings.local.json"), - atomically: true, encoding: .utf8 - ) - - // Global settings with the target plugin - let globalSettings = """ - { - "enabledPlugins": { - "my-plugin": true - } - } - """ - try globalSettings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) - - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), projectRoot: projectRoot, environment: env) - let result = check.check() - guard case let .pass(msg) = result else { - Issue.record("Expected .pass, got \(result)") - return - } - #expect(msg == "enabled") + @Test("A user-scope install still loads in a project") + func userInstallPassesInProject() { + let result = check("lint@acme", projectRoot: project, installed: [ + InstalledPlugin(id: "lint@acme", scope: "user", enabled: true, projectPath: nil), + ]) + #expect(result == .pass("enabled (user scope)")) } - @Test("fail when plugin not enabled in either scope") - func failWhenNotEnabledInEitherScope() throws { - let home = try makeGlobalTmpDir(label: "plugin-both-fail") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let projectRoot = home.appendingPathComponent("my-project") - let claudeDir = projectRoot.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) + @Test("A disabled install fails") + func disabledInstallFails() { + let result = check("lint@acme", projectRoot: nil, installed: [ + InstalledPlugin(id: "lint@acme", scope: "user", enabled: false, projectPath: nil), + ]) + #expect(result == .fail("installed but disabled")) + } - let projectSettings = """ - { "enabledPlugins": { "other-plugin": true } } - """ - try projectSettings.write( - to: claudeDir.appendingPathComponent("settings.local.json"), - atomically: true, encoding: .utf8 - ) + @Test("The same name from another marketplace is not this plugin") + func otherMarketplaceDoesNotMatch() { + let result = check("lint@acme", projectRoot: nil, installed: [ + InstalledPlugin(id: "lint@other", scope: "user", enabled: true, projectPath: nil), + ]) + #expect(result == .fail("not installed")) + } - let globalSettings = """ - { "enabledPlugins": { "another-plugin": true } } - """ - try globalSettings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) + @Test("Global scope lists from the home directory, not the current project") + func globalListsFromHome() { + let cli = MockClaudeCLI() + let home = URL(fileURLWithPath: "/tmp/mcs-plugin-check-home") + // A local install in `project` would read as enabled if the listing ran there. + cli.installedPlugins = [InstalledPlugin(id: "lint@acme", scope: "local", enabled: true, projectPath: project.path)] + let result = PluginCheck(pluginRef: PluginRef("lint@acme"), projectRoot: nil, listing: PluginListing(claudeCLI: cli), homeDirectory: home).check() + #expect(result == .fail("installed but disabled")) + } - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), projectRoot: projectRoot, environment: env) - let result = check.check() - guard case .fail = result else { + @Test("An unreadable plugin list fails rather than passing") + func unreadableListFails() { + let cli = MockClaudeCLI() + let failing = FailingListCLI(base: cli) + let result = PluginCheck(pluginRef: PluginRef("lint@acme"), projectRoot: nil, listing: PluginListing(claudeCLI: failing)).check() + guard case let .fail(message) = result else { Issue.record("Expected .fail, got \(result)") return } + #expect(message.contains("could not list plugins")) } +} - @Test("warn via global fallback when project settings.local.json is invalid") - func warnWhenProjectSettingsInvalidFallsBackToGlobal() throws { - let home = try makeGlobalTmpDir(label: "plugin-invalid-project") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let projectRoot = home.appendingPathComponent("my-project") - let claudeDir = projectRoot.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) - - // Invalid project settings - try "not valid json".write( - to: claudeDir.appendingPathComponent("settings.local.json"), - atomically: true, encoding: .utf8 - ) - - // Valid global settings - let globalSettings = """ - { - "enabledPlugins": { - "my-plugin": true - } - } - """ - try globalSettings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) - - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), projectRoot: projectRoot, environment: env) - let result = check.check() - guard case let .warn(msg) = result else { - Issue.record("Expected .warn, got \(result)") - return - } - #expect(msg.contains("settings.local.json is unreadable")) +/// Wraps the mock so only `plugin list` fails. +private struct FailingListCLI: ClaudeCLI { + let base: MockClaudeCLI + var isAvailable: Bool { + true } - @Test("pass via global when projectRoot set but no settings.local.json exists") - func passWhenProjectSettingsAbsentFallsBackToGlobal() throws { - let home = try makeGlobalTmpDir(label: "plugin-no-project-settings") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let projectRoot = home.appendingPathComponent("my-project") - let claudeDir = projectRoot.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) - - let globalSettings = """ - { - "enabledPlugins": { - "my-plugin": true - } - } - """ - try globalSettings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) - - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), projectRoot: projectRoot, environment: env) - let result = check.check() - guard case let .pass(msg) = result else { - Issue.record("Expected .pass, got \(result)") - return - } - #expect(msg == "enabled") + func mcpAdd(name: String, scope: String, arguments: [String], workingDirectory: URL?) -> ShellResult { + base.mcpAdd(name: name, scope: scope, arguments: arguments, workingDirectory: workingDirectory) } - @Test("pass via global when plugin explicitly false in project settings") - func passWhenPluginExplicitlyFalseInProject() throws { - let home = try makeGlobalTmpDir(label: "plugin-false-project") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let projectRoot = home.appendingPathComponent("my-project") - let claudeDir = projectRoot.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) - - let projectSettings = """ - { "enabledPlugins": { "my-plugin": false } } - """ - try projectSettings.write( - to: claudeDir.appendingPathComponent("settings.local.json"), - atomically: true, encoding: .utf8 - ) - - let globalSettings = """ - { "enabledPlugins": { "my-plugin": true } } - """ - try globalSettings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) - - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), projectRoot: projectRoot, environment: env) - let result = check.check() - guard case let .pass(msg) = result else { - Issue.record("Expected .pass, got \(result)") - return - } - #expect(msg == "enabled") + func mcpRemove(name: String, scope: String, workingDirectory: URL?) -> ShellResult { + base.mcpRemove(name: name, scope: scope, workingDirectory: workingDirectory) } - @Test("fail with corrupt message when project settings invalid and no global settings") - func failWhenProjectCorruptAndNoGlobal() throws { - let home = try makeGlobalTmpDir(label: "plugin-corrupt-no-global") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let projectRoot = home.appendingPathComponent("my-project") - let claudeDir = projectRoot.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) - - try "not valid json".write( - to: claudeDir.appendingPathComponent("settings.local.json"), - atomically: true, encoding: .utf8 - ) - - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), projectRoot: projectRoot, environment: env) - let result = check.check() - guard case let .fail(msg) = result else { - Issue.record("Expected .fail, got \(result)") - return - } - #expect(msg.contains("settings.local.json is corrupt")) - #expect(msg.contains("settings.json not found")) + func pluginMarketplaceAdd(repo: String) -> ShellResult { + base.pluginMarketplaceAdd(repo: repo) } - @Test("fail with corrupt message when project settings invalid and plugin not in global") - func failWhenProjectCorruptAndPluginNotInGlobal() throws { - let home = try makeGlobalTmpDir(label: "plugin-corrupt-not-global") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - let projectRoot = home.appendingPathComponent("my-project") - let claudeDir = projectRoot.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) + func pluginMarketplaceList() -> ShellResult { + base.pluginMarketplaceList() + } - try "not valid json".write( - to: claudeDir.appendingPathComponent("settings.local.json"), - atomically: true, encoding: .utf8 - ) + func pluginList(workingDirectory _: URL?) -> ShellResult { + ShellResult(exitCode: 1, stdout: "", stderr: "boom") + } - let globalSettings = """ - { "enabledPlugins": { "other-plugin": true } } - """ - try globalSettings.write(to: env.claudeSettings, atomically: true, encoding: .utf8) + func pluginInstall(id: String, scope: String, workingDirectory: URL?) -> ShellResult { + base.pluginInstall(id: id, scope: scope, workingDirectory: workingDirectory) + } - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), projectRoot: projectRoot, environment: env) - let result = check.check() - guard case let .fail(msg) = result else { - Issue.record("Expected .fail, got \(result)") - return - } - #expect(msg == "not enabled (settings.local.json is corrupt)") + func pluginRemove(id: String, scope: String, workingDirectory: URL?) -> ShellResult { + base.pluginRemove(id: id, scope: scope, workingDirectory: workingDirectory) } } @@ -764,12 +563,8 @@ struct DerivedDoctorCheckSandboxTests { } } - @Test("deriveDoctorCheck forwards environment to the derived check") - func deriveDoctorCheckForwardsEnv() throws { - let home = try makeGlobalTmpDir(label: "derived-all") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - + @Test("deriveDoctorCheck forwards the Claude CLI to the derived plugin check") + func deriveDoctorCheckForwardsClaudeCLI() throws { let component = ComponentDefinition( id: "test.plugin", displayName: "Test Plugin", @@ -778,21 +573,13 @@ struct DerivedDoctorCheckSandboxTests { packIdentifier: nil, installAction: .plugin(name: "my-plugin") ) + let cli = MockClaudeCLI() + cli.installedPlugins = [InstalledPlugin( + id: "my-plugin@\(Constants.Plugins.officialMarketplace)", scope: "user", enabled: true, projectPath: nil + )] - let checks = [component.deriveDoctorCheck(environment: env)].compactMap(\.self) - #expect(checks.count == 1) - - // The PluginCheck should use our sandbox settings path - if let pluginCheck = checks.first as? PluginCheck { - let result = pluginCheck.check() - // Should fail because settings.json doesn't exist in sandbox - guard case .fail = result else { - Issue.record("Expected .fail (no settings in sandbox), got \(result)") - return - } - } else { - Issue.record("Expected PluginCheck, got \(type(of: checks.first!))") - } + let check = try #require(component.deriveDoctorCheck(pluginListing: PluginListing(claudeCLI: cli)) as? PluginCheck) + #expect(check.check() == .pass("enabled")) } @Test("brewInstall derived check fails when the package is missing") @@ -818,27 +605,6 @@ struct DerivedDoctorCheckSandboxTests { } } -// MARK: - PluginCheck Invalid JSON - -extension PluginCheckSandboxTests { - @Test("fail when settings.json contains invalid JSON") - func failWhenInvalidSettings() throws { - let home = try makeGlobalTmpDir(label: "plugin-invalid") - defer { try? FileManager.default.removeItem(at: home) } - let env = Environment(home: home) - - try "not valid json".write(to: env.claudeSettings, atomically: true, encoding: .utf8) - - let check = PluginCheck(pluginRef: PluginRef("my-plugin"), environment: env) - let result = check.check() - guard case let .fail(msg) = result else { - Issue.record("Expected .fail, got \(result)") - return - } - #expect(msg.contains("invalid")) - } -} - // MARK: - ExternalHookEventExistsCheck Sandbox Tests struct ExternalHookEventExistsCheckSandboxTests { diff --git a/Tests/MCSTests/DoctorRunnerIntegrationTests.swift b/Tests/MCSTests/DoctorRunnerIntegrationTests.swift index 34e293d5..23a84e16 100644 --- a/Tests/MCSTests/DoctorRunnerIntegrationTests.swift +++ b/Tests/MCSTests/DoctorRunnerIntegrationTests.swift @@ -10,7 +10,8 @@ private func makeRunner( registry: TechPackRegistry = TechPackRegistry(), fixMode: Bool = false, globalOnly: Bool = false, - packFilter: String? = nil + packFilter: String? = nil, + claudeCLI: MockClaudeCLI = MockClaudeCLI() ) -> DoctorRunner { DoctorRunner( fixMode: fixMode, @@ -19,7 +20,8 @@ private func makeRunner( globalOnly: globalOnly, registry: registry, environment: Environment(home: home), - projectRootOverride: projectRoot + projectRootOverride: projectRoot, + claudeCLI: claudeCLI ) } @@ -98,47 +100,34 @@ struct DoctorRunnerIntegrationTests { #expect(onlyA.issues == all.issues - 1) } - @Test("PluginCheck passes when plugin is enabled in project settings.local.json") - func pluginCheckPassesWithProjectSettings() throws { + @Test("PluginCheck passes for a plugin installed local to the project") + func pluginCheckPassesForLocalInstall() throws { let (home, project) = try makeSandboxProject(label: "runner-plugin-project") defer { try? FileManager.default.removeItem(at: home) } - let pluginComponent = ComponentDefinition( - id: "test-pack.my-plugin", - displayName: "My Plugin", - description: "Test plugin", - type: .plugin, - packIdentifier: "test-pack", - installAction: .plugin(name: "my-plugin") - ) let pack = MockTechPack( identifier: "test-pack", displayName: "Test Pack", - components: [pluginComponent] + components: [ComponentDefinition( + id: "test-pack.my-plugin", + displayName: "My Plugin", + description: "Test plugin", + type: .plugin, + packIdentifier: "test-pack", + installAction: .plugin(name: "my-plugin@acme") + )] ) - let registry = TechPackRegistry(packs: [pack]) - - // Write project state var state = try ProjectState(projectRoot: project) state.recordPack("test-pack") try state.save() - // Write plugin enablement to project-scoped settings.local.json only - let claudeDir = project.appendingPathComponent(Constants.FileNames.claudeDirectory) - let projectSettings = """ - { - "enabledPlugins": { - "my-plugin": true - } - } - """ - try projectSettings.write( - to: claudeDir.appendingPathComponent("settings.local.json"), - atomically: true, encoding: .utf8 + let cli = MockClaudeCLI() + cli.installedPlugins = [InstalledPlugin( + id: "my-plugin@acme", scope: Constants.PluginScope.local, enabled: true, projectPath: project.path + )] + var runner = makeRunner( + home: home, projectRoot: project, registry: TechPackRegistry(packs: [pack]), claudeCLI: cli ) - // No global settings.json — plugin is only project-scoped - - var runner = makeRunner(home: home, projectRoot: project, registry: registry) #expect(try runner.run().issues == 0) } diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index c16fb46f..5a21bb6e 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -68,7 +68,8 @@ private struct LifecycleTestBed { globalOnly: true, registry: registry, environment: env, - projectRootOverride: nil + projectRootOverride: nil, + claudeCLI: mockCLI ) } @@ -4443,3 +4444,114 @@ struct SettingsSubKeyOwnershipTests { return state } } + +// MARK: - Plugins install per scope + +struct PluginScopeLifecycleTests { + private func pluginPack(_ identifier: String, plugin: String = "lint@acme") -> MockTechPack { + MockTechPack( + identifier: identifier, + displayName: identifier, + components: [ComponentDefinition( + id: "\(identifier).plugin", + displayName: "Lint plugin", + description: "Plugin", + type: .plugin, + packIdentifier: identifier, + installAction: .plugin(name: plugin) + )] + ) + } + + @Test("Project: sync installs local to the project, doctor passes, removal uninstalls from that scope") + func projectLifecycle() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let pack = pluginPack("lint-pack") + let registry = TechPackRegistry(packs: [pack]) + + try bed.makeConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + #expect(bed.mockCLI.pluginInstallCalls == [MockClaudeCLI.PluginCall( + id: "lint@acme", scope: Constants.PluginScope.local, workingDirectory: bed.project + )]) + #expect(try bed.projectState().artifacts(for: "lint-pack")?.plugins == ["lint@acme"]) + + var doctor = bed.makeDoctorRunner(registry: registry) + #expect(try doctor.run().issues == 0) + + try bed.makeConfigurator(registry: registry).configure(packs: [], confirmRemovals: false) + #expect(bed.mockCLI.pluginRemoveCalls == [MockClaudeCLI.PluginCall( + id: "lint@acme", scope: Constants.PluginScope.local, workingDirectory: bed.project + )]) + #expect(bed.mockCLI.installedPlugins.isEmpty) + } + + @Test("Global: sync installs at user scope") + func globalInstallsAtUserScope() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let pack = pluginPack("lint-pack") + + try bed.makeGlobalSyncConfigurator(registry: TechPackRegistry(packs: [pack])) + .configure(packs: [pack], confirmRemovals: false) + + #expect(bed.mockCLI.pluginInstallCalls.map(\.scope) == [Constants.PluginScope.user]) + #expect(try bed.globalState().artifacts(for: "lint-pack")?.plugins == ["lint@acme"]) + } + + @Test("A plugin another pack in the same scope declares is kept on removal") + func sharedPluginKeptWhileClaimed() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let packA = pluginPack("pack-a") + let packB = pluginPack("pack-b") + let registry = TechPackRegistry(packs: [packA, packB]) + + try bed.makeConfigurator(registry: registry).configure(packs: [packA, packB], confirmRemovals: false) + try bed.makeConfigurator(registry: registry).configure(packs: [packB], confirmRemovals: false) + #expect(bed.mockCLI.pluginRemoveCalls.isEmpty) + + try bed.makeConfigurator(registry: registry).configure(packs: [], confirmRemovals: false) + #expect(bed.mockCLI.pluginRemoveCalls.count == 1) + } + + @Test("An install mcs didn't make is neither recorded nor removed") + func preexistingInstallIsNotOwned() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + bed.mockCLI.installedPlugins = [InstalledPlugin( + id: "lint@acme", scope: Constants.PluginScope.local, enabled: true, projectPath: bed.project.path + )] + let pack = pluginPack("lint-pack") + let registry = TechPackRegistry(packs: [pack]) + + try bed.makeConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + #expect(bed.mockCLI.pluginInstallCalls.isEmpty) + #expect(try bed.projectState().artifacts(for: "lint-pack")?.plugins.isEmpty == true) + + try bed.makeConfigurator(registry: registry).configure(packs: [], confirmRemovals: false) + #expect(bed.mockCLI.pluginRemoveCalls.isEmpty) + } + + @Test("Project sync keeps the CLI's enabledPlugins entry and drops the legacy bare-name one") + func legacyBareKeyDroppedCLIKeyKept() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let pack = pluginPack("lint-pack") + let registry = TechPackRegistry(packs: [pack]) + try bed.makeConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + + // What an older release left: its own bare key next to the one the CLI wrote. + var state = try bed.projectState() + var record = try #require(state.artifacts(for: "lint-pack")) + record.settingsKeys = ["enabledPlugins.lint"] + state.setArtifacts(record, for: "lint-pack") + try state.save() + try JSONSerialization.data(withJSONObject: ["enabledPlugins": ["lint": true, "lint@acme": true]]) + .write(to: bed.settingsLocalPath) + + try bed.makeConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + + #expect(try Settings.load(from: bed.settingsLocalPath).enabledPlugins == ["lint@acme": true]) + } +} diff --git a/Tests/MCSTests/PluginRefTests.swift b/Tests/MCSTests/PluginRefTests.swift index 156086e4..385e45ca 100644 --- a/Tests/MCSTests/PluginRefTests.swift +++ b/Tests/MCSTests/PluginRefTests.swift @@ -32,4 +32,28 @@ struct PluginRefTests { #expect(ref.bareName == "my-plugin") #expect(ref.marketplaceRepo == "custom-marketplace") } + + @Test("The marketplace name is known unless the reference names a repo") + func marketplaceName() { + #expect(PluginRef("pr-review-toolkit").marketplaceName == Constants.Plugins.officialMarketplace) + #expect(PluginRef("lint@acme").marketplaceName == "acme") + #expect(PluginRef("lint@myorg/my-marketplace").marketplaceName == nil) + #expect(PluginRef("x@anthropics/claude-plugins-official").marketplaceName == Constants.Plugins.officialMarketplace) + } + + @Test("A repo reference resolves its id through the marketplace list") + func pluginIDFromRepo() { + let marketplaces = [PluginMarketplace(name: "hud", repo: "MyOrg/My-Marketplace")] + #expect(PluginRef("lint@myorg/my-marketplace").pluginID(in: marketplaces) == "lint@hud") + #expect(PluginRef("lint@other/repo").pluginID(in: marketplaces) == nil) + #expect(PluginRef("lint@acme").pluginID(in: []) == "lint@acme") + } + + @Test("An id matches on name and, when known, marketplace") + func matchesID() { + #expect(PluginRef("lint@acme").matches(id: "lint@acme")) + #expect(!PluginRef("lint@acme").matches(id: "lint@other")) + #expect(PluginRef("lint@org/repo").matches(id: "lint@whatever")) + #expect(!PluginRef("lint@acme").matches(id: "lint-extra@acme")) + } } diff --git a/Tests/MCSTests/ProjectSyncTests.swift b/Tests/MCSTests/ProjectSyncTests.swift index 1ff25275..988ba4c0 100644 --- a/Tests/MCSTests/ProjectSyncTests.swift +++ b/Tests/MCSTests/ProjectSyncTests.swift @@ -627,23 +627,30 @@ struct AutoDerivedSettingsTests { #expect(command?.contains("~/.claude") != true) } - @Test("plugin component auto-derives enabledPlugins entry") - func pluginAutoDerivesEnabledPlugins() throws { + @Test("plugin component installs local to the project and leaves enabledPlugins to the CLI") + func pluginInstallsLocalToProject() throws { let tmpDir = try makeTmpDir() defer { try? FileManager.default.removeItem(at: tmpDir) } + let cli = MockClaudeCLI() - let pack = makePluginPack() - - let claudeDir = tmpDir.appendingPathComponent(".claude") - try FileManager.default.createDirectory(at: claudeDir, withIntermediateDirectories: true) - - let configurator = makeConfigurator(projectPath: tmpDir, home: tmpDir) - try configurator.configure(packs: [pack]) - - let settingsPath = claudeDir.appendingPathComponent("settings.local.json") - let result = try Settings.load(from: settingsPath) - - #expect(result.enabledPlugins?["pr-review-toolkit"] == true) + let env = Environment(home: tmpDir) + try Configurator( + environment: env, + output: output, + shell: ShellRunner(environment: env), + strategy: ProjectSyncStrategy(projectPath: tmpDir, environment: env), + claudeCLI: cli + ).configure(packs: [makePluginPack()]) + + #expect(cli.pluginInstallCalls == [MockClaudeCLI.PluginCall( + id: "pr-review-toolkit@\(Constants.Plugins.officialMarketplace)", + scope: Constants.PluginScope.local, + workingDirectory: tmpDir + )]) + let artifacts = try ProjectState(projectRoot: tmpDir).artifacts(for: "test-pack") + #expect(artifacts?.plugins == ["pr-review-toolkit@claude-plugins-official"]) + let settings = try Settings.load(from: tmpDir.appendingPathComponent(".claude/settings.local.json")) + #expect(settings.enabledPlugins?["pr-review-toolkit"] == nil) } @Test("hookFile without hookEvent does not generate settings entry") diff --git a/Tests/MCSTests/ResourceRefCounterTests.swift b/Tests/MCSTests/ResourceRefCounterTests.swift index a8eddec6..a47fe0d9 100644 --- a/Tests/MCSTests/ResourceRefCounterTests.swift +++ b/Tests/MCSTests/ResourceRefCounterTests.swift @@ -37,18 +37,6 @@ private func brewComponent(id: String, pack: String, package: String) -> Compone ) } -/// Creates a ComponentDefinition with a plugin install action. -private func pluginComponent(id: String, pack: String, pluginName: String) -> ComponentDefinition { - ComponentDefinition( - id: id, - displayName: pluginName, - description: "Plugin: \(pluginName)", - type: .plugin, - packIdentifier: pack, - installAction: .plugin(name: pluginName) - ) -} - /// Creates a ComponentDefinition with a gitignore-entries install action. private func gitignoreComponent(id: String, pack: String, entries: [String]) -> ComponentDefinition { ComponentDefinition( @@ -159,62 +147,6 @@ struct ResourceRefCounterTests { // MARK: - Plugin needed by two projects → keep - @Test("Plugin needed by another project is kept") - func pluginTwoProjects() throws { - let home = try makeTmpHome() - defer { try? FileManager.default.removeItem(at: home) } - - let env = Environment(home: home) - - // Create two real project directories - let projectA = home.appendingPathComponent("project-a") - let projectB = home.appendingPathComponent("project-b") - try FileManager.default.createDirectory(at: projectA, withIntermediateDirectories: true) - try FileManager.default.createDirectory(at: projectB, withIntermediateDirectories: true) - - // Both projects use pack-x which declares the plugin - try writeProjectState(projectRoot: projectA, packs: ["pack-x"]) - try writeProjectState(projectRoot: projectB, packs: ["pack-x"]) - - // Empty global state (no global packs) - try writeGlobalState(home: home, packs: []) - - // Index tracks both projects - try writeIndex(home: home, entries: [ - (projectA.path, ["pack-x"]), - (projectB.path, ["pack-x"]), - ]) - - let registry = TechPackRegistry(packs: [ - StubTechPack( - identifier: "pack-x", - displayName: "Pack X", - description: "Test", - components: [ - pluginComponent( - id: "x.plugin", pack: "pack-x", - pluginName: "anthropics/claude-plugins-official/pr-review-toolkit" - ), - ] - ), - ]) - - let counter = ResourceRefCounter( - environment: env, - output: CLIOutput(), - registry: registry - ) - - // Removing from project-a — project-b still needs it - let result = counter.isStillNeeded( - .plugin("anthropics/claude-plugins-official/pr-review-toolkit"), - excludingScope: projectA.path, - excludingPack: "pack-x" - ) - - #expect(result, "Should be kept — project-b still uses pack-x which declares the plugin") - } - // MARK: - Same pack in global + project → both counted @Test("Same pack globally and per-project keeps resource when removing from project") @@ -520,62 +452,6 @@ struct ResourceRefCounterTests { // MARK: - Plugin bare name matching - @Test("Plugin matching works across different full-name formats") - func pluginBareNameMatching() throws { - let home = try makeTmpHome() - defer { try? FileManager.default.removeItem(at: home) } - - let env = Environment(home: home) - - // Global state: pack-a owns bare name, pack-b owns @ format - // PluginRef uses @ as separator: "name@repo/path" - try writeGlobalState(home: home, packs: [ - ("pack-a", PackArtifactRecord(plugins: ["pr-review-toolkit"])), - ("pack-b", PackArtifactRecord(plugins: ["pr-review-toolkit@anthropics/claude-plugins-official"])), - ]) - - try writeIndex(home: home, entries: [ - (ProjectIndex.globalSentinel, ["pack-a", "pack-b"]), - ]) - - let registry = TechPackRegistry(packs: [ - StubTechPack( - identifier: "pack-a", - displayName: "Pack A", - description: "Test", - components: [ - pluginComponent(id: "a.plugin", pack: "pack-a", pluginName: "pr-review-toolkit"), - ] - ), - StubTechPack( - identifier: "pack-b", - displayName: "Pack B", - description: "Test", - components: [ - pluginComponent( - id: "b.plugin", pack: "pack-b", - pluginName: "pr-review-toolkit@anthropics/claude-plugins-official" - ), - ] - ), - ]) - - let counter = ResourceRefCounter( - environment: env, - output: CLIOutput(), - registry: registry - ) - - // Removing pack-a — pack-b also has the same plugin (different format) - let result = counter.isStillNeeded( - .plugin("pr-review-toolkit"), - excludingScope: ProjectIndex.globalSentinel, - excludingPack: "pack-a" - ) - - #expect(result, "Should be kept — pack-b has same plugin in different format") - } - // MARK: - No other scopes at all → safe to remove @Test("No other scopes means safe to remove") diff --git a/Tests/MCSTests/TestHelpers.swift b/Tests/MCSTests/TestHelpers.swift index 2c183cff..a40555e1 100644 --- a/Tests/MCSTests/TestHelpers.swift +++ b/Tests/MCSTests/TestHelpers.swift @@ -40,19 +40,74 @@ final class MockClaudeCLI: ClaudeCLI, @unchecked Sendable { return result } + struct PluginCall: Equatable { + let id: String + let scope: String + var workingDirectory: URL? + } + + var marketplaceAddCalls: [String] = [] + var pluginInstallCalls: [PluginCall] = [] + var pluginRemoveCalls: [PluginCall] = [] + + /// What `plugin marketplace list` reports. Defaults to the official marketplace. + var marketplaces = [PluginMarketplace( + name: Constants.Plugins.officialMarketplace, repo: Constants.Plugins.officialMarketplaceRepo + )] + + /// What `plugin list` reports. A successful install adds to it and a removal drops from it, + /// so a sync → doctor → remove sequence sees what the real CLI would. + var installedPlugins: [InstalledPlugin] = [] + @discardableResult - func pluginMarketplaceAdd(repo _: String) -> ShellResult { - result + func pluginMarketplaceAdd(repo: String) -> ShellResult { + marketplaceAddCalls.append(repo) + return result + } + + func pluginMarketplaceList() -> ShellResult { + jsonResult(marketplaces) + } + + func pluginList(workingDirectory: URL?) -> ShellResult { + jsonResult(installedPlugins.map { plugin in + // Mirrors the CLI: a local install is enabled only when listed from its own project. + guard plugin.scope == Constants.PluginScope.local else { return plugin } + let here = plugin.isInstall(atScope: plugin.scope, projectDirectory: workingDirectory) + return InstalledPlugin( + id: plugin.id, scope: plugin.scope, enabled: plugin.enabled && here, projectPath: plugin.projectPath + ) + }) } @discardableResult - func pluginInstall(ref _: PluginRef) -> ShellResult { - result + func pluginInstall(id: String, scope: String, workingDirectory: URL?) -> ShellResult { + pluginInstallCalls.append(PluginCall(id: id, scope: scope, workingDirectory: workingDirectory)) + if result.succeeded { + installedPlugins.append(InstalledPlugin( + id: id, scope: scope, enabled: true, + projectPath: scope == Constants.PluginScope.local ? workingDirectory?.path : nil + )) + } + return result } @discardableResult - func pluginRemove(ref _: PluginRef) -> ShellResult { - result + func pluginRemove(id: String, scope: String, workingDirectory: URL?) -> ShellResult { + pluginRemoveCalls.append(PluginCall(id: id, scope: scope, workingDirectory: workingDirectory)) + if result.succeeded { + installedPlugins.removeAll { $0.id == id && $0.isInstall(atScope: scope, projectDirectory: workingDirectory) } + } + return result + } + + private func jsonResult(_ value: some Encodable) -> ShellResult { + do { + let data = try JSONEncoder().encode(value) + return ShellResult(exitCode: 0, stdout: String(decoding: data, as: UTF8.self), stderr: "") + } catch { + return ShellResult(exitCode: 1, stdout: "", stderr: error.localizedDescription) + } } } From 3b81e5d1c4e088f9119d4f3b9a8ee9d205588e50 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Thu, 8 Oct 2026 17:54:58 +0200 Subject: [PATCH 2/3] ISSUE-450: Keep plugin ownership accurate across updates, removals and doctor --fix - Uninstall a plugin a pack stops declaring; a kept plugin moves to the pack still claiming it - Treat an uninstall as done when the plugin is no longer listed at the scope, and never claim an install made blind - Doctor re-lists plugins after a re-sync and warns on disabled ones; global sync saves stripped legacy keys --- CLAUDE.md | 4 +- Sources/mcs/Core/Constants.swift | 2 +- Sources/mcs/Core/PluginRef.swift | 16 ++- Sources/mcs/Core/Settings.swift | 29 +++-- Sources/mcs/Doctor/CoreDoctorChecks.swift | 17 ++- Sources/mcs/Doctor/DoctorRunner.swift | 5 +- .../mcs/Doctor/ScopeDuplicationCheck.swift | 10 +- Sources/mcs/Sync/ComponentExecutor.swift | 48 ++++--- Sources/mcs/Sync/Configurator.swift | 121 ++++++++---------- Sources/mcs/Sync/ConfiguratorSupport.swift | 2 + Sources/mcs/Sync/GlobalSyncStrategy.swift | 10 +- Sources/mcs/Sync/ProjectSyncStrategy.swift | 11 +- Tests/MCSTests/ComponentExecutorTests.swift | 32 ++++- .../CoreDoctorCheckSandboxTests.swift | 51 ++------ .../MCSTests/LifecycleIntegrationTests.swift | 94 ++++++++++++++ Tests/MCSTests/TestHelpers.swift | 6 +- 16 files changed, 291 insertions(+), 167 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5d9de5f9..04a08dc1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,7 +82,7 @@ mcs config set # Set a configuration value (true/false) - `Settings.swift` — Codable model for `settings.json` and `settings.local.json`, deep-merge - `Backup.swift` — timestamped backups for mixed-ownership files (CLAUDE.local.md), backup discovery and deletion - `GitignoreManager.swift` — global gitignore management, core entry list -- `ClaudeIntegration.swift` — `claude mcp add/remove` (with scope support), `claude plugin install/uninstall/list` and `marketplace add/list` (scope + working directory, like MCP) +- `ClaudeIntegration.swift` — `claude mcp add/remove` (with scope support), `claude plugin install/uninstall/list` (scope + working directory, like MCP) and `marketplace add/list` - `ClaudePrerequisite.swift` — Claude Code CLI availability check with optional Homebrew auto-install - `Homebrew.swift` — brew detection, package install/uninstall, and `provides(_:)` — the one availability predicate shared by `ComponentExecutor` and `BrewPackageCheck` (PATH under `bareName(of:)`, falling back to `brew list`) - `FileHasher.swift` — SHA-256 file and directory hashing via CryptoKit (used by `PackTrustManager` and `ComponentExecutor`) @@ -212,7 +212,7 @@ swiftlint --fix - **Non-interactive prompt resolution**: when stdin is not a TTY, every prompt resolves as seeded/stored value → declared default (a `select` default must be an option; a `fileDetect` default must be a match, or with zero matches any non-empty value; otherwise an explicit `""` counts) → failure. `CrossPackPromptResolver.planValues` builds the key set and reuse partition that both sync (`resolveAllValues`) and the preflight (`resolveNonInteractively`) start from, and the preflight replays sync's stages in order — reused priors (per `Configurator.priorReuse`), shared prompts, then each pack for the keys still open, since a pack skips keys an earlier pack produced — so the two cannot disagree. `Configurator.configure` runs it against the post-removal priors before any removal or install and throws `PromptResolutionError` (every `pack: key`, never values, with a scope-specific remedy), otherwise merges its answers so no prompt reader runs. `--dry-run` only warns; `mcs update` skips a failing scope, finishes the rest, then exits non-zero. Two older prompts still read stdin off-TTY (tracked in #417) - **Packs install whole**: a selected pack installs every component it declares, in declaration order (project sync still installs brew packages first). There is no per-component selection — `mcs.yaml` could not express one, so the same file would install different sets on different machines. `dependencies` / `isRequired` in a manifest are decoded only into `deprecatedKeys` for a `mcs pack validate` warning. Exclusions left in state by the removed `--customize` are announced by sync and dry-run, then installed and cleared (emptied, not removed — older releases require the key) by the first real sync; until a global sync clears them, `ScopeDuplicationCheck` refuses to remove a project copy - **Per-project artifacts**: skills, hooks, commands, and `settings.local.json` go to `/.claude/`; only brew packages are global -- **Plugins install per scope, and the Claude CLI owns `enabledPlugins`**: project sync runs `claude plugin install @ -s local` from the project directory, global sync `-s user`; mcs never writes `enabledPlugins` (Claude Code ignores anything but the `name@marketplace` key) and project composition preserves what the CLI wrote to `settings.local.json`. A plugin is recorded only when mcs's install put it there, and removed from its own scope unless another pack in that scope claims it. `PluginCheck` reads `claude plugin list --json` +- **Plugins install per scope, and the Claude CLI owns `enabledPlugins`**: project sync runs `claude plugin install @ -s local` from the project directory, global sync `-s user`; mcs never writes `enabledPlugins` (Claude Code ignores anything but the `name@marketplace` key) and project composition preserves what the CLI wrote to `settings.local.json`. A plugin is recorded only on the pack whose install put it there, carried over only while that pack still declares it, and uninstalled from its own scope unless another pack in that scope claims it — which then takes over the record. `PluginCheck` reads `claude plugin list --json` - **MCP scope defaults to `local`**: per-user, per-project isolation via `claude mcp add -s local` (stored in `~/.claude.json` keyed by project path) - **Convergent sync**: `ProjectState` records per-pack `PackArtifactRecord` (MCP servers, files, template sections, hook commands, settings keys, settings hash, brew packages, plugins, gitignore entries, file hashes); re-running converges to desired state by diffing previous vs. selected packs. `mcs pack remove` discovers all scopes via `ProjectIndex` and runs the same `unconfigurePack()` convergence for each - **External pack protocol**: `TechPack` protocol with `ExternalPackAdapter` bridging YAML manifests (`techpack.yaml`) to the same install/doctor/sync flows diff --git a/Sources/mcs/Core/Constants.swift b/Sources/mcs/Core/Constants.swift index f9bf7508..a4c8db9a 100644 --- a/Sources/mcs/Core/Constants.swift +++ b/Sources/mcs/Core/Constants.swift @@ -56,7 +56,7 @@ enum Constants { /// Stderr text `claude mcp remove` prints when the server is not registered in that scope. static let mcpServerNotFound = "No MCP server named" - /// Stderr text `claude plugin remove` prints when the plugin is not installed. + /// Stderr text `claude plugin uninstall` prints when the plugin is not installed. static let pluginNotFound = "not found in installed plugins" } diff --git a/Sources/mcs/Core/PluginRef.swift b/Sources/mcs/Core/PluginRef.swift index 8d58c272..3905c077 100644 --- a/Sources/mcs/Core/PluginRef.swift +++ b/Sources/mcs/Core/PluginRef.swift @@ -2,11 +2,11 @@ import Foundation /// Parsed representation of a plugin reference from `techpack.yaml`. /// -/// Plugin names in manifests use the format `name@repo` where: -/// - `name` is the bare plugin name passed to `claude plugin install/remove` -/// - `repo` is the marketplace repository (e.g. `anthropics/claude-plugins-official`) +/// Plugin names in manifests use the format `name@source` where `source` is either a +/// marketplace repository (`anthropics/claude-plugins-official`) or a marketplace's name +/// (`claude-plugins-official`). The CLI is driven by the resolved `name@` id. /// -/// When no `@repo` suffix is present, the official Anthropic marketplace is assumed. +/// When no `@source` suffix is present, the official Anthropic marketplace is assumed. struct PluginRef: Equatable { /// The bare plugin name (e.g. `pr-review-toolkit`). let bareName: String @@ -68,7 +68,7 @@ struct PluginRef: Equatable { /// Whether `id` (`name@marketplace`) is this plugin. func matches(id: String) -> Bool { let parts = id.split(separator: "@", maxSplits: 1) - guard String(parts[0]) == bareName else { return false } + guard let name = parts.first, String(name) == bareName else { return false } guard let marketplaceName, parts.count == 2 else { return true } return String(parts[1]) == marketplaceName } @@ -151,6 +151,12 @@ extension PackArtifactRecord { let bareName = PluginRef(name).bareName return plugins.contains { PluginRef($0).bareName == bareName } } + + /// A re-sync finds an owned plugin already installed and does not record it again, so ownership + /// is carried over, but only while the pack still declares it: a dropped one must read as stale. + func pluginsStillDeclared(by pack: any TechPack) -> [String] { + plugins.filter { pack.declaresPlugin($0) } + } } extension TechPack { diff --git a/Sources/mcs/Core/Settings.swift b/Sources/mcs/Core/Settings.swift index ba616eee..446f0f0d 100644 --- a/Sources/mcs/Core/Settings.swift +++ b/Sources/mcs/Core/Settings.swift @@ -336,7 +336,10 @@ struct Settings: Codable { /// Remove settings keys that mcs previously owned but are no longer in the template. /// Key paths use dot notation: `env.KEY`, `permissions.defaultMode`, `enabledPlugins.NAME`. /// Single-part paths remove from `extraJSON` or typed properties as appropriate. - mutating func removeKeys(_ keyPaths: [String]) { + /// - Returns: Whether anything was removed. + @discardableResult + mutating func removeKeys(_ keyPaths: [String]) -> Bool { + var removed = false for keyPath in keyPaths { let parts = keyPath.split(separator: ".", maxSplits: 1) if parts.count == 2 { @@ -344,26 +347,29 @@ struct Settings: Codable { let key = String(parts[1]) switch section { case "hooks": - hooks?.removeValue(forKey: key) + removed = hooks?.removeValue(forKey: key) != nil || removed case "enabledPlugins": - enabledPlugins?.removeValue(forKey: key) + removed = enabledPlugins?.removeValue(forKey: key) != nil || removed default: // Handle dotted paths for extraJSON keys (e.g. "env.FOO", // "permissions.defaultMode") - removeExtraJSONSubKey(topLevel: section, subKey: key) + removed = removeExtraJSONSubKey(topLevel: section, subKey: key) || removed } } else { // Single-part key — check typed properties first, then extraJSON switch keyPath { case "hooks": + removed = hooks != nil || removed hooks = nil case "enabledPlugins": + removed = enabledPlugins != nil || removed enabledPlugins = nil default: - extraJSON.removeValue(forKey: keyPath) + removed = extraJSON.removeValue(forKey: keyPath) != nil || removed } } } + return removed } // MARK: - File I/O @@ -494,19 +500,22 @@ struct Settings: Codable { // MARK: - Private Helpers /// Remove a sub-key from an extraJSON entry that holds a JSON object. - private mutating func removeExtraJSONSubKey(topLevel: String, subKey: String) { + private mutating func removeExtraJSONSubKey(topLevel: String, subKey: String) -> Bool { guard let data = extraJSON[topLevel], - var dict = try? JSONSerialization.jsonObject(with: data) as? [String: Any] + var dict = try? JSONSerialization.jsonObject(with: data) as? [String: Any], + dict.removeValue(forKey: subKey) != nil else { - return + return false } - dict.removeValue(forKey: subKey) if dict.isEmpty { extraJSON.removeValue(forKey: topLevel) } else if let newData = try? JSONSerialization.data(withJSONObject: dict) { extraJSON[topLevel] = newData + } else { + // Re-serialization failure: keep existing value (no-op) + return false } - // Re-serialization failure: keep existing value (no-op) + return true } } diff --git a/Sources/mcs/Doctor/CoreDoctorChecks.swift b/Sources/mcs/Doctor/CoreDoctorChecks.swift index 2e13f513..a13982e7 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -111,23 +111,30 @@ struct PluginCheck: DoctorCheck { } func check() -> CheckResult { - let installs: [InstalledPlugin] + // A `local` install belongs to one project; another project's says nothing about this scope. + let installsHere: [InstalledPlugin] switch listing.plugins(in: projectRoot ?? homeDirectory) { case let .success(plugins): - installs = plugins.filter { pluginRef.matches(id: $0.id) } + installsHere = plugins.filter { + pluginRef.matches(id: $0.id) && $0.isInstall(atScope: $0.scope, projectDirectory: projectRoot) + } case let .failure(failure): return .fail(failure.localizedDescription) } let expectedScope = projectRoot == nil ? Constants.PluginScope.user : Constants.PluginScope.local - if installs.contains(where: { $0.enabled && $0.isInstall(atScope: expectedScope, projectDirectory: projectRoot) }) { + if installsHere.contains(where: { $0.enabled && $0.scope == expectedScope }) { return .pass(projectRoot == nil ? "enabled" : "enabled (project)") } // Not mcs's install, but the plugin does load here. - if let other = installs.first(where: \.enabled) { + if let other = installsHere.first(where: \.enabled) { return .pass("enabled (\(other.scope) scope)") } - return .fail(installs.isEmpty ? "not installed" : "installed but disabled") + // Disabling is the user's choice and a re-sync would not undo it, so this only warns. + if let disabled = installsHere.first { + return .warn("installed but disabled — enable with: claude plugin enable \(disabled.id) -s \(disabled.scope)") + } + return .fail("not installed") } func fix() -> FixResult { diff --git a/Sources/mcs/Doctor/DoctorRunner.swift b/Sources/mcs/Doctor/DoctorRunner.swift index 78dfba02..988463ae 100644 --- a/Sources/mcs/Doctor/DoctorRunner.swift +++ b/Sources/mcs/Doctor/DoctorRunner.swift @@ -54,6 +54,8 @@ struct DoctorRunner { private var pendingFixes: [CollectedCheck] = [] private let shell: any ShellRunning private let claudeCLI: (any ClaudeCLI)? + // Shared by every `PluginCheck` in a run; a re-sync installs through a listing of its own. + private let pluginListing: PluginListing private enum SyncTarget: Hashable { case global @@ -133,6 +135,7 @@ struct DoctorRunner { self.projectRootOverride = projectRootOverride self.shell = shell ?? ShellRunner(environment: environment) self.claudeCLI = claudeCLI + pluginListing = PluginListing(claudeCLI: claudeCLI ?? ClaudeIntegration(shell: self.shell)) output = CLIOutput(warningCounter: warningCounter) } @@ -227,7 +230,6 @@ struct DoctorRunner { } // Layer 1+2: Derived + supplementary checks from installed components (per scope) - let pluginListing = PluginListing(claudeCLI: claudeCLI ?? ClaudeIntegration(shell: ShellRunner(environment: env))) for scope in scopes { allPackIDs.formUnion(scope.packIDs) @@ -829,6 +831,7 @@ struct DoctorRunner { } // A throw can leave the scope partly written, so the checks report what is on disk now. + pluginListing.invalidate() output.plain("") for check in resync.checks { switch check.check() { diff --git a/Sources/mcs/Doctor/ScopeDuplicationCheck.swift b/Sources/mcs/Doctor/ScopeDuplicationCheck.swift index 081addad..c64a9566 100644 --- a/Sources/mcs/Doctor/ScopeDuplicationCheck.swift +++ b/Sources/mcs/Doctor/ScopeDuplicationCheck.swift @@ -86,7 +86,8 @@ struct ScopeDuplicationCheck: DoctorCheck { strategy: ProjectSyncStrategy(projectPath: projectRoot, environment: environment) ) // Default `refCountScope` (nil → this project's path): the global scope still counts, so - // brew packages, plugins and gitignore entries report `.stillNeeded` and stay in place. + // brew packages and gitignore entries report `.stillNeeded` and stay in place. Plugins are + // per scope, so the project's `local` install goes and the global `user` one stays. // Passing `packRemoveSentinel` here would exclude every scope and remove them. configurator.unconfigurePack(packID, state: &state) @@ -194,9 +195,10 @@ struct ScopeDuplicationCheck: DoctorCheck { /// resolved per component and is the same in both scopes, but the directory differs, so the /// two entries are distinct strings that both fire. Everything else either shadows (project /// `settings.local.json` over global - /// `settings.json`, MCP `local` over `user`), is installed once (the project scope skips brew - /// packages and plugins entirely), writes one idempotent line (gitignore), or is a one-shot - /// side effect rather than a standing artifact (`shellCommand`). + /// `settings.json`, MCP `local` over `user`), is the same plugin enabled at two scopes and + /// loaded once, is installed once (the project scope skips brew packages), writes one + /// idempotent line (gitignore), or is a one-shot side effect rather than a standing artifact + /// (`shellCommand`). /// /// Deliberately exhaustive: a new install action should not silently default to "harmless". private static func duplicatesAcrossScopes(_ action: ComponentInstallAction) -> Bool { diff --git a/Sources/mcs/Sync/ComponentExecutor.swift b/Sources/mcs/Sync/ComponentExecutor.swift index 8b836c23..f85cd71d 100644 --- a/Sources/mcs/Sync/ComponentExecutor.swift +++ b/Sources/mcs/Sync/ComponentExecutor.swift @@ -60,8 +60,10 @@ struct ComponentExecutor { /// mcs ran the install, so it owns it. `userScopeCopy` is set when the plugin is also /// enabled for every project, which a `local` install alone does not need. case installed(id: String, userScopeCopy: Bool) - /// Already installed at this scope by something else; mcs takes no ownership. + /// Already installed at this scope, by the user or another pack; mcs takes no new ownership. case alreadyInstalled(id: String) + /// Installed, but the installs beforehand couldn't be listed, so it may have been the user's. + case installedUnowned(id: String) case failed } @@ -72,16 +74,19 @@ struct ComponentExecutor { return .failed } let ref = PluginRef(fullName) - if ref.hasMarketplaceRepo { - claudeCLI.pluginMarketplaceAdd(repo: ref.marketplaceRepo) - } + // A failed add is often a marketplace that is already there; it only matters when the + // plugin then can't be resolved, and its error is the one that explains why. + let added = ref.hasMarketplaceRepo ? claudeCLI.pluginMarketplaceAdd(repo: ref.marketplaceRepo) : nil guard let id = resolvePluginID(ref) else { - output.warn("Could not find the marketplace for plugin '\(fullName)' — is it added? Skipping") + let addError = added.flatMap { $0.succeeded ? nil : String($0.stderr.prefix(200)) } + let cause = addError.map { "adding \(ref.marketplaceRepo) failed: \($0)" } ?? "is it added?" + output.warn("Could not find the marketplace for plugin '\(fullName)' — \(cause). Skipping") return .failed } let listing = pluginListing ?? PluginListing(claudeCLI: claudeCLI) let installed = installedPlugins(listing) - if installed.contains(where: { $0.id == id && $0.isInstall(atScope: scope, projectDirectory: mcpWorkingDirectory) }) { + if let installed, + installed.contains(where: { $0.id == id && $0.isInstall(atScope: scope, projectDirectory: mcpWorkingDirectory) }) { return .alreadyInstalled(id: id) } let result = claudeCLI.pluginInstall(id: id, scope: scope, workingDirectory: mcpWorkingDirectory) @@ -90,6 +95,7 @@ struct ComponentExecutor { output.warn("Could not install plugin '\(id)': \(String(result.stderr.prefix(200)))") return .failed } + guard let installed else { return .installedUnowned(id: id) } let userScopeCopy = scope != Constants.PluginScope.user && installed.contains { $0.id == id && $0.scope == Constants.PluginScope.user && $0.enabled } return .installed(id: id, userScopeCopy: userScopeCopy) @@ -106,11 +112,13 @@ struct ComponentExecutor { // A marketplace removed since install can't be resolved; the CLI still matches a bare name. let id = resolvePluginID(ref) ?? ref.bareName let result = claudeCLI.pluginRemove(id: id, scope: scope, workingDirectory: mcpWorkingDirectory) - pluginListing?.invalidate() - // Reliable only because a `local` lookup runs in the scope's own project: from anywhere - // else the CLI would answer for a different project. - if result.stderr.contains(Constants.CLI.pluginNotFound) { - output.dimmed(" Plugin '\(ref.bareName)' is not installed — dropping it from tracking") + let listing = pluginListing ?? PluginListing(claudeCLI: claudeCLI) + listing.invalidate() + // A failed uninstall of a plugin that isn't at this scope is already done. Asked of the + // listing rather than read from stderr, whose wording differs per case and per CLI version. + // Reliable only because a `local` listing runs in the scope's own project. + if !result.succeeded, !isInstalled(ref, scope: scope, listing: listing, stderr: result.stderr) { + output.dimmed(" Plugin '\(ref.bareName)' is not installed at \(scope) scope — dropping it from tracking") return true } guard result.succeeded else { @@ -121,6 +129,15 @@ struct ComponentExecutor { return true } + private func isInstalled(_ ref: PluginRef, scope: String, listing: PluginListing, stderr: String) -> Bool { + switch listing.plugins(in: mcpWorkingDirectory) { + case let .success(plugins): + plugins.contains { ref.matches(id: $0.id) && $0.isInstall(atScope: scope, projectDirectory: mcpWorkingDirectory) } + case .failure: + !stderr.contains(Constants.CLI.pluginNotFound) + } + } + private func resolvePluginID(_ ref: PluginRef) -> String? { if ref.marketplaceName != nil { return ref.pluginID(in: []) } let result = claudeCLI.pluginMarketplaceList() @@ -137,14 +154,15 @@ struct ComponentExecutor { } } - /// An unreadable listing only costs an install that turns out to be a no-op, so it is not an error. - private func installedPlugins(_ listing: PluginListing) -> [InstalledPlugin] { + /// nil when unreadable: the install still runs, but without knowing what was there before, + /// mcs can't claim it. + private func installedPlugins(_ listing: PluginListing) -> [InstalledPlugin]? { switch listing.plugins(in: mcpWorkingDirectory) { case let .success(plugins): return plugins case let .failure(failure): - output.dimmed(" \(failure.localizedDescription) — installing anyway") - return [] + output.warn(" \(failure.localizedDescription) — installing anyway, but mcs won't remove it later") + return nil } } diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index c301e41c..273b2b84 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -425,7 +425,7 @@ struct Configurator { var removedServers: Set = [] var removedBrewPackages: Set = [] - // Remove MCS-owned brew packages and plugins (with reference counting) + // Remove MCS-owned brew packages (with reference counting) and plugins let excludeScope = refCountScope ?? scope.scopeIdentifier let refCounter = ResourceRefCounter( environment: environment, @@ -446,11 +446,15 @@ struct Configurator { } remaining.brewPackages.removeAll { removedBrewPackages.contains($0) } - let unremovedPlugins = removeOwnedPlugins(artifacts.plugins, of: packID, exec: exec, state: state) + let unremovedPlugins = releasePlugins( + artifacts.plugins, + otherPacks: state.configuredPacks.filter { $0 != packID }.map { ($0, registry.pack(for: $0)) }, + exec: exec, state: &state + ) for pluginName in unremovedPlugins { output.warn(" Could not remove plugin '\(PluginRef(pluginName).bareName)' — re-run '\(retryHint)' to retry") } - remaining.plugins = remaining.plugins.filter { unremovedPlugins.contains($0) } + remaining.plugins = unremovedPlugins // Remove MCP servers for server in artifacts.mcpServers @@ -902,12 +906,12 @@ struct Configurator { shell: shell, output: output ) - recordSharedPluginOwnership(pack, artifacts: &artifacts, state: state) reconcileStaleArtifacts( previousArtifacts: previousArtifacts, currentArtifacts: &artifacts, packID: pack.identifier, - scopePacks: packs + scopePacks: packs, + state: &state ) state.setArtifacts(artifacts, for: pack.identifier) state.recordPack(pack.identifier) @@ -954,20 +958,6 @@ struct Configurator { } } - /// A plugin another pack in this scope already installed counts as this pack's too, so the - /// last pack to leave the scope is the one that uninstalls it rather than nobody. - private func recordSharedPluginOwnership( - _ pack: any TechPack, artifacts: inout PackArtifactRecord, state: ProjectState - ) { - for component in pack.components { - guard case let .plugin(name) = component.installAction, !artifacts.plugins.contains(name) else { continue } - let ownedInScope = state.configuredPacks.contains { otherID in - otherID != pack.identifier && state.artifacts(for: otherID)?.ownsPlugin(name) == true - } - if ownedInScope { artifacts.recordPlugin(name) } - } - } - /// Remove artifacts that were tracked in the previous sync but are absent from the current one. /// /// After `installArtifacts()` produces a fresh `PackArtifactRecord`, this method diffs it @@ -981,7 +971,8 @@ struct Configurator { previousArtifacts: PackArtifactRecord?, currentArtifacts: inout PackArtifactRecord, packID: String, - scopePacks: [any TechPack] + scopePacks: [any TechPack], + state: inout ProjectState ) { guard let previous = previousArtifacts else { return } @@ -1024,8 +1015,8 @@ struct Configurator { } } - // Gitignore entries, brew packages and plugins are all ref-counted, so one counter serves - // all three. Building it is free — three stored properties, no I/O until it is queried. + // Gitignore entries and brew packages are both ref-counted, so one counter serves both. + // Building it is free — three stored properties, no I/O until it is queried. let refCounter = ResourceRefCounter( environment: environment, output: output, registry: registry ) @@ -1051,10 +1042,9 @@ struct Configurator { } } - // Brew packages and plugins (ref-counted) + // Brew packages (ref-counted) let staleBrew = Set(previous.brewPackages).subtracting(currentArtifacts.brewPackages) - let stalePlugins = Set(previous.plugins).subtracting(currentArtifacts.plugins) - if !staleBrew.isEmpty || !stalePlugins.isEmpty { + if !staleBrew.isEmpty { for package in staleBrew { let result = removeBrewArtifact( package, exec: exec, refCounter: refCounter, @@ -1068,14 +1058,18 @@ struct Configurator { output.warn(" Could not remove stale brew package '\(package)' — will retry on next sync") } } - for name in stalePlugins { - let claimants = scopePacks.filter { $0.identifier != packID } - .map { PluginClaimant(packID: $0.identifier, pack: $0, record: nil) } - if !removePluginArtifact(name, exec: exec, otherClaimants: claimants) { - currentArtifacts.plugins.append(name) - output.warn(" Could not remove stale plugin '\(PluginRef(name).bareName)' — will retry on next sync") - } - } + } + + // Plugins (claimed only within this scope) + let stalePlugins = previous.plugins.filter { !currentArtifacts.plugins.contains($0) } + let unremovedPlugins = releasePlugins( + stalePlugins, + otherPacks: scopePacks.filter { $0.identifier != packID }.map { ($0.identifier, $0) }, + exec: exec, state: &state + ) + for name in unremovedPlugins { + currentArtifacts.plugins.append(name) + output.warn(" Could not remove stale plugin '\(PluginRef(name).bareName)' — will retry on next sync") } } @@ -1151,7 +1145,7 @@ struct Configurator { // MARK: - Artifact Removal Helpers - /// Result of attempting to remove a ref-counted resource (brew package or plugin). + /// Result of attempting to remove a ref-counted resource (brew package or gitignore entry). private enum RefCountedRemovalResult { /// Resource was successfully uninstalled. case removed @@ -1199,45 +1193,42 @@ struct Configurator { return .failed } - /// Remove a plugin from this scope unless another pack in the same scope still claims it. + /// Uninstall plugins a pack owned from this scope, unless another pack in the scope still + /// claims one. A kept plugin moves to that pack's record, so whichever pack leaves last still + /// uninstalls it instead of nobody. /// /// Plugins install per scope (`local` in a project, `user` globally), so only packs sharing the - /// scope can share an install; other scopes hold installs of their own. - /// Returns the plugins that could not be removed and stay recorded. - private func removeOwnedPlugins( - _ plugins: [String], of packID: String, exec: ComponentExecutor, state: ProjectState - ) -> [String] { - let otherClaimants = state.configuredPacks.filter { $0 != packID }.map { - PluginClaimant(packID: $0, pack: registry.pack(for: $0), record: state.artifacts(for: $0)) - } - return plugins.filter { !removePluginArtifact($0, exec: exec, otherClaimants: otherClaimants) } - } - - private struct PluginClaimant { - let packID: String - let pack: (any TechPack)? - let record: PackArtifactRecord? - } - - /// Returns `false` only when an uninstall was attempted and failed; a plugin another pack in - /// the scope still claims is kept and counts as handled. - private func removePluginArtifact( - _ name: String, + /// scope can share an install. Returns the plugins whose uninstall failed and stay recorded. + private func releasePlugins( + _ plugins: [String], + otherPacks: [(id: String, pack: (any TechPack)?)], exec: ComponentExecutor, - otherClaimants: [PluginClaimant] - ) -> Bool { - // Conservative, as in `ResourceRefCounter`: an unloadable pack may still declare it. - if let claimant = otherClaimants.first(where: { $0.record?.ownsPlugin(name) == true || $0.pack?.declaresPlugin(name) ?? true }) { - output.dimmed(" Keeping plugin '\(PluginRef(name).bareName)' — still needed by \(claimant.packID)") - return true + state: inout ProjectState + ) -> [String] { + var unremoved: [String] = [] + for name in plugins { + // Conservative, as in `ResourceRefCounter`: an unloadable pack may still declare it. + let claimant = otherPacks.first { other in + state.artifacts(for: other.id)?.ownsPlugin(name) == true || other.pack?.declaresPlugin(name) ?? true + }?.id + guard let claimant else { + if !exec.removePlugin(name, scope: scope.pluginScope) { unremoved.append(name) } + continue + } + var record = state.artifacts(for: claimant) ?? PackArtifactRecord() + if !record.ownsPlugin(name) { + record.recordPlugin(name) + state.setArtifacts(record, for: claimant) + } + output.dimmed(" Keeping plugin '\(PluginRef(name).bareName)' — still needed by \(claimant)") } - return exec.removePlugin(name, scope: scope.pluginScope) + return unremoved } /// Remove a single gitignore entry with reference counting, absorbing the do/catch. /// /// `GitignoreManager` resolves one file for the whole machine, so an entry is a shared - /// resource exactly like a brew package or a plugin — another scope can still claim the + /// resource exactly like a brew package — another scope can still claim the /// same physical line. /// /// Logs the "Keeping" message when the entry is still needed, and the underlying error on @@ -1285,7 +1276,7 @@ struct Configurator { // MARK: - Global Dependencies - /// Auto-install brew packages (project scope only); plugins install per scope with the pack. + /// Auto-install brew packages (project scope only). private func autoInstallGlobalDependencies(_ pack: any TechPack) { let exec = makeExecutor() for component in pack.components { diff --git a/Sources/mcs/Sync/ConfiguratorSupport.swift b/Sources/mcs/Sync/ConfiguratorSupport.swift index ec01c5f5..021f5245 100644 --- a/Sources/mcs/Sync/ConfiguratorSupport.swift +++ b/Sources/mcs/Sync/ConfiguratorSupport.swift @@ -491,6 +491,8 @@ enum ConfiguratorSupport { case .installed: artifacts.recordPlugin(name) output.success(" \(component.displayName) installed (scope: \(scope))") + case .installedUnowned: + output.success(" \(component.displayName) installed (scope: \(scope)), not tracked") case .alreadyInstalled: output.dimmed(" \(component.displayName) already installed, skipping") case .failed: diff --git a/Sources/mcs/Sync/GlobalSyncStrategy.swift b/Sources/mcs/Sync/GlobalSyncStrategy.swift index 24e19e09..131cac24 100644 --- a/Sources/mcs/Sync/GlobalSyncStrategy.swift +++ b/Sources/mcs/Sync/GlobalSyncStrategy.swift @@ -56,7 +56,7 @@ struct GlobalSyncStrategy: SyncStrategy { artifacts.fileHashesShippedOnly = true // Carry forward ownership records from previous sync artifacts.brewPackages = previousArtifacts?.brewPackages ?? [] - artifacts.plugins = previousArtifacts?.plugins ?? [] + artifacts.plugins = previousArtifacts?.pluginsStillDeclared(by: pack) ?? [] for component in pack.components { if ComponentExecutor.isAlreadyInstalled(component) { @@ -198,14 +198,14 @@ struct GlobalSyncStrategy: SyncStrategy { settings.hooks = hooks.isEmpty ? nil : hooks } let groupCountAfter = settings.hooks?.values.reduce(0) { $0 + $1.count } ?? 0 - let strippedContent = groupCountAfter < groupCountBefore // Strip previously-tracked settings keys (enabledPlugins + settingsMerge extraJSON) // before re-composing, so removed components don't leave stale entries. let allPreviousKeys = previousSettingsKeys.values.flatMap(\.self) - if !allPreviousKeys.isEmpty { - settings.removeKeys(allPreviousKeys) - } + // A stripped key has to reach disk even when nothing is recomposed: step 6b resets each + // pack's record to what it contributes now, so an unsaved strip would orphan the key. + let strippedKeys = settings.removeKeys(allPreviousKeys) + let strippedContent = groupCountAfter < groupCountBefore || strippedKeys // Collect top-level keys to pass as dropKeys, preventing Layer 3 re-injection let dropKeys = Settings.topLevelKeys(of: allPreviousKeys) diff --git a/Sources/mcs/Sync/ProjectSyncStrategy.swift b/Sources/mcs/Sync/ProjectSyncStrategy.swift index 2136664c..180401de 100644 --- a/Sources/mcs/Sync/ProjectSyncStrategy.swift +++ b/Sources/mcs/Sync/ProjectSyncStrategy.swift @@ -60,7 +60,7 @@ struct ProjectSyncStrategy: SyncStrategy { ) -> PackArtifactRecord { var artifacts = PackArtifactRecord() artifacts.fileHashesShippedOnly = true - artifacts.plugins = previousArtifacts?.plugins ?? [] + artifacts.plugins = previousArtifacts?.pluginsStillDeclared(by: pack) ?? [] for component in pack.components { if ComponentExecutor.isAlreadyInstalled(component) { @@ -168,15 +168,14 @@ struct ProjectSyncStrategy: SyncStrategy { let allPreviousKeys = previousSettingsKeys.values.flatMap(\.self) let dropKeys = Settings.topLevelKeys(of: allPreviousKeys) // Start from the existing extra keys minus what packs owned, so a pack value merges under - // the user's instead of replacing the whole object. Hooks and plugins are still rebuilt - // from the packs alone. Ownership stops one level down: an array such as - // `permissions.allow` is owned or skipped whole, so Claude Code's approvals in one a pack - // owns are still rewritten on sync. + // the user's instead of replacing the whole object. `enabledPlugins` is kept as the CLI + // wrote it; hooks are still rebuilt from the packs alone. Ownership stops one level down: + // an array such as `permissions.allow` is owned or skipped whole, so Claude Code's + // approvals in one a pack owns are still rewritten on sync. do { var existing = try Settings.load(from: scope.settingsPath) existing.removeKeys(allPreviousKeys) settings.extraJSON = existing.extraJSON - // Written by `claude plugin install -s local`, not by mcs. settings.enabledPlugins = existing.enabledPlugins } catch { output.warn( diff --git a/Tests/MCSTests/ComponentExecutorTests.swift b/Tests/MCSTests/ComponentExecutorTests.swift index d08b134e..77cf2125 100644 --- a/Tests/MCSTests/ComponentExecutorTests.swift +++ b/Tests/MCSTests/ComponentExecutorTests.swift @@ -15,10 +15,17 @@ struct ComponentExecutorTests { // MARK: - removePlugin - private func removePlugin(returning result: ShellResult) -> Bool { + private let userCopy = InstalledPlugin(id: "lint@acme", scope: "user", enabled: true, projectPath: nil) + + private func removePlugin( + returning result: ShellResult, + scope: String = Constants.PluginScope.user, + installed: [InstalledPlugin] = [] + ) -> Bool { let cli = MockClaudeCLI() cli.result = result - return makeExecutor(cli).removePlugin("lint@acme", scope: Constants.PluginScope.user) + cli.installedPlugins = installed + return makeExecutor(cli).removePlugin("lint@acme", scope: scope) } @Test("removePlugin counts a plugin that is already uninstalled as removed") @@ -27,9 +34,17 @@ struct ComponentExecutorTests { #expect(removePlugin(returning: ShellResult(exitCode: 1, stdout: "", stderr: stderr))) } - @Test("removePlugin reports any other failure") + @Test("removePlugin counts a plugin left only at another scope as removed from this one") + func removePluginOnlyAtOtherScope() { + let stderr = "✘ Failed to uninstall plugin \"lint@acme\": Plugin \"lint@acme\" is installed in user scope, not local." + #expect(removePlugin( + returning: ShellResult(exitCode: 1, stdout: "", stderr: stderr), scope: Constants.PluginScope.local, installed: [userCopy] + )) + } + + @Test("removePlugin reports a failure that leaves the plugin installed at this scope") func removePluginRealFailure() { - #expect(!removePlugin(returning: ShellResult(exitCode: 1, stdout: "", stderr: "network unreachable"))) + #expect(!removePlugin(returning: ShellResult(exitCode: 1, stdout: "", stderr: "network unreachable"), installed: [userCopy])) } // MARK: - installProjectFile placeholder substitution @@ -179,4 +194,13 @@ struct ComponentExecutorTests { let outcome = makeExecutor(cli, project: project).installPlugin("lint@acme", scope: Constants.PluginScope.local) #expect(outcome == .installed(id: "lint@acme", userScopeCopy: true)) } + + @Test("installPlugin claims nothing when it couldn't see what was installed before") + func installPluginUnlistedIsUnowned() { + let cli = MockClaudeCLI() + cli.pluginListFails = true + let outcome = makeExecutor(cli).installPlugin("lint@acme", scope: Constants.PluginScope.user) + #expect(outcome == .installedUnowned(id: "lint@acme")) + #expect(cli.pluginInstallCalls.count == 1) + } } diff --git a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift index d4307a5a..8634c1cb 100644 --- a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift @@ -256,7 +256,7 @@ struct PluginCheckTests { let result = check("lint@acme", projectRoot: project, installed: [ InstalledPlugin(id: "lint@acme", scope: "local", enabled: true, projectPath: "/tmp/elsewhere"), ]) - #expect(result == .fail("installed but disabled")) + #expect(result == .fail("not installed")) } @Test("A user-scope install still loads in a project") @@ -267,12 +267,12 @@ struct PluginCheckTests { #expect(result == .pass("enabled (user scope)")) } - @Test("A disabled install fails") - func disabledInstallFails() { + @Test("A disabled install warns with the enable command instead of asking for a re-sync") + func disabledInstallWarns() { let result = check("lint@acme", projectRoot: nil, installed: [ InstalledPlugin(id: "lint@acme", scope: "user", enabled: false, projectPath: nil), ]) - #expect(result == .fail("installed but disabled")) + #expect(result == .warn("installed but disabled — enable with: claude plugin enable lint@acme -s user")) } @Test("The same name from another marketplace is not this plugin") @@ -290,14 +290,15 @@ struct PluginCheckTests { // A local install in `project` would read as enabled if the listing ran there. cli.installedPlugins = [InstalledPlugin(id: "lint@acme", scope: "local", enabled: true, projectPath: project.path)] let result = PluginCheck(pluginRef: PluginRef("lint@acme"), projectRoot: nil, listing: PluginListing(claudeCLI: cli), homeDirectory: home).check() - #expect(result == .fail("installed but disabled")) + #expect(result == .fail("not installed")) + #expect(cli.pluginListDirectories == [home]) } @Test("An unreadable plugin list fails rather than passing") func unreadableListFails() { let cli = MockClaudeCLI() - let failing = FailingListCLI(base: cli) - let result = PluginCheck(pluginRef: PluginRef("lint@acme"), projectRoot: nil, listing: PluginListing(claudeCLI: failing)).check() + cli.pluginListFails = true + let result = PluginCheck(pluginRef: PluginRef("lint@acme"), projectRoot: nil, listing: PluginListing(claudeCLI: cli)).check() guard case let .fail(message) = result else { Issue.record("Expected .fail, got \(result)") return @@ -306,42 +307,6 @@ struct PluginCheckTests { } } -/// Wraps the mock so only `plugin list` fails. -private struct FailingListCLI: ClaudeCLI { - let base: MockClaudeCLI - var isAvailable: Bool { - true - } - - func mcpAdd(name: String, scope: String, arguments: [String], workingDirectory: URL?) -> ShellResult { - base.mcpAdd(name: name, scope: scope, arguments: arguments, workingDirectory: workingDirectory) - } - - func mcpRemove(name: String, scope: String, workingDirectory: URL?) -> ShellResult { - base.mcpRemove(name: name, scope: scope, workingDirectory: workingDirectory) - } - - func pluginMarketplaceAdd(repo: String) -> ShellResult { - base.pluginMarketplaceAdd(repo: repo) - } - - func pluginMarketplaceList() -> ShellResult { - base.pluginMarketplaceList() - } - - func pluginList(workingDirectory _: URL?) -> ShellResult { - ShellResult(exitCode: 1, stdout: "", stderr: "boom") - } - - func pluginInstall(id: String, scope: String, workingDirectory: URL?) -> ShellResult { - base.pluginInstall(id: id, scope: scope, workingDirectory: workingDirectory) - } - - func pluginRemove(id: String, scope: String, workingDirectory: URL?) -> ShellResult { - base.pluginRemove(id: id, scope: scope, workingDirectory: workingDirectory) - } -} - // MARK: - ProjectIndexCheck Sandbox Tests struct ProjectIndexCheckSandboxTests { diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index 5a21bb6e..eb3743a0 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -4553,5 +4553,99 @@ struct PluginScopeLifecycleTests { try bed.makeConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) #expect(try Settings.load(from: bed.settingsLocalPath).enabledPlugins == ["lint@acme": true]) + // The claim goes with the key, or `SettingsKeysCheck` would fail on it forever. + #expect(try bed.projectState().artifacts(for: "lint-pack")?.settingsKeys.isEmpty == true) + } + + @Test("Global sync drops the legacy bare-name enabledPlugins entry and keeps the CLI's") + func globalLegacyBareKeyDropped() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + var config = MCSConfig() + config.updateCheck = false + try config.save(to: bed.env.mcsConfigFile) + let pack = pluginPack("lint-pack") + let registry = TechPackRegistry(packs: [pack]) + try bed.makeGlobalSyncConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + + var state = try bed.globalState() + var record = try #require(state.artifacts(for: "lint-pack")) + record.settingsKeys = ["enabledPlugins.lint"] + state.setArtifacts(record, for: "lint-pack") + try state.save() + try JSONSerialization.data(withJSONObject: ["enabledPlugins": ["lint": true, "lint@acme": true]]) + .write(to: bed.env.claudeSettings) + + try bed.makeGlobalSyncConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + + #expect(try Settings.load(from: bed.env.claudeSettings).enabledPlugins == ["lint@acme": true]) + #expect(try bed.globalState().artifacts(for: "lint-pack")?.settingsKeys.isEmpty == true) + } + + @Test("A plugin a pack update drops is uninstalled from the project") + func droppedPluginIsUninstalled() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let v1 = pluginPack("lint-pack") + try bed.makeConfigurator(registry: TechPackRegistry(packs: [v1])).configure(packs: [v1], confirmRemovals: false) + + let v2 = MockTechPack(identifier: "lint-pack", displayName: "lint-pack", components: []) + try bed.makeConfigurator(registry: TechPackRegistry(packs: [v2])).configure(packs: [v2], confirmRemovals: false) + + #expect(bed.mockCLI.pluginRemoveCalls == [MockClaudeCLI.PluginCall( + id: "lint@acme", scope: Constants.PluginScope.local, workingDirectory: bed.project + )]) + #expect(try bed.projectState().artifacts(for: "lint-pack")?.plugins.isEmpty ?? true) + } + + @Test("A plugin one pack drops while another still declares it stays, owned by the other pack") + func droppedPluginHandedToClaimant() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let packA = pluginPack("pack-a") + let packB = pluginPack("pack-b") + try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA, packB])) + .configure(packs: [packA, packB], confirmRemovals: false) + + let packAv2 = MockTechPack(identifier: "pack-a", displayName: "pack-a", components: []) + let registry = TechPackRegistry(packs: [packAv2, packB]) + try bed.makeConfigurator(registry: registry).configure(packs: [packAv2, packB], confirmRemovals: false) + #expect(bed.mockCLI.pluginRemoveCalls.isEmpty) + #expect(try bed.projectState().artifacts(for: "pack-b")?.plugins == ["lint@acme"]) + + try bed.makeConfigurator(registry: registry).configure(packs: [packAv2], confirmRemovals: false) + #expect(bed.mockCLI.pluginRemoveCalls.count == 1) + } + + @Test("Removing a pack while another configured pack can't be loaded hands the plugin to that pack") + func unloadableClaimantTakesOverPlugin() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let packA = pluginPack("pack-a") + let packB = pluginPack("pack-b", plugin: "other@acme") + try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA, packB])) + .configure(packs: [packA, packB], confirmRemovals: false) + var state = try bed.projectState() + + // pack-b is configured but missing from the registry, so it might still declare the plugin. + bed.makeConfigurator(registry: TechPackRegistry(packs: [packA])).unconfigurePack("pack-a", state: &state) + + #expect(bed.mockCLI.pluginRemoveCalls.isEmpty) + #expect(state.artifacts(for: "pack-b")?.plugins.contains("lint@acme") == true) + } + + @Test("doctor --fix reinstalls a plugin removed out of band and reports it fixed") + func doctorFixReinstallsPlugin() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let pack = pluginPack("lint-pack") + let registry = TechPackRegistry(packs: [pack]) + try bed.makeConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + bed.mockCLI.installedPlugins = [] + #expect(try bed.runDoctor(registry: registry).issues > 0) + + var runner = bed.makeDoctorRunner(registry: registry, fixMode: true) + #expect(try runner.run().isHealthy) + #expect(bed.mockCLI.pluginInstallCalls.count == 2) } } diff --git a/Tests/MCSTests/TestHelpers.swift b/Tests/MCSTests/TestHelpers.swift index a40555e1..5b29ecce 100644 --- a/Tests/MCSTests/TestHelpers.swift +++ b/Tests/MCSTests/TestHelpers.swift @@ -58,6 +58,8 @@ final class MockClaudeCLI: ClaudeCLI, @unchecked Sendable { /// What `plugin list` reports. A successful install adds to it and a removal drops from it, /// so a sync → doctor → remove sequence sees what the real CLI would. var installedPlugins: [InstalledPlugin] = [] + var pluginListDirectories: [URL?] = [] + var pluginListFails = false @discardableResult func pluginMarketplaceAdd(repo: String) -> ShellResult { @@ -70,7 +72,9 @@ final class MockClaudeCLI: ClaudeCLI, @unchecked Sendable { } func pluginList(workingDirectory: URL?) -> ShellResult { - jsonResult(installedPlugins.map { plugin in + pluginListDirectories.append(workingDirectory) + if pluginListFails { return ShellResult(exitCode: 1, stdout: "", stderr: "list failed") } + return jsonResult(installedPlugins.map { plugin in // Mirrors the CLI: a local install is enabled only when listed from its own project. guard plugin.scope == Constants.PluginScope.local else { return plugin } let here = plugin.isInstall(atScope: plugin.scope, projectDirectory: workingDirectory) From 15910612837336a4a4314089e3e8719fa269b156 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Thu, 8 Oct 2026 18:58:24 +0200 Subject: [PATCH 3/3] ISSUE-450: Fail the plugin check when this scope's install is missing - A disabled copy at another scope no longer turns a missing install into a warning, so doctor --fix reinstalls it - Another project's project-scope install no longer counts for this project --- Sources/mcs/Core/PluginRef.swift | 5 +++-- Sources/mcs/Doctor/CoreDoctorChecks.swift | 9 +++++---- Tests/MCSTests/CoreDoctorCheckSandboxTests.swift | 16 ++++++++++++++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/Sources/mcs/Core/PluginRef.swift b/Sources/mcs/Core/PluginRef.swift index 3905c077..2f3a5b17 100644 --- a/Sources/mcs/Core/PluginRef.swift +++ b/Sources/mcs/Core/PluginRef.swift @@ -88,10 +88,11 @@ struct InstalledPlugin: Codable, Equatable { /// The project a `local`- or `project`-scoped install belongs to. let projectPath: String? - /// Whether this install is the one a sync at `scope` (in `projectDirectory`, for `local`) owns. + /// Whether this install is at `scope` and, unless that is `user`, belongs to `projectDirectory`. func isInstall(atScope scope: String, projectDirectory: URL?) -> Bool { guard self.scope == scope else { return false } - guard scope == Constants.PluginScope.local else { return true } + // `local` and `project` installs are listed for every project; only `user` applies everywhere. + guard scope != Constants.PluginScope.user else { return true } guard let projectDirectory, let projectPath else { return false } return URL(fileURLWithPath: projectPath).resolvingSymlinksInPath().path == projectDirectory.resolvingSymlinksInPath().path diff --git a/Sources/mcs/Doctor/CoreDoctorChecks.swift b/Sources/mcs/Doctor/CoreDoctorChecks.swift index a13982e7..737829cd 100644 --- a/Sources/mcs/Doctor/CoreDoctorChecks.swift +++ b/Sources/mcs/Doctor/CoreDoctorChecks.swift @@ -111,7 +111,7 @@ struct PluginCheck: DoctorCheck { } func check() -> CheckResult { - // A `local` install belongs to one project; another project's says nothing about this scope. + // Another project's install says nothing about this scope. let installsHere: [InstalledPlugin] switch listing.plugins(in: projectRoot ?? homeDirectory) { case let .success(plugins): @@ -126,12 +126,13 @@ struct PluginCheck: DoctorCheck { if installsHere.contains(where: { $0.enabled && $0.scope == expectedScope }) { return .pass(projectRoot == nil ? "enabled" : "enabled (project)") } - // Not mcs's install, but the plugin does load here. + // Installed at another scope, but it still loads here. if let other = installsHere.first(where: \.enabled) { return .pass("enabled (\(other.scope) scope)") } - // Disabling is the user's choice and a re-sync would not undo it, so this only warns. - if let disabled = installsHere.first { + // Disabling is the user's choice and a re-sync would not undo it, so this only warns. A + // disabled copy elsewhere doesn't count: the missing install here is what a re-sync restores. + if let disabled = installsHere.first(where: { $0.scope == expectedScope }) { return .warn("installed but disabled — enable with: claude plugin enable \(disabled.id) -s \(disabled.scope)") } return .fail("not installed") diff --git a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift index 8634c1cb..baaec570 100644 --- a/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift +++ b/Tests/MCSTests/CoreDoctorCheckSandboxTests.swift @@ -275,6 +275,22 @@ struct PluginCheckTests { #expect(result == .warn("installed but disabled — enable with: claude plugin enable lint@acme -s user")) } + @Test("A project missing its local install fails even when a disabled user copy exists") + func missingLocalWithDisabledUserCopyFails() { + let result = check("lint@acme", projectRoot: project, installed: [ + InstalledPlugin(id: "lint@acme", scope: "user", enabled: false, projectPath: nil), + ]) + #expect(result == .fail("not installed")) + } + + @Test("A project-scope install in another project does not count") + func otherProjectsProjectScopeInstallFails() { + let result = check("lint@acme", projectRoot: project, installed: [ + InstalledPlugin(id: "lint@acme", scope: "project", enabled: false, projectPath: "/tmp/elsewhere"), + ]) + #expect(result == .fail("not installed")) + } + @Test("The same name from another marketplace is not this plugin") func otherMarketplaceDoesNotMatch() { let result = check("lint@acme", projectRoot: nil, installed: [