feat(migrate): --up-if-clean on up + atlas Executor recover wrapper#14
Merged
Conversation
Two upstream fixes for staging-deploy blockers: 1. core-dump#150 — --up-if-clean flag was only on repair-dirty, not on up. core-dump's Dockerfile.migrate CMD invokes workflow-migrate up ... --up-if-clean which cobra rejects in v0.3.6. Add the flag to up as an idempotency hint; effective behavior is the same as plain up (already exits 0 when no migrations pending), but the flag is now accepted so deploy CMDs that pass it succeed. 2. workflow#513 — atlas Executor.Execute panics with runtime error: index out of range [0] with length 0 against the core-dump migrations corpus. Defensive recover wrapper around all atlas Executor calls converts the panic into a typed error containing the phase name (atlas-execute panic / atlas-pending panic) so callers can identify which atlas operation panicked. Root-cause investigation deferred; this is the must-have defensive fix. Driver-level seam (atlasExecutor interface + newAtlasExecutorForTest + openForTest package vars) lets tests inject a panicking fake to validate the recover wrapper without a real database. Same pattern used by buildDriverAndRequestForTest on the CLI side. TDD: each new test was written first and verified to fail before implementation. Regression invariant: reverting runWithRecover in Up() causes TestUp_RecoversAtlasExecutorPanic to fail with the original panic. See workflow/docs/plans/2026-05-02-staging-deploy-blockers-design.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses two staging-deploy blockers by (1) making the up subcommand accept --up-if-clean (so deploy CMDs that pass it don’t fail cobra parsing) and (2) adding a defensive recover() wrapper around Atlas executor calls to prevent process-killing panics and surface phase-labeled errors instead.
Changes:
- Register
--up-if-cleanonmigrate upand thread it through the command’s execution path (plus a test seam for driver construction). - Wrap Atlas executor
ExecuteNandPendingcalls withrunWithRecoverand add seams/interfaces to allow panic-injection tests. - Add unit tests for the new CLI flag behavior and Atlas panic recovery; update the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/root.go | Adds --up-if-clean to up, prints a distinct message when set, and introduces a test seam for driver/request construction. |
| pkg/cli/root_test.go | Adds CLI-level tests ensuring up accepts --up-if-clean and remains a no-op on clean DB / applies when pending (via fake driver). |
| internal/atlas/driver.go | Introduces atlasExecutor interface, test seams, and runWithRecover to convert Atlas panics during Up/Status into returned errors. |
| internal/atlas/driver_recover_test.go | Adds unit tests for runWithRecover plus a fake panicking executor test for Driver.Up. |
| CHANGELOG.md | Documents the new flag acceptance and Atlas panic recovery behavior under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address 4 Copilot nits on PR #14: - Replace "typed error" with "wrapped error" in runWithRecover doc-comment, CHANGELOG.md Unreleased entry, and driver_recover_test.go comments. The implementation uses fmt.Errorf("%s panic: %v", ...) which produces a plain formatted error, not a custom Go error type. - Drop dead `_ = drv` blank assignment in Status() — drv is consumed by the next line (newAtlasExecutorForTest call) so the blank assignment was unreachable noise. No behavior change. All tests green. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
Two upstream fixes for staging-deploy blockers in GoCodeAlone/core-dump:
--up-if-cleanflag was only on therepair-dirtysubcommand in v0.3.6. core-dump'sDockerfile.migrateCMD passes it toup, which cobra rejects. Add the flag toupas an idempotency hint — same exit-0 behavior when no migrations are pending, but now accepted.Executor.Executepanics withindex out of range [0] with length 0against core-dump's migrations corpus. Defensiverecover()wrapper converts the panic into a typed error naming the phase.Changes
pkg/cli/root.go: register--up-if-cleanonupsubcommand; thread through; introducebuildDriverAndRequestForTesttest seam.internal/atlas/driver.go: introduceatlasExecutorinterface +openForTest+newAtlasExecutorForTesttest seams;runWithRecover()helper; wrapExecuteN/Pendingcalls inUp/Statusto convert panics → typed errors.pkg/cli/root_test.go: flag-acceptance test + idempotency-on-clean-DB test + applies-when-pending test.internal/atlas/driver_recover_test.go: 3runWithRecoverunit tests +TestUp_RecoversAtlasExecutorPanicusing panicking fake executor (no real DB needed).CHANGELOG.md: Unreleased entry for both fixes.Test plan
go test -short ./...— all packages green (db-requiring tests skipped in short mode, covered by existing CI postgres workflow)runWithRecoverinUp()causesTestUp_RecoversAtlasExecutorPanicto fail with the originalpanic: runtime error: index out of range [0] with length 0Regression invariant (TDD proof)
With fix reverted (
runWithRecoverreplaced by directex.ExecuteNcall):With fix restored:
🤖 Generated with Claude Code