commit verification workflow

go/cmd/dolt/commands/cherry-pick.go

@@ -26,6 +26,7 @@ import (
 	"github.com/dolthub/dolt/go/cmd/dolt/errhand"
 	"github.com/dolthub/dolt/go/libraries/doltcore/branch_control"
 	"github.com/dolthub/dolt/go/libraries/doltcore/env"
+	"github.com/dolthub/dolt/go/libraries/doltcore/env/actions"
 	"github.com/dolthub/dolt/go/libraries/utils/argparser"
 	"github.com/dolthub/dolt/go/store/util/outputpager"
 	eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1"
@@ -51,6 +52,12 @@ var ErrCherryPickConflictsOrViolations = errors.NewKind("error: Unable to apply
 	"To undo all changes from this cherry-pick operation, use `dolt cherry-pick --abort`.\n" +
 	"For more information on handling conflicts, see: https://docs.dolthub.com/concepts/dolt/git/conflicts")
 
+var ErrCherryPickVerificationFailed = errors.NewKind("error: Commit verification failed. Your changes are staged " +
+	"in the working set. Fix the failing tests, use `dolt add` to stage your changes, then " +
+	"`dolt cherry-pick --continue` to complete the cherry-pick.\n" +
+	"To undo all changes from this cherry-pick operation, use `dolt cherry-pick --abort`.\n" +
+	"Run `dolt sql -q 'select * from dolt_test_run()` to see which tests are failing.")
+
 type CherryPickCmd struct{}
 
 // Name returns the name of the Dolt cli command. This is what is used on the command line to invoke the command.
@@ -166,6 +173,10 @@ hint: commit your changes (dolt commit -am \"<message>\") or reset them (dolt re
 	}
 	rows, err := cli.GetRowsForSql(queryist, sqlCtx, q)
 	if err != nil {
+		if actions.ErrCommitVerificationFailed.Is(err) {
+			cli.PrintErrln(err.Error())
+			return ErrCherryPickVerificationFailed.New()
+		}
 		errorText := err.Error()
 		switch {
 		case strings.Contains("nothing to commit", errorText):
@@ -250,20 +261,27 @@ func cherryPickContinue(sqlCtx *sql.Context, queryist cli.Queryist) error {
 	query := "call dolt_cherry_pick('--continue')"
 	rows, err := cli.GetRowsForSql(queryist, sqlCtx, query)
 	if err != nil {
+		if actions.ErrCommitVerificationFailed.Is(err) {
+			cli.PrintErrln(err.Error())
+			return ErrCherryPickVerificationFailed.New()
+		}
 		return err
 	}
 
 	if len(rows) != 1 {
 		return fmt.Errorf("error: unexpected number of rows returned from dolt_cherry_pick: %d", len(rows))
 	}
-	if len(rows[0]) != 4 {
+
+	// No version of dolt_cherry_pick has ever returned less than 4 columns. We don't set an upper bound here to
+	// allow for servers to add more columns in the future without breaking compatibility with older clients.
+	if len(rows[0]) < 4 {
 		return fmt.Errorf("error: unexpected number of columns returned from dolt_cherry_pick: %d", len(rows[0]))
 	}
 
 	row := rows[0]
 
 	// We expect to get an error if there were problems, but we also could get any of the conflicts and
-	// vacation counts being greater than 0 if there were problems. If we got here without an error,
+	// violation counts being greater than 0 if there were problems. If we got here without an error,
 	// but we have conflicts or violations, we should report and stop.
 	dataConflicts, err := getInt64ColAsInt64(row[1])
 	if err != nil {

go/cmd/dolt/commands/merge.go

@@ -33,6 +33,7 @@ import (
 	"github.com/dolthub/dolt/go/libraries/doltcore/diff"
 	"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
 	"github.com/dolthub/dolt/go/libraries/doltcore/env"
+	"github.com/dolthub/dolt/go/libraries/doltcore/env/actions"
 	"github.com/dolthub/dolt/go/libraries/doltcore/merge"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dprocedures"
 	"github.com/dolthub/dolt/go/libraries/utils/argparser"
@@ -165,6 +166,11 @@ func (cmd MergeCmd) Exec(ctx context.Context, commandStr string, args []string,
 		return 1
 	}
 
+	if msg := getMergeMessage(mergeResultRow); strings.HasPrefix(msg, actions.CommitVerificationFailedPrefix) {
+		cli.Println(msg)
+		return 1
+	}
+
 	if !apr.Contains(cli.AbortParam) {
 		//todo: refs with the `remotes/` prefix will fail to get a hash
 		headHash, headHashErr := getHashOf(queryist.Queryist, queryist.Context, "HEAD")
@@ -751,3 +757,17 @@ func everythingUpToDate(row sql.Row) (bool, error) {
 
 	return false, nil
 }
+
+// getMergeMessage extracts the message column from a merge result row.
+func getMergeMessage(row sql.Row) string {
+	if len(row) == 3 {
+		if msg, ok := row[2].(string); ok {
+			return msg
+		}
+	} else if len(row) == 4 {
+		if msg, ok := row[3].(string); ok {
+			return msg
+		}
+	}
+	return ""
+}

go/cmd/dolt/commands/rebase.go

@@ -28,6 +28,7 @@ import (
 	"github.com/dolthub/dolt/go/cmd/dolt/errhand"
 	"github.com/dolthub/dolt/go/libraries/doltcore/dconfig"
 	"github.com/dolthub/dolt/go/libraries/doltcore/env"
+	"github.com/dolthub/dolt/go/libraries/doltcore/env/actions"
 	"github.com/dolthub/dolt/go/libraries/doltcore/rebase"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dprocedures"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess"
@@ -429,7 +430,8 @@ func syncCliBranchToSqlSessionBranch(ctx *sql.Context, dEnv *env.DoltEnv) error
 }
 
 // isRebaseConflictError checks if the given error represents a rebase pause condition
-// (data conflicts) that should not abort the rebase but instead allow the user to resolve/continue.
+// (data conflicts or verification failure) that should not abort the rebase but instead
+// allow the user to resolve/continue.
 func isRebaseConflictError(err error) bool {
 	if err == nil {
 		return false
@@ -439,8 +441,16 @@ func isRebaseConflictError(err error) bool {
 	if dprocedures.ErrRebaseDataConflict.Is(err) {
 		return true
 	}
+	if dprocedures.ErrRebaseVerificationFailed.Is(err) {
+		return true
+	}
+	if actions.ErrCommitVerificationFailed.Is(err) {
+		return true
+	}
 
 	// For over-the-wire errors that lose their type, match against error message patterns
 	errMsg := err.Error()
-	return strings.HasPrefix(errMsg, dprocedures.RebaseDataConflictPrefix)
+	return strings.HasPrefix(errMsg, dprocedures.RebaseDataConflictPrefix) ||
+		strings.HasPrefix(errMsg, dprocedures.RebaseVerificationFailedPrefix) ||
+		strings.HasPrefix(errMsg, actions.CommitVerificationFailedPrefix)
 }

go/libraries/doltcore/cherry_pick/cherry_pick.go

@@ -82,6 +82,14 @@ func CherryPick(ctx *sql.Context, commit string, options CherryPickOptions) (str
 		return "", nil, fmt.Errorf("failed to get roots for current session")
 	}
 
+	// Capture the working set BEFORE applying cherry-pick changes. If commit verification
+	// fails later, we need to set the merge state with preMergeWorking = original working
+	// root so that --abort properly restores the pre-cherry-pick state.
+	preApplyWs, wsErr := doltSession.WorkingSet(ctx, dbName)
+	if wsErr != nil {
+		return "", nil, wsErr
+	}
+
 	mergeResult, commitMsg, originalCommit, err := cherryPick(ctx, doltSession, roots, dbName, commit, options.EmptyCommitHandling)
 	if err != nil {
 		return "", mergeResult, err
@@ -131,6 +139,20 @@ func CherryPick(ctx *sql.Context, commit string, options CherryPickOptions) (str
 
 	pendingCommit, err := doltSession.NewPendingCommit(ctx, dbName, roots, *commitProps)
 	if err != nil {
+		// If verification failed, set merge state and return via the result (not as a Go error).
+		// Returning a Go error would roll back the transaction, undoing all working set changes.
+		// Returning nil error instead lets CommitTransaction persist the staged changes and merge
+		// state, so the user can fix the failing tests and --continue.
+		if actions.ErrCommitVerificationFailed.Is(err) {
+			newWs := preApplyWs.StartCherryPick(originalCommit, commit).
+				WithWorkingRoot(roots.Working).
+				WithStagedRoot(roots.Staged)
+			if wsErr := doltSession.SetWorkingSet(ctx, dbName, newWs); wsErr != nil {
+				return "", nil, wsErr
+			}
+			mergeResult.CommitVerificationErr = err
+			return "", mergeResult, nil
+		}
 		return "", nil, err
 	}
 	if pendingCommit == nil {
@@ -231,9 +253,10 @@ func AbortCherryPick(ctx *sql.Context, dbName string) error {
 	return doltSession.SetWorkingSet(ctx, dbName, newWs)
 }
 
-// ContinueCherryPick continues a cherry-pick merge that was paused due to conflicts.
-// It checks that conflicts have been resolved and creates the final commit with the
-// original commit's metadata.
+// ContinueCherryPick continues a cherry-pick merge that was paused due to conflicts or
+// commit verification failure. It checks that conflicts have been resolved and creates
+// the final commit with the original commit's metadata. Returns (hash, dataConflicts,
+// schemaConflicts, constraintViolations, verificationFailures, error).
 func ContinueCherryPick(ctx *sql.Context, dbName string) (string, int, int, int, error) {
 	doltSession := dsess.DSessFromSess(ctx.Session)
 
@@ -310,6 +333,9 @@ func ContinueCherryPick(ctx *sql.Context, dbName string) (string, int, int, int,
 
 	pendingCommit, err := doltSession.NewPendingCommit(ctx, dbName, roots, commitProps)
 	if err != nil {
+		if actions.ErrCommitVerificationFailed.Is(err) {
+			return "", 0, 0, 0, err
+		}
 		return "", 0, 0, 0, fmt.Errorf("error: failed to create pending commit: %w", err)
 	}
 	if pendingCommit == nil {

go/libraries/doltcore/env/actions/commit.go

@@ -22,12 +22,17 @@ import (
 
 	gms "github.com/dolthub/go-mysql-server"
 	"github.com/dolthub/go-mysql-server/sql"
+	goerrors "gopkg.in/src-d/go-errors.v1"
 
 	"github.com/dolthub/dolt/go/libraries/doltcore/diff"
 	"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
 	"github.com/dolthub/dolt/go/store/datas"
 )
 
+const CommitVerificationFailedPrefix = "commit verification failed:"
+
+var ErrCommitVerificationFailed = goerrors.NewKind(CommitVerificationFailedPrefix + " %s")
+
 type CommitStagedProps struct {
 	Message          string
 	Date             time.Time
@@ -45,10 +50,10 @@ const (
 	DoltCommitVerificationGroups = "dolt_commit_verification_groups"
 )
 
-// GetCommitRunTestGroups returns the test groups to run for commit operations
+// getCommitRunTestGroups returns the test groups to run for commit operations
 // Returns empty slice if no tests should be run, ["*"] if all tests should be run,
 // or specific group names if only those groups should be run
-func GetCommitRunTestGroups() []string {
+func getCommitRunTestGroups() []string {
 	_, val, ok := sql.SystemVariables.GetGlobal(DoltCommitVerificationGroups)
 	if !ok {
 		return nil
@@ -147,7 +152,7 @@ func GetCommitStaged(
 	}
 
 	if !props.SkipVerification {
-		testGroups := GetCommitRunTestGroups()
+		testGroups := getCommitRunTestGroups()
 		if len(testGroups) > 0 {
 			err := runCommitVerification(ctx, testGroups)
 			if err != nil {
@@ -164,6 +169,9 @@ func GetCommitStaged(
 	return db.NewPendingCommit(ctx, roots, mergeParents, props.Amend, meta)
 }
 
+// runCommitVerification runs the commit verification tests for the given test groups.
+// If any tests fail, it returns ErrCommitVerificationFailed wrapping the failure details.
+// Callers can use errors.Is(err, ErrCommitVerificationFailed) to detect this case.
 func runCommitVerification(ctx *sql.Context, testGroups []string) error {
 	type sessionInterface interface {
 		sql.Session
@@ -216,7 +224,7 @@ func runTestsUsingDtablefunctions(ctx *sql.Context, engine *gms.Engine, testGrou
 	}
 
 	if len(allFailures) > 0 {
-		return fmt.Errorf("commit verification failed: %s", strings.Join(allFailures, ", "))
+		return ErrCommitVerificationFailed.New(strings.Join(allFailures, ", "))
 	}
 
 	return nil

go/libraries/doltcore/merge/merge.go

@@ -87,9 +87,10 @@ func MergeCommits(ctx *sql.Context, tableResolver doltdb.TableResolver, commit,
 }
 
 type Result struct {
-	Root            doltdb.RootValue
-	SchemaConflicts []SchemaConflict
-	Stats           map[doltdb.TableName]*MergeStats
+	Root                  doltdb.RootValue
+	SchemaConflicts       []SchemaConflict
+	Stats                 map[doltdb.TableName]*MergeStats
+	CommitVerificationErr error
 }
 
 func (r Result) HasSchemaConflicts() bool {

go/libraries/doltcore/sqle/dprocedures/dolt_cherry_pick.go

@@ -25,6 +25,7 @@ import (
 	"github.com/dolthub/dolt/go/libraries/doltcore/branch_control"
 	"github.com/dolthub/dolt/go/libraries/doltcore/cherry_pick"
 	"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
+	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess"
 )
 
 var ErrEmptyCherryPick = errors.New("cannot cherry-pick empty string")
@@ -64,6 +65,7 @@ func doltCherryPick(ctx *sql.Context, args ...string) (sql.RowIter, error) {
 // doDoltCherryPick attempts to perform a cherry-pick merge based on the arguments specified in |args| and returns
 // the new, created commit hash (if it was successful created), a count of the number of tables with data conflicts,
 // a count of the number of tables with schema conflicts, and a count of the number of tables with constraint violations.
+// Verification failures are returned as errors.
 func doDoltCherryPick(ctx *sql.Context, args []string) (string, int, int, int, error) {
 	// Get the information for the sql context.
 	dbName := ctx.GetCurrentDatabase()
@@ -119,6 +121,15 @@ func doDoltCherryPick(ctx *sql.Context, args []string) (string, int, int, int, e
 	}
 
 	if mergeResult != nil {
+		if mergeResult.CommitVerificationErr != nil {
+			// Commit the transaction to persist the dirty working set to the staged working set.
+			// This allows the user to address the verification failure and then `--continue` the cherry-pick.
+			doltSession := dsess.DSessFromSess(ctx.Session)
+			if txErr := doltSession.CommitTransaction(ctx, doltSession.GetTransaction()); txErr != nil {
+				return "", 0, 0, 0, txErr
+			}
+			return "", 0, 0, 0, mergeResult.CommitVerificationErr
+		}
 		return "",
 			mergeResult.CountOfTablesWithDataConflicts(),
 			mergeResult.CountOfTablesWithSchemaConflicts(),

go/libraries/doltcore/sqle/dprocedures/dolt_merge.go

@@ -245,6 +245,10 @@ func performMerge(
 				}
 				ctx.Warn(DoltMergeWarningCode, "%s", err.Error())
 				return ws, "", hasConflictsOrViolations, threeWayMerge, "", err
+			} else if actions.ErrCommitVerificationFailed.Is(err) {
+				// We don't return an error here because that rolls back the transaction, while we actually need the
+				// staged data to allow the user to address the verification problem.
+				return ws, "", noConflictsOrViolations, threeWayMerge, err.Error(), nil
 			} else if err != nil {
 				return ws, "", noConflictsOrViolations, threeWayMerge, "", err
 			}
@@ -314,6 +318,9 @@ func performMerge(
 		}
 		commit, _, err = doDoltCommit(ctx, args)
 		if err != nil {
+			if actions.ErrCommitVerificationFailed.Is(err) {
+				return ws, "", noConflictsOrViolations, threeWayMerge, err.Error(), nil
+			}
 			return ws, commit, noConflictsOrViolations, threeWayMerge, "", err
 		}
 	}
@@ -457,6 +464,9 @@ func executeNoFFMerge(
 		SkipVerification: skipVerification,
 	})
 	if err != nil {
+		if actions.ErrCommitVerificationFailed.Is(err) {
+			return ws, nil, err
+		}
 		return nil, nil, err
 	}
 

go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go

@@ -144,6 +144,13 @@ var ErrRebaseDataConflict = goerrors.NewKind(RebaseDataConflictPrefix + " %s (%s
 	"Resolve the conflicts and remove them from the dolt_conflicts_<table> tables, " +
 	"then continue the rebase by calling dolt_rebase('--continue')")
 
+const RebaseVerificationFailedPrefix = "commit verification failed while rebasing commit"
+
+// ErrRebaseVerificationFailed is used when commit verification tests fail while rebasing a commit.
+// The workspace is left dirty so the user can fix the failing tests and retry using --continue.
+var ErrRebaseVerificationFailed = goerrors.NewKind(RebaseVerificationFailedPrefix + " %s (%s): %s\n\n" +
+	"Fix the failing tests and stage your changes, then continue the rebase by calling dolt_rebase('--continue')")
+
 var EditPausePrefix = "edit action paused at commit"
 
 // createEditPauseMessage creates a pause message for edit actions
@@ -985,6 +992,19 @@ func handleRebaseCherryPick(
 		return newRebaseError(ErrRebaseDataConflict.New(planStep.CommitHash, planStep.CommitMsg))
 	}
 
+	if mergeResult != nil && mergeResult.CommitVerificationErr != nil {
+		if doltSession.GetTransaction() == nil {
+			_, txErr := doltSession.StartTransaction(ctx, sql.ReadWrite)
+			if txErr != nil {
+				return newRebaseError(txErr)
+			}
+		}
+		if txErr := doltSession.CommitTransaction(ctx, doltSession.GetTransaction()); txErr != nil {
+			return newRebaseError(txErr)
+		}
+		return newRebaseError(mergeResult.CommitVerificationErr)
+	}
+
 	// If this is an edit action and no conflicts occurred, pause the rebase to allow user modifications
 	if planStep.Action == rebase.RebaseActionEdit && err == nil {
 		return newRebasePause(createEditPauseMessage(planStep.CommitHash, planStep.CommitMsg))