Deselecting a pack deletes user-edited files without checking the recorded hash

[bguidolim]

Summary

mcs sync removes file artifacts by calling FileManager.removeItem on the recorded path without checking whether the file still matches what mcs installed. If a user edits an mcs-installed skill, agent, hook script, or command and later deselects the pack, their edits are deleted with no warning.

This is silent data loss, and it affects exactly the artifacts users are most likely to hand-edit after installation.

Reproduction

1. mcs sync with a pack that ships a skill.
2. Edit the installed skill's markdown to taste.
3. Re-run mcs sync and deselect that pack.
4. The edited file is gone. Output shows only Removed: <path>.

Root cause

Both SyncStrategy implementations delete unconditionally:

• Sources/mcs/Sync/ProjectSyncStrategy.swift — removeFileArtifact(relativePath:output:)
• Sources/mcs/Sync/GlobalSyncStrategy.swift — same method

Each resolves the path via PathContainment.safePath, checks fileExists, then calls try fm.removeItem(at: fullPath). There is no content comparison.

The information needed is already recorded and already used elsewhere. PackArtifactRecord.fileHashes stores a SHA-256 per installed file, and FileContentCheck uses it to report drift in mcs doctor. Removal simply never consults it — Configurator.unconfigurePack iterates artifacts.files, deletes each one, and only afterwards purges fileHashes for the deleted entries.

The structural blocker is the protocol signature: removeFileArtifact(relativePath:output:) receives only a relative path, so a strategy has no access to the recorded hash even though the caller holds it.

Proposed fix

1. Pass the expected hash to the removal call — widen the SyncStrategy method so the strategy can compare before deleting.
2. Compare before deleting. Hash matches the record → remove. Hash differs → preserve the file and warn, naming the path and the reason it was kept.
3. Track files individually, never a directory as one artifact. A skill is a directory; if the user added a file inside it, directory-level removal takes that too. Remove tracked files individually and remove the directory only once it is empty.
4. lstat before mutation and refuse to follow a symlink substituted for a tracked path.
5. Consider recording an origin (created vs adopted) on the artifact record so a future adopt-an-existing-file path cannot delete something mcs never installed.

Note on state schema

⚠️ If this adds a field to the state file, be careful which type it goes on. PackArtifactRecord has a hand-written init(from:) using decodeIfPresent, so adding a field there is non-breaking. StateStorage does not — it relies on synthesized Codable, whose decoder does not consult property defaults for missing keys, so its non-optional defaulted properties are effectively required. Adding a non-optional field to StateStorage breaks every existing state file. Make it Optional or give StateStorage the same hand-written decoder, and cover it with a decode test against a pre-field state file.

Acceptance criteria

• Integration test in LifecycleIntegrationTests: install a skill, modify it on disk, deselect the pack, assert the file still exists with the user's content and that drift is reported. This test must fail on main today.
• Mirror test: an unmodified file is removed, so the fix cannot regress into "never delete anything".
• Directory case: a user-added file inside an mcs-installed skill directory survives deselection.
• Backward-compat decode test if the state schema changes.

Related question to answer while in this code

ExternalPackLoader throws on an incompatible minMCSVersion, but loadAll — the sync path — catches and downgrades to warn-and-skip. A version-skipped pack is then absent from the list handed to Configurator while ProjectState still lists it in configuredPacks with a populated PackArtifactRecord — the same shape as a user deselection.

Verify whether the removal path fires in that case. If it does, an older mcs meeting a newer pack silently tears down that pack's artifacts, which is a second data-loss path through the same code and should be fixed here.