fix(sqlmigrate): address low-severity review findings

Core library:
- Replace O(n*m) isApplied linear scan with map lookups (appliedSets)
- Wrap exec errors with ErrExecFailed in Up and Down
- Add tests for Collect with subpath and empty subpath
- Add tests for Latest and Drop convenience wrappers
- Add tests for Applied() error propagation in Up/Down/GetStatus
- Add tests for ErrExecFailed wrapping
- Add tests for context propagation and cancellation
- Store received context in mock for propagation assertions

shmigrate:
- Return ErrExecFailed on missing migration file instead of writing
  a comment and continuing
- Use r.FS for file stat (consistent with Applied), falling back
  to os.Stat when FS is nil

database/sqlmigrate/shmigrate/shmigrate.go

@@ -73,15 +73,16 @@ func (r *Migrator) ExecDown(ctx context.Context, m sqlmigrate.Migration, sql str
 }
 
 func (r *Migrator) exec(name, suffix, label string) error {
-	path := shellQuote(unclean(filepath.Join(r.MigrationsDir, name+suffix)))
+	relPath := filepath.Join(r.MigrationsDir, name+suffix)
+	path := shellQuote(unclean(relPath))
 	cmd := strings.Replace(r.SqlCommand, "%s", path, 1)
 
 	syncCmd := strings.Replace(r.SqlCommand, "%s", shellQuote(unclean(r.LogQueryPath)), 1)
 	logPath := shellQuote(unclean(r.LogPath))
 
 	fmt.Fprintf(r.Writer, "# %s %s\n", label, name)
-	if _, err := os.Stat(filepath.Join(r.MigrationsDir, name+suffix)); err != nil {
+	if err := r.statFile(relPath); err != nil {
-		fmt.Fprintln(r.Writer, "# ERROR: MISSING FILE")
+		return fmt.Errorf("%w: %s%s: %w", sqlmigrate.ErrExecFailed, name, suffix, err)
 	}
 	fmt.Fprintln(r.Writer, cmd)
 	fmt.Fprintf(r.Writer, "%s > %s\n", syncCmd, logPath)
@@ -90,6 +91,16 @@ func (r *Migrator) exec(name, suffix, label string) error {
 	return nil
 }
 
+// statFile checks if a file exists, using r.FS when set.
+func (r *Migrator) statFile(path string) error {
+	if r.FS != nil {
+		_, err := fs.Stat(r.FS, path)
+		return err
+	}
+	_, err := os.Stat(path)
+	return err
+}
+
 // Applied reads the migrations log file and returns applied migrations.
 // Supports two formats:
 //   - New: "id\tname" (tab-separated, written by updated _migrations.sql)

database/sqlmigrate/sqlmigrate.go

@@ -157,16 +157,27 @@ func NamesOnly(names []string) []Script {
 	return ddls
 }
 
-// isApplied returns true if the Script matches any applied entry by name or ID.
+// appliedSets builds name and ID lookup sets from applied migrations.
-func isApplied(d Script, applied []Migration) bool {
+func appliedSets(applied []Migration) (names map[string]bool, ids map[string]bool) {
+	names = make(map[string]bool, len(applied))
+	ids = make(map[string]bool, len(applied))
 	for _, a := range applied {
-		if a.Name == d.Name {
+		names[a.Name] = true
-			return true
+		if a.ID != "" {
-		}
+			ids[a.ID] = true
-		if d.ID != "" && a.ID != "" && a.ID == d.ID {
-			return true
 		}
 	}
+	return names, ids
+}
+
+// isApplied returns true if the Script matches any applied entry by name or ID.
+func isApplied(d Script, names map[string]bool, ids map[string]bool) bool {
+	if names[d.Name] {
+		return true
+	}
+	if d.ID != "" && ids[d.ID] {
+		return true
+	}
 	return false
 }
 
@@ -196,9 +207,11 @@ func Up(ctx context.Context, r Migrator, ddls []Script, n int) ([]Migration, err
 		return nil, fmt.Errorf("%w: %w", ErrQueryApplied, err)
 	}
 
+	names, ids := appliedSets(applied)
+
 	var pending []Script
 	for _, d := range ddls {
-		if !isApplied(d, applied) {
+		if !isApplied(d, names, ids) {
 			pending = append(pending, d)
 		}
 	}
@@ -216,7 +229,7 @@ func Up(ctx context.Context, r Migrator, ddls []Script, n int) ([]Migration, err
 			return ran, err
 		}
 		if err := r.ExecUp(ctx, d.Migration, d.Up); err != nil {
-			return ran, fmt.Errorf("%s (up): %w", d.Name, err)
+			return ran, fmt.Errorf("%w: %s (up): %w", ErrExecFailed, d.Name, err)
 		}
 		ran = append(ran, d.Migration)
 	}
@@ -264,7 +277,7 @@ func Down(ctx context.Context, r Migrator, ddls []Script, n int) ([]Migration, e
 			return ran, fmt.Errorf("%w: %s", ErrMissingScript, a.Name)
 		}
 		if err := r.ExecDown(ctx, a, d.Down); err != nil {
-			return ran, fmt.Errorf("%s (down): %w", a.Name, err)
+			return ran, fmt.Errorf("%w: %s (down): %w", ErrExecFailed, a.Name, err)
 		}
 		ran = append(ran, a)
 	}
@@ -279,9 +292,11 @@ func GetStatus(ctx context.Context, r Migrator, ddls []Script) (*Status, error)
 		return nil, fmt.Errorf("%w: %w", ErrQueryApplied, err)
 	}
 
+	names, ids := appliedSets(applied)
+
 	var pending []Migration
 	for _, d := range ddls {
-		if !isApplied(d, applied) {
+		if !isApplied(d, names, ids) {
 			pending = append(pending, d.Migration)
 		}
 	}

database/sqlmigrate/sqlmigrate_test.go

@@ -31,13 +31,16 @@ func names(ms []sqlmigrate.Migration) []string {
 
 // mockMigrator tracks applied migrations in memory.
 type mockMigrator struct {
-	applied   []sqlmigrate.Migration
+	applied    []sqlmigrate.Migration
-	execErr   error // if set, ExecUp/ExecDown return this on every call
+	execErr    error // if set, ExecUp/ExecDown return this on every call
-	upCalls   []string
+	upCalls    []string
-	downCalls []string
+	downCalls  []string
+	lastCtx    context.Context
+	appliedErr error // if set, Applied returns this error
 }
 
-func (m *mockMigrator) ExecUp(_ context.Context, mig sqlmigrate.Migration, _ string) error {
+func (m *mockMigrator) ExecUp(ctx context.Context, mig sqlmigrate.Migration, _ string) error {
+	m.lastCtx = ctx
 	m.upCalls = append(m.upCalls, mig.Name)
 	if m.execErr != nil {
 		return m.execErr
@@ -49,7 +52,8 @@ func (m *mockMigrator) ExecUp(_ context.Context, mig sqlmigrate.Migration, _ str
 	return nil
 }
 
-func (m *mockMigrator) ExecDown(_ context.Context, mig sqlmigrate.Migration, _ string) error {
+func (m *mockMigrator) ExecDown(ctx context.Context, mig sqlmigrate.Migration, _ string) error {
+	m.lastCtx = ctx
 	m.downCalls = append(m.downCalls, mig.Name)
 	if m.execErr != nil {
 		return m.execErr
@@ -58,7 +62,11 @@ func (m *mockMigrator) ExecDown(_ context.Context, mig sqlmigrate.Migration, _ s
 	return nil
 }
 
-func (m *mockMigrator) Applied(_ context.Context) ([]sqlmigrate.Migration, error) {
+func (m *mockMigrator) Applied(ctx context.Context) ([]sqlmigrate.Migration, error) {
+	m.lastCtx = ctx
+	if m.appliedErr != nil {
+		return nil, m.appliedErr
+	}
 	return slices.Clone(m.applied), nil
 }
 
@@ -141,6 +149,34 @@ func TestCollect(t *testing.T) {
 		}
 	})
 
+	t.Run("subpath", func(t *testing.T) {
+		fsys := fstest.MapFS{
+			"migrations/001_init.up.sql":   {Data: []byte("CREATE TABLE a;")},
+			"migrations/001_init.down.sql": {Data: []byte("DROP TABLE a;")},
+		}
+		ddls, err := sqlmigrate.Collect(fsys, "migrations")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(ddls) != 1 || ddls[0].Name != "001_init" {
+			t.Errorf("got %v, want [001_init]", ddls)
+		}
+	})
+
+	t.Run("empty subpath", func(t *testing.T) {
+		fsys := fstest.MapFS{
+			"001_init.up.sql":   {Data: []byte("CREATE TABLE a;")},
+			"001_init.down.sql": {Data: []byte("DROP TABLE a;")},
+		}
+		ddls, err := sqlmigrate.Collect(fsys, "")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(ddls) != 1 || ddls[0].Name != "001_init" {
+			t.Errorf("got %v, want [001_init]", ddls)
+		}
+	})
+
 	t.Run("ignores non-sql files", func(t *testing.T) {
 		fsys := fstest.MapFS{
 			"001_init.up.sql":   {Data: []byte("UP")},
@@ -480,3 +516,156 @@ func TestGetStatus(t *testing.T) {
 		}
 	})
 }
+
+// --- Latest / Drop ---
+
+func TestLatest(t *testing.T) {
+	ctx := t.Context()
+	ddls := []sqlmigrate.Script{
+		{Migration: sqlmigrate.Migration{Name: "001_init"}, Up: "CREATE TABLE a;", Down: "DROP TABLE a;"},
+		{Migration: sqlmigrate.Migration{Name: "002_users"}, Up: "CREATE TABLE b;", Down: "DROP TABLE b;"},
+	}
+	m := &mockMigrator{}
+	ran, err := sqlmigrate.Latest(ctx, m, ddls)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !slices.Equal(names(ran), []string{"001_init", "002_users"}) {
+		t.Errorf("applied = %v, want all", names(ran))
+	}
+}
+
+func TestDrop(t *testing.T) {
+	ctx := t.Context()
+	ddls := []sqlmigrate.Script{
+		{Migration: sqlmigrate.Migration{Name: "001_init"}, Up: "CREATE TABLE a;", Down: "DROP TABLE a;"},
+		{Migration: sqlmigrate.Migration{Name: "002_users"}, Up: "CREATE TABLE b;", Down: "DROP TABLE b;"},
+	}
+	m := &mockMigrator{applied: migs("001_init", "002_users")}
+	rolled, err := sqlmigrate.Drop(ctx, m, ddls)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(rolled) != 2 {
+		t.Fatalf("rolled %d, want 2", len(rolled))
+	}
+	if rolled[0].Name != "002_users" {
+		t.Errorf("first rollback = %q, want 002_users", rolled[0].Name)
+	}
+}
+
+// --- Applied error propagation ---
+
+func TestAppliedError(t *testing.T) {
+	ctx := t.Context()
+	ddls := []sqlmigrate.Script{
+		{Migration: sqlmigrate.Migration{Name: "001_init"}, Up: "UP", Down: "DOWN"},
+	}
+	dbErr := errors.New("connection refused")
+
+	t.Run("Up wraps with ErrQueryApplied", func(t *testing.T) {
+		m := &mockMigrator{appliedErr: dbErr}
+		_, err := sqlmigrate.Up(ctx, m, ddls, -1)
+		if !errors.Is(err, sqlmigrate.ErrQueryApplied) {
+			t.Errorf("got %v, want ErrQueryApplied", err)
+		}
+		if !errors.Is(err, dbErr) {
+			t.Errorf("got %v, want underlying %v", err, dbErr)
+		}
+	})
+
+	t.Run("Down wraps with ErrQueryApplied", func(t *testing.T) {
+		m := &mockMigrator{appliedErr: dbErr}
+		_, err := sqlmigrate.Down(ctx, m, ddls, -1)
+		if !errors.Is(err, sqlmigrate.ErrQueryApplied) {
+			t.Errorf("got %v, want ErrQueryApplied", err)
+		}
+	})
+
+	t.Run("GetStatus wraps with ErrQueryApplied", func(t *testing.T) {
+		m := &mockMigrator{appliedErr: dbErr}
+		_, err := sqlmigrate.GetStatus(ctx, m, ddls)
+		if !errors.Is(err, sqlmigrate.ErrQueryApplied) {
+			t.Errorf("got %v, want ErrQueryApplied", err)
+		}
+	})
+}
+
+// --- ErrExecFailed wrapping ---
+
+func TestExecFailedWrapping(t *testing.T) {
+	ctx := t.Context()
+	ddls := []sqlmigrate.Script{
+		{Migration: sqlmigrate.Migration{Name: "001_init"}, Up: "UP", Down: "DOWN"},
+	}
+
+	t.Run("Up wraps with ErrExecFailed", func(t *testing.T) {
+		m := &mockMigrator{execErr: errors.New("syntax error")}
+		_, err := sqlmigrate.Up(ctx, m, ddls, -1)
+		if !errors.Is(err, sqlmigrate.ErrExecFailed) {
+			t.Errorf("got %v, want ErrExecFailed", err)
+		}
+	})
+
+	t.Run("Down wraps with ErrExecFailed", func(t *testing.T) {
+		m := &mockMigrator{
+			applied: migs("001_init"),
+			execErr: errors.New("syntax error"),
+		}
+		_, err := sqlmigrate.Down(ctx, m, ddls, 1)
+		if !errors.Is(err, sqlmigrate.ErrExecFailed) {
+			t.Errorf("got %v, want ErrExecFailed", err)
+		}
+	})
+}
+
+// --- Context propagation ---
+
+func TestContextPropagation(t *testing.T) {
+	ctx := t.Context()
+	ddls := []sqlmigrate.Script{
+		{Migration: sqlmigrate.Migration{Name: "001_init"}, Up: "UP", Down: "DOWN"},
+	}
+
+	t.Run("Up passes context to Migrator", func(t *testing.T) {
+		m := &mockMigrator{}
+		_, _ = sqlmigrate.Up(ctx, m, ddls, -1)
+		if m.lastCtx != ctx {
+			t.Error("context not propagated to ExecUp")
+		}
+	})
+
+	t.Run("Down passes context to Migrator", func(t *testing.T) {
+		m := &mockMigrator{applied: migs("001_init")}
+		_, _ = sqlmigrate.Down(ctx, m, ddls, 1)
+		if m.lastCtx != ctx {
+			t.Error("context not propagated to ExecDown")
+		}
+	})
+
+	t.Run("Up stops on cancelled context", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(t.Context())
+		cancel()
+		m := &mockMigrator{}
+		ran, err := sqlmigrate.Up(ctx, m, ddls, -1)
+		if err == nil {
+			t.Fatal("expected error")
+		}
+		if len(ran) != 0 {
+			t.Errorf("applied %d on cancelled ctx, want 0", len(ran))
+		}
+	})
+
+	t.Run("Down stops on cancelled context", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(t.Context())
+		cancel()
+		m := &mockMigrator{applied: migs("001_init")}
+		rolled, err := sqlmigrate.Down(ctx, m, ddls, 1)
+		if err == nil {
+			t.Fatal("expected error")
+		}
+		if len(rolled) != 0 {
+			t.Errorf("rolled %d on cancelled ctx, want 0", len(rolled))
+		}
+	})
+}