From 4609fbc068e0c7df2c1216bbe3cd363c5b9da5d2 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 24 Apr 2026 23:11:45 +0200 Subject: [PATCH 1/2] Prune orphan prompt values on pack removal - Drop resolvedValues entries whose keys no surviving pack declares, so later packs declaring the same key are asked fresh instead of inheriting a stale prior from a removed pack. - Apply pruning inside unconfigurePack so both mcs sync deselection and mcs pack remove federated cleanup benefit from the same logic. - Cover orphan removal and shared-key retention with lifecycle tests. --- Sources/mcs/Core/ProjectState.swift | 7 ++ Sources/mcs/Sync/Configurator.swift | 18 +++++ .../MCSTests/LifecycleIntegrationTests.swift | 77 +++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/Sources/mcs/Core/ProjectState.swift b/Sources/mcs/Core/ProjectState.swift index 022ccbe..aec5af1 100644 --- a/Sources/mcs/Core/ProjectState.swift +++ b/Sources/mcs/Core/ProjectState.swift @@ -218,6 +218,13 @@ struct ProjectState { storage.resolvedValues = values } + /// Remove resolved-value entries whose keys are not in the provided set. + mutating func pruneResolvedValues(keepingKeys keys: Set) { + guard let current = storage.resolvedValues, !current.isEmpty else { return } + let kept = current.filter { keys.contains($0.key) } + storage.resolvedValues = kept.isEmpty ? nil : kept + } + // MARK: - Persistence /// Save to disk. Updates internal state with timestamp and version. diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index dd7126c..c5008c5 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -331,6 +331,7 @@ struct Configurator { guard let artifacts = state.artifacts(for: packID) else { output.dimmed("No artifact record for \(packID) — skipping") state.removePack(packID) + pruneOrphanResolvedValues(state: &state) return } @@ -489,6 +490,23 @@ struct Configurator { state.setArtifacts(remaining, for: packID) output.warn("Some artifacts for \(packID) could not be removed. Re-run '\(scope.syncHint)' to retry.") } + pruneOrphanResolvedValues(state: &state) + } + + /// Drop `state.resolvedValues` entries whose keys are not declared by any currently-configured pack. + /// Invoked at the tail of `unconfigurePack` so both `mcs sync` deselection and `mcs pack remove` + /// federated cleanup prune orphans — a later pack declaring the same key is asked fresh instead + /// of seeing a stale "prior" from a removed pack. + private func pruneOrphanResolvedValues(state: inout ProjectState) { + let priors = state.resolvedValues ?? [:] + let survivingPacks = state.configuredPacks.compactMap { registry.pack(for: $0) } + let context = strategy.makeConfigContext( + output: output, resolvedValues: priors, priorValues: priors + ) + let declared = CrossPackPromptResolver.collectDeclaredPrompts( + packs: survivingPacks, context: context + ) + state.pruneResolvedValues(keepingKeys: Set(declared.lazy.map(\.key))) } /// Remove artifacts for components that were previously included but are now excluded. diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index 7163484..c3f4a16 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -1601,6 +1601,83 @@ struct PromptValueReuseLifecycleTests { #expect(final.resolvedValues?["KEY_A"] != "SHOULD_NOT_APPEAR") } + @Test("Removing a pack prunes its resolvedValues; a later pack with same key is asked fresh") + func removedPackOrphanPruned() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + // First sync: pack A declares BRANCH_PREFIX + let packA = MockPromptTechPack( + identifier: "pack-a", + displayName: "Pack A", + prompts: [inputPrompt("BRANCH_PREFIX")], + defaultAnswer: { "a-\($0)" } + ) + try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA])) + .configure(packs: [packA], confirmRemovals: false) + + // Seed the user's custom answer + var state = try bed.projectState() + state.setResolvedValues(["BRANCH_PREFIX": "bruno"]) + try state.save() + + // Deselect pack A: registry still knows the pack (so unconfigure can resolve + // survivors) but configure() passes an empty selection → removal triggers prune. + try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA])) + .configure(packs: [], confirmRemovals: false) + + // BRANCH_PREFIX should be pruned — no surviving pack declares it. + let afterRemoval = try bed.projectState() + #expect(afterRemoval.resolvedValues?["BRANCH_PREFIX"] == nil) + + // Pack B (different identifier) declares the same key with a different default. + let packB = MockPromptTechPack( + identifier: "pack-b", + displayName: "Pack B", + prompts: [inputPrompt("BRANCH_PREFIX")], + defaultAnswer: { "b-\($0)" } + ) + try bed.makeConfigurator(registry: TechPackRegistry(packs: [packB])) + .configure(packs: [packB], confirmRemovals: false) + + // Pack B sees no prior for BRANCH_PREFIX → mock falls back to its defaultAnswer, + // NOT the stale "bruno" from removed pack A. + let final = try bed.projectState() + #expect(final.resolvedValues?["BRANCH_PREFIX"] == "b-BRANCH_PREFIX") + } + + @Test("Shared resolved key preserved when one of two declaring packs is removed") + func sharedKeyRetainedAfterPartialRemoval() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let packA = MockPromptTechPack( + identifier: "pack-a", + displayName: "Pack A", + prompts: [inputPrompt("BRANCH_PREFIX")], + defaultAnswer: { "a-\($0)" } + ) + let packB = MockPromptTechPack( + identifier: "pack-b", + displayName: "Pack B", + prompts: [inputPrompt("BRANCH_PREFIX")], + defaultAnswer: { "b-\($0)" } + ) + try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA, packB])) + .configure(packs: [packA, packB], confirmRemovals: false) + + var state = try bed.projectState() + state.setResolvedValues(["BRANCH_PREFIX": "bruno"]) + try state.save() + + // Remove pack A; pack B still declares BRANCH_PREFIX so the value must survive. + try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA, packB])) + .configure(packs: [packB], confirmRemovals: false) + + let final = try bed.projectState() + #expect(final.resolvedValues?["BRANCH_PREFIX"] == "bruno") + } + @Test("--customize forces re-ask even when priors are available") func customizeForceReAsk() throws { let bed = try LifecycleTestBed() From c255cdd77d1cd9c7bcee3952804d40912d9b7f02 Mon Sep 17 00:00:00 2001 From: Bruno Guidolim Date: Fri, 24 Apr 2026 23:29:31 +0200 Subject: [PATCH 2/2] Apply review feedback for orphan prune - Skip pruning entirely when any configured pack is unresolvable from the registry, matching ResourceRefCounter's conservative fallback. - Cover the fail-safe with a lifecycle test that drives unconfigurePack with a narrowed registry. - Drop incidental WHAT comments in the new tests. --- Sources/mcs/Sync/Configurator.swift | 11 ++++- .../MCSTests/LifecycleIntegrationTests.swift | 44 +++++++++++++++++-- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/Sources/mcs/Sync/Configurator.swift b/Sources/mcs/Sync/Configurator.swift index c5008c5..7db328d 100644 --- a/Sources/mcs/Sync/Configurator.swift +++ b/Sources/mcs/Sync/Configurator.swift @@ -498,8 +498,17 @@ struct Configurator { /// federated cleanup prune orphans — a later pack declaring the same key is asked fresh instead /// of seeing a stale "prior" from a removed pack. private func pruneOrphanResolvedValues(state: inout ProjectState) { + var survivingPacks: [any TechPack] = [] + for packID in state.configuredPacks { + guard let pack = registry.pack(for: packID) else { + // Conservative fallback matching ResourceRefCounter: if any configured pack + // can't be loaded from the registry, we can't enumerate its declared keys, + // so skip pruning rather than risk dropping values that still belong. + return + } + survivingPacks.append(pack) + } let priors = state.resolvedValues ?? [:] - let survivingPacks = state.configuredPacks.compactMap { registry.pack(for: $0) } let context = strategy.makeConfigContext( output: output, resolvedValues: priors, priorValues: priors ) diff --git a/Tests/MCSTests/LifecycleIntegrationTests.swift b/Tests/MCSTests/LifecycleIntegrationTests.swift index c3f4a16..3392368 100644 --- a/Tests/MCSTests/LifecycleIntegrationTests.swift +++ b/Tests/MCSTests/LifecycleIntegrationTests.swift @@ -1606,7 +1606,6 @@ struct PromptValueReuseLifecycleTests { let bed = try LifecycleTestBed() defer { bed.cleanup() } - // First sync: pack A declares BRANCH_PREFIX let packA = MockPromptTechPack( identifier: "pack-a", displayName: "Pack A", @@ -1616,7 +1615,6 @@ struct PromptValueReuseLifecycleTests { try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA])) .configure(packs: [packA], confirmRemovals: false) - // Seed the user's custom answer var state = try bed.projectState() state.setResolvedValues(["BRANCH_PREFIX": "bruno"]) try state.save() @@ -1630,7 +1628,6 @@ struct PromptValueReuseLifecycleTests { let afterRemoval = try bed.projectState() #expect(afterRemoval.resolvedValues?["BRANCH_PREFIX"] == nil) - // Pack B (different identifier) declares the same key with a different default. let packB = MockPromptTechPack( identifier: "pack-b", displayName: "Pack B", @@ -1670,7 +1667,7 @@ struct PromptValueReuseLifecycleTests { state.setResolvedValues(["BRANCH_PREFIX": "bruno"]) try state.save() - // Remove pack A; pack B still declares BRANCH_PREFIX so the value must survive. + // Pack B still declares BRANCH_PREFIX, so the value must survive removal of A. try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA, packB])) .configure(packs: [packB], confirmRemovals: false) @@ -1678,6 +1675,45 @@ struct PromptValueReuseLifecycleTests { #expect(final.resolvedValues?["BRANCH_PREFIX"] == "bruno") } + @Test("Pruning skips when a configured survivor pack is missing from the registry") + func pruningSkippedWhenSurvivorUnresolvable() throws { + let bed = try LifecycleTestBed() + defer { bed.cleanup() } + + let packA = MockPromptTechPack( + identifier: "pack-a", + displayName: "Pack A", + prompts: [inputPrompt("KEY_A")], + defaultAnswer: { "a-\($0)" } + ) + let packB = MockPromptTechPack( + identifier: "pack-b", + displayName: "Pack B", + prompts: [inputPrompt("KEY_B")], + defaultAnswer: { "b-\($0)" } + ) + try bed.makeConfigurator(registry: TechPackRegistry(packs: [packA, packB])) + .configure(packs: [packA, packB], confirmRemovals: false) + + var state = try bed.projectState() + state.setResolvedValues(["KEY_A": "user-a", "KEY_B": "user-b"]) + try state.save() + + // Direct unconfigure with a registry that omits pack-a simulates pack-a's + // directory being manually removed from ~/.mcs/packs/ — pack-a stays in + // state.configuredPacks but can no longer be resolved. The prune helper + // must refuse to run rather than silently drop keys that still belong. + state = try bed.projectState() + let narrowConfigurator = bed.makeConfigurator( + registry: TechPackRegistry(packs: [packB]) + ) + narrowConfigurator.unconfigurePack("pack-b", state: &state) + try state.save() + + let final = try bed.projectState() + #expect(final.resolvedValues?["KEY_A"] == "user-a") + } + @Test("--customize forces re-ask even when priors are available") func customizeForceReAsk() throws { let bed = try LifecycleTestBed()