plan 9: undo checks trash identity, groups by directory, re-checks at execution, removes directories it recreates

5 files changed, 318 insertions, 35 deletions

diff --git a/internal/apply/apply.go b/internal/apply/apply.go
index 5624b27..684641e 100644
--- a/internal/apply/apply.go
+++ b/internal/apply/apply.go
@@ -180,7 +180,7 @@ func runFileStep(step plan.Step) StepResult {
 		return StepResult{Step: step, Status: "failed", Detail: err.Error()}
 	}
 
-	made, err := mkdirAllTracked(filepath.Dir(dst))
+	made, err := MkdirAllTracked(filepath.Dir(dst))
 	if err != nil {
 		return StepResult{Step: step, Status: "failed", Detail: err.Error(), Made: made, DisplacedEntry: displacedEntry}
 	}
diff --git a/internal/apply/fs.go b/internal/apply/fs.go
index 823d5c8..56737e8 100644
--- a/internal/apply/fs.go
+++ b/internal/apply/fs.go
@@ -201,11 +201,11 @@ func splitExt(name string) (stem, ext string) {
 	return name[:i], name[i:]
 }
 
-// mkdirAllTracked creates dir and any missing ancestors (mode 0755),
+// MkdirAllTracked creates dir and any missing ancestors (mode 0755),
 // returning every directory it actually created, outermost first, so undo
 // can later remove the empty ones again. A directory that already existed
 // is not included, and nothing is created or returned on error.
