fix(migrations): use numeric ordering to allow 3-digit migrations#198
Open
Lutherwaves wants to merge 1 commit into
Open
fix(migrations): use numeric ordering to allow 3-digit migrations#198Lutherwaves wants to merge 1 commit into
Lutherwaves wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughMigration execution now orders files by parsed numeric ID rather than lexical filename order. Shared parsing is used for both sorting and the applied-ID gate, with deterministic fallback behavior and tests for numeric ordering and malformed filenames. ChangesNumeric-aware migration ordering
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@storage/migration_internal_test.go`:
- Around line 116-123: The helper function slicesIndex is redundant—replace all
calls to slicesIndex(s, v) with slices.Index(s, v) from the standard library and
add the "slices" import; then remove the slicesIndex function entirely. Locate
usages in tests (any references to slicesIndex) and update them to slices.Index,
update the import block to include "slices", and delete the slicesIndex helper
function to avoid duplicate boilerplate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d48fbc4b-56b1-451d-9a47-870f7e281670
📒 Files selected for processing (2)
storage/migration.gostorage/migration_internal_test.go
Lutherwaves
force-pushed
the
fix/numeric-migration-sort
branch
from
June 3, 2026 10:54
946e881 to
1faea2d
Compare
The runner derived each migration id from the numeric filename prefix and
gated application on a numeric high-water mark, but chose run order with
sort.Strings — a lexical sort. The two agree only while every id is the same
width. The first 3-digit migration ("100__") sorts before "10__".."99__"
lexically, so on a fresh database the runner applies 100/101 first, advances
the numeric mark past 99, then silently skips migrations 10..99. Incrementally
migrated databases (mark already >= 99) are unaffected, so this only bites
fresh environments: new dev setups, CI, ephemeral test databases.
Sort by the parsed numeric id — the same value the apply gate uses — with a
lexical tie-break, so run order and gate stay consistent regardless of digit
count and zero-padding is no longer load-bearing. Extract migrationID as the
shared parse helper used by both paths.
Fixes #197
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Lutherwaves
force-pushed
the
fix/numeric-migration-sort
branch
from
June 3, 2026 11:03
1faea2d to
f6eff91
Compare
Lutherwaves
added
bug
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@storage/migration_internal_test.go`:
- Around line 104-115: Update sortMigrationKeys so its comparator is total for
mixed inputs: place keys with parseable numeric prefixes before unparseable
keys, compare numeric keys numerically and invalid keys lexically. Extend
TestSortMigrationKeysFallsBackToLexicalForUnparseablePrefix with mixed
valid/invalid cases that verify valid keys always precede invalid keys and
invalid keys are deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ed732d4-1685-4c62-ba92-6dbda1fe8637
📒 Files selected for processing (2)
storage/migration.gostorage/migration_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/migration.go
Comment on lines
+104
to
+115
| func TestSortMigrationKeysFallsBackToLexicalForUnparseablePrefix(t *testing.T) { | ||
| // Keys whose prefix is not a number must still sort deterministically | ||
| // rather than panic; runMigrations reports the bad id when it gets there. | ||
| keys := []string{"10__ok", "bad__name", "2__ok", "another"} | ||
| sortMigrationKeys(keys) | ||
|
|
||
| // Numeric-prefixed keys keep numeric order among themselves; the rest are | ||
| // ordered lexically relative to whatever they compare against. | ||
| if got := slices.Index(keys, "2__ok"); got > slices.Index(keys, "10__ok") { | ||
| t.Fatalf("2__ok should precede 10__ok, got order %v", keys) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Demonstrate transitivity violation in the current upstream sortMigrationKeys
cat << 'EOF' > test_sort.go
package main
import (
"fmt"
"slices"
"strconv"
"strings"
)
func migrationID(key string) (int, error) {
prefix, _, ok := strings.Cut(key, "__")
if !ok {
return 0, fmt.Errorf("err")
}
return strconv.Atoi(prefix)
}
func sortMigrationKeys(keys []string) {
slices.SortStableFunc(keys, func(a, b string) int {
idA, errA := migrationID(a)
idB, errB := migrationID(b)
if errA != nil || errB != nil {
return strings.Compare(a, b)
}
if idA != idB {
if idA < idB {
return -1
}
return 1
}
return strings.Compare(a, b)
})
}
func main() {
// Demonstrating inconsistent sorting based on initial order due to transitivity violation
k1 := []string{"10__ok", "2__ok", "1_err"}
sortMigrationKeys(k1)
fmt.Printf("Sorted starting from A,B,C: %v\n", k1)
k2 := []string{"1_err", "2__ok", "10__ok"}
sortMigrationKeys(k2)
fmt.Printf("Sorted starting from C,B,A: %v\n", k2)
}
EOF
go run test_sort.goRepository: tink3rlabs/magic
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
git ls-files 'storage/*' | sort
echo '---'
ast-grep outline storage/migration.go --view expanded || true
echo '---'
ast-grep outline storage/migration_internal_test.go --view expanded || true
echo '---'
# Show the relevant sections with line numbers.
sed -n '1,240p' storage/migration.go | cat -n
echo '---'
sed -n '1,220p' storage/migration_internal_test.go | cat -nRepository: tink3rlabs/magic
Length of output: 15125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'storage/*' | sort
echo '---'
ast-grep outline storage/migration.go --view expanded || true
echo '---'
ast-grep outline storage/migration_internal_test.go --view expanded || true
echo '---'
sed -n '1,240p' storage/migration.go | cat -n
echo '---'
sed -n '1,220p' storage/migration_internal_test.go | cat -nRepository: tink3rlabs/magic
Length of output: 15125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' storage/migration.go | cat -nRepository: tink3rlabs/magic
Length of output: 7269
Make the migration-key comparator total for mixed inputs
The current fallback to strings.Compare when either side is unparseable breaks transitivity (10__ok < 1_err, 1_err < 2__ok, but 10__ok > 2__ok). That can make slices.SortStableFunc produce orderings that depend on slice state. Compare parseable and unparseable keys in separate buckets (valid first, invalid last) and add a mixed-key test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/migration_internal_test.go` around lines 104 - 115, Update
sortMigrationKeys so its comparator is total for mixed inputs: place keys with
parseable numeric prefixes before unparseable keys, compare numeric keys
numerically and invalid keys lexically. Extend
TestSortMigrationKeysFallsBackToLexicalForUnparseablePrefix with mixed
valid/invalid cases that verify valid keys always precede invalid keys and
invalid keys are deterministic.
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
1 participant
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.
Closes #197.
Problem
Migration files are named
<id>__<description>.yaml. The runner derives each migration id by parsing the numeric prefix and gates application on a numeric high-water mark (apply whenid > mark). But it chose run order withsort.Strings(keys)— a lexical sort.Lexical and numeric order agree only while every id has the same digit width. The first 3-digit migration (
100__…) sorts before10__…..99__…lexically (the char after100is_> any digit). So on a fresh database (mark = 0) the runner:01..09(mark → 9),100,101next in lexical order (mark → 101),10..99, whose ids are now below the mark, and silently skips all of them — no error, recorded as success.Incrementally migrated databases (mark already ≥ 99) are unaffected, so this only bites fresh environments: new dev setups, CI, ephemeral test databases.
Fix
Order keys by their parsed numeric id — the same value the apply gate uses — with a lexical tie-break for equal ids and a lexical fallback for unparseable prefixes (so ordering stays deterministic; a bad id is still surfaced by the existing per-migration check).
migrationIDis extracted as the single parse helper shared by both the ordering and the gate, so they can no longer disagree.Tests
storage/migration_internal_test.go:TestSortMigrationKeysOrdersNumericallyNotLexically— mixed 1/2/3-digit ids incl. a tie on100; asserts1,2,9,10,99,100,100order (would fail undersort.Strings).TestSortMigrationKeysFallsBackToLexicalForUnparseablePrefix— non-numeric prefixes sort deterministically without panicking.go build ./...,go vet ./storage/, andgo test ./storage/all pass.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests