From a21e02f0099b7a91dd3fe470a7bbab17577efb8b Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 2 Oct 2026 11:46:59 +0200 Subject: [PATCH 1/3] ISSUE-449: Track pack settings ownership per sub-key - Record only the sub-keys a pack's settings file actually wrote, warning when an existing value wins - Project sync merges onto the user's settings.local.json instead of rebuilding it from empty - Narrow legacy whole-object records to the keys the pack declares, leaving unknown ones untouched --- Sources/mcs/Core/Settings.swift | 89 ++++++++++--- Sources/mcs/Sync/Configurator.swift | 31 ++++- Sources/mcs/Sync/ConfiguratorSupport.swift | 67 +++++++++- Sources/mcs/Sync/GlobalSyncStrategy.swift | 2 +- Sources/mcs/Sync/ProjectSyncStrategy.swift | 22 +++- .../MCSTests/LifecycleIntegrationTests.swift | 117 +++++++++++++++++- Tests/MCSTests/SettingsMergeTests.swift | 23 +++- 7 files changed, 315 insertions(+), 36 deletions(-) diff --git a/Sources/mcs/Core/Settings.swift b/Sources/mcs/Core/Settings.swift index e8f75bd5..ba616eee 100644 --- a/Sources/mcs/Core/Settings.swift +++ b/Sources/mcs/Core/Settings.swift @@ -207,11 +207,12 @@ struct Settings: Codable { /// scalars/arrays use "existing wins" semantics. /// /// - Returns: The hook groups dropped by deduplication whose matcher disagreed with the group - /// that survived. An identical duplicate is the ordinary case and is not reported. Callers - /// with somewhere to report to should surface these; the result is discardable because most - /// callers merge settings that cannot collide. + /// that survived (an identical duplicate is the ordinary case and is not reported), and the + /// extra-JSON key paths `other` actually wrote versus the ones it lost to an existing, + /// different value. Callers with somewhere to report to should surface the losses; the + /// result is discardable because most callers merge settings that cannot collide. @discardableResult - mutating func merge(with other: Settings) -> [DroppedHookGroup] { + mutating func merge(with other: Settings) -> MergeReport { var dropped: [DroppedHookGroup] = [] // Hooks: deduplicate by command @@ -259,27 +260,75 @@ struct Settings: Codable { // Extra JSON: generic merge for all non-typed keys (env, permissions, // alwaysThinkingEnabled, attribution, and any future keys). - for (key, valueData) in other.extraJSON { - if let existingData = extraJSON[key] { - // Both have the key — attempt dict-level merge if both are JSON objects - if let selfDict = try? JSONSerialization.jsonObject(with: existingData) as? [String: Any], - let otherDict = try? JSONSerialization.jsonObject(with: valueData) as? [String: Any] { - var merged = selfDict - for (k, v) in otherDict where merged[k] == nil { - merged[k] = v - } - if let data = try? JSONSerialization.data(withJSONObject: merged) { - extraJSON[key] = data + var landed: [String] = [] + var skipped: [String] = [] + for (key, valueData) in other.extraJSON.sorted(by: { $0.key < $1.key }) { + guard let existingData = extraJSON[key] else { + extraJSON[key] = valueData + landed.append(contentsOf: Self.keyPaths(of: key, in: valueData)) + continue + } + // Both have the key — attempt dict-level merge if both are JSON objects + if let selfDict = try? JSONSerialization.jsonObject(with: existingData) as? [String: Any], + let otherDict = try? JSONSerialization.jsonObject(with: valueData) as? [String: Any] { + var merged = selfDict + var landedHere: [String] = [] + for (subKey, value) in otherDict.sorted(by: { $0.key < $1.key }) { + if let current = merged[subKey] { + if !Self.jsonEqual(current, value) { skipped.append("\(key).\(subKey)") } + } else { + merged[subKey] = value + landedHere.append("\(key).\(subKey)") } - // Re-serialization failure: keep existing value (no-op) } - // Non-dict: existing wins (no action) - } else { - extraJSON[key] = valueData + if let data = try? JSONSerialization.data(withJSONObject: merged) { + extraJSON[key] = data + landed.append(contentsOf: landedHere) + } + // Re-serialization failure: keep existing value (no-op) + } else if !Self.jsonFragmentsEqual(existingData, valueData) { + // Non-dict: existing wins + skipped.append(key) } } - return dropped + return MergeReport(droppedHookGroups: dropped, landedKeyPaths: landed, skippedKeyPaths: skipped) + } + + struct MergeReport { + var droppedHookGroups: [DroppedHookGroup] = [] + var landedKeyPaths: [String] = [] + var skippedKeyPaths: [String] = [] + } + + // `save(to:dropKeys:)` must not re-inject any of these from the destination file: the + // in-memory value is already the destination's minus what was owned, so a key left empty + // once its owned sub-keys are removed has to stay gone. + static func topLevelKeys(of keyPaths: [String]) -> Set { + Set(keyPaths.map { String($0.split(separator: ".", maxSplits: 1)[0]) }) + } + + /// One level down for objects, so a pack that writes `env.FOO` never claims the user's `env.BAR`. + private static func keyPaths(of key: String, in valueData: Data) -> [String] { + guard let dict = try? JSONSerialization.jsonObject(with: valueData) as? [String: Any], + !dict.isEmpty + else { + return [key] + } + return dict.keys.sorted().map { "\(key).\($0)" } + } + + private static func jsonEqual(_ lhs: Any, _ rhs: Any) -> Bool { + (lhs as AnyObject).isEqual(rhs) + } + + private static func jsonFragmentsEqual(_ lhs: Data, _ rhs: Data) -> Bool { + guard let left = try? JSONSerialization.jsonObject(with: lhs, options: .fragmentsAllowed), + let right = try? JSONSerialization.jsonObject(with: rhs, options: .fragmentsAllowed) + else { + return lhs == rhs + } + return jsonEqual(left, right) } // MARK: - Stale key removal diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index 528b8f6e..385db181 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -478,6 +478,8 @@ struct Configurator { // Remove auto-derived hook commands and contributed settings keys let hasHooksToRemove = !artifacts.hookCommands.isEmpty + // Keyed to the record, not its expansion: a legacy key left untouched still has to be + // cleared from state, or every later removal warns about it again. let hasSettingsToRemove = !artifacts.settingsKeys.isEmpty if hasHooksToRemove || hasSettingsToRemove { var settings: Settings @@ -490,6 +492,9 @@ struct Configurator { output.warn("Some artifacts for \(packID) could not be removed. Re-run '\(retryHint)' to retry.") return false } + let settingsKeys = expandedSettingsKeys( + artifacts.settingsKeys, pack: registry.pack(for: packID), packID: packID, onDisk: settings + ) if hasHooksToRemove { let commandsToRemove = Set(artifacts.hookCommands) if var hooks = settings.hooks { @@ -504,10 +509,10 @@ struct Configurator { } } if hasSettingsToRemove { - settings.removeKeys(artifacts.settingsKeys) + settings.removeKeys(settingsKeys) } do { - let dropKeys = Set(artifacts.settingsKeys.filter { !$0.contains(".") }) + let dropKeys = Settings.topLevelKeys(of: settingsKeys) try settings.save(to: scope.settingsPath, dropKeys: dropKeys) remaining.hookCommands = [] remaining.settingsKeys = [] @@ -515,7 +520,7 @@ struct Configurator { for cmd in artifacts.hookCommands { output.dimmed(" Removed hook: \(cmd)") } - for key in artifacts.settingsKeys { + for key in settingsKeys { output.dimmed(" Removed setting: \(key)") } } catch { @@ -875,11 +880,20 @@ struct Configurator { var previousSettingsKeys: [String: [String]] = [:] var previousTemplateSections: [String: [String]] = [:] + // Composition reloads this and reports a parse failure; an unreadable file simply leaves + // legacy records unexpanded here. + var onDiskSettings = Settings() + do { + onDiskSettings = try Settings.load(from: scope.settingsPath) + } catch {} + for pack in packs { let previousArtifacts = state.artifacts(for: pack.identifier) // Snapshot previous metadata before overwriting (needed by steps 6-7) - previousSettingsKeys[pack.identifier] = previousArtifacts?.settingsKeys ?? [] + previousSettingsKeys[pack.identifier] = expandedSettingsKeys( + previousArtifacts?.settingsKeys ?? [], pack: pack, packID: pack.identifier, onDisk: onDiskSettings + ) previousTemplateSections[pack.identifier] = previousArtifacts?.templateSections ?? [] let isNew = additions.contains(pack.identifier) @@ -1244,6 +1258,15 @@ struct Configurator { } } + private func expandedSettingsKeys( + _ keyPaths: [String], pack: (any TechPack)?, packID: String, onDisk: Settings + ) -> [String] { + ConfiguratorSupport.expandLegacySettingsKeys( + keyPaths, pack: pack, packID: packID, onDisk: onDisk, + settingsFileName: scope.settingsPath.lastPathComponent, output: output + ) + } + // MARK: - Global Dependencies /// Auto-install brew packages and plugins (project scope only). diff --git a/Sources/mcs/Sync/ConfiguratorSupport.swift b/Sources/mcs/Sync/ConfiguratorSupport.swift index cc712b2e..5e7f1b2d 100644 --- a/Sources/mcs/Sync/ConfiguratorSupport.swift +++ b/Sources/mcs/Sync/ConfiguratorSupport.swift @@ -404,10 +404,19 @@ enum ConfiguratorSupport { guard case let .settingsMerge(source) = component.installAction, let source else { continue } do { let packSettings = try Settings.load(from: source, substituting: resolvedValues) - if !packSettings.extraJSON.isEmpty { - contributedKeys[pack.identifier, default: []].append(contentsOf: packSettings.extraJSON.keys) + let report = settings.merge(with: packSettings) + // Only what landed is owned: a key the user (or an earlier pack) already set stays + // theirs, so removing this pack can never delete it. + if !report.landedKeyPaths.isEmpty { + contributedKeys[pack.identifier, default: []].append(contentsOf: report.landedKeyPaths) } - for dropped in settings.merge(with: packSettings) { + for keyPath in report.skippedKeyPaths { + output.warn( + "\(pack.displayName): '\(keyPath)' from \(source.lastPathComponent) was not applied" + + " — a different value is already set, and it is kept" + ) + } + for dropped in report.droppedHookGroups { output.warn( "\(pack.displayName): hook group for '\(dropped.command)' under \(dropped.event)" + " was not merged — \(source.lastPathComponent) declares matcher" @@ -426,6 +435,58 @@ enum ConfiguratorSupport { return (hasContent, contributedKeys) } + /// State written before per-sub-key ownership records a whole object such as `env`, and + /// stripping it would delete the user's entries inside it. Narrow it to the sub-keys the pack + /// declares today; when that can't be known, leave the object alone rather than guess. + static func expandLegacySettingsKeys( + _ keyPaths: [String], + pack: (any TechPack)?, + packID: String, + onDisk: Settings, + settingsFileName: String, + output: CLIOutput + ) -> [String] { + let objectKeys = Set(keyPaths.filter { key in + guard !key.contains("."), let data = onDisk.extraJSON[key] else { return false } + return (try? JSONSerialization.jsonObject(with: data)) is [String: Any] + }) + guard !objectKeys.isEmpty else { return keyPaths } + + // Merging into an empty value records exactly the paths a real sync would own. + var declared: [String: [String]] = [:] + for component in pack?.components ?? [] { + guard case let .settingsMerge(source) = component.installAction, let source else { continue } + do { + var scratch = Settings() + for keyPath in try scratch.merge(with: Settings.load(from: source)).landedKeyPaths { + let topLevel = String(keyPath.split(separator: ".", maxSplits: 1)[0]) + if objectKeys.contains(topLevel), keyPath != topLevel { + declared[topLevel, default: []].append(keyPath) + } + } + } catch { + output.warn("Could not read \(source.lastPathComponent) from \(packID): \(error.localizedDescription)") + } + } + + var expanded: [String] = [] + for keyPath in keyPaths { + guard objectKeys.contains(keyPath) else { + expanded.append(keyPath) + continue + } + if let subKeys = declared[keyPath] { + expanded.append(contentsOf: subKeys) + } else { + output.warn( + "\(packID): leaving '\(keyPath)' in \(settingsFileName) untouched — the pack's" + + " entries in it can't be told apart from yours. Remove them by hand if they're stale." + ) + } + } + return expanded + } + /// Compute per-pack SHA-256 hashes of contributed settings values from the on-disk file. /// /// Reads the settings file once and hashes each pack's key-value pairs independently. diff --git a/Sources/mcs/Sync/GlobalSyncStrategy.swift b/Sources/mcs/Sync/GlobalSyncStrategy.swift index ae2ab950..656b5e63 100644 --- a/Sources/mcs/Sync/GlobalSyncStrategy.swift +++ b/Sources/mcs/Sync/GlobalSyncStrategy.swift @@ -211,7 +211,7 @@ struct GlobalSyncStrategy: SyncStrategy { } // Collect top-level keys to pass as dropKeys, preventing Layer 3 re-injection - let dropKeys = Set(allPreviousKeys.filter { !$0.contains(".") }) + let dropKeys = Settings.topLevelKeys(of: allPreviousKeys) var (hasContent, contributedKeys) = ConfiguratorSupport.mergePackComponentsIntoSettings( packs: packs, diff --git a/Sources/mcs/Sync/ProjectSyncStrategy.swift b/Sources/mcs/Sync/ProjectSyncStrategy.swift index 7fff3695..1e26a4e3 100644 --- a/Sources/mcs/Sync/ProjectSyncStrategy.swift +++ b/Sources/mcs/Sync/ProjectSyncStrategy.swift @@ -142,9 +142,21 @@ struct ProjectSyncStrategy: SyncStrategy { ) throws -> (contributedKeys: [String: [String]], settingsHashes: [String: String]) { var settings = Settings() - // Collect top-level keys to prevent Layer 3 re-injection of stale extraJSON keys let allPreviousKeys = previousSettingsKeys.values.flatMap(\.self) - let dropKeys = Set(allPreviousKeys.filter { !$0.contains(".") }) + let dropKeys = Settings.topLevelKeys(of: allPreviousKeys) + // Start from what is already there minus what packs owned, as global scope does, so a pack + // value merges under the user's (Claude Code writes `permissions` here too) instead of + // replacing the whole object. + do { + var existing = try Settings.load(from: scope.settingsPath) + existing.removeKeys(allPreviousKeys) + settings.extraJSON = existing.extraJSON + } catch { + output.warn( + "Could not parse \(scope.settingsPath.lastPathComponent): \(error.localizedDescription)" + + " — its contents will be replaced" + ) + } let (hasContent, contributedKeys) = ConfiguratorSupport.mergePackComponentsIntoSettings( packs: packs, @@ -154,10 +166,12 @@ struct ProjectSyncStrategy: SyncStrategy { output: output ) - if hasContent { + if hasContent || !settings.extraJSON.isEmpty { do { try settings.save(to: scope.settingsPath, dropKeys: dropKeys) - output.success("Composed \(scope.settingsPath.lastPathComponent)") + if hasContent { + output.success("Composed \(scope.settingsPath.lastPathComponent)") + } } catch { output.error("Could not write \(scope.settingsPath.lastPathComponent): \(error.localizedDescription)") output.error("Hooks and plugins will not be active. Re-run '\(scope.syncHint)' after fixing the issue.") diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index e1a0311d..dcc2e63d 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -324,7 +324,7 @@ struct SinglePackLifecycleTests { let artifacts = state.artifacts(for: "test-pack") #expect(artifacts != nil) #expect(artifacts?.templateSections.contains("test-pack") == true) - #expect(artifacts?.settingsKeys.contains("env") == true) + #expect(artifacts?.settingsKeys.contains("env.LINT_ENABLED") == true) #expect(artifacts?.hookCommands.contains(bed.projectHookCommand("test-pack/lint.sh")) == true) #expect(artifacts?.mcpServers.contains { $0.name == "test-mcp" } == true) @@ -4297,3 +4297,118 @@ struct PackRemoveCleanupFailureTests { #expect(bed.mockCLI.mcpRemoveCalls.allSatisfy { $0.workingDirectory == bed.project }) } } + +// MARK: - Settings sub-key ownership + +struct SettingsSubKeyOwnershipTests { + private func readJSON(_ url: URL) throws -> [String: Any] { + try #require(JSONSerialization.jsonObject(with: Data(contentsOf: url)) as? [String: Any]) + } + + private func writeJSON(_ json: [String: Any], to url: URL) throws { + try FileManager.default.createDirectory( + at: url.deletingLastPathComponent(), withIntermediateDirectories: true + ) + try JSONSerialization.data(withJSONObject: json).write(to: url) + } + + private func envPack(_ bed: LifecycleTestBed) throws -> MockTechPack { + let source = try bed.makeSettingsSource(content: """ + { "env": { "PACK_VAR": "from-pack", "SHARED": "pack-value" } } + """) + return MockTechPack( + identifier: "env-pack", + displayName: "Env Pack", + components: [bed.settingsComponent(pack: "env-pack", id: "settings", source: source)] + ) + } + + @Test("Global: the user's env entries survive re-sync and removal, and win a conflict") + func globalUserEnvSurvives() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + try writeJSON(["env": ["USER_VAR": "mine", "SHARED": "user-value"]], to: bed.env.claudeSettings) + let pack = try envPack(bed) + let registry = TechPackRegistry(packs: [pack]) + + for _ in 0 ..< 2 { + try bed.makeGlobalSyncConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + let env = try #require(readJSON(bed.env.claudeSettings)["env"] as? [String: String]) + #expect(env == ["USER_VAR": "mine", "SHARED": "user-value", "PACK_VAR": "from-pack"]) + } + #expect(try bed.globalState().artifacts(for: "env-pack")?.settingsKeys == ["env.PACK_VAR"]) + + try bed.makeGlobalSyncConfigurator(registry: registry).configure(packs: [], confirmRemovals: false) + let env = try #require(readJSON(bed.env.claudeSettings)["env"] as? [String: String]) + #expect(env == ["USER_VAR": "mine", "SHARED": "user-value"]) + } + + @Test("Project: entries Claude Code or the user wrote to settings.local.json survive sync and removal") + func projectUserEntriesSurvive() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + try writeJSON( + ["env": ["USER_VAR": "mine"], "permissions": ["allow": ["Bash(ls)"]]], + to: bed.settingsLocalPath + ) + let pack = try envPack(bed) + let registry = TechPackRegistry(packs: [pack]) + + try bed.makeConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + var json = try readJSON(bed.settingsLocalPath) + #expect(json["env"] as? [String: String] == ["USER_VAR": "mine", "PACK_VAR": "from-pack", "SHARED": "pack-value"]) + #expect((json["permissions"] as? [String: [String]])?["allow"] == ["Bash(ls)"]) + + try bed.makeConfigurator(registry: registry).configure(packs: [], confirmRemovals: false) + json = try readJSON(bed.settingsLocalPath) + #expect(json["env"] as? [String: String] == ["USER_VAR": "mine"]) + #expect((json["permissions"] as? [String: [String]])?["allow"] == ["Bash(ls)"]) + } + + @Test("A legacy whole-object record is narrowed to the keys the pack declares") + func legacyRecordIsExpanded() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let pack = try envPack(bed) + let registry = TechPackRegistry(packs: [pack]) + try bed.makeGlobalSyncConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + + // What an older release recorded, plus a user entry added afterwards. + var state = try seedLegacyRecord(bed) + try state.save() + var json = try readJSON(bed.env.claudeSettings) + var env = try #require(json["env"] as? [String: String]) + env["USER_VAR"] = "mine" + json["env"] = env + try writeJSON(json, to: bed.env.claudeSettings) + + try bed.makeGlobalSyncConfigurator(registry: registry).configure(packs: [pack], confirmRemovals: false) + + let after = try #require(readJSON(bed.env.claudeSettings)["env"] as? [String: String]) + #expect(after["USER_VAR"] == "mine") + #expect(try bed.globalState().artifacts(for: "env-pack")?.settingsKeys == ["env.PACK_VAR", "env.SHARED"]) + } + + @Test("A legacy whole-object record for an unloadable pack is left in place and cleared from state") + func legacyRecordForUnloadablePackIsLeftAlone() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + let pack = try envPack(bed) + try bed.makeGlobalSyncConfigurator(registry: TechPackRegistry(packs: [pack])) + .configure(packs: [pack], confirmRemovals: false) + var state = try seedLegacyRecord(bed) + + bed.makeGlobalSyncConfigurator(registry: TechPackRegistry()).unconfigurePack("env-pack", state: &state) + + #expect(try readJSON(bed.env.claudeSettings)["env"] != nil) + #expect(state.artifacts(for: "env-pack")?.settingsKeys.isEmpty ?? true) + } + + private func seedLegacyRecord(_ bed: LifecycleTestBed) throws -> ProjectState { + var state = try bed.globalState() + var record = try #require(state.artifacts(for: "env-pack")) + record.settingsKeys = ["env"] + state.setArtifacts(record, for: "env-pack") + return state + } +} diff --git a/Tests/MCSTests/SettingsMergeTests.swift b/Tests/MCSTests/SettingsMergeTests.swift index aad05f9d..889cdea4 100644 --- a/Tests/MCSTests/SettingsMergeTests.swift +++ b/Tests/MCSTests/SettingsMergeTests.swift @@ -124,7 +124,7 @@ struct SettingsMergeTests { ) var base = Settings(hooks: ["PreToolUse": [group]]) - let dropped = base.merge(with: Settings(hooks: ["PreToolUse": [group]])) + let dropped = base.merge(with: Settings(hooks: ["PreToolUse": [group]])).droppedHookGroups // The ordinary case — a pack repeating a hook it already declared as a component. Nothing // is lost, so warning about it would be noise. @@ -151,7 +151,7 @@ struct SettingsMergeTests { ], ]) - let dropped = base.merge(with: other) + let dropped = base.merge(with: other).droppedHookGroups // Dedup by command keeps the installed group, so the incoming matcher never takes effect. // Silently discarding it is how a hook ends up registered under a matcher nobody asked for. @@ -182,7 +182,24 @@ struct SettingsMergeTests { ], ]) - #expect(base.merge(with: other).isEmpty) + #expect(base.merge(with: other).droppedHookGroups.isEmpty) + } + + @Test("Merge reports the sub-keys it wrote and the ones an existing different value kept") + func mergeReportsLandedAndSkippedKeyPaths() throws { + var base = Settings() + base.extraJSON["env"] = try JSONSerialization.data(withJSONObject: ["USER": "u", "SAME": "x", "CLASH": "user"]) + base.extraJSON["model"] = try JSONSerialization.data(withJSONObject: "opus", options: .fragmentsAllowed) + var other = Settings() + other.extraJSON["env"] = try JSONSerialization.data(withJSONObject: ["NEW": "n", "SAME": "x", "CLASH": "pack"]) + other.extraJSON["model"] = try JSONSerialization.data(withJSONObject: "sonnet", options: .fragmentsAllowed) + other.extraJSON["attribution"] = try JSONSerialization.data(withJSONObject: ["commit": ""]) + + let report = base.merge(with: other) + + #expect(report.landedKeyPaths == ["attribution.commit", "env.NEW"]) + // An identical value is not a conflict, so it is neither owned nor reported. + #expect(report.skippedKeyPaths == ["env.CLASH", "model"]) } @Test("Hooks merge across different events") From 4a2beaf62ecd4a63a29aef6d9212402ba599e73f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 04:06:05 +0000 Subject: [PATCH 2/3] ISSUE-449: Reset settings ownership for packs that land no keys A pack whose settings values were all already set kept its previous settingsKeys, so removing it could delete a key another pack had since landed, and a legacy whole-object record was never replaced. Narrow the project-sync comment: array values such as permissions.allow are still owned whole. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01CqExB5C9uXqPH2vKqj6dqa --- Sources/mcs/Sync/Configurator.swift | 15 ++++----- Sources/mcs/Sync/ProjectSyncStrategy.swift | 5 +-- .../MCSTests/LifecycleIntegrationTests.swift | 31 +++++++++++++++++++ 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index 385db181..05fbdac4 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -342,13 +342,14 @@ struct Configurator { resolvedValues: allValues, output: output ) - // 6b. Record contributed settings keys and value hashes in artifact records - for (packID, keys) in contributedKeys { - if var artifacts = state.artifacts(for: packID) { - artifacts.settingsKeys = keys - artifacts.settingsHash = settingsHashes[packID] - state.setArtifacts(artifacts, for: packID) - } + // 6b. Record contributed settings keys and value hashes in artifact records. Every pack is + // reset, not just the ones that landed a key: a pack whose values were all already set + // owns nothing now, and a stale claim would let its removal delete another pack's key. + for pack in packs { + guard var artifacts = state.artifacts(for: pack.identifier) else { continue } + artifacts.settingsKeys = contributedKeys[pack.identifier] ?? [] + artifacts.settingsHash = settingsHashes[pack.identifier] + state.setArtifacts(artifacts, for: pack.identifier) } // 7. Compose CLAUDE markdown file diff --git a/Sources/mcs/Sync/ProjectSyncStrategy.swift b/Sources/mcs/Sync/ProjectSyncStrategy.swift index 1e26a4e3..c524b1db 100644 --- a/Sources/mcs/Sync/ProjectSyncStrategy.swift +++ b/Sources/mcs/Sync/ProjectSyncStrategy.swift @@ -145,8 +145,9 @@ struct ProjectSyncStrategy: SyncStrategy { let allPreviousKeys = previousSettingsKeys.values.flatMap(\.self) let dropKeys = Settings.topLevelKeys(of: allPreviousKeys) // Start from what is already there minus what packs owned, as global scope does, so a pack - // value merges under the user's (Claude Code writes `permissions` here too) instead of - // replacing the whole object. + // value merges under the user's instead of replacing the whole object. Ownership stops one + // level down: an array such as `permissions.allow` is owned or skipped whole, so Claude + // Code's approvals in one a pack owns are still rewritten on sync. do { var existing = try Settings.load(from: scope.settingsPath) existing.removeKeys(allPreviousKeys) diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index dcc2e63d..c16fb46f 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -4404,6 +4404,37 @@ struct SettingsSubKeyOwnershipTests { #expect(state.artifacts(for: "env-pack")?.settingsKeys.isEmpty ?? true) } + @Test("A pack whose value another pack already landed gives up its claim, so its removal keeps the key") + func shadowedPackReleasesOwnership() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + func sharedPack(_ id: String) throws -> MockTechPack { + let source = try bed.makeSettingsSource(content: """ + { "env": { "SHARED": "same" } } + """) + return MockTechPack( + identifier: id, + displayName: id, + components: [bed.settingsComponent(pack: id, id: "settings", source: source)] + ) + } + let first = try sharedPack("first-pack") + let second = try sharedPack("second-pack") + let registry = TechPackRegistry(packs: [first, second]) + + try bed.makeGlobalSyncConfigurator(registry: registry).configure(packs: [second], confirmRemovals: false) + #expect(try bed.globalState().artifacts(for: "second-pack")?.settingsKeys == ["env.SHARED"]) + + try bed.makeGlobalSyncConfigurator(registry: registry).configure(packs: [first, second], confirmRemovals: false) + var state = try bed.globalState() + #expect(state.artifacts(for: "first-pack")?.settingsKeys == ["env.SHARED"]) + #expect(state.artifacts(for: "second-pack")?.settingsKeys.isEmpty == true) + + bed.makeGlobalSyncConfigurator(registry: registry).unconfigurePack("second-pack", state: &state) + let env = try #require(readJSON(bed.env.claudeSettings)["env"] as? [String: String]) + #expect(env["SHARED"] == "same") + } + private func seedLegacyRecord(_ bed: LifecycleTestBed) throws -> ProjectState { var state = try bed.globalState() var record = try #require(state.artifacts(for: "env-pack")) From cd950fac3bc876cd81676d38b3c308351212e340 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Thu, 8 Oct 2026 16:52:00 +0200 Subject: [PATCH 3/3] ISSUE-449: Correct settings composition comments - Project sync carries over existing extra keys only; hooks and plugins are still rebuilt from packs - Legacy expansion yields every declared path, the upper bound of what a sync could own --- Sources/mcs/Sync/ConfiguratorSupport.swift | 2 +- Sources/mcs/Sync/ProjectSyncStrategy.swift | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Sources/mcs/Sync/ConfiguratorSupport.swift b/Sources/mcs/Sync/ConfiguratorSupport.swift index 5e7f1b2d..f3d36302 100644 --- a/Sources/mcs/Sync/ConfiguratorSupport.swift +++ b/Sources/mcs/Sync/ConfiguratorSupport.swift @@ -452,7 +452,7 @@ enum ConfiguratorSupport { }) guard !objectKeys.isEmpty else { return keyPaths } - // Merging into an empty value records exactly the paths a real sync would own. + // Merging into an empty value yields every path the pack declares: the most a sync could own. var declared: [String: [String]] = [:] for component in pack?.components ?? [] { guard case let .settingsMerge(source) = component.installAction, let source else { continue } diff --git a/Sources/mcs/Sync/ProjectSyncStrategy.swift b/Sources/mcs/Sync/ProjectSyncStrategy.swift index c524b1db..00a0715c 100644 --- a/Sources/mcs/Sync/ProjectSyncStrategy.swift +++ b/Sources/mcs/Sync/ProjectSyncStrategy.swift @@ -144,10 +144,11 @@ struct ProjectSyncStrategy: SyncStrategy { let allPreviousKeys = previousSettingsKeys.values.flatMap(\.self) let dropKeys = Settings.topLevelKeys(of: allPreviousKeys) - // Start from what is already there minus what packs owned, as global scope does, so a pack - // value merges under the user's instead of replacing the whole object. Ownership stops one - // level down: an array such as `permissions.allow` is owned or skipped whole, so Claude - // Code's approvals in one a pack owns are still rewritten on sync. + // Start from the existing extra keys minus what packs owned, so a pack value merges under + // the user's instead of replacing the whole object. Hooks and plugins are still rebuilt + // from the packs alone. Ownership stops one level down: an array such as + // `permissions.allow` is owned or skipped whole, so Claude Code's approvals in one a pack + // owns are still rewritten on sync. do { var existing = try Settings.load(from: scope.settingsPath) existing.removeKeys(allPreviousKeys)