-func mkdirAllTracked(dir string) ([]string, error) {
+func MkdirAllTracked(dir string) ([]string, error) {
 	dir = filepath.Clean(dir)
 	if fi, err := os.Stat(dir); err == nil {
 		if !fi.IsDir() {
@@ -219,7 +219,7 @@ func mkdirAllTracked(dir string) ([]string, error) {
 	parent := filepath.Dir(dir)
 	var made []string
 	if parent != dir {
-		parentMade, err := mkdirAllTracked(parent)
+		parentMade, err := MkdirAllTracked(parent)
 		if err != nil {
 			return nil, err
 		}
diff --git a/internal/engine/apply.go b/internal/engine/apply.go
index 83ea4fe..94d65e4 100644
--- a/internal/engine/apply.go
+++ b/internal/engine/apply.go
@@ -343,21 +343,26 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) {
 		return nil, fmt.Errorf("engine: plan undo: run %s is itself an undo and cannot be undone", runID)
 	}
 
-	var order []string
-	byFile := map[string][]journal.Entry{}
+	// Entries are grouped by directory and file together (review M7): one run
+	// spans every directory, and two directories can each hold a file of the
+	// same name.
+	type fileKey struct{ dir, file string }
+	var order []fileKey
+	byFile := map[fileKey][]journal.Entry{}
 	for _, en := range entries {
 		if en.File == "" { // run-start / run-end
 			continue
 		}
-		if _, seen := byFile[en.File]; !seen {
-			order = append(order, en.File)
+		k := fileKey{en.Dir, en.File}
+		if _, seen := byFile[k]; !seen {
+			order = append(order, k)
 		}
-		byFile[en.File] = append(byFile[en.File], en)
+		byFile[k] = append(byFile[k], en)
 	}
 
 	up := &UndoPlan{Run: runID}
-	for _, file := range order {
-		uf := planUndoFile(file, byFile[file])
+	for _, k := range order {
+		uf := planUndoFile(k.dir, k.file, byFile[k])
 		// Critical finding, Task 8's review: a file every one of whose
 		// entries has Status != "ok" (declined by the ORIGINAL run's own
 		// review, or skipped, or failed before anything happened) yields
@@ -459,8 +464,8 @@ func isFileAffecting(action string) bool {
 // two steps of one file that touch the same path, and every un-contended
 // check keeps behaving exactly as before, since the projection only ever
 // overrides a real occupant that this same chain is itself about to clear.
-func planUndoFile(file string, ents []journal.Entry) UndoFile {
-	uf := UndoFile{File: file, Dir: dirOf(ents)}
+func planUndoFile(dir, file string, ents []journal.Entry) UndoFile {
+	uf := UndoFile{File: file, Dir: dir}
 	proj := newUndoProjection()
 	for i := len(ents) - 1; i >= 0; i-- {
 		en := ents[i]
@@ -560,17 +565,6 @@ func needsOccupancyCheck(action string) bool {
 	return false
 }
 
-// dirOf returns the first non-empty Dir among ents, which should all agree
-// since one file is always processed within one configured directory.
-func dirOf(ents []journal.Entry) string {
-	for _, en := range ents {
-		if en.Dir != "" {
-			return en.Dir
-		}
-	}
-	return ""
-}
-
 // reverseStep computes the UndoStep for one logged "ok" entry, per spec
 // §10's reversal table. It only stats the filesystem to decide Refused (the
 // "changed since" and "trash entry is gone" checks); it never mutates
@@ -597,9 +591,7 @@ func reverseStep(en journal.Entry) UndoStep {
 		us.Refused = refuseIfChanged(en)
 	case "trash", "displace":
 		us.Src, us.Dst = en.Dst, en.Src
-		if _, err := os.Stat(en.Dst); err != nil {
-			us.Refused = "the trash entry is gone"
-		}
+		us.Refused = refuseIfTrashChanged(en)
 	case "mkdir":
 		us.Src = en.Dst
 	}
@@ -635,6 +627,38 @@ func refuseIfChanged(en journal.Entry) string {
 	return ""
 }
 
+// refuseIfTrashChanged is the trash reversal's identity check (review M2):
+// the entry must still be the file this run put there - the size and mtime
+// the run logged for it - and its trashinfo must still record the path it
+// came from. Emptying the Trash and trashing another file of the same name
+// would otherwise have undo restore that file instead.
+func refuseIfTrashChanged(en journal.Entry) string {
+	fi, err := os.Lstat(en.Dst)
+	if err != nil {
+		return "the trash entry is gone"
+	}
+	if fi.Size() != en.Size || !fi.ModTime().Equal(en.ModTime) {
+		return fmt.Sprintf("the trash entry %s is not the file this run put there", en.Detail)
+	}
+	if p, err := trash.InfoPath(en.Detail); err != nil || p != en.Src {
+		return fmt.Sprintf("the trash entry %s now belongs to another file", en.Detail)
+	}
+	return ""
+}
+
+// recheck repeats, at execution time, the identity check PlanUndo made
+// (review undo F8): an undo plan is shown and approved first, and a file
+// changed in that window must not be moved back or trashed.
+func recheck(step UndoStep) string {
+	switch step.Action {
+	case "undo-move", "undo-rename", "undo-copy":
+		return refuseIfChanged(step.Original)
+	case "undo-trash", "undo-displace":
+		return refuseIfTrashChanged(step.Original)
+	}
+	return ""
+}
+
 // refuseIfSrcExists is the second half of every "reversal puts a file back
 // at a fixed path" refusal condition: reversing would silently clobber
 // whatever is there now. Fix wave item 1: it is no longer reverseStep's own
@@ -773,7 +797,10 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer,
 		tallyFile(result, fr.Steps, func(i int) bool { return isFileAffecting(f.Steps[i].Action) })
 		for i, us := range f.Steps {
 			if us.Action == "undo-mkdir" && fr.Steps[i].Status == "failed" {
-				retries = append(retries, dirRetry{dir: us.Src, dirName: f.Dir, file: f.File, step: i + 1})
+				retries = append(retries, dirRetry{dir: us.Src, dirName: f.Dir, file: f.File, step: i + 1, log: true})
+			}
+			for _, made := range fr.Steps[i].Made {
+				retries = append(retries, dirRetry{dir: made})
 			}
 		}
 	}
@@ -802,6 +829,10 @@ type dirRetry struct {
 	dirName string
 	file    string
 	step    int
+	// log is false for a directory this undo run itself created on the way
+	// (runUndoStep's Made): the original run's log already says it was
+	// removed, so removing it again needs no entry of its own.
+	log bool
 }
 
 // retryDirRemovals is ApplyUndo's run-wide second pass (Task 1, plan 5): once
@@ -833,6 +864,9 @@ func (e *Engine) retryDirRemovals(j *journal.Writer, run string, candidates []di
 			// original refusal already recorded this, and it stands.
 			continue
 		}
+		if !c.log {
+			continue
+		}
 		if err := j.Append(journal.Entry{
 			Time: e.Now(), Run: run, Dir: c.dirName, File: c.file, Step: c.step,
 			Action: "undo-mkdir", Status: "ok", Src: c.dir,
@@ -924,16 +958,24 @@ func (e *Engine) undoFile(f UndoFile, j *journal.Writer, run string) (FileResult
 // Step field does not apply here (there is no plan.Kind for an undo) and is
 // left zero.
 func runUndoStep(step UndoStep) apply.StepResult {
+	if why := recheck(step); why != "" {
+		return apply.StepResult{Status: "failed", Detail: why}
+	}
 	switch step.Action {
 	case "undo-move", "undo-rename":
-		if err := os.MkdirAll(filepath.Dir(step.Dst), 0o755); err != nil {
-			return apply.StepResult{Status: "failed", Detail: err.Error()}
+		// The directories created here are returned in Made: ApplyUndo removes
+		// them again once empty, since an earlier file's undo-mkdir may already
+		// have removed the directory this file passes back through (review
+		// undo F6).
+		made, err := apply.MkdirAllTracked(filepath.Dir(step.Dst))
+		if err != nil {
+			return apply.StepResult{Status: "failed", Detail: err.Error(), Made: made}
 		}
 		if err := renameOrCopy(step.Src, step.Dst); err != nil {
-			return apply.StepResult{Status: "failed", Detail: err.Error()}
+			return apply.StepResult{Status: "failed", Detail: err.Error(), Made: made}
 		}
 		size, mtime := statSizeModTime(step.Dst)
-		return apply.StepResult{Status: "ok", Dst: step.Dst, Size: size, ModTime: mtime}
+		return apply.StepResult{Status: "ok", Dst: step.Dst, Size: size, ModTime: mtime, Made: made}
 
 	case "undo-copy":
 		entry, err := trash.Put(step.Src)
diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go
index a54bffc..104fb73 100644
--- a/internal/engine/apply_test.go
+++ b/internal/engine/apply_test.go
@@ -749,11 +749,19 @@ func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Original is what the run logged for the trash step: the entry name in
+	// Detail, and the entry's own path, size and mtime (checked again at
+	// execution time, review undo F8).
+	entryPath := filepath.Join(trash.Dir(), "files", entry)
+	fi, err := os.Stat(entryPath)
+	if err != nil {
+		t.Fatal(err)
+	}
 	step := UndoStep{
 		Action:   "undo-trash",
 		Src:      "/this/path/does/not/exist/files/wrong-name",
 		Dst:      target,
-		Original: journal.Entry{Detail: entry},
+		Original: journal.Entry{Action: "trash", Src: target, Dst: entryPath, Size: fi.Size(), ModTime: fi.ModTime(), Detail: entry},
 	}
 	sr := runUndoStep(step)
 	if sr.Status != "ok" {
@@ -863,11 +871,15 @@ func TestUndoFileContinuesPastFailedMkdir(t *testing.T) {
 	defer j.Close()
 	e := &Engine{Now: time.Now}
 
+	keepInfo, err := os.Stat(keep)
+	if err != nil {
+		t.Fatal(err)
+	}
 	uf := UndoFile{
 		File: "f", Dir: "d",
 		Steps: []UndoStep{
 			{Action: "undo-mkdir", Src: nonEmpty},
-			{Action: "undo-copy", Src: keep},
+			{Action: "undo-copy", Src: keep, Original: journal.Entry{Action: "copy", Dst: keep, Size: keepInfo.Size(), ModTime: keepInfo.ModTime()}},
 		},
 	}
 	fr, err := e.undoFile(uf, j, "run2")
@@ -967,9 +979,13 @@ func TestApplyUndoDoesNotCountFailedMkdirAsFailed(t *testing.T) {
 	defer j.Close()
 	e := &Engine{Now: time.Now}
 
+	dstInfo, err := os.Stat(dst)
+	if err != nil {
+		t.Fatal(err)
+	}
 	up := &UndoPlan{Run: "r", Files: []UndoFile{
 		{File: "a.pdf", Dir: "d", Steps: []UndoStep{
-			{Action: "undo-move", Src: dst, Dst: src},
+			{Action: "undo-move", Src: dst, Dst: src, Original: journal.Entry{Action: "move", Src: src, Dst: dst, Size: dstInfo.Size(), ModTime: dstInfo.ModTime()}},
 			{Action: "undo-mkdir", Src: dir},
 		}},
 	}}
diff --git a/internal/engine/undo_identity_test.go b/internal/engine/undo_identity_test.go
new file mode 100644
index 0000000..9c93b3b
--- /dev/null
+++ b/internal/engine/undo_identity_test.go
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package engine
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+	"time"
+
+	"krino/internal/journal"
+	"krino/internal/plan"
+	"krino/internal/trash"
+)
+
+// appliedRun makes each directory of files under the sandbox home (a map of
+// directory name to file name to content, every file two hours old), writes
+// rules for each, and applies every chain of every directory in one run. It
+// returns the engine, the run id, the home directory and the log path.
+func appliedRun(t *testing.T, files map[string]map[string]string, rules map[string]string) (*Engine, string, string, string) {
+	t.Helper()
+	h := sandbox(t)
+	old := time.Now().Add(-2 * time.Hour)
+	var names []string
+	for dir, fs := range files {
+		names = append(names, dir)
+		for name, body := range fs {
+			p := filepath.Join(h, dir, name)
+			if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
+				t.Fatal(err)
+			}
+			if err := os.WriteFile(p, []byte(body), 0o644); err != nil {
+				t.Fatal(err)
+			}
+			if err := os.Chtimes(p, old, old); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
+	sort.Strings(names)
+	main := writeConfig(t, h, `(include "`+strings.Join(names, `" "`)+`")`, rules)
+	e, errs := Load(main)
+	if len(errs) > 0 {
+		t.Fatal(errs)
+	}
+	logPath := filepath.Join(h, ".local", "state", "krino", "krino.log")
+	j, err := journal.Open(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer j.Close()
+	run := journal.NewRunID(time.Now())
+	claims := plan.NewClaims()
+	for _, d := range e.Dirs {
+		dp, err := e.Plan(context.Background(), d, claims)
+		if err != nil {
+			t.Fatal(err)
+		}
+		approved := map[string]bool{}
+		for _, c := range dp.Chains {
+			approved[c.File.Rel] = true
+		}
+		res, err := e.Apply(context.Background(), dp, approved, j, run)
+		if err != nil || res.Failed != 0 {
+			t.Fatalf("apply %s: %v, %+v", d.Name, err, res)
+		}
+	}
+	return e, run, h, logPath
+}
+
+// undoFileNamed returns the plan's file for dir and name, failing the test
+// when there is none.
+func undoFileNamed(t *testing.T, up *UndoPlan, dir, name string) UndoFile {
+	t.Helper()
+	for _, f := range up.Files {
+		if f.Dir == dir && f.File == name {
+			return f
+		}
+	}
+	t.Fatalf("no %s/%s in the undo plan: %+v", dir, name, up.Files)
+	return UndoFile{}
+}
+
+// TestUndoRefusesAReusedTrashEntry: the Trash is emptied and another file of
+// the same name trashed after the run; undo must not restore that file in
+// place of the one the run trashed (review M2).
+func TestUndoRefusesAReusedTrashEntry(t *testing.T) {
+	e, run, h, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (delete))\n"})
+	if err := os.RemoveAll(trash.Dir()); err != nil {
+		t.Fatal(err)
+	}
+	other := filepath.Join(h, "Documents", "r-a.pdf")
+	if err := os.MkdirAll(filepath.Dir(other), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(other, []byte("an unrelated document"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if entry, err := trash.Put(other); err != nil || entry != "r-a.pdf" {
+		t.Fatalf("trash.Put = %q, %v", entry, err)
+	}
+	up, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if f := undoFileNamed(t, up, "dl", "a.pdf"); !strings.Contains(f.Refused, "trash entry") {
+		t.Errorf("Refused = %q; want a refusal naming the trash entry", f.Refused)
+	}
+}
+
+// TestUndoRefusesATrashEntryThatChanged: a trash entry that is no longer the
+// file the run put there (its size changed) is refused, like a moved file
+// that changed (review M2).
+func TestUndoRefusesATrashEntryThatChanged(t *testing.T) {
+	e, run, _, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (delete))\n"})
+	if err := os.WriteFile(filepath.Join(trash.Dir(), "files", "a.pdf"), []byte("a longer, different body"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	up, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if f := undoFileNamed(t, up, "dl", "a.pdf"); f.Refused == "" {
+		t.Error("a changed trash entry was not refused")
+	}
+}
+
+// TestUndoKeepsSameNamedFilesOfTwoDirectoriesApart: a.pdf from dl and
+// a.pdf from scans, moved in one run, are two files to undo, each refused or
+// restored on its own (review M7).
+func TestUndoKeepsSameNamedFilesOfTwoDirectoriesApart(t *testing.T) {
+	e, run, h, logPath := appliedRun(t,
+		map[string]map[string]string{"dl": {"a.pdf": "from dl"}, "scans": {"a.pdf": "from scans"}},
+		map[string]string{
+			"dl":    "(path \"~/dl\")\n(rule \"r\" (move \"~/Archive\"))\n",
+			"scans": "(path \"~/scans\")\n(rule \"r\" (move \"~/Archive\"))\n",
+		})
+	up, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(up.Files) != 2 {
+		t.Fatalf("undo plan has %d files, want 2: %+v", len(up.Files), up.Files)
+	}
+	scansCopy := undoFileNamed(t, up, "scans", "a.pdf").Steps[0].Src
+	if err := os.WriteFile(scansCopy, []byte("from scans, edited since"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	up, err = e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if undoFileNamed(t, up, "scans", "a.pdf").Refused == "" || undoFileNamed(t, up, "dl", "a.pdf").Refused != "" {
+		t.Fatalf("want only the edited scans file refused: %+v", up.Files)
+	}
+	j, err := journal.Open(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer j.Close()
+	if _, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now())); err != nil {
+		t.Fatal(err)
+	}
+	if b, err := os.ReadFile(filepath.Join(h, "dl", "a.pdf")); err != nil || string(b) != "from dl" {
+		t.Errorf("dl/a.pdf after undo: %q, %v", b, err)
+	}
+}
+
+// TestUndoRechecksAtExecution: a file edited after the undo was planned (for
+// example while its review was open) is not moved back (review undo F8).
+func TestUndoRechecksAtExecution(t *testing.T) {
+	e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"})
+	up, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	moved := filepath.Join(h, "dl", "Out", "a.pdf")
+	if err := os.WriteFile(moved, []byte("edited during review"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	j, err := journal.Open(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer j.Close()
+	res, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now()))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if res.Failed != 1 {
+		t.Errorf("Failed = %d, want 1", res.Failed)
+	}
+	if b, _ := os.ReadFile(moved); string(b) != "edited during review" {
+		t.Errorf("the edited file was moved or changed: %q", b)
+	}
+}
+
+// TestUndoLeavesNoDirectoriesBehind: undo passing a file back through a
+// directory it had already removed for another file recreates it; that
+// directory must be gone again when the undo ends (review undo F6).
+func TestUndoLeavesNoDirectoriesBehind(t *testing.T) {
+	e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out/{mtime:%Y}\") (move \"Out\"))\n"})
+	up, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	j, err := journal.Open(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer j.Close()
+	if res, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now())); err != nil || res.Failed != 0 {
+		t.Fatalf("undo: %v, %+v", err, res)
+	}
+	if _, err := os.Stat(filepath.Join(h, "dl", "Out")); !os.IsNotExist(err) {
+		t.Errorf("dl/Out is left behind: %v", err)
+	}
+}