#337: Type PackUpdater update results so failures surface a non-zero exit#349
Merged
Conversation
…exit - Replace the single skipped(String) outcome with typed cases (fetch failure, broken checkout, invalid manifest, trust-declined, internal error) - Pack-update commands now exit non-zero when non-interactive (CI) or when every attempted pack failed; a trust-decline stays zero-exit - Centralize the exit-code decision so all three update callers stay in contract
There was a problem hiding this comment.
Pull request overview
This PR refines pack update outcomes so commands can distinguish expected trust declines from hard update failures and return non-zero exits when appropriate.
Changes:
- Replaces opaque skipped results with typed
PackUpdater.UpdateResultfailure/success cases. - Adds shared non-zero exit policy and applies it across pack update flows.
- Adds unit coverage for result classification, trust decline, invalid manifests, and non-interactive lockfile update failure.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Sources/mcs/Sync/PackUpdater.swift |
Introduces typed update results, hard-failure classification, and shared exit policy. |
Sources/mcs/Sync/LockfileOperations.swift |
Tracks attempted/failed packs and throws non-zero exit failures for update failures. |
Sources/mcs/Commands/UpdateCommand.swift |
Carries failed/skipped update state through reapply and delays failure signalling. |
Sources/mcs/Commands/PackCommand.swift |
Applies typed update results and non-zero exit policy to mcs pack update. |
Tests/MCSTests/PackUpdaterTests.swift |
Updates and expands PackUpdater result tests. |
Tests/MCSTests/LockfileOperationsTests.swift |
Adds non-interactive failure exit coverage for updatePacks(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fold .localCheckoutBroken into .fetchFailed (a broken checkout already surfaces as fetchFailed since update() reads HEAD first) - Reword .internalError reason to "could not verify pack scripts" (avoid mis-attributing user-environment errors to bugs) - Add shouldExitNonZero truth-table test; document deprecated sync --update pre-sync throw
- Add exit-status notes to docs/cli.md: hard failures exit non-zero in CI or when all packs fail; trust-declines stay zero
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
mcs pack update,mcs sync --update, andmcs updatealways exited zero — even when every pack failed with a transport error — because every non-success outcome was flattened into one opaque "skipped" string. CI and the--all-projectsfan-out had no failure signal, and a user declining a trust prompt looked identical to a crash. This distinguishes the outcomes so genuine failures surface a non-zero exit while an expected trust-decline stays zero.Changes
Test plan
swift testpasses locallyswiftformat --lint .andswiftlintpass without violationsmcs pack update <pack>and decline the trust prompt → expect exit code 0 and a non-scary notice.mcs update --global < /dev/null→ expect a non-zero exit.mcs updateagainst healthy packs → expect exit 0 and packs re-applied.Checklist for engine changes
LifecycleIntegrationTestsorDoctorRunnerIntegrationTests)