hotfix: preserve earlier-exercise code when resetting, syncing, or replacing on merge conflict (#49)

Author: roblaszczak

trainings/exercise_replace.go

@@ -0,0 +1,95 @@
+package trainings
+
+import (
+	"fmt"
+
+	"github.com/spf13/afero"
+
+	"github.com/ThreeDotsLabs/cli/trainings/files"
+	"github.com/ThreeDotsLabs/cli/trainings/genproto"
+	"github.com/ThreeDotsLabs/cli/trainings/git"
+)
+
+// replaceExerciseFiles writes the given files into exerciseDir and DELETES any
+// working-tree file in that dir not in the list.
+//
+// INVARIANT: after this call, exerciseDir is 1:1 with `files` — no stale user
+// content. This is the SINGLE POINT that enforces the "sync/replace must not
+// leave stale user files" invariant. Every code path that replaces the user's
+// solution with example or start-state files MUST route through this function
+// (via replaceExerciseFilesAndCommit).
+//
+// Do NOT inline this call with files.NewFilesSilent — that path is additive and
+// will reintroduce the project-style-training staleness bug (empty placeholders
+// from earlier scaffolds overwriting the user's filled-in work; see the
+// 0001_init_orders.up.sql regression).
+func replaceExerciseFiles(
+	trainingRootFs *afero.BasePathFs,
+	replacementFiles []*genproto.File,
+	exerciseDir string,
+) error {
+	return files.NewFilesSilentDeleteUnused().WriteExerciseFiles(replacementFiles, trainingRootFs, exerciseDir)
+}
+
+// mergeStartStateFiles returns the starting state of an exercise:
+// golden(prev) with scaffold(current) overlaid on top. Scaffold wins on path
+// collisions because it represents the delta from end-of-prev to start-of-current.
+// Pass nil goldenFiles for the first exercise.
+func mergeStartStateFiles(
+	goldenFiles []*genproto.File,
+	scaffoldFiles []*genproto.File,
+) []*genproto.File {
+	byPath := make(map[string]*genproto.File, len(goldenFiles)+len(scaffoldFiles))
+	for _, f := range goldenFiles {
+		byPath[f.Path] = f
+	}
+	for _, f := range scaffoldFiles {
+		byPath[f.Path] = f // scaffold wins on collisions
+	}
+	merged := make([]*genproto.File, 0, len(byPath))
+	for _, f := range byPath {
+		merged = append(merged, f)
+	}
+	return merged
+}
+
+// replaceExerciseFilesAndCommit is the complete orchestration for replacing
+// the user's exercise files with a given file list: save backup → write files
+// (1:1, deletes extras) → stage → commit. ALL callers that replace the user's
+// solution with example / start-state content MUST go through this function:
+//   - overrideWithGolden ('s' action):       files = golden(current)
+//   - g during next/merge-conflict:          files = golden(prev) + scaffold(current)
+//   - resetCleanFiles:                       files = golden(prev) + scaffold(current)
+//
+// The backup branch is REQUIRED — destructive ops must always be recoverable.
+// If saveToBackupBranch returns errBackupAborted, that error is returned directly
+// so callers can distinguish user-abort from other failures.
+//
+// Returns (committed=true, nil) on success with a commit created, (false, nil)
+// if the resulting state matched HEAD with nothing to commit, or (false, err)
+// on any failure.
+func replaceExerciseFilesAndCommit(
+	gitOps *git.Ops,
+	trainingRootFs *afero.BasePathFs,
+	replacementFiles []*genproto.File,
+	exerciseDir string,
+	backupBranch string,
+	commitMsg string,
+) (committed bool, err error) {
+	if err := saveToBackupBranch(gitOps, backupBranch); err != nil {
+		return false, err
+	}
+	if err := replaceExerciseFiles(trainingRootFs, replacementFiles, exerciseDir); err != nil {
+		return false, fmt.Errorf("could not replace exercise files: %w", err)
+	}
+	if err := gitOps.AddAll(exerciseDir); err != nil {
+		return false, fmt.Errorf("could not stage replaced files: %w", err)
+	}
+	if !gitOps.HasStagedChanges() {
+		return false, nil
+	}
+	if err := gitOps.Commit(commitMsg); err != nil {
+		return false, fmt.Errorf("could not commit replaced files: %w", err)
+	}
+	return true, nil
+}

trainings/exercise_replace_test.go

@@ -0,0 +1,115 @@
+package trainings
+
+import (
+	"sort"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/ThreeDotsLabs/cli/trainings/genproto"
+)
+
+func TestMergeStartStateFiles(t *testing.T) {
+	t.Run("first exercise (nil golden) returns scaffold as-is", func(t *testing.T) {
+		scaffold := []*genproto.File{
+			{Path: "a.txt", Content: "A"},
+			{Path: "b.txt", Content: "B"},
+		}
+		merged := mergeStartStateFiles(nil, scaffold)
+		assertFilesByPath(t, merged, map[string]string{
+			"a.txt": "A",
+			"b.txt": "B",
+		})
+	})
+
+	t.Run("scaffold wins on path collision", func(t *testing.T) {
+		golden := []*genproto.File{
+			{Path: "shared.txt", Content: "from golden"},
+			{Path: "golden-only.txt", Content: "G"},
+		}
+		scaffold := []*genproto.File{
+			{Path: "shared.txt", Content: "from scaffold"}, // overrides golden
+			{Path: "scaffold-only.txt", Content: "S"},
+		}
+		merged := mergeStartStateFiles(golden, scaffold)
+		assertFilesByPath(t, merged, map[string]string{
+			"shared.txt":        "from scaffold",
+			"golden-only.txt":   "G",
+			"scaffold-only.txt": "S",
+		})
+	})
+
+	t.Run("regression: golden with filled-in placeholder is preserved when scaffold does not redeliver it", func(t *testing.T) {
+		// This is the 0001_init_orders.up.sql scenario.
+		// Earlier exercises scaffolded the file as empty; the user filled it in.
+		// By a later exercise, the scaffold no longer includes that file — only
+		// the prev-exercise golden does. The start state must preserve the
+		// filled-in content.
+		golden := []*genproto.File{
+			{Path: "migrations/0001_init.sql", Content: "CREATE TABLE ..."},
+			{Path: "common.go", Content: "package common"},
+		}
+		scaffold := []*genproto.File{
+			{Path: "new_for_this_exercise.go", Content: "package new"},
+		}
+		merged := mergeStartStateFiles(golden, scaffold)
+		assertFilesByPath(t, merged, map[string]string{
+			"migrations/0001_init.sql": "CREATE TABLE ...", // survives from golden
+			"common.go":                "package common",
+			"new_for_this_exercise.go": "package new",
+		})
+	})
+}
+
+func TestReplaceExerciseFiles_is1to1(t *testing.T) {
+	// The invariant: after replaceExerciseFiles, exerciseDir contains exactly
+	// the replacement files — any extras are deleted. This is load-bearing
+	// for the "sync with example" and "replace on conflict" UX: user's project
+	// must never silently diverge from what the caller asked for.
+	fs := afero.NewMemMapFs()
+	rootFs := afero.NewBasePathFs(fs, "/").(*afero.BasePathFs)
+
+	// Pre-populate exerciseDir with some files that should be removed.
+	require.NoError(t, afero.WriteFile(fs, "/ex/stale_placeholder.sql", []byte("-- todo"), 0644))
+	require.NoError(t, afero.WriteFile(fs, "/ex/user_scratch.txt", []byte("notes"), 0644))
+	require.NoError(t, afero.WriteFile(fs, "/ex/keep_me.go", []byte("old content"), 0644))
+
+	replacement := []*genproto.File{
+		{Path: "keep_me.go", Content: "new content"},
+		{Path: "fresh.go", Content: "fresh"},
+	}
+	require.NoError(t, replaceExerciseFiles(rootFs, replacement, "ex"))
+
+	// keep_me.go is overwritten
+	got, err := afero.ReadFile(fs, "/ex/keep_me.go")
+	require.NoError(t, err)
+	assert.Equal(t, "new content", string(got))
+
+	// fresh.go is created
+	got, err = afero.ReadFile(fs, "/ex/fresh.go")
+	require.NoError(t, err)
+	assert.Equal(t, "fresh", string(got))
+
+	// stale_placeholder.sql and user_scratch.txt are DELETED (not in replacement).
+	// This is the 1:1 invariant the fix enforces.
+	_, err = fs.Stat("/ex/stale_placeholder.sql")
+	assert.True(t, err != nil, "stale placeholder should have been deleted")
+	_, err = fs.Stat("/ex/user_scratch.txt")
+	assert.True(t, err != nil, "user scratch should have been deleted")
+}
+
+func assertFilesByPath(t *testing.T, got []*genproto.File, want map[string]string) {
+	t.Helper()
+	byPath := map[string]string{}
+	for _, f := range got {
+		byPath[f.Path] = f.Content
+	}
+	paths := make([]string, 0, len(byPath))
+	for p := range byPath {
+		paths = append(paths, p)
+	}
+	sort.Strings(paths)
+	assert.Equal(t, want, byPath, "merged files mismatch; paths present: %v", paths)
+}

trainings/exercises.go

@@ -0,0 +1,71 @@
+package trainings
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/ThreeDotsLabs/cli/trainings/genproto"
+)
+
+// fetchGoldenFiles wraps GetGoldenSolution with the standard error path.
+// Used by overrideWithGolden ('s'), g handlers, and reset — every caller that
+// needs the example-solution file list for a specific exercise.
+func (h *Handlers) fetchGoldenFiles(
+	ctx context.Context,
+	trainingName, exerciseID, token string,
+) ([]*genproto.File, error) {
+	resp, err := h.newGrpcClient().GetGoldenSolution(ctx, &genproto.GetGoldenSolutionRequest{
+		TrainingName: trainingName,
+		ExerciseId:   exerciseID,
+		Token:        token,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to fetch example solution for exercise %s: %w", exerciseID, err)
+	}
+	return resp.Files, nil
+}
+
+// resolvePreviousExercise returns the ID and module/exercise path of the exercise
+// immediately preceding currentExerciseID in the training's order. Returns
+// ("", "", nil) when currentExerciseID is the first exercise.
+//
+// One GetExercises call per invocation — callers may cache the result per-command
+// if they need it more than once, but the common case (single reset / single 'g')
+// only needs it once.
+func (h *Handlers) resolvePreviousExercise(
+	ctx context.Context,
+	trainingName, token, currentExerciseID string,
+) (prevExerciseID, prevModuleExercisePath string, err error) {
+	resp, err := h.newGrpcClient().GetExercises(ctx, &genproto.GetExercisesRequest{
+		TrainingName: trainingName,
+		Token:        token,
+	})
+	if err != nil {
+		return "", "", fmt.Errorf("failed to list exercises: %w", err)
+	}
+
+	type flatExercise struct {
+		id                 string
+		moduleExercisePath string
+	}
+	var flat []flatExercise
+	for _, module := range resp.Modules {
+		for _, exercise := range module.Exercises {
+			flat = append(flat, flatExercise{
+				id:                 exercise.Id,
+				moduleExercisePath: module.Name + "/" + exercise.Name,
+			})
+		}
+	}
+
+	for i, e := range flat {
+		if e.id == currentExerciseID {
+			if i == 0 {
+				return "", "", nil // first exercise — no predecessor
+			}
+			return flat[i-1].id, flat[i-1].moduleExercisePath, nil
+		}
+	}
+
+	return "", "", fmt.Errorf("exercise %s not found in training %s", currentExerciseID, trainingName)
+}

trainings/files/files.go

@@ -65,6 +65,13 @@ func NewFilesSilent() Files {
 // NewFilesSilentDeleteUnused creates a Files that writes silently and deletes
 // files not present in the server response. Use for override operations where
 // the exercise directory should exactly match the example solution.
+//
+// Used by trainings.replaceExerciseFiles to enforce the "1:1 with the replacement
+// file list" invariant on "sync with example" and "replace on merge conflict"
+// flows. Do NOT switch the replace primitive to NewFilesSilent (additive) — that
+// path allowed stale empty placeholders from earlier scaffolds to overwrite
+// filled-in user code in project-style trainings (the 0001_init_orders.up.sql
+// regression).
 func NewFilesSilentDeleteUnused() Files {
 	return Files{
 		forceOverwrite:    true,

trainings/jump.go

@@ -305,13 +305,13 @@ func (h *Handlers) Jump(ctx context.Context, exerciseID string) error {
 			case '\n':
 				// Continue — no file ops, just update config pointer
 			case 'r':
-				if _, err := h.resetCleanFiles(gitOps, initBranch, moduleExercisePath, resp.Dir); err != nil {
+				if _, err := h.resetCleanFiles(ctx, gitOps, trainingRootFs, resp.ExerciseId, moduleExercisePath, resp.Dir); err != nil {
 					if errors.Is(err, errBackupAborted) {
 						return nil // user aborted — clean exit
 					}
 					return err
 				}
-				writeServerFiles(resp.FilesToCreate, trainingRootFs, resp.Dir, gitOps, moduleExercisePath)
+				// resetCleanFiles already fetched fresh scaffold+golden and wrote the start state.
 			case 's':
 				if err := h.checkoutSolution(ctx, gitOps, successfulVerificationId, moduleExercisePath, trainingRootFs); err != nil {
 					if errors.Is(err, errBackupAborted) {
@@ -323,7 +323,7 @@ func (h *Handlers) Jump(ctx context.Context, exerciseID string) error {
 		}
 	}
 
-	_, err = h.setExercise(trainingRootFs, resp, trainingRoot, false)
+	_, err = h.setExercise(ctx, trainingRootFs, resp, trainingRoot, false)
 	return err
 }