diff --git a/CLAUDE.md b/CLAUDE.md index 821a2b1..13741c9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -146,7 +146,7 @@ mcs config set # Set a configuration value (true/false) - `DestinationCollisionResolver.swift` — auto-namespaces `copyPackFile` destinations when multiple packs target the same `(destination, fileType)` pair - `PackInstaller.swift` — auto-installs missing pack components - `PackUpdater.swift` — shared fetch → validate → trust cycle for updating a single git pack (used by `UpdatePack` and `LockfileOperations`) -- `ResourceRefCounter.swift` — two-tier reference counting (global artifacts + project index manifests) for safe brew/plugin removal +- `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 - `LockfileOperations.swift` — reads/writes `mcs.lock.yaml`, checks out locked versions, updates lockfile - `SyncDeltaSummary.swift` — computes add/remove/keep deltas between previous and selected pack sets and renders the review-changes summary shown before destructive sync operations @@ -213,5 +213,5 @@ swiftlint --fix - **Lockfile support (opt-in)**: `mcs.lock.yaml` pins pack commits for reproducible builds. Generation is opt-in — enable with `mcs config set generate-lockfile true` to write on every sync. `--lock` checks out pinned commits from an existing lockfile. Tri-state semantics on `generate-lockfile`: `true` writes, `false` is silent (explicit opt-out), `nil` (never configured) surfaces a one-time drift warning if a stale lockfile exists — the upgrade nudge - **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 or plugins. 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 +- **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 - **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/Doctor/ScopeDuplicationCheck.swift b/Sources/mcs/Doctor/ScopeDuplicationCheck.swift index 39a8c1b..671f5e1 100644 --- a/Sources/mcs/Doctor/ScopeDuplicationCheck.swift +++ b/Sources/mcs/Doctor/ScopeDuplicationCheck.swift @@ -77,7 +77,6 @@ struct ScopeDuplicationCheck: DoctorCheck { let output = CLIOutput() let shell = ShellRunner(environment: environment) var state = inputs.projectState - let sharedGitignoreEntries = inputs.globalState.artifacts(for: packID)?.gitignoreEntries ?? [] let configurator = Configurator( environment: environment, @@ -87,8 +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 and plugins report `.stillNeeded` and stay installed. Passing - // `packRemoveSentinel` here would exclude every scope and uninstall them. + // brew packages, plugins and gitignore entries report `.stillNeeded` and stay in place. + // Passing `packRemoveSentinel` here would exclude every scope and remove them. configurator.unconfigurePack(packID, state: &state) do { @@ -104,40 +103,11 @@ struct ScopeDuplicationCheck: DoctorCheck { return .failed("some artifacts could not be removed — re-run 'mcs sync' to retry") } - restoreSharedGitignoreEntries(sharedGitignoreEntries, shell: shell, output: output) pruneProjectIndex(output: output) return .fixed("removed project-scoped '\(packID)' (global copy kept)") } - /// Re-add gitignore lines the global copy still claims. - /// - /// `GitignoreManager` writes one file for the whole machine, so a gitignore entry is a shared - /// resource exactly like a brew package or a plugin — but it is the one such resource - /// `ResourceRefCounter.Resource` omits, so `unconfigurePack` deletes it on behalf of any single - /// scope. `addEntry` is idempotent, so re-adding is safe. - /// - /// This only repairs the doctor path. The same unguarded delete is a live bug in `mcs sync` - /// deselection, `mcs pack remove` (where the first scope strips the lines out from under the - /// scopes that follow), and stale-artifact reconciliation. Fixing it properly means adding a - /// third `ResourceRefCounter.Resource` case so `Configurator.removeGitignoreArtifact` becomes - /// ref-counted like its brew and plugin siblings — tracked as a follow-up. - private func restoreSharedGitignoreEntries( - _ entries: [String], - shell: any ShellRunning, - output: CLIOutput - ) { - guard !entries.isEmpty else { return } - let manager = GitignoreManager(shell: shell) - for entry in entries { - do { - try manager.addEntry(entry) - } catch { - output.warn("Could not restore gitignore entry '\(entry)': \(error.localizedDescription)") - } - } - } - /// Drop this pack from this project's entry in `~/.mcs/projects.yaml`. /// Failure warns rather than fails, mirroring `Configurator.saveStateAndUpdateIndex`. private func pruneProjectIndex(output: CLIOutput) { diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index 798e15a..e2e1077 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -538,14 +538,23 @@ struct Configurator { } } - // Remove gitignore entries + // Remove gitignore entries (with reference counting) if !artifacts.gitignoreEntries.isEmpty { let gitignoreManager = GitignoreManager(shell: shell) var removedEntries: Set = [] - for entry in artifacts.gitignoreEntries - where removeGitignoreArtifact(entry, gitignoreManager: gitignoreManager) { - removedEntries.insert(entry) - output.dimmed(" Removed gitignore entry: \(entry)") + for entry in artifacts.gitignoreEntries { + let result = removeGitignoreArtifact( + entry, gitignoreManager: gitignoreManager, refCounter: refCounter, + excludingScope: excludeScope, excludingPack: packID + ) + switch result { + case .removed, .stillNeeded: + removedEntries.insert(entry) + if case .removed = result { output.dimmed(" Removed gitignore entry: \(entry)") } + case .failed: + // Helper already warned — leave the claim in `remaining` so sync retries. + break + } } remaining.gitignoreEntries.removeAll { removedEntries.contains($0) } } @@ -674,10 +683,21 @@ struct Configurator { case let .gitignoreEntries(entries): let gitignoreManager = GitignoreManager(shell: shell) - for entry in entries - where removeGitignoreArtifact(entry, gitignoreManager: gitignoreManager) { - artifacts.gitignoreEntries.removeAll { $0 == entry } - output.dimmed(" Removed gitignore entry: \(entry)") + for entry in entries { + let result = removeGitignoreArtifact( + entry, gitignoreManager: gitignoreManager, refCounter: refCounter, + excludingScope: scope.scopeIdentifier, + excludingPack: pack.identifier + ) + switch result { + case .removed: + artifacts.gitignoreEntries.removeAll { $0 == entry } + output.dimmed(" Removed gitignore entry: \(entry)") + case .stillNeeded, .failed: + // Kept by another scope, or the helper already warned — either way + // the claim stays on the record, as brew and plugins do here. + break + } } case .shellCommand, .settingsMerge: @@ -974,14 +994,27 @@ struct Configurator { } } - // Gitignore entries + // 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. + let refCounter = ResourceRefCounter( + environment: environment, output: output, registry: registry + ) + + // Gitignore entries (ref-counted) let staleGitignore = Set(previous.gitignoreEntries).subtracting(currentArtifacts.gitignoreEntries) if !staleGitignore.isEmpty { let gitignoreManager = GitignoreManager(shell: shell) for entry in staleGitignore { - if removeGitignoreArtifact(entry, gitignoreManager: gitignoreManager) { + let result = removeGitignoreArtifact( + entry, gitignoreManager: gitignoreManager, refCounter: refCounter, + excludingScope: scope.scopeIdentifier, excludingPack: packID + ) + switch result { + case .removed: output.dimmed(" Removed stale gitignore entry: \(entry)") - } else { + case .stillNeeded: + break + case .failed: // Helper already warned — re-add for retry on next sync currentArtifacts.gitignoreEntries.append(entry) } @@ -992,9 +1025,6 @@ struct Configurator { let staleBrew = Set(previous.brewPackages).subtracting(currentArtifacts.brewPackages) let stalePlugins = Set(previous.plugins).subtracting(currentArtifacts.plugins) if !staleBrew.isEmpty || !stalePlugins.isEmpty { - let refCounter = ResourceRefCounter( - environment: environment, output: output, registry: registry - ) for package in staleBrew { let result = removeBrewArtifact( package, exec: exec, refCounter: refCounter, @@ -1108,7 +1138,7 @@ struct Configurator { /// Remove a brew package with reference counting. /// - /// Logs the "Keeping" message when the package is still needed by another scope. + /// Logs the "Keeping" message when the package is still needed by another scope or pack. /// Callers provide their own success/failure context messages. private func removeBrewArtifact( _ package: String, @@ -1122,7 +1152,7 @@ struct Configurator { excludingScope: excludingScope, excludingPack: excludingPack ) { - output.dimmed(" Keeping brew package '\(package)' — still needed by another scope") + output.dimmed(" Keeping brew package '\(package)' — still needed by another scope or pack") return .stillNeeded } if exec.uninstallBrewPackage(package) { @@ -1133,7 +1163,7 @@ struct Configurator { /// Remove a plugin with reference counting. /// - /// Logs the "Keeping" message when the plugin is still needed by another scope. + /// Logs the "Keeping" message when the plugin is still needed by another scope or pack. /// Callers provide their own success/failure context messages. private func removePluginArtifact( _ name: String, @@ -1147,7 +1177,7 @@ struct Configurator { excludingScope: excludingScope, excludingPack: excludingPack ) { - output.dimmed(" Keeping plugin '\(PluginRef(name).bareName)' — still needed by another scope") + output.dimmed(" Keeping plugin '\(PluginRef(name).bareName)' — still needed by another scope or pack") return .stillNeeded } if exec.removePlugin(name) { @@ -1156,20 +1186,43 @@ struct Configurator { return .failed } - /// Remove a single gitignore entry, absorbing the do/catch. + /// 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 + /// same physical line. /// - /// Logs a warning on failure. Callers handle success logging with their own context. - /// - Returns: `true` if the entry was successfully removed. + /// Logs the "Keeping" message when the entry is still needed, and the underlying error on + /// failure. Callers provide their own success context messages and must not warn again. private func removeGitignoreArtifact( _ entry: String, - gitignoreManager: GitignoreManager - ) -> Bool { + gitignoreManager: GitignoreManager, + refCounter: ResourceRefCounter, + excludingScope: String, + excludingPack: String + ) -> RefCountedRemovalResult { + let resource = ResourceRefCounter.Resource.gitignoreEntry(entry) + // Checked separately from `isStillNeeded` — which guards it too, for every caller — so the + // message names the real reason. A core line is retained because mcs owns it, not because + // some other claimant exists, and telling the user otherwise sends them looking for one. + if resource.isProtected { + output.dimmed(" Keeping gitignore entry '\(entry)' — core entry managed by mcs") + return .stillNeeded + } + if refCounter.isStillNeeded( + resource, + excludingScope: excludingScope, + excludingPack: excludingPack + ) { + output.dimmed(" Keeping gitignore entry '\(entry)' — still needed by another scope or pack") + return .stillNeeded + } do { try gitignoreManager.removeEntry(entry) - return true + return .removed } catch { output.warn(" Could not remove gitignore entry '\(entry)': \(error.localizedDescription)") - return false + return .failed } } diff --git a/Sources/mcs/Sync/ResourceRefCounter.swift b/Sources/mcs/Sync/ResourceRefCounter.swift index e9d1f70..afa6903 100644 --- a/Sources/mcs/Sync/ResourceRefCounter.swift +++ b/Sources/mcs/Sync/ResourceRefCounter.swift @@ -1,26 +1,56 @@ import Foundation -/// Determines whether a global resource (brew package or plugin) can be safely -/// removed by checking all projects and the global scope for references. +/// Determines whether a global resource (brew package, plugin, 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. +/// 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 { let environment: Environment let output: CLIOutput let registry: TechPackRegistry + /// Decoded state, read once per counter and shared by every `isStillNeeded` call it serves. + /// + /// One removal pass queries once per artifact, and a pack can declare many — nothing rewrites + /// these files in between, except this type's own stale-entry pruning, which the cache + /// collapses from a warning-and-write per query into one for the whole pass. + private let cache = StateCache() + + private final class StateCache { + var globalState: ProjectState? + var indexData: ProjectIndex.IndexData? + var globalStateLoaded = false + var indexLoaded = false + } + 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)'" + } + } + + /// Resources the tool owns rather than any pack, and that no pack may delete. + /// + /// Reference counting answers "does another *pack* claim this?" — a question that can + /// never protect something no pack ever claimed. `GitignoreManager.coreEntries` are + /// 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 let .gitignoreEntry(entry): GitignoreManager.coreEntries.contains(entry) } } } @@ -28,7 +58,7 @@ struct ResourceRefCounter { /// Check if a resource is still needed by any scope OTHER than the one being removed. /// /// - Parameters: - /// - resource: The brew package or plugin to check. + /// - resource: The brew package, plugin, or gitignore entry to check. /// - scopePath: The scope being removed (project path, `ProjectIndex.globalSentinel`, /// or `ProjectIndex.packRemoveSentinel` when removing a pack entirely). /// - packID: The pack being unconfigured within that scope. @@ -38,22 +68,51 @@ struct ResourceRefCounter { excludingScope scopePath: String, excludingPack packID: String ) -> Bool { - checkGlobalArtifacts(resource, excludingScope: scopePath, excludingPack: packID) + resource.isProtected + || checkGlobalArtifacts(resource, excludingScope: scopePath, excludingPack: packID) || checkProjectIndex(resource, excludingScope: scopePath, excludingPack: packID) } // MARK: - Private + /// Decode `global-state.json` once per counter. `nil` means unreadable, which callers treat + /// as "keep the resource" — the conservative direction. + private func cachedGlobalState() -> ProjectState? { + guard !cache.globalStateLoaded else { return cache.globalState } + cache.globalStateLoaded = true + do { + cache.globalState = try ProjectState(stateFile: environment.globalStateFile) + } catch { + output.warn( + "Could not read global state (\(error.localizedDescription)) " + + "— keeping shared resources as a precaution" + ) + } + return cache.globalState + } + + /// Load `projects.yaml` once per counter. `nil` means unreadable — same conservative rule. + private func cachedIndexData() -> ProjectIndex.IndexData? { + guard !cache.indexLoaded else { return cache.indexData } + cache.indexLoaded = true + do { + cache.indexData = try ProjectIndex(path: environment.projectsIndexFile).load() + } catch { + output.warn( + "Could not read project index (\(error.localizedDescription)) " + + "— keeping shared resources as a precaution" + ) + } + return cache.indexData + } + /// Check if any other pack in global-state.json owns the resource. private func checkGlobalArtifacts( _ resource: Resource, excludingScope scopePath: String, excludingPack packID: String ) -> Bool { - guard let globalState = try? ProjectState(stateFile: environment.globalStateFile) else { - output.warn("Could not read global state — keeping \(resource.displayName) as a precaution") - return true - } + guard let globalState = cachedGlobalState() else { return true } for otherPackID in globalState.configuredPacks { // Skip the pack being removed if we're in the global scope or removing the pack entirely @@ -72,6 +131,8 @@ struct ResourceRefCounter { if artifacts.plugins.contains(where: { PluginRef($0).bareName == refBareName }) { return true } + case let .gitignoreEntry(entry): + if artifacts.gitignoreEntries.contains(entry) { return true } } } @@ -84,18 +145,17 @@ struct ResourceRefCounter { excludingScope scopePath: String, excludingPack packID: String ) -> Bool { - let indexFile = ProjectIndex(path: environment.projectsIndexFile) - guard var indexData = try? indexFile.load() else { - output.warn("Could not read project index — keeping \(resource.displayName) as a precaution") - return true - } + guard var indexData = cachedIndexData() else { return true } let fm = FileManager.default var stalePaths: [String] = [] + var stillNeeded = false - for entry in indexData.projects { - // Skip the scope being removed - if entry.path == scopePath { continue } + search: for entry in indexData.projects { + // Skip the global scope's own entry when that is what is being removed: + // `checkGlobalArtifacts` already covers it by *ownership*, and re-scanning it here + // by *declaration* would keep resources the pack never actually installed. + if entry.path == scopePath, entry.path == ProjectIndex.globalSentinel { continue } // Validate project still exists (skip __global__ — always valid) if entry.path != ProjectIndex.globalSentinel { @@ -105,22 +165,32 @@ struct ResourceRefCounter { } } - // Check each pack in this scope + // Check each pack in this scope. The scope being removed is still scanned — a + // *sibling* pack there is a genuine referent, and skipping the whole entry let a + // shared artifact be deleted out from under a pack that still declares it. for otherPackID in entry.packs { - // When removing a pack entirely, skip that pack in every scope - if scopePath == ProjectIndex.packRemoveSentinel, otherPackID == packID { continue } + // Never count the pack being unconfigured: in the scope it is leaving, and in + // every scope when the pack is being removed outright. + if otherPackID == packID, + entry.path == scopePath || scopePath == ProjectIndex.packRemoveSentinel { + continue + } if packDeclaresResource(packID: otherPackID, resource: resource) { - // Clean up stale entries we found along the way before returning - pruneStaleEntries(stalePaths, in: &indexData, indexFile: indexFile) - return true + stillNeeded = true + break search } } } - // Clean up any stale entries we found - pruneStaleEntries(stalePaths, in: &indexData, indexFile: indexFile) + // Prune stale entries found along the way — one write per counter, matched or not, and + // the pruned copy goes back into the cache so later queries don't repeat the warning. + pruneStaleEntries( + stalePaths, in: &indexData, + indexFile: ProjectIndex(path: environment.projectsIndexFile) + ) + cache.indexData = indexData - return false + return stillNeeded } /// Check if a pack's manifest declares the given resource. @@ -138,6 +208,10 @@ struct ResourceRefCounter { 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)): + if entries.contains(entry) { return true } default: break } diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index 354bd13..a6c0bb4 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -2414,6 +2414,23 @@ struct HookInterpreterLifecycleTests { } } +/// Configure `pack` in the project first, then globally — `filterGloballyBlocked` rejects the +/// reverse order, so this is the only sequence that produces a both-scope pack. +private func configureBothScopes( + bed: LifecycleTestBed, + pack: any TechPack, + registry: TechPackRegistry, + projectExclusions: [String: Set] = [:], + globalExclusions: [String: Set] = [:] +) throws { + try bed.makeConfigurator(registry: registry).configure( + packs: [pack], confirmRemovals: false, excludedComponents: projectExclusions + ) + try bed.makeGlobalSyncConfigurator(registry: registry).configure( + packs: [pack], confirmRemovals: false, excludedComponents: globalExclusions + ) +} + // MARK: - Scope Duplication (Issue #371) /// A pack configured in both the global scope and a project installs its artifacts twice. @@ -2424,23 +2441,6 @@ struct HookInterpreterLifecycleTests { /// so every fixture is built with the real configurators rather than hand-planted JSON. @Suite("Scope duplication check") struct ScopeDuplicationCheckTests { - /// Configure `pack` in the project first, then globally — `filterGloballyBlocked` rejects the - /// reverse order, so this is the only sequence that produces a both-scope pack. - private func configureBothScopes( - bed: LifecycleTestBed, - pack: any TechPack, - registry: TechPackRegistry, - projectExclusions: [String: Set] = [:], - globalExclusions: [String: Set] = [:] - ) throws { - try bed.makeConfigurator(registry: registry).configure( - packs: [pack], confirmRemovals: false, excludedComponents: projectExclusions - ) - try bed.makeGlobalSyncConfigurator(registry: registry).configure( - packs: [pack], confirmRemovals: false, excludedComponents: globalExclusions - ) - } - private func checks( bed: LifecycleTestBed, registry: TechPackRegistry, @@ -2826,3 +2826,114 @@ struct ScopeDuplicationCheckTests { #expect(duplicated.issues > baseline.issues) } } + +// MARK: - Gitignore Reference Counting (Issue #378) + +/// `GitignoreManager` resolves one file for the whole machine, so a gitignore entry is a shared +/// resource like a brew package or a plugin: two scopes hold two claims on one physical line. +/// +/// Both entry points are exercised separately because they orchestrate removal differently: +/// deselection during `mcs sync` runs inside `configure()`, while `mcs pack remove` calls +/// `unconfigurePack` directly and runs none of its install or ensure steps — so on that path a +/// ref-counting miss has nothing after it to put the line back. +@Suite("Gitignore reference counting") +struct GitignoreRefCountTests { + private func ignorePack(id: String, entry: String) -> any TechPack { + MockTechPack( + identifier: id, + displayName: id, + components: [ + ComponentDefinition( + id: "\(id).ignores", + displayName: "ignores", + description: "Gitignore entries", + type: .configuration, + packIdentifier: id, + dependencies: [], + isRequired: true, + installAction: .gitignoreEntries(entries: [entry]) + ), + ] + ) + } + + @Test("Deselecting in the project keeps a line the global scope still claims") + func syncDeselectionKeepsGloballyClaimedEntry() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let pack = ignorePack(id: "ignore-pack", entry: ".mcs-scratch") + let registry = TechPackRegistry(packs: [pack]) + + try configureBothScopes(bed: bed, pack: pack, registry: registry) + + let gitignore = bed.home.appendingPathComponent(".config/git/ignore") + #expect(try String(contentsOf: gitignore, encoding: .utf8).contains(".mcs-scratch")) + + // Deselect in the project only. + try bed.makeConfigurator(registry: registry) + .configure(packs: [], confirmRemovals: false) + + let after = try String(contentsOf: gitignore, encoding: .utf8) + #expect(after.contains(".mcs-scratch"), "Global scope still claims the line") + + let globalClaims = try bed.globalState().artifacts(for: "ignore-pack")?.gitignoreEntries + #expect(globalClaims == [".mcs-scratch"], "Global record is untouched") + + let projectState = try bed.projectState() + #expect(!projectState.configuredPacks.contains("ignore-pack"), "Project claim released") + } + + @Test("Pack removal keeps a line another pack still declares") + func packRemoveKeepsEntryDeclaredByAnotherPack() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let packA = ignorePack(id: "pack-a", entry: ".shared-ignore") + let packB = ignorePack(id: "pack-b", entry: ".shared-ignore") + let registry = TechPackRegistry(packs: [packA, packB]) + + try bed.makeConfigurator(registry: registry) + .configure(packs: [packA, packB], confirmRemovals: false) + + let gitignore = bed.home.appendingPathComponent(".config/git/ignore") + #expect(try String(contentsOf: gitignore, encoding: .utf8).contains(".shared-ignore")) + + // The `mcs pack remove pack-a` shape: `packRemoveSentinel` excludes pack-a in every + // scope, so only pack-b's declaration can keep the line. + var state = try bed.projectState() + bed.makeConfigurator(registry: registry).unconfigurePack( + "pack-a", state: &state, refCountScope: ProjectIndex.packRemoveSentinel + ) + try state.save() + + let after = try String(contentsOf: gitignore, encoding: .utf8) + #expect(after.contains(".shared-ignore"), "pack-b still declares the line") + #expect(!state.configuredPacks.contains("pack-a")) + #expect(state.configuredPacks.contains("pack-b")) + } + + @Test("Pack removal deletes a line no one else claims") + func packRemoveDeletesSoleClaimedEntry() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let pack = ignorePack(id: "solo-pack", entry: ".solo-ignore") + let registry = TechPackRegistry(packs: [pack]) + + try bed.makeConfigurator(registry: registry) + .configure(packs: [pack], confirmRemovals: false) + + let gitignore = bed.home.appendingPathComponent(".config/git/ignore") + #expect(try String(contentsOf: gitignore, encoding: .utf8).contains(".solo-ignore")) + + var state = try bed.projectState() + bed.makeConfigurator(registry: registry).unconfigurePack( + "solo-pack", state: &state, refCountScope: ProjectIndex.packRemoveSentinel + ) + try state.save() + + let after = try String(contentsOf: gitignore, encoding: .utf8) + #expect(!after.contains(".solo-ignore"), "Nothing else claims it — ref counting must not over-keep") + } +} diff --git a/Tests/MCSTests/ResourceRefCounterTests.swift b/Tests/MCSTests/ResourceRefCounterTests.swift index ddbc606..dd39a58 100644 --- a/Tests/MCSTests/ResourceRefCounterTests.swift +++ b/Tests/MCSTests/ResourceRefCounterTests.swift @@ -53,6 +53,20 @@ private func pluginComponent(id: String, pack: String, pluginName: String) -> Co ) } +/// Creates a ComponentDefinition with a gitignore-entries install action. +private func gitignoreComponent(id: String, pack: String, entries: [String]) -> ComponentDefinition { + ComponentDefinition( + id: id, + displayName: "Gitignore", + description: "Gitignore: \(entries.joined(separator: ", "))", + type: .configuration, + packIdentifier: pack, + dependencies: [], + isRequired: true, + installAction: .gitignoreEntries(entries: entries) + ) +} + struct ResourceRefCounterTests { private func makeTmpHome() throws -> URL { let dir = FileManager.default.temporaryDirectory @@ -609,4 +623,281 @@ struct ResourceRefCounterTests { #expect(!result, "No other scope references it — safe to remove") } + + // MARK: - Gitignore entries + + // `GitignoreManager` resolves one file for the whole machine, so a pack installed in two + // scopes holds two claims on a single physical line. + + @Test("Same pack globally and per-project keeps gitignore entry when removing from project") + func dualScopeKeepGitignoreOnProjectRemoval() throws { + let home = try makeTmpHome() + defer { try? FileManager.default.removeItem(at: home) } + + let env = Environment(home: home) + + let projectA = home.appendingPathComponent("project-a") + try FileManager.default.createDirectory(at: projectA, withIntermediateDirectories: true) + try writeProjectState(projectRoot: projectA, packs: ["pack-z"]) + + // Global state: pack-z owns ".mcs-scratch" + try writeGlobalState(home: home, packs: [ + ("pack-z", PackArtifactRecord(gitignoreEntries: [".mcs-scratch"])), + ]) + + try writeIndex(home: home, entries: [ + (ProjectIndex.globalSentinel, ["pack-z"]), + (projectA.path, ["pack-z"]), + ]) + + let registry = TechPackRegistry(packs: [ + StubTechPack( + identifier: "pack-z", + displayName: "Pack Z", + description: "Test", + components: [ + gitignoreComponent(id: "z.ignores", pack: "pack-z", entries: [".mcs-scratch"]), + ] + ), + ]) + + let counter = ResourceRefCounter( + environment: env, + output: CLIOutput(), + registry: registry + ) + + let result = counter.isStillNeeded( + .gitignoreEntry(".mcs-scratch"), + excludingScope: projectA.path, + excludingPack: "pack-z" + ) + + #expect(result, "Should be kept — global scope still claims .mcs-scratch via pack-z") + } + + @Test("Two different packs sharing a gitignore entry → kept") + func differentPacksSameGitignoreEntry() throws { + let home = try makeTmpHome() + defer { try? FileManager.default.removeItem(at: home) } + + let env = Environment(home: home) + + let projectA = home.appendingPathComponent("project-a") + try FileManager.default.createDirectory(at: projectA, withIntermediateDirectories: true) + try writeProjectState(projectRoot: projectA, packs: ["pack-b"]) + + try writeGlobalState(home: home, packs: [ + ("pack-a", PackArtifactRecord(gitignoreEntries: [".env"])), + ]) + + // pack-b in the project declares the same entry + try writeIndex(home: home, entries: [ + (ProjectIndex.globalSentinel, ["pack-a"]), + (projectA.path, ["pack-b"]), + ]) + + let registry = TechPackRegistry(packs: [ + StubTechPack( + identifier: "pack-a", + displayName: "Pack A", + description: "Test", + components: [gitignoreComponent(id: "a.ignores", pack: "pack-a", entries: [".env"])] + ), + StubTechPack( + identifier: "pack-b", + displayName: "Pack B", + description: "Test", + components: [gitignoreComponent(id: "b.ignores", pack: "pack-b", entries: [".env"])] + ), + ]) + + let counter = ResourceRefCounter( + environment: env, + output: CLIOutput(), + registry: registry + ) + + // This is the `mcs pack remove` shape: packRemoveSentinel excludes pack-a everywhere, + // so only pack-b's declaration can keep the line. + let result = counter.isStillNeeded( + .gitignoreEntry(".env"), + excludingScope: ProjectIndex.packRemoveSentinel, + excludingPack: "pack-a" + ) + + #expect(result, "Should be kept — pack-b in project-a also declares .env") + } + + @Test("Gitignore entry claimed by no other scope is safe to remove") + func soleClaimantGitignoreEntry() throws { + let home = try makeTmpHome() + defer { try? FileManager.default.removeItem(at: home) } + + let env = Environment(home: home) + + try writeGlobalState(home: home, packs: [ + ("pack-a", PackArtifactRecord(gitignoreEntries: [".mcs-scratch"])), + ]) + + try writeIndex(home: home, entries: [ + (ProjectIndex.globalSentinel, ["pack-a"]), + ]) + + let registry = TechPackRegistry(packs: [ + StubTechPack( + identifier: "pack-a", + displayName: "Pack A", + description: "Test", + components: [ + gitignoreComponent(id: "a.ignores", pack: "pack-a", entries: [".mcs-scratch"]), + ] + ), + ]) + + let counter = ResourceRefCounter( + environment: env, + output: CLIOutput(), + registry: registry + ) + + let result = counter.isStillNeeded( + .gitignoreEntry(".mcs-scratch"), + excludingScope: ProjectIndex.globalSentinel, + excludingPack: "pack-a" + ) + + #expect(!result, "No other scope claims it — safe to remove") + } + + @Test("A sibling pack in the same project keeps a shared gitignore entry") + func siblingPackInSameProjectKeepsEntry() throws { + let home = try makeTmpHome() + defer { try? FileManager.default.removeItem(at: home) } + + let env = Environment(home: home) + + // One project, two packs. `installAndReconcileArtifacts` installs and reconciles per + // pack in sequence, so pack-b can install the shared line and pack-a can then drop its + // now-stale claim in the same run — with nothing left to re-add it afterwards. + let projectA = home.appendingPathComponent("project-a") + try FileManager.default.createDirectory(at: projectA, withIntermediateDirectories: true) + try writeProjectState(projectRoot: projectA, packs: ["pack-a", "pack-b"]) + + try writeIndex(home: home, entries: [ + (projectA.path, ["pack-a", "pack-b"]), + ]) + + let registry = TechPackRegistry(packs: [ + StubTechPack( + identifier: "pack-a", + displayName: "Pack A", + description: "Test", + components: [] + ), + StubTechPack( + identifier: "pack-b", + displayName: "Pack B", + description: "Test", + components: [gitignoreComponent(id: "b.ignores", pack: "pack-b", entries: [".shared"])] + ), + ]) + + let counter = ResourceRefCounter( + environment: env, + output: CLIOutput(), + registry: registry + ) + + let result = counter.isStillNeeded( + .gitignoreEntry(".shared"), + excludingScope: projectA.path, + excludingPack: "pack-a" + ) + + #expect(result, "Should be kept — pack-b in the same project still declares .shared") + } + + @Test("The pack being unconfigured is not its own referent in its own scope") + func removedPackDoesNotCountItselfInItsOwnScope() throws { + let home = try makeTmpHome() + defer { try? FileManager.default.removeItem(at: home) } + + let env = Environment(home: home) + + let projectA = home.appendingPathComponent("project-a") + try FileManager.default.createDirectory(at: projectA, withIntermediateDirectories: true) + try writeProjectState(projectRoot: projectA, packs: ["pack-a"]) + + try writeIndex(home: home, entries: [ + (projectA.path, ["pack-a"]), + ]) + + let registry = TechPackRegistry(packs: [ + StubTechPack( + identifier: "pack-a", + displayName: "Pack A", + description: "Test", + components: [gitignoreComponent(id: "a.ignores", pack: "pack-a", entries: [".solo"])] + ), + ]) + + let counter = ResourceRefCounter( + environment: env, + output: CLIOutput(), + registry: registry + ) + + // Now that the removing scope is scanned rather than skipped wholesale, the pack being + // removed must still be excluded there — otherwise nothing would ever be removable. + let result = counter.isStillNeeded( + .gitignoreEntry(".solo"), + excludingScope: projectA.path, + excludingPack: "pack-a" + ) + + #expect(!result, "Only the pack being removed declares it — safe to remove") + } + + @Test("Core gitignore entries are never removable, even with no claimant") + func coreGitignoreEntryAlwaysKept() throws { + let home = try makeTmpHome() + defer { try? FileManager.default.removeItem(at: home) } + + let env = Environment(home: home) + + // A pack that declares a core line as its own. No other scope exists at all, so both + // ref-count tiers report "not needed" — only the core guard keeps the line. + try writeGlobalState(home: home, packs: [ + ("pack-a", PackArtifactRecord(gitignoreEntries: ["*.local.*"])), + ]) + try writeIndex(home: home, entries: [ + (ProjectIndex.globalSentinel, ["pack-a"]), + ]) + + let registry = TechPackRegistry(packs: [ + StubTechPack( + identifier: "pack-a", + displayName: "Pack A", + description: "Test", + components: [gitignoreComponent(id: "a.ignores", pack: "pack-a", entries: ["*.local.*"])] + ), + ]) + + let counter = ResourceRefCounter( + environment: env, + output: CLIOutput(), + registry: registry + ) + + #expect(GitignoreManager.coreEntries.contains("*.local.*"), "Precondition: a core entry") + + let result = counter.isStillNeeded( + .gitignoreEntry("*.local.*"), + excludingScope: ProjectIndex.packRemoveSentinel, + excludingPack: "pack-a" + ) + + #expect(result, "Core lines are mcs's own — a pack must not be able to delete one") + } }