ISSUE-378: Reference-count gitignore entries before removing them

CLAUDE.md

@@ -146,7 +146,7 @@ mcs config set <key> <value>     # 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 (`<pack-id>/`) for hooks/commands/agents/generic, or directory name suffix (`-<pack-id>`) for skills (which require flat one-level directories). First pack keeps the clean name; subsequent packs get namespaced. Skill renames emit a warning

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) {

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<String> = []
-            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
         }
     }