ISSUE-373: Regression test: 'mcs update' must not remove both-scope packs - #383
Merged
Merged
Conversation
…th scopes - Extract the per-scope re-apply into UpdateCommand.reapplyScope so a sandboxed test can drive the real resolver → configure chain - Add a lifecycle test that installs a pack in both scopes, re-applies via --project and bare update, and asserts the pack and its hook survive - Add LifecycleTestBed.globalState() alongside projectState() Claude-Session: https://claude.ai/code/session_01MkBJdGBUoZ2aRtchjaKXMN
There was a problem hiding this comment.
🟡 Changes recommended
The re-apply implementation still filters out skipped/unresolved packs before calling Configurator.configure(confirmRemovals: false), which can silently unconfigure packs and delete artifacts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an end-to-end regression test to ensure mcs update’s per-scope “re-apply” phase cannot accidentally remove a pack that is installed in both global + project scopes, and refactors the update implementation to make that re-apply path directly testable from the integration suite.
Changes:
- Extracts the per-scope update re-apply logic into a new
UpdateCommand.reapplyScopestatic helper so integration tests can drive the real resolver → configurator chain in a sandboxed environment. - Adds a lifecycle integration test that installs the same hook pack in both scopes, deletes the project hook artifact, runs the update re-apply flow, and asserts both-scope configuration + artifact restoration.
- Adds a
LifecycleTestBed.globalState()convenience accessor for reading sandboxed global state.
File summaries
| File | Description |
|---|---|
| Sources/mcs/Commands/UpdateCommand.swift | Extracts re-apply logic into a static helper to enable targeted, sandboxed integration testing. |
| Tests/MCSTests/LifecycleIntegrationTests.swift | Adds an integration regression test ensuring both-scope packs survive update re-apply and restores a removed hook artifact. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+346
to
350
| var packs: [any TechPack] = [] | ||
| for packID in run.configuredPackIDs.subtracting(skippedPackIDs).sorted() { | ||
| guard let pack = registry.pack(for: packID) else { | ||
| output.warn(" \(packID): tracked in state but missing from pack registry — skipping. Run 'mcs pack add' to restore it.") | ||
| continue |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #373.
mcs updatere-applies each scope's already-configured pack set, so the global-pack block from #370 never reaches it and a pack installed in both scopes survives. That safety was a property of where the update path sourced its list, not something any test asserted, and an earlier--packiteration broke exactly this by filtering the list. This pins it.While writing the test I found that the same re-apply path drops skipped packs (fetch failed, trust declined, missing from registry) from the list, which unconfigures them when a scope holds two or more packs. Filed as #382; not fixed here.
Changes
--projectand baremcs update, and asserts the pack is still configured in both scopes and the hook was restored (proving the re-apply ran rather than returned early).Test plan
swift testpasses locallyswiftformat --lint .andswiftlintpass without violationsmcs sync,mcs doctor)Manual: with a pack in both scopes, run
mcs update --projectandmcs update→ expect the pack still listed in the project's.claude/.mcs-projectand its artifacts intact.Checklist for engine changes
LifecycleIntegrationTestsorDoctorRunnerIntegrationTests)https://claude.ai/code/session_01MkBJdGBUoZ2aRtchjaKXMN