fix(store): apply repairable cloud-upgrade mutations even when a blocker is queued#452
Merged
Merged
Conversation
…ker is queued (#446) Previously RepairCloudUpgrade returned Applied:false and skipped all repairs when BlockedCount > 0, even if RepairableCount was also > 0. The message also always picked Findings[0] (lowest seq, often a repairable entry) instead of the first non-repairable finding, and omitted entity_key. Now the repairable subset is applied first inside the existing transaction path; the blocked report then carries Applied:true, accurate counts, and a message that scans Findings for the first non-repairable entry (with entity_key) so operators see the actual blocker.
There was a problem hiding this comment.
Pull request overview
Fixes issue #446 where engram cloud upgrade repair --apply aborted the entire repair pass when any non-repairable blocker existed among findings, and produced a misleading manual-action message pointing at Findings[0] (lowest seq) rather than the actual blocker.
Changes:
- In
RepairCloudUpgrade, apply the repairable subset whenapply=trueregardless of whether a blocker is present, then returnClass=BlockedwithApplied=true. - Select the message finding by scanning for the first non-repairable entry (instead of
Findings[0]) and includeentity_keyin the formatted message; differentiate messages forapply=truemixed vsapply=falsemixed vs blocker-only cases. - Add a regression sub-test seeding a low-seq repairable observation mutation and a high-seq blocked relation mutation, asserting
Applied=true,Class=Blocked, and that the message names the blocker's seq and entity_key.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/store/store.go | Restructures RepairCloudUpgrade to apply repairable subset before reporting blockers, picks the real blocker for the message, and adds entity_key to manual-action text. |
| internal/store/store_test.go | New sub-test under TestUpgradeRepairDryRunAndApply covering the mixed repairable+blocked scenario described in #446. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 #446 —
engram cloud upgrade repair --applywas all-or-nothing: if any non-repairable blocker existed among repairable findings, it returnedApplied: falseand repaired nothing. The message also namedFindings[0](lowest seq, usually a repairable entry) instead of the actual blocker.Change
RepairCloudUpgrade(internal/store/store.go): when both repairable findings and a blocker are present withapply: true, apply the repairable subset first, then returnClass: BlockedwithApplied: true, the count repaired, and the actual blocker'sseq+entity_keyin the message.Test plan
TDD red→green. New sub-test seeds a low-seq repairable mutation + a high-seq blocker, calls
RepairCloudUpgrade(apply: true), and assertsApplied: true,Class == Blocked, and the message names the blocker's seq (not the repairable entry).go test ./internal/store/...and-raceclean.Notes
Passed adversarial review: partial apply runs in its own
withTx(atomic), is idempotent (re-running skips repaired entries), and the only non-test caller makes no control-flow decision onApplied.