diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index 720e5c8..798e15a 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -193,7 +193,7 @@ struct Configurator { packs: packs, output: output, filesystemContext: fsContext ) let headerLabel = scope.isGlobalScope ? "Plan (Global)" : "Plan" - ConfiguratorSupport.dryRunSummary( + let additions = ConfiguratorSupport.dryRunSummary( packs: packs, state: state, header: headerLabel, @@ -201,6 +201,16 @@ struct Configurator { artifactSummary: { strategy.printArtifactSummary($0, output: output) }, removalSummary: { strategy.printRemovalSummary($0, output: output) } ) + + // Emitted after the plan so it reads as a consequence of it, reusing the plan's own + // additions set so the warning can never disagree with what was just printed. + ConfiguratorSupport.warnProjectDuplication( + isGlobalScope: scope.isGlobalScope, + additions: additions, + packs: packs, + environment: environment, + output: output + ) } // MARK: - Configure (Multi-Pack) @@ -251,6 +261,21 @@ struct Configurator { } } + // Advisory only: a pack entering the global scope duplicates its artifacts in every + // project that already holds it. Read-only — it never touches `packs` or the index. + // + // Placed after the removal gate, not before: cancelling there returns without installing + // anything, and a warning about duplicates that were never created would be a lie — one + // that also costs a needless index read. Still ahead of all install work, so it precedes + // the artifacts it warns about. + ConfiguratorSupport.warnProjectDuplication( + isGlobalScope: scope.isGlobalScope, + additions: additions, + packs: packs, + environment: environment, + output: output + ) + for packID in removals.sorted() { unconfigurePack(packID, state: &state) } diff --git a/Sources/mcs/Sync/ConfiguratorSupport.swift b/Sources/mcs/Sync/ConfiguratorSupport.swift index 58ab256..9f1f605 100644 --- a/Sources/mcs/Sync/ConfiguratorSupport.swift +++ b/Sources/mcs/Sync/ConfiguratorSupport.swift @@ -24,6 +24,98 @@ enum ConfiguratorSupport { }) } + /// Warning lines naming the tracked projects that already configure packs newly entering + /// the *global* scope. Empty when nothing applies. + /// + /// The mirror of `globallyBlockedIDs`: that rule stops a globally-installed pack being added + /// to a project; this one reports the reverse move. Installing globally is legitimate, but it + /// duplicates every hook and skill in the projects that already hold the pack — the global and + /// project copies register as distinct settings entries and both fire. + /// + /// Keyed on `additions` rather than the whole selection, so re-syncing a pack the global scope + /// already has stays silent, and `mcs update` — which re-applies each scope's existing set — + /// never warns at all. + /// + /// Pure so the wording itself is testable: `CLIOutput` writes straight to stdout and the suite + /// has no way to capture it. Element 0 is the `warn` header, the rest are plain detail lines; + /// there is exactly one header however many packs are involved. + /// + /// - Parameter pathExists: Injected so tests never reach the real filesystem. + static func projectDuplicationWarning( + additions: Set, + displayNames: [String: String], + index: ProjectIndex.IndexData, + pathExists: (String) -> Bool = { FileManager.default.fileExists(atPath: $0) } + ) -> [String] { + var details: [String] = [] + for packID in additions.sorted() { + // The global scope carries its own index entry. It is the scope being installed + // into, not a project that ends up with a duplicate. + let paths = index.projects + .filter { !$0.isGlobal && $0.packs.contains(packID) } + .map(\.path) + .filter(pathExists) + .sorted() + guard !paths.isEmpty else { continue } + details.append(" \(displayNames[packID] ?? packID) → \(paths.joined(separator: ", "))") + } + guard !details.isEmpty else { return [] } + + return ["\(details.count) pack(s) being installed globally are already configured in other projects:"] + + details + + [ + " Their hooks and skills will run twice there until the project copy is removed.", + " Run 'mcs doctor --fix' in those projects to drop the project copy.", + ] + } + + /// Emit `projectDuplicationWarning`, doing nothing outside the global scope. + /// + /// The scope gate lives here rather than at each call site: only a global install can create + /// this duplication, so every caller needs the same check, and a future one that forgot it + /// would print a nonsensical "installed globally" notice during a project sync. + /// + /// Advisory only: it never filters the pack set — `Configurator.configure` treats that set as a + /// complete desired state and unconfigures anything missing from it — and never writes to the + /// index, stale entries included. An unreadable index degrades to a notice rather than failing + /// the sync; `ProjectIndexCheck` is what reports a broken index. + static func warnProjectDuplication( + isGlobalScope: Bool, + additions: Set, + packs: [any TechPack], + environment: Environment, + output: CLIOutput + ) { + // Ordered so a project-scope sync and a no-op global sync both return before any disk read. + guard isGlobalScope, !additions.isEmpty else { return } + + let index: ProjectIndex.IndexData + do { + index = try ProjectIndex(path: environment.projectsIndexFile).load() + } catch { + output.warn("Could not read project index: \(error.localizedDescription)") + output.plain(" Skipping the check for packs already configured in other projects.") + return + } + + let displayNames = Dictionary( + packs.map { ($0.identifier, $0.displayName) }, + uniquingKeysWith: { first, _ in first } + ) + let lines = projectDuplicationWarning( + additions: additions, + displayNames: displayNames, + index: index + ) + guard let header = lines.first else { return } + + output.plain("") + output.warn(header) + for line in lines.dropFirst() { + output.plain(line) + } + } + /// Build a `ComponentExecutor` from the common dependencies. static func makeExecutor( environment: Environment, @@ -49,6 +141,10 @@ enum ConfiguratorSupport { /// /// Shared orchestration for both project and global dry-run flows. /// Callers provide scope-specific closures for artifact and removal display. + /// + /// Returns the pack IDs newly entering this scope, so a caller needing the same diff reuses + /// this one rather than re-deriving it and risking the two disagreeing. + @discardableResult static func dryRunSummary( packs: [any TechPack], state: ProjectState, @@ -56,7 +152,7 @@ enum ConfiguratorSupport { output: CLIOutput, artifactSummary: (_ pack: any TechPack) -> Void, removalSummary: (_ artifacts: PackArtifactRecord) -> Void - ) { + ) -> Set { let selectedIDs = Set(packs.map(\.identifier)) let previousIDs = state.configuredPacks @@ -71,7 +167,7 @@ enum ConfiguratorSupport { output.info("No packs selected. Nothing would change.") output.plain("") output.dimmed("No changes made (dry run).") - return + return additions } // Show additions @@ -112,6 +208,7 @@ enum ConfiguratorSupport { } output.plain("") output.dimmed("No changes made (dry run).") + return additions } /// Present per-pack component multi-select and return excluded component IDs. diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index 4c2faac..cca9172 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -43,10 +43,13 @@ private struct LifecycleTestBed { ) } - func makeGlobalSyncConfigurator(registry: TechPackRegistry = TechPackRegistry()) -> Configurator { + func makeGlobalSyncConfigurator( + registry: TechPackRegistry = TechPackRegistry(), + warningCounter: WarningCounter? = nil + ) -> Configurator { Configurator( environment: env, - output: CLIOutput(colorsEnabled: false), + output: CLIOutput(colorsEnabled: false, warningCounter: warningCounter), shell: ShellRunner(environment: env), registry: registry, strategy: GlobalSyncStrategy(environment: env), @@ -1067,6 +1070,149 @@ struct GlobalScopeLifecycleTests { } } +// MARK: - Scenario 5c: Global install warns about projects that already hold the pack + +struct GlobalDuplicationWarningTests { + /// Seed `~/.mcs/projects.yaml` as a prior `mcs sync` in each project would have. + private func seedIndex(env: Environment, entries: [(String, [String])]) throws { + let indexFile = ProjectIndex(path: env.projectsIndexFile) + var data = ProjectIndex.IndexData() + for entry in entries { + indexFile.upsert(projectPath: entry.0, packIDs: entry.1, in: &data) + } + try indexFile.save(data) + } + + /// Project entries only. The `__global__` entry legitimately changes during a global + /// sync (step 11 upserts it), so it is excluded from the read-only assertion. + private func projectEntries(env: Environment) throws -> [ProjectIndex.ProjectEntry] { + try ProjectIndex(path: env.projectsIndexFile).load() + .projects + .filter { !$0.isGlobal } + .sorted { $0.path < $1.path } + } + + private func makeProjects(in home: URL, _ names: [String]) throws -> [URL] { + try names.map { name in + let url = home.appendingPathComponent(name) + try FileManager.default.createDirectory(at: url, withIntermediateDirectories: true) + return url + } + } + + private func duplicatedPack(bed: LifecycleTestBed) throws -> MockTechPack { + let hookSource = try bed.makeHookSource(name: "dup-hook.sh") + return MockTechPack( + identifier: "dup-pack", + displayName: "Dup Pack", + components: [ + bed.hookComponent(pack: "dup-pack", id: "hook", source: hookSource, destination: "dup-hook.sh"), + ] + ) + } + + @Test("Warns when tracked projects already configure a pack entering the global scope") + func warnsAndLeavesProjectEntriesUntouched() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let projects = try makeProjects(in: bed.home, ["project-a", "project-b"]) + try seedIndex(env: bed.env, entries: projects.map { ($0.path, ["dup-pack"]) }) + let before = try projectEntries(env: bed.env) + + let pack = try duplicatedPack(bed: bed) + let counter = WarningCounter() + try bed.makeGlobalSyncConfigurator(registry: TechPackRegistry(packs: [pack]), warningCounter: counter) + .configure(packs: [pack], confirmRemovals: false) + + // One `warn` header covers every pack and project it names, so the count is + // stable no matter how many duplicates there are. + #expect(counter.count == 1) + + // Advisory only: the warning must not touch the projects it names, nor prune + // their index entries. Only the global scope changes. + let after = try projectEntries(env: bed.env) + #expect(after == before) + #expect(try ProjectState(stateFile: bed.env.globalStateFile).configuredPacks.contains("dup-pack")) + } + + @Test("Silent when no tracked project holds the pack being installed globally") + func silentWithoutOverlap() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let projects = try makeProjects(in: bed.home, ["project-a"]) + try seedIndex(env: bed.env, entries: projects.map { ($0.path, ["other-pack"]) }) + + let pack = try duplicatedPack(bed: bed) + let counter = WarningCounter() + try bed.makeGlobalSyncConfigurator(registry: TechPackRegistry(packs: [pack]), warningCounter: counter) + .configure(packs: [pack], confirmRemovals: false) + + #expect(counter.count == 0) + } + + @Test("Re-syncing a pack the global scope already has does not warn again") + func silentOnSecondGlobalSync() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let projects = try makeProjects(in: bed.home, ["project-a"]) + try seedIndex(env: bed.env, entries: projects.map { ($0.path, ["dup-pack"]) }) + + let pack = try duplicatedPack(bed: bed) + let registry = TechPackRegistry(packs: [pack]) + let first = WarningCounter() + try bed.makeGlobalSyncConfigurator(registry: registry, warningCounter: first) + .configure(packs: [pack], confirmRemovals: false) + #expect(first.count == 1) + + // The trigger is a transition, not an identity: the pack is no longer an addition, + // so the second sync is quiet. This is also what keeps `mcs update` silent. + let second = WarningCounter() + try bed.makeGlobalSyncConfigurator(registry: registry, warningCounter: second) + .configure(packs: [pack], confirmRemovals: false) + #expect(second.count == 0) + } + + @Test("--dry-run surfaces the same warning and installs nothing") + func warnsOnDryRunWithoutInstalling() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let projects = try makeProjects(in: bed.home, ["project-a"]) + try seedIndex(env: bed.env, entries: projects.map { ($0.path, ["dup-pack"]) }) + + let pack = try duplicatedPack(bed: bed) + let counter = WarningCounter() + try bed.makeGlobalSyncConfigurator(registry: TechPackRegistry(packs: [pack]), warningCounter: counter) + .dryRun(packs: [pack]) + + // `dryRun` keeps no diff of its own, so this covers the second call site's + // locally-derived additions set. + #expect(counter.count == 1) + #expect(try ProjectState(stateFile: bed.env.globalStateFile).configuredPacks.isEmpty) + } + + @Test("An unreadable project index degrades to a notice — the sync still completes") + func unreadableIndexDoesNotFailSync() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + try "{{ not yaml".write(to: bed.env.projectsIndexFile, atomically: true, encoding: .utf8) + + let pack = try duplicatedPack(bed: bed) + let counter = WarningCounter() + try bed.makeGlobalSyncConfigurator(registry: TechPackRegistry(packs: [pack]), warningCounter: counter) + .configure(packs: [pack], confirmRemovals: false) + + // One warning for the unreadable index. The later index *write* also fails, but + // reports through `output.error`, which does not touch the counter. + #expect(counter.count == 1) + #expect(try ProjectState(stateFile: bed.env.globalStateFile).configuredPacks.contains("dup-pack")) + } +} + // MARK: - Scenario 5b: Shell Command Component Lifecycle struct ShellCommandLifecycleTests { diff --git a/Tests/MCSTests/SyncCommandTests.swift b/Tests/MCSTests/SyncCommandTests.swift index 2574fda..03c8162 100644 --- a/Tests/MCSTests/SyncCommandTests.swift +++ b/Tests/MCSTests/SyncCommandTests.swift @@ -197,6 +197,116 @@ struct SyncCommandTests { ) #expect(blocked.isEmpty) } + + // MARK: - Project duplication warning (the reverse of the block above) + + /// Build an index from `(path, packs)` pairs. `lastSynced` is irrelevant to the query. + private func makeIndex(_ entries: [(String, [String])]) -> ProjectIndex.IndexData { + ProjectIndex.IndexData( + projects: entries.map { + ProjectIndex.ProjectEntry(path: $0.0, packs: $0.1, lastSynced: "") + } + ) + } + + private let allPathsExist: (String) -> Bool = { _ in true } + + @Test("Names every project already holding a pack that is entering the global scope") + func warnsNamingAffectedProjects() { + let lines = ConfiguratorSupport.projectDuplicationWarning( + additions: ["ios"], + displayNames: ["ios": "iOS"], + index: makeIndex([("/dev/b", ["ios"]), ("/dev/a", ["ios"]), ("/dev/c", ["android"])]), + pathExists: allPathsExist + ) + // Paths sorted so the output is stable across index orderings. + #expect(lines.count == 4) + #expect(lines[0] == "1 pack(s) being installed globally are already configured in other projects:") + #expect(lines[1] == " iOS → /dev/a, /dev/b") + #expect(lines[3].contains("mcs doctor --fix")) + } + + @Test("Excludes the global sentinel — it is the scope being installed into") + func excludesGlobalSentinel() { + let lines = ConfiguratorSupport.projectDuplicationWarning( + additions: ["ios"], + displayNames: ["ios": "iOS"], + index: makeIndex([(ProjectIndex.globalSentinel, ["ios"])]), + pathExists: allPathsExist + ) + #expect(lines.isEmpty) + } + + @Test("Drops projects whose directory no longer exists") + func dropsStaleProjectPaths() { + let index = makeIndex([("/dev/gone", ["ios"]), ("/dev/live", ["ios"])]) + let lines = ConfiguratorSupport.projectDuplicationWarning( + additions: ["ios"], + displayNames: ["ios": "iOS"], + index: index, + pathExists: { $0 == "/dev/live" } + ) + #expect(lines[1] == " iOS → /dev/live") + + // Every path stale is the same as no duplication at all. + let allStale = ConfiguratorSupport.projectDuplicationWarning( + additions: ["ios"], + displayNames: ["ios": "iOS"], + index: index, + pathExists: { _ in false } + ) + #expect(allStale.isEmpty) + } + + @Test("Silent when no project holds the pack being added globally") + func silentWhenNoProjectHoldsPack() { + let lines = ConfiguratorSupport.projectDuplicationWarning( + additions: ["ios"], + displayNames: ["ios": "iOS"], + index: makeIndex([("/dev/a", ["android"])]), + pathExists: allPathsExist + ) + #expect(lines.isEmpty) + } + + @Test("Silent when nothing is being added — the transition rule that keeps 'mcs update' quiet") + func silentWhenNoAdditions() { + // `mcs update` re-applies each scope's existing pack set, so `additions` is always + // empty there. Warning on identity instead would make every update noisy. + let lines = ConfiguratorSupport.projectDuplicationWarning( + additions: [], + displayNames: ["ios": "iOS"], + index: makeIndex([("/dev/a", ["ios"])]), + pathExists: allPathsExist + ) + #expect(lines.isEmpty) + } + + @Test("Falls back to the identifier when no display name is known") + func fallsBackToIdentifier() { + let lines = ConfiguratorSupport.projectDuplicationWarning( + additions: ["ios"], + displayNames: [:], + index: makeIndex([("/dev/a", ["ios"])]), + pathExists: allPathsExist + ) + #expect(lines[1] == " ios → /dev/a") + } + + @Test("Multiple packs share one header line so the warning count stays 1") + func multiplePacksEmitOneHeader() { + let lines = ConfiguratorSupport.projectDuplicationWarning( + additions: ["ios", "backend"], + displayNames: ["ios": "iOS", "backend": "Backend"], + index: makeIndex([("/dev/a", ["ios"]), ("/dev/b", ["backend"])]), + pathExists: allPathsExist + ) + // header + one line per pack (sorted by id) + two trailing advice lines + #expect(lines.count == 5) + #expect(lines[0] == "2 pack(s) being installed globally are already configured in other projects:") + #expect(lines[1] == " Backend → /dev/b") + #expect(lines[2] == " iOS → /dev/a") + } } // MARK: - Guard: cwd inside ~/.claude detection