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

[bguidolim]

Summary

A pack synced into both a project and the global scope records the same gitignore lines in two
places, but git only has one such file per machine. Removing the pack from either scope deleted
those lines outright, so files that were previously ignored started appearing in git status
across every repo on the machine. mcs sync papered over this on the next run of the surviving
scope; mcs pack remove never re-runs the steps that would restore them, so there the loss was
permanent.

Making gitignore scope-aware is the better end state and is worth a follow-up, but its migration
has to decide whether a line already in the global file is still claimed elsewhere — which is the
primitive this PR adds.

Changes

• A gitignore line is now removed only once no other scope or pack still claims it.
• mcs's own core ignore lines are protected outright: a pack may declare one, but can no longer
  delete it. No pack record owns them, so reference counting alone could never have vouched for them.
• Removing a pack that declares several entries now reads the shared state files once for the whole
  pass instead of once per entry. That also stops the duplicate "project not found" warnings and the
  repeated index rewrites the per-entry path was producing.
• mcs doctor --fix on a scope-duplicated pack no longer re-adds lines it just deleted; the removal
  it calls is correct on its own now.

Test plan

• swift test passes locally
• swiftformat --lint . and swiftlint pass without violations
• Affected commands verified with a real pack (e.g. mcs sync, mcs doctor)

To verify by hand with a pack declaring gitignoreEntries:

1. mcs sync --pack <pack> in a project, then mcs sync --global --pack <pack> → the entries appear
   once in the global gitignore.
2. Re-run mcs sync in the project and deselect the pack → expect the lines to survive, with
   Keeping gitignore entry '<entry>' — still needed by another scope in the output.
3. mcs doctor → expect no missing entries failure.
4. mcs pack remove <pack> with no other pack declaring those entries → expect the lines to go.

Checklist for engine changes

• Any fix() implementation does cleanup/migration only — never installs or registers resources
• Integration tests updated for new features (LifecycleIntegrationTests or DoctorRunnerIntegrationTests)
• Docs updated if behavior changed (CLAUDE.md, docs/, techpack.yaml schema in ExternalPackManifest.swift)

Closes #378

https://claude.ai/code/session_018DxgSbTvRtshXfmVZp1cKo

[Copilot]

🟡 Changes recommended

Two moderate reference-counting defects can leave gitignore state incorrect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds reference counting for globally shared gitignore entries.

Changes:

• Protects entries claimed by other scopes or packs.
• Preserves core mcs ignore entries.
• Caches shared state during removal.
• Removes the doctor restoration workaround.
• Adds documentation and regression tests.

File summaries

File	Review
Tests/MCSTests/ResourceRefCounterTests.swift	Adds unit coverage for gitignore reference decisions.
Tests/MCSTests/LifecycleIntegrationTests.swift	Adds lifecycle coverage for shared-entry removal.
Sources/mcs/Sync/ResourceRefCounter.swift	Adds gitignore reference counting and caching. Moderate (1 vote): skipping the entire current project can miss another pack’s claim and incorrectly delete a shared entry.
Sources/mcs/Sync/Configurator.swift	Applies reference counting during cleanup. Moderate (2 votes): stale state across multiple removals can leave orphaned entries. Nit (1 vote): the message should say “another scope or pack.”
Sources/mcs/Doctor/ScopeDuplicationCheck.swift	Removes obsolete restoration logic.
CLAUDE.md	Documents shared gitignore handling.

Review details

Suppressed comments (1)

Sources/mcs/Sync/Configurator.swift:1209

• This can also return .stillNeeded because another pack in the same scope declares the entry (for example, the added pack-removal test does exactly that), so “another scope” is inaccurate. Report “another scope or pack” to match the condition being checked.

            output.dimmed("  Keeping gitignore entry '\(entry)' — still needed by another scope")

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bguidolim]

Thanks — both findings verified against the code, and both were real.

Same-project sibling (ResourceRefCounter) — fixed in 53699fa. checkProjectIndex now scans the removing scope's index entry and excludes only the pack being unconfigured, instead of skipping the entry wholesale. The global scope's own entry is still skipped, since checkGlobalArtifacts already covers it by ownership and re-scanning it by declaration would keep resources a pack never installed.

Worth noting the gap was not gitignore-specific: on the same path, removing a pack from a project could already uninstall a brew package that a sibling pack in that project still declared. That is fixed too.

Two regression tests were added. One asserts the sibling keeps the entry — it fails against the pre-fix source, verified by running it against the previous commit. The other asserts the pack being removed is not its own referent, which is the failure mode the fix introduces if the pack-id exclusion is ever dropped: without it nothing would be removable and every "keeps the line" test would still pass.

Wording nit — fixed. Applied to the brew and plugin messages as well, which carried the same inaccuracy and would otherwise have drifted apart.

Reciprocal stale reference during multi-pack removal — confirmed, and filed as #385 rather than fixed here. The premise holds: the removal loop does not save between packs, so each unconfigurePack reads a snapshot that still lists its siblings and both release their claim. But this lives in the shared primitive and has applied to brew packages and plugins since reference counting landed; this PR inherits it by extending the primitive to a third resource kind rather than introducing it. It also fails in the safe direction — a resource is kept rather than deleted, where the bug this PR fixes was deletion of a line still in use. Fixing it properly means giving the counter access to the run's uncommitted intent for all three resource kinds, which is a change to the primitive's contract and wants its own review.

[Copilot]

🟡 Changes recommended

Multi-pack removal can permanently retain an unclaimed gitignore entry, and the associated regression path is not covered.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

Sources/mcs/Sync/Configurator.swift:1209

• For a protected core entry with no other claimant, this message says another scope or pack needs it even though isStillNeeded returned true solely because isProtected did. Emit a distinct “core entry managed by mcs” message before the reference-count check so users are told the actual reason the line was retained.

            output.dimmed("  Keeping gitignore entry '\(entry)' — still needed by another scope or pack")

Sources/mcs/Sync/ResourceRefCounter.swift:178

• Scanning the removing scope creates the same stale-peer problem for project syncs: if two packs in one project share a resource and are deselected together, each unconfigure call reads the unchanged index and counts the other pack, so neither removes the now-unclaimed resource. Exclude all packs pending removal in this convergence pass, while still allowing genuinely surviving siblings to count.

                if otherPackID == packID,
                   entry.path == scopePath || scopePath == ProjectIndex.packRemoveSentinel {
                    continue
                }
                if packDeclaresResource(packID: otherPackID, resource: resource) {

Tests/MCSTests/LifecycleIntegrationTests.swift:2906

• This bypasses the actual mcs pack remove orchestration: it invokes one configurator directly and never exercises the global-plus-project federated loop or its final index update. The acceptance criterion about releasing the line only after the last scope therefore remains uncovered. Configure the same pack in both scopes and drive the command-level removal path (or an extracted federated-removal helper).

        var state = try bed.projectState()
        bed.makeConfigurator(registry: registry).unconfigurePack(
            "pack-a", state: &state, refCountScope: ProjectIndex.packRemoveSentinel
        )

Tests/MCSTests/LifecycleIntegrationTests.swift:2837

• This newly added suite documentation describes the old bug as current behavior: after this change, deselection should not strip the line for a later sync to restore it. Rephrase this as historical context or explain only that the two cleanup paths use different orchestration.

/// Both entry points are exercised separately — `mcs sync` deselection restores stripped lines on
/// the next sync of the surviving scope, but `mcs pack remove` calls `unconfigurePack` directly
/// and runs no step that would add anything back, so a miss there is permanent.

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Balanced