From 4548eeca8ac9a1a91d5aba9cc7a139834afc9544 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Thu, 26 Mar 2026 13:11:43 +0100 Subject: [PATCH 1/2] Protect pre-existing user files from copyPackFile overwrite (#300) - Add filesystem-aware collision resolver: hooks always namespace into / subdirectories; other types namespace only when a pre-existing untracked file is detected at the destination - Centralize tracked-files computation and namespacing rule into shared helpers (PackArtifactRecord.allTrackedFiles, namespacedDestination) - Add unit tests (HookAlwaysNamespaceTests, UserFileConflictTests) and integration tests (UserFileProtectionTests) covering all scenarios --- Sources/mcs/Core/ProjectState.swift | 11 + Sources/mcs/Doctor/DoctorRunner.swift | 10 +- Sources/mcs/Install/Configurator.swift | 13 +- .../DestinationCollisionResolver.swift | 148 ++++++-- Sources/mcs/Install/GlobalSyncStrategy.swift | 6 + Sources/mcs/Install/ProjectSyncStrategy.swift | 6 + Sources/mcs/Install/SyncStrategy.swift | 11 + .../DestinationCollisionResolverTests.swift | 324 ++++++++++++++---- Tests/MCSTests/GlobalConfiguratorTests.swift | 4 +- .../MCSTests/LifecycleIntegrationTests.swift | 226 ++++++++++-- Tests/MCSTests/ProjectConfiguratorTests.swift | 6 +- 11 files changed, 652 insertions(+), 113 deletions(-) diff --git a/Sources/mcs/Core/ProjectState.swift b/Sources/mcs/Core/ProjectState.swift index 9b0f6a4..022ccbe 100644 --- a/Sources/mcs/Core/ProjectState.swift +++ b/Sources/mcs/Core/ProjectState.swift @@ -35,6 +35,11 @@ struct PackArtifactRecord: Codable, Equatable { && gitignoreEntries.isEmpty && fileHashes.isEmpty } + /// Union of all tracked file paths across a collection of artifact records. + static func allTrackedFiles(from records: some Collection) -> Set { + Set(records.flatMap(\.files)) + } + /// Custom decoder for backward compatibility — existing JSON files may lack /// newer keys (brewPackages, plugins, gitignoreEntries, fileHashes). init(from decoder: Decoder) throws { @@ -162,6 +167,12 @@ struct ProjectState { storage.mcsVersion } + /// All file paths tracked across all configured packs. + /// Used by `DestinationCollisionResolver` to distinguish mcs-managed files from user files. + var allTrackedFiles: Set { + PackArtifactRecord.allTrackedFiles(from: storage.packArtifacts.values) + } + // MARK: - Pack Artifacts /// Get the artifact record for a pack, if any. diff --git a/Sources/mcs/Doctor/DoctorRunner.swift b/Sources/mcs/Doctor/DoctorRunner.swift index 7471249..7a82c79 100644 --- a/Sources/mcs/Doctor/DoctorRunner.swift +++ b/Sources/mcs/Doctor/DoctorRunner.swift @@ -31,6 +31,14 @@ struct DoctorRunner { let excludedComponentIDs: Set let label: String let artifactsByPack: [String: PackArtifactRecord] + + func makeCollisionContext(environment: Environment) -> (any CollisionFilesystemContext)? { + let trackedFiles = PackArtifactRecord.allTrackedFiles(from: artifactsByPack.values) + if let root = effectiveProjectRoot { + return ProjectCollisionContext(projectPath: root, trackedFiles: trackedFiles) + } + return GlobalCollisionContext(environment: environment, trackedFiles: trackedFiles) + } } let environment: Environment @@ -149,7 +157,7 @@ struct DoctorRunner { let scopePacks = DestinationCollisionResolver.resolveCollisions( packs: availablePacks.filter { scope.packIDs.contains($0.identifier) }, - output: output + output: output, filesystemContext: scope.makeCollisionContext(environment: env) ) for pack in scopePacks { diff --git a/Sources/mcs/Install/Configurator.swift b/Sources/mcs/Install/Configurator.swift index a9d19ca..db6e5e1 100644 --- a/Sources/mcs/Install/Configurator.swift +++ b/Sources/mcs/Install/Configurator.swift @@ -107,8 +107,11 @@ struct Configurator { /// Compute and display what `configure` would do, without making any changes. func dryRun(packs: [any TechPack]) throws { - let packs = DestinationCollisionResolver.resolveCollisions(packs: packs, output: output) let state = try ProjectState(stateFile: scope.stateFile) + let fsContext = strategy.makeCollisionContext(trackedFiles: state.allTrackedFiles) + let packs = DestinationCollisionResolver.resolveCollisions( + packs: packs, output: output, filesystemContext: fsContext + ) let headerLabel = scope.isGlobalScope ? "Plan (Global)" : "Plan" ConfiguratorSupport.dryRunSummary( packs: packs, @@ -133,10 +136,12 @@ struct Configurator { confirmRemovals: Bool = true, excludedComponents: [String: Set] = [:] ) throws { - let packs = DestinationCollisionResolver.resolveCollisions(packs: packs, output: output) - let selectedIDs = Set(packs.map(\.identifier)) - var state = try ProjectState(stateFile: scope.stateFile) + let fsContext = strategy.makeCollisionContext(trackedFiles: state.allTrackedFiles) + let packs = DestinationCollisionResolver.resolveCollisions( + packs: packs, output: output, filesystemContext: fsContext + ) + let selectedIDs = Set(packs.map(\.identifier)) let previousIDs = state.configuredPacks let removals = previousIDs.subtracting(selectedIDs) diff --git a/Sources/mcs/Install/DestinationCollisionResolver.swift b/Sources/mcs/Install/DestinationCollisionResolver.swift index dc67427..e08d2e3 100644 --- a/Sources/mcs/Install/DestinationCollisionResolver.swift +++ b/Sources/mcs/Install/DestinationCollisionResolver.swift @@ -1,25 +1,87 @@ import Foundation -/// Detects cross-pack `copyPackFile` destination collisions and applies conditional namespacing. +// MARK: - Filesystem Context + +/// Provides filesystem awareness to the collision resolver for detecting +/// pre-existing user files at `copyPackFile` destinations. +protocol CollisionFilesystemContext { + /// Whether a file or directory exists at the resolved destination path. + func fileExists(destination: String, fileType: CopyFileType) -> Bool + /// Whether the destination is tracked by any pack in the current project state. + func isTrackedByPack(destination: String, fileType: CopyFileType) -> Bool +} + +/// Project-scoped filesystem context — resolves paths relative to `/.claude/`. +struct ProjectCollisionContext: CollisionFilesystemContext { + let projectPath: URL + let trackedFiles: Set + + func fileExists(destination: String, fileType: CopyFileType) -> Bool { + let baseDir = fileType.projectBaseDirectory(projectPath: projectPath) + let destURL = baseDir.appendingPathComponent(destination) + return FileManager.default.fileExists(atPath: destURL.path) + } + + func isTrackedByPack(destination: String, fileType: CopyFileType) -> Bool { + let baseDir = fileType.projectBaseDirectory(projectPath: projectPath) + let destURL = baseDir.appendingPathComponent(destination) + let relPath = PathContainment.relativePath(of: destURL.path, within: projectPath.path) + return trackedFiles.contains(relPath) + } +} + +/// Global-scoped filesystem context — resolves paths relative to `~/.claude/`. +struct GlobalCollisionContext: CollisionFilesystemContext { + let environment: Environment + let trackedFiles: Set + + func fileExists(destination: String, fileType: CopyFileType) -> Bool { + let destURL = fileType.destinationURL(in: environment, destination: destination) + return FileManager.default.fileExists(atPath: destURL.path) + } + + func isTrackedByPack(destination: String, fileType: CopyFileType) -> Bool { + let destURL = fileType.destinationURL(in: environment, destination: destination) + let relPath = PathContainment.relativePath( + of: destURL.path, within: environment.claudeDirectory.path + ) + return trackedFiles.contains(relPath) + } +} + +// MARK: - Collision Resolver + +/// Detects `copyPackFile` destination collisions and applies conditional namespacing. /// -/// When no collision exists, destinations remain flat (e.g., `.claude/commands/pr.md`). -/// When two+ packs define the same `(destination, fileType)`: -/// - **Non-skill types**: destinations are prefixed with `/` subdirectory -/// - **Skills**: destinations are suffixed with `-` (first pack keeps the clean name) -/// because Claude Code requires flat one-level directories for skill discovery. +/// Three sources of collision are handled (when `filesystemContext` is provided): +/// 1. **Hooks always namespace** into `/` subdirectories, protecting user hooks. +/// 2. **Cross-pack collisions**: two+ packs targeting the same `(destination, fileType)`. +/// 3. **User-file conflicts**: a pack targets a path occupied by a file not tracked by mcs. +/// +/// Without `filesystemContext`, only cross-pack collisions are resolved (backward compat). enum DestinationCollisionResolver { - /// Returns a new pack array with destinations namespaced only for conflicting files. - /// Emits warnings for skill collisions via `output`. - static func resolveCollisions(packs: [any TechPack], output: CLIOutput) -> [any TechPack] { - // Phase 1: Scan all packs for copyPackFile destinations - // Key: (destination, fileType) → [(packIndex, componentIndex)] + /// Returns a new pack array with destinations namespaced where needed. + static func resolveCollisions( + packs: [any TechPack], + output: CLIOutput, + filesystemContext: (any CollisionFilesystemContext)? = nil + ) -> [any TechPack] { + var packComponentOverrides: [Int: [Int: String]] = [:] var collisionMap: [DestinationKey: [CollisionEntry]] = [:] + // Phase 1: Build collision map and always-namespace hooks (single pass). for (packIndex, pack) in packs.enumerated() { for (componentIndex, component) in pack.components.enumerated() { guard case let .copyPackFile(_, destination, fileType) = component.installAction else { continue } + + // Hooks always namespace into / when filesystem context is available. + if filesystemContext != nil, fileType == .hook { + packComponentOverrides[packIndex, default: [:]][componentIndex] = + namespacedDestination(destination: destination, packIdentifier: pack.identifier, fileType: fileType) + } + let key = DestinationKey(destination: destination, fileType: fileType) collisionMap[key, default: []].append( CollisionEntry(packIndex: packIndex, componentIndex: componentIndex) @@ -27,9 +89,8 @@ enum DestinationCollisionResolver { } } - // Phase 2: Identify actual collisions (2+ distinct pack indices) - var packComponentOverrides: [Int: [Int: String]] = [:] // packIndex → (componentIndex → newDestination) - + // Phase 1a: Apply cross-pack collision namespacing (2+ distinct packs). + // Guards skip entries already resolved above. for (key, entries) in collisionMap { let distinctPackIndices = Set(entries.map(\.packIndex)) guard distinctPackIndices.count >= 2 else { continue } @@ -46,7 +107,36 @@ enum DestinationCollisionResolver { } } - // Phase 3: Build result — wrap only packs that have overrides + // Phase 1b: User-file conflict detection. + // For non-hook entries not yet resolved, check if the destination is occupied + // by a file not tracked by any pack. + if let ctx = filesystemContext { + for (packIndex, pack) in packs.enumerated() { + for (componentIndex, component) in pack.components.enumerated() { + // Skip if already resolved by Phase 0 or 1a + if packComponentOverrides[packIndex]?[componentIndex] != nil { continue } + + guard case let .copyPackFile(_, destination, fileType) = component.installAction else { + continue + } + + if ctx.fileExists(destination: destination, fileType: fileType), + !ctx.isTrackedByPack(destination: destination, fileType: fileType) { + let namespaced = namespacedDestination( + destination: destination, packIdentifier: pack.identifier, fileType: fileType + ) + packComponentOverrides[packIndex, default: [:]][componentIndex] = namespaced + output.warn( + "Pre-existing file at '\(fileType.subdirectory)\(destination)' is not managed by mcs" + + " \u{2014} pack '\(pack.identifier)' will install to " + + "'\(fileType.subdirectory)\(namespaced)' instead" + ) + } + } + } + } + + // Phase 2: Build result — wrap only packs that have overrides guard !packComponentOverrides.isEmpty else { return packs } return packs.enumerated().map { packIndex, pack in @@ -78,22 +168,37 @@ enum DestinationCollisionResolver { // MARK: - Private - /// For non-skill types, prefix all colliding entries with `/`. + /// Computes the namespaced destination for a given file type. + /// Skills get a `-` suffix; all other types get a `/` subdirectory prefix. + private static func namespacedDestination( + destination: String, packIdentifier: String, fileType: CopyFileType + ) -> String { + if fileType == .skill { + "\(destination)-\(packIdentifier)" + } else { + "\(packIdentifier)/\(destination)" + } + } + + /// For non-skill types, prefix colliding entries with `/`. + /// Skips entries already resolved by an earlier phase. private static func applySubdirectoryPrefix( entries: [CollisionEntry], packs: [any TechPack], overrides: inout [Int: [Int: String]] ) { for entry in entries { + guard overrides[entry.packIndex]?[entry.componentIndex] == nil else { continue } let pack = packs[entry.packIndex] let component = pack.components[entry.componentIndex] - guard case let .copyPackFile(_, destination, _) = component.installAction else { continue } - let namespaced = "\(pack.identifier)/\(destination)" - overrides[entry.packIndex, default: [:]][entry.componentIndex] = namespaced + guard case let .copyPackFile(_, destination, fileType) = component.installAction else { continue } + overrides[entry.packIndex, default: [:]][entry.componentIndex] = + namespacedDestination(destination: destination, packIdentifier: pack.identifier, fileType: fileType) } } /// For skills, the first pack keeps the clean name; subsequent packs get `-` suffix. + /// Skips entries already resolved by an earlier phase. private static func applySkillSuffix( entries: [CollisionEntry], destination: String, @@ -105,8 +210,11 @@ enum DestinationCollisionResolver { let firstPackName = packs[firstPackIndex].identifier for entry in entries where entry.packIndex != firstPackIndex { + guard overrides[entry.packIndex]?[entry.componentIndex] == nil else { continue } let pack = packs[entry.packIndex] - let suffixed = "\(destination)-\(pack.identifier)" + let suffixed = namespacedDestination( + destination: destination, packIdentifier: pack.identifier, fileType: .skill + ) overrides[entry.packIndex, default: [:]][entry.componentIndex] = suffixed output.warn( "Skill '\(destination)' from pack '\(pack.identifier)' renamed to " + diff --git a/Sources/mcs/Install/GlobalSyncStrategy.swift b/Sources/mcs/Install/GlobalSyncStrategy.swift index 56f3e0a..274c287 100644 --- a/Sources/mcs/Install/GlobalSyncStrategy.swift +++ b/Sources/mcs/Install/GlobalSyncStrategy.swift @@ -30,6 +30,12 @@ struct GlobalSyncStrategy: SyncStrategy { ) } + // MARK: - Collision Context + + func makeCollisionContext(trackedFiles: Set) -> (any CollisionFilesystemContext)? { + GlobalCollisionContext(environment: environment, trackedFiles: trackedFiles) + } + // MARK: - Artifact Installation func installArtifacts( diff --git a/Sources/mcs/Install/ProjectSyncStrategy.swift b/Sources/mcs/Install/ProjectSyncStrategy.swift index 2a85b0f..0809fd8 100644 --- a/Sources/mcs/Install/ProjectSyncStrategy.swift +++ b/Sources/mcs/Install/ProjectSyncStrategy.swift @@ -36,6 +36,12 @@ struct ProjectSyncStrategy: SyncStrategy { ) } + // MARK: - Collision Context + + func makeCollisionContext(trackedFiles: Set) -> (any CollisionFilesystemContext)? { + ProjectCollisionContext(projectPath: projectPath, trackedFiles: trackedFiles) + } + // MARK: - Artifact Installation func installArtifacts( diff --git a/Sources/mcs/Install/SyncStrategy.swift b/Sources/mcs/Install/SyncStrategy.swift index 1188dd5..7dc39cd 100644 --- a/Sources/mcs/Install/SyncStrategy.swift +++ b/Sources/mcs/Install/SyncStrategy.swift @@ -70,6 +70,12 @@ protocol SyncStrategy { output: CLIOutput ) throws -> Set? + /// Build a filesystem context for collision resolution. + /// + /// Returns a `CollisionFilesystemContext` that enables the resolver to detect + /// pre-existing user files at `copyPackFile` destinations. + func makeCollisionContext(trackedFiles: Set) -> (any CollisionFilesystemContext)? + /// Derive the relative file path that artifact tracking records for a `copyPackFile` component. /// /// Global scope computes relative to `~/.claude/`. @@ -94,6 +100,11 @@ protocol SyncStrategy { // MARK: - Default Implementations extension SyncStrategy { + /// Default: no filesystem context (backward compat — cross-pack collisions only). + func makeCollisionContext(trackedFiles _: Set) -> (any CollisionFilesystemContext)? { + nil + } + /// Default removal summary — prints all non-empty artifact fields. /// /// Uses `scope.claudeFilePath.lastPathComponent` for template section labels. diff --git a/Tests/MCSTests/DestinationCollisionResolverTests.swift b/Tests/MCSTests/DestinationCollisionResolverTests.swift index 149e9b2..1c934c0 100644 --- a/Tests/MCSTests/DestinationCollisionResolverTests.swift +++ b/Tests/MCSTests/DestinationCollisionResolverTests.swift @@ -2,40 +2,65 @@ import Foundation @testable import mcs import Testing -struct DestinationCollisionResolverTests { - private let output = CLIOutput(colorsEnabled: false) - private let dummySource = URL(fileURLWithPath: "/tmp/dummy.sh") - - private func makeComponent( - pack: String, id: String, destination: String, fileType: CopyFileType - ) -> ComponentDefinition { - ComponentDefinition( - id: "\(pack).\(id)", - displayName: id, - description: "Test component", - type: fileType == .hook ? .hookFile : (fileType == .command ? .command : (fileType == .skill ? .skill : .agent)), - packIdentifier: pack, - dependencies: [], - isRequired: false, - installAction: .copyPackFile(source: dummySource, destination: destination, fileType: fileType), - supplementaryChecks: [] - ) +// MARK: - Shared Test Helpers + +private let collisionTestOutput = CLIOutput(colorsEnabled: false) +private let collisionTestDummySource = URL(fileURLWithPath: "/tmp/dummy.sh") + +private func makeCollisionComponent( + pack: String, id: String, destination: String, fileType: CopyFileType +) -> ComponentDefinition { + let componentType: ComponentType = switch fileType { + case .hook: .hookFile + case .command: .command + case .skill: .skill + default: .agent + } + return ComponentDefinition( + id: "\(pack).\(id)", + displayName: id, + description: "Test component", + type: componentType, + packIdentifier: pack, + dependencies: [], + isRequired: false, + installAction: .copyPackFile(source: collisionTestDummySource, destination: destination, fileType: fileType), + supplementaryChecks: [] + ) +} + +private func makeCollisionPack(id: String, components: [ComponentDefinition]) -> MockTechPack { + MockTechPack(identifier: id, displayName: id, components: components) +} + +// MARK: - Mock Filesystem Context + +private struct MockCollisionContext: CollisionFilesystemContext { + var existingFiles: Set = [] + var trackedFiles: Set = [] + + func fileExists(destination: String, fileType: CopyFileType) -> Bool { + existingFiles.contains("\(fileType.subdirectory)\(destination)") } - private func makePack(id: String, components: [ComponentDefinition]) -> MockTechPack { - MockTechPack(identifier: id, displayName: id, components: components) + func isTrackedByPack(destination: String, fileType: CopyFileType) -> Bool { + trackedFiles.contains("\(fileType.subdirectory)\(destination)") } +} + +// MARK: - Cross-Pack Collision Tests (backward compat — no filesystemContext) +struct DestinationCollisionResolverTests { @Test("No collision — destinations unchanged") func noCollision() { - let packA = makePack(id: "pack-a", components: [ - makeComponent(pack: "pack-a", id: "lint", destination: "lint.sh", fileType: .hook), + let packA = makeCollisionPack(id: "pack-a", components: [ + makeCollisionComponent(pack: "pack-a", id: "lint", destination: "lint.sh", fileType: .hook), ]) - let packB = makePack(id: "pack-b", components: [ - makeComponent(pack: "pack-b", id: "format", destination: "format.sh", fileType: .hook), + let packB = makeCollisionPack(id: "pack-b", components: [ + makeCollisionComponent(pack: "pack-b", id: "format", destination: "format.sh", fileType: .hook), ]) - let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: output) + let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: collisionTestOutput) // No wrapping needed — original packs returned #expect(result.count == 2) @@ -49,14 +74,14 @@ struct DestinationCollisionResolverTests { @Test("Two packs with same hook destination — both get subdirectory namespace") func hookCollision() { - let packA = makePack(id: "pack-a", components: [ - makeComponent(pack: "pack-a", id: "lint", destination: "lint.sh", fileType: .hook), + let packA = makeCollisionPack(id: "pack-a", components: [ + makeCollisionComponent(pack: "pack-a", id: "lint", destination: "lint.sh", fileType: .hook), ]) - let packB = makePack(id: "pack-b", components: [ - makeComponent(pack: "pack-b", id: "lint", destination: "lint.sh", fileType: .hook), + let packB = makeCollisionPack(id: "pack-b", components: [ + makeCollisionComponent(pack: "pack-b", id: "lint", destination: "lint.sh", fileType: .hook), ]) - let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: output) + let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: collisionTestOutput) #expect(result.count == 2) if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { @@ -69,14 +94,14 @@ struct DestinationCollisionResolverTests { @Test("Skill collision — first pack keeps clean name, second gets suffix") func skillCollisionSuffix() { - let packA = makePack(id: "pack-a", components: [ - makeComponent(pack: "pack-a", id: "my-skill", destination: "my-skill", fileType: .skill), + let packA = makeCollisionPack(id: "pack-a", components: [ + makeCollisionComponent(pack: "pack-a", id: "my-skill", destination: "my-skill", fileType: .skill), ]) - let packB = makePack(id: "pack-b", components: [ - makeComponent(pack: "pack-b", id: "my-skill", destination: "my-skill", fileType: .skill), + let packB = makeCollisionPack(id: "pack-b", components: [ + makeCollisionComponent(pack: "pack-b", id: "my-skill", destination: "my-skill", fileType: .skill), ]) - let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: output) + let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: collisionTestOutput) #expect(result.count == 2) // First pack keeps clean name @@ -91,17 +116,17 @@ struct DestinationCollisionResolverTests { @Test("Three packs — only the two that collide get namespaced") func partialCollision() { - let packA = makePack(id: "pack-a", components: [ - makeComponent(pack: "pack-a", id: "lint", destination: "lint.sh", fileType: .hook), + let packA = makeCollisionPack(id: "pack-a", components: [ + makeCollisionComponent(pack: "pack-a", id: "lint", destination: "lint.sh", fileType: .hook), ]) - let packB = makePack(id: "pack-b", components: [ - makeComponent(pack: "pack-b", id: "lint", destination: "lint.sh", fileType: .hook), + let packB = makeCollisionPack(id: "pack-b", components: [ + makeCollisionComponent(pack: "pack-b", id: "lint", destination: "lint.sh", fileType: .hook), ]) - let packC = makePack(id: "pack-c", components: [ - makeComponent(pack: "pack-c", id: "format", destination: "format.sh", fileType: .hook), + let packC = makeCollisionPack(id: "pack-c", components: [ + makeCollisionComponent(pack: "pack-c", id: "format", destination: "format.sh", fileType: .hook), ]) - let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB, packC], output: output) + let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB, packC], output: collisionTestOutput) // pack-a and pack-b collide on lint.sh → namespaced if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { @@ -118,14 +143,14 @@ struct DestinationCollisionResolverTests { @Test("Same destination but different fileType — no collision") func differentFileTypeNoCollision() { - let packA = makePack(id: "pack-a", components: [ - makeComponent(pack: "pack-a", id: "pr", destination: "pr.md", fileType: .command), + let packA = makeCollisionPack(id: "pack-a", components: [ + makeCollisionComponent(pack: "pack-a", id: "pr", destination: "pr.md", fileType: .command), ]) - let packB = makePack(id: "pack-b", components: [ - makeComponent(pack: "pack-b", id: "pr", destination: "pr.md", fileType: .agent), + let packB = makeCollisionPack(id: "pack-b", components: [ + makeCollisionComponent(pack: "pack-b", id: "pr", destination: "pr.md", fileType: .agent), ]) - let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: output) + let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: collisionTestOutput) if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { #expect(destination == "pr.md") @@ -135,14 +160,14 @@ struct DestinationCollisionResolverTests { } } - @Test("Single pack — no namespace applied") + @Test("Single pack — no namespace without filesystem context (backward compat)") func singlePackNoNamespace() { - let pack = makePack(id: "my-pack", components: [ - makeComponent(pack: "my-pack", id: "lint", destination: "lint.sh", fileType: .hook), - makeComponent(pack: "my-pack", id: "pr", destination: "pr.md", fileType: .command), + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "lint", destination: "lint.sh", fileType: .hook), + makeCollisionComponent(pack: "my-pack", id: "pr", destination: "pr.md", fileType: .command), ]) - let result = DestinationCollisionResolver.resolveCollisions(packs: [pack], output: output) + let result = DestinationCollisionResolver.resolveCollisions(packs: [pack], output: collisionTestOutput) if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { #expect(destination == "lint.sh") @@ -154,16 +179,16 @@ struct DestinationCollisionResolverTests { @Test("Mixed: skill collision gets suffix, command collision gets subdirectory") func mixedCollisionTypes() { - let packA = makePack(id: "pack-a", components: [ - makeComponent(pack: "pack-a", id: "my-skill", destination: "my-skill", fileType: .skill), - makeComponent(pack: "pack-a", id: "pr", destination: "pr.md", fileType: .command), + let packA = makeCollisionPack(id: "pack-a", components: [ + makeCollisionComponent(pack: "pack-a", id: "my-skill", destination: "my-skill", fileType: .skill), + makeCollisionComponent(pack: "pack-a", id: "pr", destination: "pr.md", fileType: .command), ]) - let packB = makePack(id: "pack-b", components: [ - makeComponent(pack: "pack-b", id: "my-skill", destination: "my-skill", fileType: .skill), - makeComponent(pack: "pack-b", id: "pr", destination: "pr.md", fileType: .command), + let packB = makeCollisionPack(id: "pack-b", components: [ + makeCollisionComponent(pack: "pack-b", id: "my-skill", destination: "my-skill", fileType: .skill), + makeCollisionComponent(pack: "pack-b", id: "pr", destination: "pr.md", fileType: .command), ]) - let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: output) + let result = DestinationCollisionResolver.resolveCollisions(packs: [packA, packB], output: collisionTestOutput) // Pack A: skill keeps clean name, command gets subdirectory if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { @@ -181,3 +206,184 @@ struct DestinationCollisionResolverTests { } } } + +// MARK: - Hook Always-Namespace Tests (with filesystemContext) + +struct HookAlwaysNamespaceTests { + @Test("Hook always namespaced with filesystem context — single pack") + func hookAlwaysNamespaced() { + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "lint", destination: "lint.sh", fileType: .hook), + ]) + let ctx = MockCollisionContext() + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [pack], output: collisionTestOutput, filesystemContext: ctx + ) + + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "my-pack/lint.sh") + } + } + + @Test("Hook stays flat without filesystem context (backward compat)") + func hookNoNamespaceWithoutContext() { + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "lint", destination: "lint.sh", fileType: .hook), + ]) + + let result = DestinationCollisionResolver.resolveCollisions(packs: [pack], output: collisionTestOutput) + + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "lint.sh") + } + } + + @Test("Non-hook command stays flat with context when no conflict") + func commandStaysFlatWithContext() { + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "lint", destination: "lint.sh", fileType: .hook), + makeCollisionComponent(pack: "my-pack", id: "pr", destination: "pr.md", fileType: .command), + ]) + let ctx = MockCollisionContext() + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [pack], output: collisionTestOutput, filesystemContext: ctx + ) + + // Hook is namespaced + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "my-pack/lint.sh") + } + // Command stays flat (no conflict) + if case let .copyPackFile(_, destination, _) = result[0].components[1].installAction { + #expect(destination == "pr.md") + } + } + + @Test("Two packs: hooks always namespaced, cross-pack collision guard skips them") + func hookCrossPackWithContext() { + let packA = makeCollisionPack(id: "pack-a", components: [ + makeCollisionComponent(pack: "pack-a", id: "lint", destination: "lint.sh", fileType: .hook), + ]) + let packB = makeCollisionPack(id: "pack-b", components: [ + makeCollisionComponent(pack: "pack-b", id: "lint", destination: "lint.sh", fileType: .hook), + ]) + let ctx = MockCollisionContext() + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [packA, packB], output: collisionTestOutput, filesystemContext: ctx + ) + + // Both hooks namespaced by Phase 0 — Phase 1a's cross-pack detection doesn't double-namespace + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "pack-a/lint.sh") + } + if case let .copyPackFile(_, destination, _) = result[1].components[0].installAction { + #expect(destination == "pack-b/lint.sh") + } + } +} + +// MARK: - User-File Conflict Tests + +struct UserFileConflictTests { + @Test("Untracked command file on disk — pack namespaced to /") + func userFileCommandConflict() { + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "pr", destination: "pr.md", fileType: .command), + ]) + let ctx = MockCollisionContext( + existingFiles: ["commands/pr.md"], // file exists on disk + trackedFiles: [] // not tracked by any pack + ) + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [pack], output: collisionTestOutput, filesystemContext: ctx + ) + + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "my-pack/pr.md") + } + } + + @Test("Untracked skill on disk — pack namespaced with suffix") + func userFileSkillConflict() { + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "review", destination: "review", fileType: .skill), + ]) + let ctx = MockCollisionContext( + existingFiles: ["skills/review"], + trackedFiles: [] + ) + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [pack], output: collisionTestOutput, filesystemContext: ctx + ) + + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "review-my-pack") + } + } + + @Test("Tracked command file on disk — no namespacing (mcs owns it)") + func trackedFileNoConflict() { + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "pr", destination: "pr.md", fileType: .command), + ]) + let ctx = MockCollisionContext( + existingFiles: ["commands/pr.md"], + trackedFiles: ["commands/pr.md"] // tracked → mcs owns it + ) + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [pack], output: collisionTestOutput, filesystemContext: ctx + ) + + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "pr.md") + } + } + + @Test("Cross-pack collision + user file — cross-pack resolver handles, no double-namespace") + func crossPackPlusUserFile() { + let packA = makeCollisionPack(id: "pack-a", components: [ + makeCollisionComponent(pack: "pack-a", id: "pr", destination: "pr.md", fileType: .command), + ]) + let packB = makeCollisionPack(id: "pack-b", components: [ + makeCollisionComponent(pack: "pack-b", id: "pr", destination: "pr.md", fileType: .command), + ]) + let ctx = MockCollisionContext( + existingFiles: ["commands/pr.md"], // user file exists too + trackedFiles: [] // not tracked + ) + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [packA, packB], output: collisionTestOutput, filesystemContext: ctx + ) + + // Both get cross-pack namespacing; Phase 1b doesn't double-namespace + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "pack-a/pr.md") + } + if case let .copyPackFile(_, destination, _) = result[1].components[0].installAction { + #expect(destination == "pack-b/pr.md") + } + } + + @Test("File does not exist on disk — no namespacing for non-hook") + func noFileNoConflict() { + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "pr", destination: "pr.md", fileType: .command), + ]) + let ctx = MockCollisionContext(existingFiles: [], trackedFiles: []) + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [pack], output: collisionTestOutput, filesystemContext: ctx + ) + + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "pr.md") + } + } +} diff --git a/Tests/MCSTests/GlobalConfiguratorTests.swift b/Tests/MCSTests/GlobalConfiguratorTests.swift index 19165e9..b013122 100644 --- a/Tests/MCSTests/GlobalConfiguratorTests.swift +++ b/Tests/MCSTests/GlobalConfiguratorTests.swift @@ -365,7 +365,7 @@ struct GlobalSettingsCompositionTests { let settingsPath = tmpDir.appendingPathComponent(".claude/settings.json") let result = try Settings.load(from: settingsPath) let command = result.hooks?["SessionStart"]?.first?.hooks?.first?.command - #expect(command == "bash ~/.claude/hooks/start.sh") + #expect(command == "bash ~/.claude/hooks/test-pack/start.sh") // Must NOT use project-relative path #expect(command?.hasPrefix("bash .claude/") != true) } @@ -538,7 +538,7 @@ struct GlobalFileInstallationTests { let configurator = makeGlobalConfigurator(home: tmpDir) try configurator.configure(packs: [pack], confirmRemovals: false) - let dest = tmpDir.appendingPathComponent(".claude/hooks/start.sh") + let dest = tmpDir.appendingPathComponent(".claude/hooks/test-pack/start.sh") #expect(FileManager.default.fileExists(atPath: dest.path)) } diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index 4f040e0..4b9405b 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -139,6 +139,30 @@ private struct LifecycleTestBed { ) } + func commandComponent( + pack: String, id: String, source: URL, destination: String + ) -> ComponentDefinition { + ComponentDefinition( + id: "\(pack).\(id)", + displayName: id, + description: "Command \(id)", + type: .command, + packIdentifier: pack, + dependencies: [], + isRequired: true, + installAction: .copyPackFile(source: source, destination: destination, fileType: .command) + ) + } + + /// Create a command source file in a temp pack directory. + func makeCommandSource(name: String, content: String = "# Command\nDo the thing.") throws -> URL { + let packDir = home.appendingPathComponent("pack-source/commands") + try FileManager.default.createDirectory(at: packDir, withIntermediateDirectories: true) + let file = packDir.appendingPathComponent(name) + try content.write(to: file, atomically: true, encoding: .utf8) + return file + } + func settingsComponent(pack: String, id: String, source: URL) -> ComponentDefinition { ComponentDefinition( id: "\(pack).\(id)", @@ -234,7 +258,7 @@ struct SinglePackLifecycleTests { try configurator.configure(packs: [pack], confirmRemovals: false) // Verify artifacts on disk - let hookFile = bed.project.appendingPathComponent(".claude/hooks/lint.sh") + let hookFile = bed.project.appendingPathComponent(".claude/hooks/test-pack/lint.sh") #expect(FileManager.default.fileExists(atPath: hookFile.path)) let settingsData = try Data(contentsOf: bed.settingsLocalPath) @@ -251,7 +275,7 @@ struct SinglePackLifecycleTests { let settings = try Settings.load(from: bed.settingsLocalPath) let postToolGroups = settings.hooks?["PostToolUse"] ?? [] let hookCommands = postToolGroups.flatMap { $0.hooks ?? [] }.compactMap(\.command) - #expect(hookCommands.contains(bed.projectHookCommand("lint.sh"))) + #expect(hookCommands.contains(bed.projectHookCommand("test-pack/lint.sh"))) // Verify MCP server was registered via MockClaudeCLI with local scope #expect(bed.mockCLI.mcpAddCalls.contains { $0.name == "test-mcp" && $0.scope == "local" }) @@ -263,7 +287,7 @@ struct SinglePackLifecycleTests { #expect(artifacts != nil) #expect(artifacts?.templateSections.contains("test-pack") == true) #expect(artifacts?.settingsKeys.contains("env") == true) - #expect(artifacts?.hookCommands.contains(bed.projectHookCommand("lint.sh")) == true) + #expect(artifacts?.hookCommands.contains(bed.projectHookCommand("test-pack/lint.sh")) == true) #expect(artifacts?.mcpServers.contains { $0.name == "test-mcp" } == true) // === Step 2: Doctor passes === @@ -448,25 +472,22 @@ struct CrossPackCollisionTests { #expect(artifactsA?.hookCommands.contains(bed.projectHookCommand("pack-a/lint.sh")) == true) #expect(artifactsB?.hookCommands.contains(bed.projectHookCommand("pack-b/lint.sh")) == true) - // === Step 2: Remove pack A — pack B now has no collision, moves to flat path === + // === Step 2: Remove pack A — pack B stays namespaced (hooks always use /) === try configurator.configure(packs: [packB], confirmRemovals: false) #expect(!FileManager.default.fileExists(atPath: fileA.path)) - // Pack B's file should now be at the flat (non-namespaced) path - let flatFileB = bed.project.appendingPathComponent(".claude/hooks/lint.sh") - #expect(FileManager.default.fileExists(atPath: flatFileB.path)) - // Old namespaced path should be cleaned up by reconcileStaleArtifacts - #expect(!FileManager.default.fileExists(atPath: fileB.path)) + // Pack B stays at its namespaced path (hooks are always namespaced) + #expect(FileManager.default.fileExists(atPath: fileB.path)) let afterSettings = try Settings.load(from: bed.settingsLocalPath) let afterGroups = afterSettings.hooks?["PreToolUse"] ?? [] let afterCommands = afterGroups.flatMap { $0.hooks ?? [] }.compactMap(\.command) #expect(!afterCommands.contains(bed.projectHookCommand("pack-a/lint.sh"))) - #expect(afterCommands.contains(bed.projectHookCommand("lint.sh"))) + #expect(afterCommands.contains(bed.projectHookCommand("pack-b/lint.sh"))) } - @Test("Single pack installs files at flat (non-namespaced) paths") - func singlePackFlatPaths() throws { + @Test("Single pack hook installs to namespaced path (hooks always use /)") + func singlePackNamespacedHook() throws { let bed = try LifecycleTestBed() defer { bed.cleanup() } @@ -489,17 +510,174 @@ struct CrossPackCollisionTests { try configurator.configure(packs: [pack], confirmRemovals: false) - // With no collision, file should be at flat path (no pack-id subdirectory) - let flatFile = bed.project.appendingPathComponent(".claude/hooks/lint.sh") + // Hooks are always namespaced into / subdirectory let namespacedFile = bed.project.appendingPathComponent(".claude/hooks/my-pack/lint.sh") - #expect(FileManager.default.fileExists(atPath: flatFile.path)) - #expect(!FileManager.default.fileExists(atPath: namespacedFile.path)) + let flatFile = bed.project.appendingPathComponent(".claude/hooks/lint.sh") + #expect(FileManager.default.fileExists(atPath: namespacedFile.path)) + #expect(!FileManager.default.fileExists(atPath: flatFile.path)) + + // Hook command should use namespaced path + let settings = try Settings.load(from: bed.settingsLocalPath) + let preToolGroups = settings.hooks?["PreToolUse"] ?? [] + let hookCommands = preToolGroups.flatMap { $0.hooks ?? [] }.compactMap(\.command) + #expect(hookCommands.contains(bed.projectHookCommand("my-pack/lint.sh"))) + } +} + +// MARK: - Scenario 2b: Pre-existing User File Protection + +struct UserFileProtectionTests { + @Test("Pre-existing user hook is preserved — pack hook installs to namespaced path") + func preExistingUserHookPreserved() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + // User manually creates a hook before mcs sync + let userHookDir = bed.project.appendingPathComponent(".claude/hooks") + try FileManager.default.createDirectory(at: userHookDir, withIntermediateDirectories: true) + let userHookFile = userHookDir.appendingPathComponent("lint.sh") + try "#!/bin/bash\necho user-hook".write(to: userHookFile, atomically: true, encoding: .utf8) - // Hook command should use flat path + let hookSource = try bed.makeHookSource(name: "pack-lint.sh", content: "#!/bin/bash\necho pack-hook") + + let pack = MockTechPack( + identifier: "my-pack", + displayName: "My Pack", + components: [ + bed.hookComponent( + pack: "my-pack", id: "lint", + source: hookSource, destination: "lint.sh", + hookRegistration: HookRegistration(event: .preToolUse) + ), + ], + templates: [] + ) + let registry = TechPackRegistry(packs: [pack]) + let configurator = bed.makeConfigurator(registry: registry) + + try configurator.configure(packs: [pack], confirmRemovals: false) + + // User's file is untouched (hooks always namespace, so pack goes to my-pack/lint.sh) + let userContent = try String(contentsOf: userHookFile, encoding: .utf8) + #expect(userContent.contains("user-hook")) + + // Pack's hook is at namespaced path + let packHookFile = bed.project.appendingPathComponent(".claude/hooks/my-pack/lint.sh") + #expect(FileManager.default.fileExists(atPath: packHookFile.path)) + let packContent = try String(contentsOf: packHookFile, encoding: .utf8) + #expect(packContent.contains("pack-hook")) + + // Hook command uses namespaced path let settings = try Settings.load(from: bed.settingsLocalPath) let preToolGroups = settings.hooks?["PreToolUse"] ?? [] let hookCommands = preToolGroups.flatMap { $0.hooks ?? [] }.compactMap(\.command) - #expect(hookCommands.contains(bed.projectHookCommand("lint.sh"))) + #expect(hookCommands.contains(bed.projectHookCommand("my-pack/lint.sh"))) + } + + @Test("Pre-existing user command is preserved — pack command installs to namespaced path") + func preExistingUserCommandPreserved() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + // User manually creates a command before mcs sync + let userCmdDir = bed.project.appendingPathComponent(".claude/commands") + try FileManager.default.createDirectory(at: userCmdDir, withIntermediateDirectories: true) + let userCmdFile = userCmdDir.appendingPathComponent("pr.md") + try "# My PR command\nuser content".write(to: userCmdFile, atomically: true, encoding: .utf8) + + let cmdSource = try bed.makeCommandSource(name: "pr.md", content: "# Pack PR\npack content") + + let pack = MockTechPack( + identifier: "my-pack", + displayName: "My Pack", + components: [ + bed.commandComponent( + pack: "my-pack", id: "pr", + source: cmdSource, destination: "pr.md" + ), + ], + templates: [] + ) + let registry = TechPackRegistry(packs: [pack]) + let configurator = bed.makeConfigurator(registry: registry) + + try configurator.configure(packs: [pack], confirmRemovals: false) + + // User's file is untouched + let userContent = try String(contentsOf: userCmdFile, encoding: .utf8) + #expect(userContent.contains("user content")) + + // Pack's command is at namespaced path + let packCmdFile = bed.project.appendingPathComponent(".claude/commands/my-pack/pr.md") + #expect(FileManager.default.fileExists(atPath: packCmdFile.path)) + let packContent = try String(contentsOf: packCmdFile, encoding: .utf8) + #expect(packContent.contains("pack content")) + } + + @Test("Tracked file does not trigger false-positive namespace on re-sync") + func trackedFileNotFalsePositive() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let cmdSource = try bed.makeCommandSource(name: "pr.md", content: "# Pack PR\npack content") + + let pack = MockTechPack( + identifier: "my-pack", + displayName: "My Pack", + components: [ + bed.commandComponent( + pack: "my-pack", id: "pr", + source: cmdSource, destination: "pr.md" + ), + ], + templates: [] + ) + let registry = TechPackRegistry(packs: [pack]) + let configurator = bed.makeConfigurator(registry: registry) + + // First sync — installs at flat path + try configurator.configure(packs: [pack], confirmRemovals: false) + let flatFile = bed.project.appendingPathComponent(".claude/commands/pr.md") + #expect(FileManager.default.fileExists(atPath: flatFile.path)) + + // Second sync — file is tracked, should stay at flat path + try configurator.configure(packs: [pack], confirmRemovals: false) + #expect(FileManager.default.fileExists(atPath: flatFile.path)) + + // No namespaced version should exist + let namespacedFile = bed.project.appendingPathComponent(".claude/commands/my-pack/pr.md") + #expect(!FileManager.default.fileExists(atPath: namespacedFile.path)) + } + + @Test("Hook always namespaced even without pre-existing file") + func hookAlwaysNamespacedSinglePack() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let hookSource = try bed.makeHookSource(name: "lint.sh", content: "#!/bin/bash\necho lint") + + let pack = MockTechPack( + identifier: "my-pack", + displayName: "My Pack", + components: [ + bed.hookComponent( + pack: "my-pack", id: "lint", + source: hookSource, destination: "lint.sh", + hookRegistration: HookRegistration(event: .preToolUse) + ), + ], + templates: [] + ) + let registry = TechPackRegistry(packs: [pack]) + let configurator = bed.makeConfigurator(registry: registry) + + try configurator.configure(packs: [pack], confirmRemovals: false) + + // Hook installed at namespaced path + let namespacedFile = bed.project.appendingPathComponent(".claude/hooks/my-pack/lint.sh") + let flatFile = bed.project.appendingPathComponent(".claude/hooks/lint.sh") + #expect(FileManager.default.fileExists(atPath: namespacedFile.path)) + #expect(!FileManager.default.fileExists(atPath: flatFile.path)) } } @@ -578,8 +756,8 @@ struct ComponentExclusionLifecycleTests { let registry = TechPackRegistry(packs: [pack]) let configurator = bed.makeConfigurator(registry: registry) - let hookAPath = bed.project.appendingPathComponent(".claude/hooks/hookA.sh") - let hookBPath = bed.project.appendingPathComponent(".claude/hooks/hookB.sh") + let hookAPath = bed.project.appendingPathComponent(".claude/hooks/my-pack/hookA.sh") + let hookBPath = bed.project.appendingPathComponent(".claude/hooks/my-pack/hookB.sh") // === Step 1: Configure with both === try configurator.configure(packs: [pack], confirmRemovals: false) @@ -629,7 +807,7 @@ struct GlobalScopeLifecycleTests { try configurator.configure(packs: [pack], confirmRemovals: false) // Verify hook installed in ~/.claude/hooks/ - let globalHook = bed.env.hooksDirectory.appendingPathComponent("global-hook.sh") + let globalHook = bed.env.hooksDirectory.appendingPathComponent("global-pack/global-hook.sh") #expect(FileManager.default.fileExists(atPath: globalHook.path)) // Verify global state @@ -801,8 +979,8 @@ struct GlobalScopeExclusionTests { let configurator = bed.makeGlobalConfigurator(registry: registry) try configurator.configure(packs: [pack], confirmRemovals: false) - let hookAPath = bed.env.hooksDirectory.appendingPathComponent("globalA.sh") - let hookBPath = bed.env.hooksDirectory.appendingPathComponent("globalB.sh") + let hookAPath = bed.env.hooksDirectory.appendingPathComponent("global-pack/globalA.sh") + let hookBPath = bed.env.hooksDirectory.appendingPathComponent("global-pack/globalB.sh") #expect(FileManager.default.fileExists(atPath: hookAPath.path)) #expect(FileManager.default.fileExists(atPath: hookBPath.path)) @@ -911,7 +1089,7 @@ struct HookMetadataLifecycleTests { let hookEntries = try #require(firstGroup["hooks"] as? [[String: Any]]) let entry = try #require(hookEntries.first) - #expect(entry["command"] as? String == bed.projectHookCommand("lint.sh")) + #expect(entry["command"] as? String == bed.projectHookCommand("meta-pack/lint.sh")) #expect(entry["timeout"] as? Int == 30) #expect(entry["async"] as? Bool == true) #expect(entry["statusMessage"] as? String == "Running lint...") diff --git a/Tests/MCSTests/ProjectConfiguratorTests.swift b/Tests/MCSTests/ProjectConfiguratorTests.swift index 8dc1c6b..b5b9281 100644 --- a/Tests/MCSTests/ProjectConfiguratorTests.swift +++ b/Tests/MCSTests/ProjectConfiguratorTests.swift @@ -638,7 +638,7 @@ struct AutoDerivedSettingsTests { let sessionGroups = result.hooks?["SessionStart"] ?? [] #expect(!sessionGroups.isEmpty) let command = sessionGroups.first?.hooks?.first?.command - #expect(command == "bash .claude/hooks/session_start.sh") + #expect(command == "bash .claude/hooks/test-pack/session_start.sh") // Should NOT use global path #expect(command?.contains("~/.claude") != true) } @@ -775,7 +775,7 @@ struct AutoDerivedSettingsTests { // Should have both entries (different commands — project-local vs global) let sessionGroups = result.hooks?["SessionStart"] ?? [] let commands = sessionGroups.compactMap { $0.hooks?.first?.command } - #expect(commands.contains("bash .claude/hooks/session_start.sh")) + #expect(commands.contains("bash .claude/hooks/test-pack/session_start.sh")) #expect(commands.contains("bash ~/.claude/hooks/session_start.sh")) #expect(sessionGroups.count == 2) } @@ -797,7 +797,7 @@ struct AutoDerivedSettingsTests { let state = try ProjectState(projectRoot: tmpDir) let artifacts = state.artifacts(for: "test-pack") #expect(artifacts != nil) - #expect(artifacts?.hookCommands.contains("bash .claude/hooks/session_start.sh") == true) + #expect(artifacts?.hookCommands.contains("bash .claude/hooks/test-pack/session_start.sh") == true) } } From 0c33e0ec0d75705e1ce878b2e0aafa5fe0cc0040 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Thu, 26 Mar 2026 13:36:15 +0100 Subject: [PATCH 2/2] Review fixes: phase comments, required makeCollisionContext, generic test - Align phase numbering in DestinationCollisionResolver comments (Phase 0/1a/1b/2) - Remove default nil from SyncStrategy.makeCollisionContext to enforce implementation - Add .generic file type user-conflict test in DestinationCollisionResolverTests --- .../DestinationCollisionResolver.swift | 8 ++++---- Sources/mcs/Install/SyncStrategy.swift | 7 ++----- .../DestinationCollisionResolverTests.swift | 19 +++++++++++++++++++ 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/Sources/mcs/Install/DestinationCollisionResolver.swift b/Sources/mcs/Install/DestinationCollisionResolver.swift index e08d2e3..51fe076 100644 --- a/Sources/mcs/Install/DestinationCollisionResolver.swift +++ b/Sources/mcs/Install/DestinationCollisionResolver.swift @@ -69,7 +69,7 @@ enum DestinationCollisionResolver { var packComponentOverrides: [Int: [Int: String]] = [:] var collisionMap: [DestinationKey: [CollisionEntry]] = [:] - // Phase 1: Build collision map and always-namespace hooks (single pass). + // Phase 0: Build collision map and always-namespace hooks (single pass). for (packIndex, pack) in packs.enumerated() { for (componentIndex, component) in pack.components.enumerated() { guard case let .copyPackFile(_, destination, fileType) = component.installAction else { @@ -90,7 +90,7 @@ enum DestinationCollisionResolver { } // Phase 1a: Apply cross-pack collision namespacing (2+ distinct packs). - // Guards skip entries already resolved above. + // Guards skip entries already resolved by Phase 0. for (key, entries) in collisionMap { let distinctPackIndices = Set(entries.map(\.packIndex)) guard distinctPackIndices.count >= 2 else { continue } @@ -108,8 +108,8 @@ enum DestinationCollisionResolver { } // Phase 1b: User-file conflict detection. - // For non-hook entries not yet resolved, check if the destination is occupied - // by a file not tracked by any pack. + // For entries not yet resolved by Phase 0 or 1a, check if the destination + // is occupied by a file not tracked by any pack. if let ctx = filesystemContext { for (packIndex, pack) in packs.enumerated() { for (componentIndex, component) in pack.components.enumerated() { diff --git a/Sources/mcs/Install/SyncStrategy.swift b/Sources/mcs/Install/SyncStrategy.swift index 7dc39cd..93397a2 100644 --- a/Sources/mcs/Install/SyncStrategy.swift +++ b/Sources/mcs/Install/SyncStrategy.swift @@ -74,6 +74,8 @@ protocol SyncStrategy { /// /// Returns a `CollisionFilesystemContext` that enables the resolver to detect /// pre-existing user files at `copyPackFile` destinations. + /// Every conformance must implement this — returning `nil` disables user-file protection + /// and hook namespacing (only cross-pack collisions are resolved). func makeCollisionContext(trackedFiles: Set) -> (any CollisionFilesystemContext)? /// Derive the relative file path that artifact tracking records for a `copyPackFile` component. @@ -100,11 +102,6 @@ protocol SyncStrategy { // MARK: - Default Implementations extension SyncStrategy { - /// Default: no filesystem context (backward compat — cross-pack collisions only). - func makeCollisionContext(trackedFiles _: Set) -> (any CollisionFilesystemContext)? { - nil - } - /// Default removal summary — prints all non-empty artifact fields. /// /// Uses `scope.claudeFilePath.lastPathComponent` for template section labels. diff --git a/Tests/MCSTests/DestinationCollisionResolverTests.swift b/Tests/MCSTests/DestinationCollisionResolverTests.swift index 1c934c0..e193a5e 100644 --- a/Tests/MCSTests/DestinationCollisionResolverTests.swift +++ b/Tests/MCSTests/DestinationCollisionResolverTests.swift @@ -371,6 +371,25 @@ struct UserFileConflictTests { } } + @Test("Untracked generic file on disk — pack namespaced to /") + func userFileGenericConflict() { + let pack = makeCollisionPack(id: "my-pack", components: [ + makeCollisionComponent(pack: "my-pack", id: "config", destination: "config.yaml", fileType: .generic), + ]) + let ctx = MockCollisionContext( + existingFiles: ["config.yaml"], // .generic has empty subdirectory + trackedFiles: [] + ) + + let result = DestinationCollisionResolver.resolveCollisions( + packs: [pack], output: collisionTestOutput, filesystemContext: ctx + ) + + if case let .copyPackFile(_, destination, _) = result[0].components[0].installAction { + #expect(destination == "my-pack/config.yaml") + } + } + @Test("File does not exist on disk — no namespacing for non-hook") func noFileNoConflict() { let pack = makeCollisionPack(id: "my-pack", components: [