From fa9ecbbe95519f7586b1ac93f4e6d6b9cbb59757 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 24 Apr 2026 19:07:39 -0400 Subject: [PATCH 1/5] test(golangmigrate): add timestamp-version regression test (pre-fix: panics) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add driver_timestamp_test.go with 3 integration tests using BMW-style timestamp-based versions (20240101000001, 20240201000001, 20240301000001): - TestDriver_Up_TimestampVersions - TestDriver_Down_TimestampVersions - TestDriver_Down_TimestampVersions_ToNil PRE-FIX PANIC (confirmed locally against postgres:16-alpine): panic: runtime error: makeslice: cap out of range [recovered, repanicked] collectApplied(0x0, 0x12688ff38541) internal/golangmigrate/driver.go:182 +0x54 (*Driver).Up(...) internal/golangmigrate/driver.go:47 +0x2bc The cap out of range occurs because collectApplied(before=0, after=20240101000001) allocates make([]string, 0, 20240101000001) — a ~2×10^13 element slice. This test must be committed before the fix so code-reviewer can verify the regression-catches-test invariant independently. Co-Authored-By: Claude Sonnet 4.6 --- .../golangmigrate/driver_timestamp_test.go | 212 ++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 internal/golangmigrate/driver_timestamp_test.go diff --git a/internal/golangmigrate/driver_timestamp_test.go b/internal/golangmigrate/driver_timestamp_test.go new file mode 100644 index 0000000..3117f60 --- /dev/null +++ b/internal/golangmigrate/driver_timestamp_test.go @@ -0,0 +1,212 @@ +package golangmigrate_test + +// TestDriver_Up_TimestampVersions and TestDriver_Down_TimestampVersions are +// regression tests for the v0.3.0 panic in collectApplied() when migration +// versions are timestamp-based (e.g. 20240101000001) rather than sequential +// (1, 2, 3). +// +// Root cause: collectApplied(before=0, after=20240101000001) called +// make([]string, 0, after-before) with cap = 2×10¹³ +// which panics: "makeslice: cap out of range". +// +// The Down() path had an equivalent bug with integer-range loops. +// +// These tests MUST PANIC on the pre-fix code and PASS after the fix. + +import ( + "context" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" + + "github.com/GoCodeAlone/workflow-plugin-migrations/internal/golangmigrate" + "github.com/GoCodeAlone/workflow-plugin-migrations/pkg/testharness" +) + +// timestampMigrations writes three pairs of up/down SQL files with +// BMW-style timestamp-based version numbers. +func timestampMigrations(t *testing.T) string { + t.Helper() + dir := t.TempDir() + + writeSQL(t, dir, "20240101000001_init.up.sql", + "CREATE TABLE users (id SERIAL PRIMARY KEY, email TEXT UNIQUE NOT NULL);") + writeSQL(t, dir, "20240101000001_init.down.sql", + "DROP TABLE IF EXISTS users;") + + writeSQL(t, dir, "20240201000001_posts.up.sql", + "CREATE TABLE posts (id SERIAL PRIMARY KEY, user_id INT);") + writeSQL(t, dir, "20240201000001_posts.down.sql", + "DROP TABLE IF EXISTS posts;") + + writeSQL(t, dir, "20240301000001_comments.up.sql", + "CREATE TABLE comments (id SERIAL PRIMARY KEY, post_id INT, body TEXT);") + writeSQL(t, dir, "20240301000001_comments.down.sql", + "DROP TABLE IF EXISTS comments;") + + return dir +} + +// TestDriver_Up_TimestampVersions exercises Up() with timestamp-based migration +// versions — the exact scenario that triggered the v0.3.0 production panic. +// +// Pre-fix: panics with "makeslice: cap out of range" in collectApplied(). +// Post-fix: applies 3 migrations, returns 3 version strings. +func TestDriver_Up_TimestampVersions(t *testing.T) { + if testing.Short() { + t.Skip("skipping: requires postgres (set up with testharness)") + } + h, err := testharness.New() + if err != nil { + t.Skipf("skipping: no postgres available: %v", err) + } + defer h.Close(t) + + dir := timestampMigrations(t) + + ctx := context.Background() + d := golangmigrate.New() + req := interfaces.MigrationRequest{ + DSN: h.DSN(), + Source: interfaces.MigrationSource{Dir: dir}, + } + + // Up: apply all 3 timestamp-versioned migrations. + // PRE-FIX: panics with "makeslice: cap out of range". + // POST-FIX: returns 3 applied version strings. + result, err := d.Up(ctx, req) + if err != nil { + t.Fatalf("Up() error: %v", err) + } + + // Verify we got back meaningful applied versions (not an empty list). + if len(result.Applied) == 0 { + t.Error("Up(): expected applied migrations, got none") + } + + // Each applied version string should be a timestamp-format number. + for _, v := range result.Applied { + if len(v) < 10 { + t.Errorf("Up(): applied version %q looks too short — expected timestamp format like 20240101000001", v) + } + } + + // Status should reflect the highest version applied. + st, err := d.Status(ctx, req) + if err != nil { + t.Fatalf("Status() error: %v", err) + } + if st.Current == "" { + t.Error("Status().Current is empty after Up()") + } + if st.Current != "20240301000001" { + t.Errorf("Status().Current = %q; want %q", st.Current, "20240301000001") + } + if len(st.Pending) != 0 { + t.Errorf("Status().Pending = %v; want empty after full Up()", st.Pending) + } + + t.Logf("Up() applied: %v", result.Applied) + t.Logf("Status after Up: current=%s pending=%v", st.Current, st.Pending) +} + +// TestDriver_Down_TimestampVersions exercises Down() with timestamp-based migration +// versions — the rollback equivalent of the v0.3.0 panic. +// +// Pre-fix: the integer-range loops at Down() lines 83-91 would iterate +// 2×10¹³ times, effectively hanging or OOM'ing before the makeslice panic. +// Post-fix: rolls back 1 migration, returns the rolled-back version string. +func TestDriver_Down_TimestampVersions(t *testing.T) { + if testing.Short() { + t.Skip("skipping: requires postgres (set up with testharness)") + } + h, err := testharness.New() + if err != nil { + t.Skipf("skipping: no postgres available: %v", err) + } + defer h.Close(t) + + dir := timestampMigrations(t) + + ctx := context.Background() + d := golangmigrate.New() + req := interfaces.MigrationRequest{ + DSN: h.DSN(), + Source: interfaces.MigrationSource{Dir: dir}, + } + + // Apply all migrations first (Up path must work for Down to be testable). + if _, err := d.Up(ctx, req); err != nil { + t.Fatalf("Up() prerequisite error: %v", err) + } + + // Down: roll back 1 step. + // PRE-FIX: hangs or panics with integer-range overflow. + // POST-FIX: returns rolled-back version string. + req.Options.Steps = 1 + result, err := d.Down(ctx, req) + if err != nil { + t.Fatalf("Down() error: %v", err) + } + + if len(result.Applied) == 0 { + t.Error("Down(): expected at least one rolled-back version, got none") + } + + // The rolled-back version should be the highest one (20240301000001). + if result.Applied[0] != "20240301000001" { + t.Errorf("Down(): rolled-back version = %q; want %q", result.Applied[0], "20240301000001") + } + + // Status should now show 20240201000001 as current. + st, err := d.Status(ctx, req) + if err != nil { + t.Fatalf("Status() after Down() error: %v", err) + } + if st.Current != "20240201000001" { + t.Errorf("Status().Current after Down() = %q; want %q", st.Current, "20240201000001") + } + + t.Logf("Down() rolled back: %v", result.Applied) + t.Logf("Status after Down: current=%s pending=%v", st.Current, st.Pending) +} + +// TestDriver_Down_TimestampVersions_ToNil exercises Down() rolling back all +// migrations from a timestamp-versioned DB back to nil (clean state). +func TestDriver_Down_TimestampVersions_ToNil(t *testing.T) { + if testing.Short() { + t.Skip("skipping: requires postgres (set up with testharness)") + } + h, err := testharness.New() + if err != nil { + t.Skipf("skipping: no postgres available: %v", err) + } + defer h.Close(t) + + dir := timestampMigrations(t) + + ctx := context.Background() + d := golangmigrate.New() + req := interfaces.MigrationRequest{ + DSN: h.DSN(), + Source: interfaces.MigrationSource{Dir: dir}, + } + + // Apply all. + if _, err := d.Up(ctx, req); err != nil { + t.Fatalf("Up() prerequisite error: %v", err) + } + + // Roll back all 3 migrations. + req.Options.Steps = 3 + result, err := d.Down(ctx, req) + if err != nil { + t.Fatalf("Down(steps=3) error: %v", err) + } + + if len(result.Applied) == 0 { + t.Error("Down(steps=3): expected rolled-back versions, got none") + } + + t.Logf("Down(steps=3) rolled back: %v", result.Applied) +} From fec39f4fe725f716784765e887471cf23512c4d9 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 24 Apr 2026 19:07:51 -0400 Subject: [PATCH 2/5] fix(golangmigrate): replace collectApplied+integer loops with versionsInRange MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: collectApplied(before, after uint) pre-allocated make([]string, 0, after-before) assuming sequential version numbers. BMW uses timestamp-based versions (e.g. 20240101000001), so after-before = 2×10^13 → makeslice: cap out of range panic. Same bug appeared in Down() with integer-range loops. Fix: introduce versionsInRange(dir, lo, hi, loNil) that walks the file source (same pattern as the existing listPendingVersions helper) to enumerate only versions actually present in the source directory. Integer arithmetic is never used for capacity allocation. - Up(): replace collectApplied(before, after) with versionsInRange - Down(): replace integer-range loops with versionsInRange + reverse slice - Delete collectApplied() entirely POST-FIX test output (postgres:16-alpine, same harness): === RUN TestDriver_Up_TimestampVersions applied: [20240101000001 20240201000001 20240301000001] Status after Up: current=20240301000001 pending=[] --- PASS (0.14s) === RUN TestDriver_Down_TimestampVersions rolled back: [20240301000001] Status after Down: current=20240201000001 pending=[20240301000001] --- PASS (0.13s) === RUN TestDriver_Down_TimestampVersions_ToNil rolled back: [20240301000001 20240201000001 20240101000001] --- PASS (0.12s) Full suite: GOWORK=off go test -race -count=1 ./... — all packages pass. Verified locally against postgres:16-alpine via docker; test panics on pre-fix code and passes on fixed code. Co-Authored-By: Claude Sonnet 4.6 --- internal/golangmigrate/driver.go | 64 ++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/internal/golangmigrate/driver.go b/internal/golangmigrate/driver.go index fa9e8e7..3b68673 100644 --- a/internal/golangmigrate/driver.go +++ b/internal/golangmigrate/driver.go @@ -34,17 +34,21 @@ func (d *Driver) Up(ctx context.Context, req interfaces.MigrationRequest) (inter if err != nil { return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate: %w", err) } - defer m.Close() + defer m.Close() //nolint:errcheck // Capture the current version before applying. - before, _, _ := m.Version() + before, _, beforeErr := m.Version() + beforeNil := errors.Is(beforeErr, migrate.ErrNilVersion) if err := m.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) { return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate up: %w", err) } after, _, _ := m.Version() - applied := collectApplied(before, after) + + // Walk the source directory to enumerate applied versions; this is safe + // for timestamp-based version numbers where after-before can be 10^13. + applied, _ := versionsInRange(req.Source.Dir, before, after, beforeNil) return interfaces.MigrationResult{ Applied: applied, @@ -62,7 +66,7 @@ func (d *Driver) Down(ctx context.Context, req interfaces.MigrationRequest) (int if err != nil { return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate: %w", err) } - defer m.Close() + defer m.Close() //nolint:errcheck steps := req.Options.Steps if steps <= 0 { @@ -78,15 +82,16 @@ func (d *Driver) Down(ctx context.Context, req interfaces.MigrationRequest) (int after, _, afterErr := m.Version() // Build list of rolled-back version strings (highest to lowest). - // If we're back at nil version (ErrNilVersion), treat after as 0. + // Walk the source directory instead of integer-range loops — safe for + // timestamp-based version numbers where before-after can be 10^13. var rolledBack []string - if errors.Is(afterErr, migrate.ErrNilVersion) { - for v := uint(1); v <= before; v++ { - rolledBack = append(rolledBack, fmt.Sprintf("%d", v)) - } - } else if after < before { - for v := after + 1; v <= before; v++ { - rolledBack = append(rolledBack, fmt.Sprintf("%d", v)) + afterNil := errors.Is(afterErr, migrate.ErrNilVersion) + if afterNil || after < before { + // versionsInRange returns versions in (after, before] ascending order; + // we reverse to produce highest-first (the order rolled back). + ascending, _ := versionsInRange(req.Source.Dir, after, before, afterNil) + for i := len(ascending) - 1; i >= 0; i-- { + rolledBack = append(rolledBack, ascending[i]) } } // If after >= before, nothing was rolled back — return empty slice. @@ -106,7 +111,7 @@ func (d *Driver) Status(_ context.Context, req interfaces.MigrationRequest) (int if err != nil { return interfaces.MigrationStatus{}, fmt.Errorf("golang-migrate: %w", err) } - defer m.Close() + defer m.Close() //nolint:errcheck version, dirty, err := m.Version() if err != nil && !errors.Is(err, migrate.ErrNilVersion) { @@ -139,7 +144,7 @@ func (d *Driver) Goto(_ context.Context, req interfaces.MigrationRequest, target if err != nil { return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate: %w", err) } - defer m.Close() + defer m.Close() //nolint:errcheck var version uint if _, err := fmt.Sscanf(target, "%d", &version); err != nil { @@ -173,17 +178,28 @@ func newMigrate(req interfaces.MigrationRequest) (*migrate.Migrate, error) { return migrate.New(sourceURL, dsn) } -// collectApplied returns a list of version strings between before (exclusive) -// and after (inclusive). If the versions are equal, returns empty. -func collectApplied(before, after uint) []string { - if after <= before { - return nil +// versionsInRange opens the file source and returns version strings v where +// lo < v <= hi (exclusive lo, inclusive hi), in ascending order. +// When loNil is true all versions v <= hi qualify (DB had no prior state). +// This replaces the deleted collectApplied() which assumed sequential version +// numbers and would panic on timestamp-based versions (e.g. 20240101000001). +func versionsInRange(dir string, lo, hi uint, loNil bool) ([]string, error) { + src := &migratefile.File{} + s, err := src.Open("file://" + dir) + if err != nil { + return nil, fmt.Errorf("golang-migrate: open source for version range: %w", err) } - applied := make([]string, 0, after-before) - for v := before + 1; v <= after; v++ { - applied = append(applied, fmt.Sprintf("%d", v)) + defer s.Close() //nolint:errcheck + + var result []string + v, err := s.First() + for err == nil { + if (loNil || v > lo) && v <= hi { + result = append(result, fmt.Sprintf("%d", v)) + } + v, err = s.Next(v) } - return applied + return result, nil } // listPendingVersions opens the file source and returns the version strings of @@ -195,7 +211,7 @@ func listPendingVersions(dir string, current uint, atNil bool) ([]string, error) if err != nil { return nil, fmt.Errorf("golang-migrate: open source for pending list: %w", err) } - defer s.Close() + defer s.Close() //nolint:errcheck var pending []string v, err := s.First() From e784a172bcd4e0e3414a474cfa004e0f3ec38651 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 24 Apr 2026 19:08:02 -0400 Subject: [PATCH 3/5] =?UTF-8?q?ci:=20add=20migrations-ci.yml=20=E2=80=94?= =?UTF-8?q?=20ephemeral=20Postgres=20regression=20gate=20on=20every=20PR?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two jobs triggered on changes to migration driver code: 1. ephemeral-migrate (golang-migrate, timestamp versions) - Runs workflow-migrate up with timestamp-based fixture migrations (20240101000001, 20240201000001, 20240301000001) — the v0.3.0 regression scenario. - Verifies tables created, runs down(1), checks table count. - Runs workflow-migrate status. 2. ephemeral-migrate-goose (goose driver, sequential versions) - Same shape using --driver goose and goose-format fixtures. - Verifies up creates 2 tables, down rolls back 1. Without the v0.3.1 fix applied, the `Run workflow-migrate up` step panics with `makeslice: cap out of range` — blocking merge. This is the prevention mechanism that would have caught v0.3.0's panic before release. Note: uses `workflow-migrate` binary directly. When wfctl gains a `migrations validate` subcommand (task #78), this workflow can be updated to invoke wfctl instead. Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/migrations-ci.yml | 185 ++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 .github/workflows/migrations-ci.yml diff --git a/.github/workflows/migrations-ci.yml b/.github/workflows/migrations-ci.yml new file mode 100644 index 0000000..ed5785a --- /dev/null +++ b/.github/workflows/migrations-ci.yml @@ -0,0 +1,185 @@ +name: Migrations CI + +# Runs workflow-migrate against an ephemeral Postgres on every PR that touches +# migration driver code. This is the regression gate that catches panics like +# the v0.3.0 collectApplied() overflow before merge. + +on: + pull_request: + paths: + - 'internal/golangmigrate/**' + - 'internal/goose/**' + - 'internal/atlas/**' + - 'cmd/workflow-migrate/**' + - 'pkg/**' + - 'go.mod' + - 'go.sum' + - '.github/workflows/migrations-ci.yml' + +jobs: + ephemeral-migrate: + name: workflow-migrate up/down (golang-migrate, timestamp versions) + runs-on: ubuntu-latest + + services: + postgres: + image: postgres:16 + env: + POSTGRES_USER: ci + POSTGRES_PASSWORD: ci + POSTGRES_DB: ci_test + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U ci" + --health-interval 2s + --health-timeout 5s + --health-retries 20 + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + + - name: Build workflow-migrate + run: | + CGO_ENABLED=0 go build -o /tmp/workflow-migrate ./cmd/workflow-migrate + + - name: Prepare fixture migrations (timestamp-based — the v0.3.0 regression scenario) + run: | + mkdir -p /tmp/fixture-migrations + + cat > /tmp/fixture-migrations/20240101000001_init.up.sql <<'SQL' + CREATE TABLE users (id SERIAL PRIMARY KEY, email TEXT UNIQUE NOT NULL); + SQL + cat > /tmp/fixture-migrations/20240101000001_init.down.sql <<'SQL' + DROP TABLE IF EXISTS users; + SQL + + cat > /tmp/fixture-migrations/20240201000001_posts.up.sql <<'SQL' + CREATE TABLE posts (id SERIAL PRIMARY KEY, user_id INT); + SQL + cat > /tmp/fixture-migrations/20240201000001_posts.down.sql <<'SQL' + DROP TABLE IF EXISTS posts; + SQL + + cat > /tmp/fixture-migrations/20240301000001_comments.up.sql <<'SQL' + CREATE TABLE comments (id SERIAL PRIMARY KEY, post_id INT, body TEXT); + SQL + cat > /tmp/fixture-migrations/20240301000001_comments.down.sql <<'SQL' + DROP TABLE IF EXISTS comments; + SQL + + - name: Run workflow-migrate up — timestamp-based versions (the v0.3.0 panic scenario) + env: + DATABASE_URL: postgres://ci:ci@localhost:5432/ci_test?sslmode=disable + run: | + /tmp/workflow-migrate up --source-dir /tmp/fixture-migrations + + - name: Verify tables created + env: + PGPASSWORD: ci + run: | + PSQL_OUTPUT=$(psql -h localhost -U ci -d ci_test -tAc '\dt') + echo "$PSQL_OUTPUT" + echo "$PSQL_OUTPUT" | grep -q 'users' || { echo "FAIL: users table missing after up"; exit 1; } + echo "$PSQL_OUTPUT" | grep -q 'posts' || { echo "FAIL: posts table missing after up"; exit 1; } + echo "$PSQL_OUTPUT" | grep -q 'comments' || { echo "FAIL: comments table missing after up"; exit 1; } + echo "All 3 tables present after up ✓" + + - name: Run workflow-migrate down — rollback smoke test + env: + DATABASE_URL: postgres://ci:ci@localhost:5432/ci_test?sslmode=disable + PGPASSWORD: ci + run: | + /tmp/workflow-migrate down --source-dir /tmp/fixture-migrations + # One migration rolled back — comments should be gone. + REMAINING=$(psql -h localhost -U ci -d ci_test -tAc \ + "SELECT count(*) FROM pg_tables WHERE schemaname='public' AND tablename != 'schema_migrations'") + REMAINING=$(echo "$REMAINING" | tr -d '[:space:]') + test "$REMAINING" = "2" || { echo "FAIL: expected 2 tables after down(1), got $REMAINING"; exit 1; } + echo "Correct table count ($REMAINING) after down(1) ✓" + + - name: Run workflow-migrate status + env: + DATABASE_URL: postgres://ci:ci@localhost:5432/ci_test?sslmode=disable + run: | + /tmp/workflow-migrate status --source-dir /tmp/fixture-migrations + + ephemeral-migrate-goose: + name: workflow-migrate up/down (goose driver, sequential versions) + runs-on: ubuntu-latest + + services: + postgres: + image: postgres:16 + env: + POSTGRES_USER: ci + POSTGRES_PASSWORD: ci + POSTGRES_DB: ci_goose + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U ci" + --health-interval 2s + --health-timeout 5s + --health-retries 20 + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + + - name: Build workflow-migrate + run: | + CGO_ENABLED=0 go build -o /tmp/workflow-migrate ./cmd/workflow-migrate + + - name: Prepare goose fixture migrations + run: | + mkdir -p /tmp/goose-migrations + + cat > /tmp/goose-migrations/00001_users.sql <<'SQL' + -- +goose Up + CREATE TABLE users (id SERIAL PRIMARY KEY, email TEXT UNIQUE NOT NULL); + -- +goose Down + DROP TABLE IF EXISTS users; + SQL + + cat > /tmp/goose-migrations/00002_posts.sql <<'SQL' + -- +goose Up + CREATE TABLE posts (id SERIAL PRIMARY KEY, user_id INT); + -- +goose Down + DROP TABLE IF EXISTS posts; + SQL + + - name: Run workflow-migrate up (goose driver) + env: + DATABASE_URL: postgres://ci:ci@localhost:5432/ci_goose?sslmode=disable + run: | + /tmp/workflow-migrate up --driver goose --source-dir /tmp/goose-migrations + + - name: Verify goose tables created + env: + PGPASSWORD: ci + run: | + PSQL_OUTPUT=$(psql -h localhost -U ci -d ci_goose -tAc '\dt') + echo "$PSQL_OUTPUT" + echo "$PSQL_OUTPUT" | grep -q 'users' || { echo "FAIL: users missing"; exit 1; } + echo "$PSQL_OUTPUT" | grep -q 'posts' || { echo "FAIL: posts missing"; exit 1; } + echo "Goose: both tables present after up ✓" + + - name: Run workflow-migrate down (goose driver) + env: + DATABASE_URL: postgres://ci:ci@localhost:5432/ci_goose?sslmode=disable + PGPASSWORD: ci + run: | + /tmp/workflow-migrate down --driver goose --source-dir /tmp/goose-migrations + REMAINING=$(psql -h localhost -U ci -d ci_goose -tAc \ + "SELECT count(*) FROM pg_tables WHERE schemaname='public' AND tablename != 'goose_db_version'") + REMAINING=$(echo "$REMAINING" | tr -d '[:space:]') + test "$REMAINING" = "1" || { echo "FAIL: expected 1 table after goose down(1), got $REMAINING"; exit 1; } + echo "Goose: correct table count ($REMAINING) after down(1) ✓" From 42fdf01a1b23870c549a40f3d9796c0982351d07 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 24 Apr 2026 19:08:10 -0400 Subject: [PATCH 4/5] chore: add .golangci.yml + nolint annotations for pre-existing patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add .golangci.yml (golangci-lint v2 config, previously absent). Add //nolint:errcheck to all existing defer .Close() calls — these are unactionable cleanup paths and the pattern was already present throughout the codebase. Add //nolint:unused to goreleaser-injected version vars (set via -ldflags at build time). Also update CHANGELOG for v0.3.1. Co-Authored-By: Claude Sonnet 4.6 --- .golangci.yml | 26 +++++++++++++++++++++++ CHANGELOG.md | 11 ++++++++++ cmd/workflow-plugin-atlas-migrate/main.go | 2 +- cmd/workflow-plugin-migrations/main.go | 2 +- internal/atlas/revisions.go | 2 +- internal/goose/driver.go | 8 +++---- pkg/cli/tenant_ensure.go | 2 +- pkg/cli/test.go | 2 +- 8 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..623e8fb --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,26 @@ +version: "2" + +run: + timeout: 5m + +linters: + default: none + enable: + - errcheck + - govet + - staticcheck + - unused + +issues: + exclusions: + rules: + # defer Close() calls — these are cleanup paths where errors are + # unactionable and the pattern is idiomatic Go (pre-existing). + - linters: [errcheck] + text: "Error return value of.*\\.Close" + # fmt.Fprintln to stdout in CLI test helper (pre-existing). + - linters: [errcheck] + text: "Error return value of `fmt\\.Fprintln`" + # goreleaser-injected version vars set via -ldflags at build time. + - linters: [unused] + text: "var version is unused" diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e7ca7d..626a9c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.3.1] - 2026-04-24 + +### Fixed + +- **P0: golang-migrate driver panics on timestamp-based migration versions** — `collectApplied(before, after uint)` pre-allocated `make([]string, 0, after-before)`, assuming sequential version numbers. BMW uses timestamp-based versions (e.g. `20240101000001`), so `after-before = 2×10¹³` → `makeslice: cap out of range` panic. **`v0.3.0` is defective and should not be used with timestamp-based migrations.** The fix replaces the integer-range loops in `Up()` and `Down()` with file-system source walking (the same pattern already used by `listPendingVersions`). New shared helper: `versionsInRange(dir, lo, hi, loNil)`. + +### Added + +- **`migrations-ci.yml` GHA workflow** — runs `workflow-migrate up` + `down` against ephemeral Postgres:16 on every PR touching migration driver code, using timestamp-based fixture migrations. This is the prevention mechanism that would have caught v0.3.0's panic before release. +- **`.golangci.yml`** — adds golangci-lint v2 config (previously absent) with errcheck exclusions for idiomatic `defer .Close()` patterns. + ## [0.3.0] - 2026-04-24 ### Added diff --git a/cmd/workflow-plugin-atlas-migrate/main.go b/cmd/workflow-plugin-atlas-migrate/main.go index 05692c6..fe62154 100644 --- a/cmd/workflow-plugin-atlas-migrate/main.go +++ b/cmd/workflow-plugin-atlas-migrate/main.go @@ -10,7 +10,7 @@ import ( ) // version is set at build time via -X main.version=. -var version string +var version string //nolint:unused func main() { sdk.Serve(atlasplugin.NewPlugin()) diff --git a/cmd/workflow-plugin-migrations/main.go b/cmd/workflow-plugin-migrations/main.go index 29f00db..9f65988 100644 --- a/cmd/workflow-plugin-migrations/main.go +++ b/cmd/workflow-plugin-migrations/main.go @@ -14,7 +14,7 @@ import ( ) // version is set at build time via -X main.version=. -var version string +var version string //nolint:unused func main() { sdk.ServePluginFull(internal.NewPlugin(), cli.NewCLIProvider(), nil) diff --git a/internal/atlas/revisions.go b/internal/atlas/revisions.go index a713fa3..b8a272f 100644 --- a/internal/atlas/revisions.go +++ b/internal/atlas/revisions.go @@ -64,7 +64,7 @@ func (r *sqlRevisionRW) ReadRevisions(ctx context.Context) ([]*atlmigrate.Revisi if err != nil { return nil, err } - defer rows.Close() + defer rows.Close() //nolint:errcheck return scanRevisions(rows) } diff --git a/internal/goose/driver.go b/internal/goose/driver.go index 647bba0..e8df905 100644 --- a/internal/goose/driver.go +++ b/internal/goose/driver.go @@ -34,7 +34,7 @@ func (d *Driver) Up(ctx context.Context, req interfaces.MigrationRequest) (inter if err != nil { return interfaces.MigrationResult{}, err } - defer db.Close() + defer db.Close() //nolint:errcheck provider, err := newProvider(db, req) if err != nil { @@ -74,7 +74,7 @@ func (d *Driver) Down(ctx context.Context, req interfaces.MigrationRequest) (int if err != nil { return interfaces.MigrationResult{}, err } - defer db.Close() + defer db.Close() //nolint:errcheck provider, err := newProvider(db, req) if err != nil { @@ -108,7 +108,7 @@ func (d *Driver) Status(ctx context.Context, req interfaces.MigrationRequest) (i if err != nil { return interfaces.MigrationStatus{}, err } - defer db.Close() + defer db.Close() //nolint:errcheck provider, err := newProvider(db, req) if err != nil { @@ -153,7 +153,7 @@ func (d *Driver) Goto(ctx context.Context, req interfaces.MigrationRequest, targ if err != nil { return interfaces.MigrationResult{}, err } - defer db.Close() + defer db.Close() //nolint:errcheck provider, err := newProvider(db, req) if err != nil { diff --git a/pkg/cli/tenant_ensure.go b/pkg/cli/tenant_ensure.go index 2b86c62..71271a6 100644 --- a/pkg/cli/tenant_ensure.go +++ b/pkg/cli/tenant_ensure.go @@ -36,7 +36,7 @@ has its own schema.`, if err != nil { return fmt.Errorf("tenant-ensure: open db: %w", err) } - defer db.Close() + defer db.Close() //nolint:errcheck if _, err = db.ExecContext(context.Background(), fmt.Sprintf(`CREATE SCHEMA IF NOT EXISTS %q`, schema)); err != nil { diff --git a/pkg/cli/test.go b/pkg/cli/test.go index 4e50fe5..fe1fc33 100644 --- a/pkg/cli/test.go +++ b/pkg/cli/test.go @@ -35,7 +35,7 @@ integration test setup (the calling process then uses that DB).`, if err := runner.Setup(ctx); err != nil { return fmt.Errorf("migrate test --keep-alive: %w", err) } - fmt.Fprintln(os.Stdout, req.DSN) + fmt.Fprintln(os.Stdout, req.DSN) //nolint:errcheck return nil } From 2cd24b71e2942735c6110cb5952a70bd7f232ac4 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 24 Apr 2026 19:24:18 -0400 Subject: [PATCH 5/5] fix(golangmigrate): address 5 Copilot review findings on PR #5 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1+2 (migrations-ci.yml): add top-level `permissions: contents: read` — minimum CodeQL-required scope. Finding 3 (Up — before version): propagate non-ErrNilVersion errors from m.Version() before migration. Previously a DB error set before=0 and silently produced a wrong applied list. Finding 4 (Up — after version + versionsInRange error): propagate non-ErrNilVersion afterErr; log (not swallow) versionsInRange error when migration succeeded but enumeration fails. Finding 5 (Down — symmetric): same before/after error handling applied to Down(); log versionsInRange enumeration error. Finding 6 (versionsInRange + listPendingVersions — end-of-stream sentinel): source.Driver.Next() returns os.ErrNotExist to signal end-of-stream per the golang-migrate source.Driver contract (source/driver.go:57). Rewrite iteration loop to distinguish os.ErrNotExist (normal break) from real errors (propagated to caller). Apply same fix to listPendingVersions for consistency. All existing tests pass; golangci-lint 0 issues. Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/migrations-ci.yml | 3 ++ internal/golangmigrate/driver.go | 54 ++++++++++++++++++++++++----- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/.github/workflows/migrations-ci.yml b/.github/workflows/migrations-ci.yml index ed5785a..c84b1a6 100644 --- a/.github/workflows/migrations-ci.yml +++ b/.github/workflows/migrations-ci.yml @@ -4,6 +4,9 @@ name: Migrations CI # migration driver code. This is the regression gate that catches panics like # the v0.3.0 collectApplied() overflow before merge. +permissions: + contents: read + on: pull_request: paths: diff --git a/internal/golangmigrate/driver.go b/internal/golangmigrate/driver.go index 3b68673..e9c9ad7 100644 --- a/internal/golangmigrate/driver.go +++ b/internal/golangmigrate/driver.go @@ -5,6 +5,8 @@ import ( "context" "errors" "fmt" + "log" + "os" "strings" "time" @@ -36,19 +38,31 @@ func (d *Driver) Up(ctx context.Context, req interfaces.MigrationRequest) (inter } defer m.Close() //nolint:errcheck - // Capture the current version before applying. + // Capture the version before applying; fail fast if the DB is unavailable. before, _, beforeErr := m.Version() - beforeNil := errors.Is(beforeErr, migrate.ErrNilVersion) + if beforeErr != nil && !errors.Is(beforeErr, migrate.ErrNilVersion) { + return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate: version before up: %w", beforeErr) + } + atNilBefore := errors.Is(beforeErr, migrate.ErrNilVersion) if err := m.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) { return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate up: %w", err) } - after, _, _ := m.Version() + after, _, afterErr := m.Version() + if afterErr != nil && !errors.Is(afterErr, migrate.ErrNilVersion) { + return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate: version after up: %w", afterErr) + } // Walk the source directory to enumerate applied versions; this is safe // for timestamp-based version numbers where after-before can be 10^13. - applied, _ := versionsInRange(req.Source.Dir, before, after, beforeNil) + applied, err := versionsInRange(req.Source.Dir, before, after, atNilBefore) + if err != nil { + // Migration succeeded but we can't enumerate applied versions — log and + // return partial info rather than hiding the applied state entirely. + log.Printf("warn: golang-migrate: applied version enumeration failed: %v", err) + applied = nil + } return interfaces.MigrationResult{ Applied: applied, @@ -73,13 +87,19 @@ func (d *Driver) Down(ctx context.Context, req interfaces.MigrationRequest) (int steps = 1 } - before, _, _ := m.Version() + before, _, beforeErr := m.Version() + if beforeErr != nil && !errors.Is(beforeErr, migrate.ErrNilVersion) { + return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate: version before down: %w", beforeErr) + } if err := m.Steps(-steps); err != nil && !errors.Is(err, migrate.ErrNoChange) { return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate down: %w", err) } after, _, afterErr := m.Version() + if afterErr != nil && !errors.Is(afterErr, migrate.ErrNilVersion) { + return interfaces.MigrationResult{}, fmt.Errorf("golang-migrate: version after down: %w", afterErr) + } // Build list of rolled-back version strings (highest to lowest). // Walk the source directory instead of integer-range loops — safe for @@ -89,7 +109,10 @@ func (d *Driver) Down(ctx context.Context, req interfaces.MigrationRequest) (int if afterNil || after < before { // versionsInRange returns versions in (after, before] ascending order; // we reverse to produce highest-first (the order rolled back). - ascending, _ := versionsInRange(req.Source.Dir, after, before, afterNil) + ascending, err := versionsInRange(req.Source.Dir, after, before, afterNil) + if err != nil { + log.Printf("warn: golang-migrate: rolled-back version enumeration failed: %v", err) + } for i := len(ascending) - 1; i >= 0; i-- { rolledBack = append(rolledBack, ascending[i]) } @@ -183,6 +206,9 @@ func newMigrate(req interfaces.MigrationRequest) (*migrate.Migrate, error) { // When loNil is true all versions v <= hi qualify (DB had no prior state). // This replaces the deleted collectApplied() which assumed sequential version // numbers and would panic on timestamp-based versions (e.g. 20240101000001). +// +// End-of-stream is signalled by os.ErrNotExist per the golang-migrate source.Driver +// contract; any other error from Next() is returned to the caller. func versionsInRange(dir string, lo, hi uint, loNil bool) ([]string, error) { src := &migratefile.File{} s, err := src.Open("file://" + dir) @@ -193,7 +219,13 @@ func versionsInRange(dir string, lo, hi uint, loNil bool) ([]string, error) { var result []string v, err := s.First() - for err == nil { + for { + if err != nil { + if errors.Is(err, os.ErrNotExist) { + break // normal end of stream + } + return result, fmt.Errorf("golang-migrate: iterate source: %w", err) + } if (loNil || v > lo) && v <= hi { result = append(result, fmt.Sprintf("%d", v)) } @@ -215,7 +247,13 @@ func listPendingVersions(dir string, current uint, atNil bool) ([]string, error) var pending []string v, err := s.First() - for err == nil { + for { + if err != nil { + if errors.Is(err, os.ErrNotExist) { + break // normal end of stream + } + return pending, fmt.Errorf("golang-migrate: iterate source for pending: %w", err) + } if atNil || v > current { pending = append(pending, fmt.Sprintf("%d", v)) }