summaryrefslogtreecommitdiff
path: root/internal
diff options
context:
space:
mode:
Diffstat (limited to 'internal')
-rw-r--r--internal/apply/apply.go2
-rw-r--r--internal/apply/apply_test.go42
-rw-r--r--internal/apply/fs.go35
-rw-r--r--internal/config/skel/krino.conf5
-rw-r--r--internal/config/skel/template.conf5
-rw-r--r--internal/dup/dup.go50
-rw-r--r--internal/dup/dup_test.go226
-rw-r--r--internal/engine/apply.go118
-rw-r--r--internal/engine/apply_test.go243
-rw-r--r--internal/engine/plan_bench_test.go108
-rw-r--r--internal/journal/read_test.go214
-rw-r--r--internal/tui/tui.go6
-rw-r--r--internal/tui/tui_test.go8
13 files changed, 1040 insertions, 22 deletions
diff --git a/internal/apply/apply.go b/internal/apply/apply.go
index 2cd52e4..f177a17 100644
--- a/internal/apply/apply.go
+++ b/internal/apply/apply.go
@@ -153,7 +153,7 @@ func runFileStep(step plan.Step) StepResult {
case plan.Move:
err = moveFile(step.Src, dst)
case plan.Rename:
- err = os.Rename(step.Src, dst)
+ err = renameFile(step.Src, dst)
}
if err != nil {
return StepResult{Step: step, Status: "failed", Detail: err.Error(), Made: made, DisplacedEntry: displacedEntry}
diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go
index 64b0176..4bf6c30 100644
--- a/internal/apply/apply_test.go
+++ b/internal/apply/apply_test.go
@@ -250,3 +250,45 @@ func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) {
t.Errorf("Made = %v, want %v (outermost first)", got[0].Made, want)
}
}
+
+// TestMoveFileRefusesOccupiedDestination is item 16 (fix round 2026-09-12,
+// plan 5 Task 2): moveFile must refuse an occupied destination on its own,
+// not merely rely on runFileStep having already checked - the exact
+// arrangement that produced plan 4's Task 5 Critical, where a helper that
+// replaced silently was trusted because some caller had checked. Called
+// directly, bypassing runFileStep's own pre-check entirely.
+func TestMoveFileRefusesOccupiedDestination(t *testing.T) {
+ dir := t.TempDir()
+ src := write(t, filepath.Join(dir, "x.pdf"), "source", 0o644)
+ dst := write(t, filepath.Join(dir, "y.pdf"), "already there", 0o644)
+
+ if err := moveFile(src, dst); err == nil {
+ t.Fatal("moveFile overwrote an existing destination")
+ }
+ if b, err := os.ReadFile(src); err != nil || string(b) != "source" {
+ t.Errorf("moveFile touched its source: %q, %v", b, err)
+ }
+ if b, err := os.ReadFile(dst); err != nil || string(b) != "already there" {
+ t.Errorf("moveFile touched its destination: %q, %v", b, err)
+ }
+}
+
+// TestRenameFileRefusesOccupiedDestination is item 16's other half:
+// runFileStep's bare os.Rename call for the Rename kind was just as
+// unguarded in itself as moveFile was. renameFile is the helper that now
+// carries the same independent guard, called directly here.
+func TestRenameFileRefusesOccupiedDestination(t *testing.T) {
+ dir := t.TempDir()
+ src := write(t, filepath.Join(dir, "x.pdf"), "source", 0o644)
+ dst := write(t, filepath.Join(dir, "y.pdf"), "already there", 0o644)
+
+ if err := renameFile(src, dst); err == nil {
+ t.Fatal("renameFile overwrote an existing destination")
+ }
+ if b, err := os.ReadFile(src); err != nil || string(b) != "source" {
+ t.Errorf("renameFile touched its source: %q, %v", b, err)
+ }
+ if b, err := os.ReadFile(dst); err != nil || string(b) != "already there" {
+ t.Errorf("renameFile touched its destination: %q, %v", b, err)
+ }
+}
diff --git a/internal/apply/fs.go b/internal/apply/fs.go
index 205089f..823d5c8 100644
--- a/internal/apply/fs.go
+++ b/internal/apply/fs.go
@@ -108,6 +108,16 @@ func moveFile(src, dst string) error {
if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil {
return err
}
+ // Item 16 (fix round 2026-09-12, plan 5 Task 2): this guard must hold
+ // independently of runFileStep's own pre-check, layered rather than
+ // moved - the exact arrangement that produced plan 4's Task 5 Critical,
+ // where a helper that replaced silently was trusted because some caller
+ // had checked. Placed immediately before the operation that would
+ // otherwise clobber dst, the same way copyFile's own guard sits right
+ // before its rename into place.
+ if err := refuseIfExists(dst); err != nil {
+ return err
+ }
err := os.Rename(src, dst)
if err == nil {
return nil
@@ -121,6 +131,31 @@ func moveFile(src, dst string) error {
return os.Remove(src)
}
+// renameFile renames src to dst, refusing on its own when dst already
+// exists rather than trusting that a caller checked first (item 16, same
+// reasoning as moveFile's guard above): a bare os.Rename silently replaces
+// an occupied destination, and runFileStep's own pre-check must not be the
+// only thing standing between a rename step and that.
+func renameFile(src, dst string) error {
+ if err := refuseIfExists(dst); err != nil {
+ return err
+ }
+ return os.Rename(src, dst)
+}
+
+// refuseIfExists reports an error naming dst if something is already there
+// (os.Lstat succeeds, following no symlink), and propagates any other stat
+// failure. A nil return means dst was confirmed absent at the moment of the
+// check.
+func refuseIfExists(dst string) error {
+ if _, err := os.Lstat(dst); err == nil {
+ return fmt.Errorf("destination already exists: %s", dst)
+ } else if !os.IsNotExist(err) {
+ return err
+ }
+ return nil
+}
+
// maxSuffixAttempts bounds nextFreeName. internal/plan/conflict.go and
// internal/trash/trash.go each have their own cap of the same size, for the
// same reason given below: nextFreeName solves yet another, independent
diff --git a/internal/config/skel/krino.conf b/internal/config/skel/krino.conf
index 48780ea..dbd883b 100644
--- a/internal/config/skel/krino.conf
+++ b/internal/config/skel/krino.conf
@@ -1,8 +1,9 @@
;; -*- mode: lisp -*-
;; vim: set ft=lisp :
;;
-;; krino's main configuration. The syntax is explained in
-;; docs/sexp-primer.md; every form is described in krino.conf(5).
+;; krino's main configuration. The syntax is explained in sexp-primer.md,
+;; installed as share/doc/krino/sexp-primer.md under the install prefix,
+;; which is ~/.local by default; every form is described in krino.conf(5).
;; The directories to sort, in this order. Each NAME has its rules in
;; dirs/NAME.conf. Add one with: krino new NAME PATH
diff --git a/internal/config/skel/template.conf b/internal/config/skel/template.conf
index 867b681..c3af09d 100644
--- a/internal/config/skel/template.conf
+++ b/internal/config/skel/template.conf
@@ -1,8 +1,9 @@
;; -*- mode: lisp -*-
;; vim: set ft=lisp :
;;
-;; krino rules for one directory. The syntax is explained in
-;; docs/sexp-primer.md; every form is described in krino.conf(5).
+;; krino rules for one directory. The syntax is explained in sexp-primer.md,
+;; installed as share/doc/krino/sexp-primer.md under the install prefix,
+;; which is ~/.local by default; every form is described in krino.conf(5).
(path "@PATH@")
diff --git a/internal/dup/dup.go b/internal/dup/dup.go
index e3c7424..32bcfb0 100644
--- a/internal/dup/dup.go
+++ b/internal/dup/dup.go
@@ -174,18 +174,47 @@ func (x *Index) Lookup(path string) (original string, dup bool, err error) {
return path, false, nil
}
orig := x.candidates[x.original(identical)].path
- return orig, orig != path, nil
+ if orig == path {
+ return orig, false, nil
+ }
+ // Spec §5.5: two names for one file are never duplicates of each other.
+ // The other name may be a hardlink, or path itself indexed a second time
+ // under a DIR that overlaps the scanned tree. identicalTo gives every
+ // member of the content class the same set, so every lookup elects the
+ // same original, and no name for that original's file is reported as a
+ // duplicate: its content always keeps at least one name. Portable:
+ // os.SameFile, never a Stat_t.Dev/Ino read (that field's type differs
+ // across freebsd/openbsd, which `make ci` vets).
+ origInfo, err := os.Lstat(orig)
+ if err != nil {
+ return "", false, err
+ }
+ pathInfo, err := os.Lstat(path)
+ if err != nil {
+ return "", false, err
+ }
+ if os.SameFile(origInfo, pathInfo) {
+ return orig, false, nil
+ }
+ return orig, true, nil
}
// identicalTo returns the indexes in group (which all share idx's size,
// idx included) whose content matches candidates[idx]: same partial hash,
-// then, only for those that collide, the same full hash. idx is the file
-// Lookup was asked about; a failure hashing it propagates, since Lookup can
-// answer nothing without it. A failure hashing any other candidate in group
-// only removes that candidate from consideration: a vanished candidate
-// (errors.Is fs.ErrNotExist) is dropped silently, any other failure is
-// recorded on the Index (see recordCandidateError) so the caller can warn
-// about it once matching is done.
+// then, only for those that collide, the same full hash. Every candidate
+// with identical bytes is included, whatever its path or inode: a hardlink
+// of idx, and idx's own path indexed a second time under an overlapping
+// extra directory, are both members. That keeps the set the same whichever
+// member Lookup was asked about, so every member elects the same original;
+// Lookup, not this function, decides that a name for the elected original's
+// own file is not a duplicate of it.
+//
+// idx is the file Lookup was asked about; a failure hashing it propagates,
+// since Lookup can answer nothing without it. A failure hashing any other
+// candidate in group only removes that candidate from consideration: a
+// vanished candidate (errors.Is fs.ErrNotExist) is dropped silently, any
+// other failure is recorded on the Index (see recordCandidateError) so the
+// caller can warn about it once matching is done.
func (x *Index) identicalTo(idx int, group []int) ([]int, error) {
idxPartial, err := x.partialHash(x.candidates[idx].path)
if err != nil {
@@ -219,9 +248,10 @@ func (x *Index) identicalTo(idx int, group []int) ([]int, error) {
x.recordCandidateError(x.candidates[j].path, err)
continue
}
- if jFull == idxFull {
- same = append(same, j)
+ if jFull != idxFull {
+ continue
}
+ same = append(same, j)
}
return same, nil
}
diff --git a/internal/dup/dup_test.go b/internal/dup/dup_test.go
index 5d2621c..ed7c1a3 100644
--- a/internal/dup/dup_test.go
+++ b/internal/dup/dup_test.go
@@ -352,3 +352,229 @@ func TestExtraDirSymlinkNotFollowed(t *testing.T) {
t.Errorf("symlinked extra dir was indexed despite the error: dup=%v orig=%s", dup, orig)
}
}
+
+// linked builds a scan.File for a hardlink of an already-put file: same
+// inode, so same content and metadata by construction, under a new name.
+func linked(t *testing.T, dir, name string, target scan.File) scan.File {
+ t.Helper()
+ p := filepath.Join(dir, name)
+ if err := os.Link(target.Path, p); err != nil {
+ t.Fatal(err)
+ }
+ fi, err := os.Stat(p)
+ if err != nil {
+ t.Fatal(err)
+ }
+ return scan.File{Path: p, Rel: name, Name: name, Size: fi.Size(), ModTime: fi.ModTime()}
+}
+
+// TestHardlinksAreNotDuplicatesOfEachOther is R10 (plan 5 Task 2, added to
+// the task outside the brief): spec §5.5 groups duplicate candidates by size
+// and hash, with no inode check, so two hardlinked names - one inode, byte-
+// identical by construction - were judged a duplicate pair. A rule of
+// (when (duplicate)) (delete) would then remove a name the user relies on
+// even though nothing was ever actually copied. os.SameFile must stop a
+// file being judged a duplicate of itself under another name.
+func TestHardlinksAreNotDuplicatesOfEachOther(t *testing.T) {
+ d := t.TempDir()
+ a := put(t, d, "a.pdf", []byte("same content"), 0)
+ b := linked(t, d, "b.pdf", a)
+
+ x, errs := NewIndex([]scan.File{a, b}, nil)
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ if orig, dup := lookup(t, x, a); dup {
+ t.Errorf("a.pdf reported as a duplicate of its own hardlink b.pdf (orig=%s)", orig)
+ }
+ if orig, dup := lookup(t, x, b); dup {
+ t.Errorf("b.pdf reported as a duplicate of its own hardlink a.pdf (orig=%s)", orig)
+ }
+}
+
+// TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile is R10's other
+// direction, and the one a naive "same size+hash means never a duplicate"
+// fix would get wrong: a.pdf and b.pdf are hardlinks of one inode, but
+// c.pdf is a genuinely separate, byte-identical copy under an extra
+// (duplicate "DIR") directory, so spec §5.5 prefers it as the original.
+// Deleting a.pdf and b.pdf then leaves the content intact in c.pdf - they
+// really are duplicates, of c.pdf, and must still be reported as such.
+func TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile(t *testing.T) {
+ scanned, filed := t.TempDir(), t.TempDir()
+ a := put(t, scanned, "a.pdf", []byte("same content"), 0)
+ b := linked(t, scanned, "b.pdf", a)
+ put(t, filed, "c.pdf", []byte("same content"), 0) // extra-dir copy: preferred as the original regardless of mtime
+
+ x, errs := NewIndex([]scan.File{a, b}, []string{filed})
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ cPath := filepath.Join(filed, "c.pdf")
+ if orig, dup := lookup(t, x, a); !dup || orig != cPath {
+ t.Errorf("a.pdf: dup=%v orig=%s, want a real duplicate of %s", dup, orig, cPath)
+ }
+ if orig, dup := lookup(t, x, b); !dup || orig != cPath {
+ t.Errorf("b.pdf: dup=%v orig=%s, want a real duplicate of %s", dup, orig, cPath)
+ }
+}
+
+// TestHardlinkUnderExtraDirWithNoOtherCopyIsNotADuplicate is R10 extended
+// to extra-directory candidates: a candidate is never a duplicate of a
+// candidate that is the same file, and extra-directory candidates are
+// candidates - the rule is not scanned-vs-scanned only. This is the shape
+// (duplicate "DIR") exists for: (duplicate "~/backup") means "the backup
+// already holds a copy, so the local name can go", but if ~/backup/a.pdf is
+// a hardlink of ~/dl/a.pdf, the backup holds no copy at all, just the same
+// file under a second name - judging the scanned file a duplicate would
+// delete the only copy while the user believes it is backed up.
+//
+// Lookup's os.SameFile check compares the elected original with the file
+// asked about whether the original was scanned or found under an extra
+// directory, so this case needs no special handling; the test pins it as
+// its own named case.
+func TestHardlinkUnderExtraDirWithNoOtherCopyIsNotADuplicate(t *testing.T) {
+ scanned, backup := t.TempDir(), t.TempDir()
+ a := put(t, scanned, "a.pdf", []byte("same content"), 0)
+ backupPath := filepath.Join(backup, "a.pdf")
+ if err := os.Link(a.Path, backupPath); err != nil {
+ t.Fatal(err)
+ }
+
+ x, errs := NewIndex([]scan.File{a}, []string{backup})
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ if orig, dup := lookup(t, x, a); dup {
+ t.Errorf("a.pdf reported as a duplicate of its own hardlink under the extra dir (orig=%s)", orig)
+ }
+}
+
+// TestThreeWayHardlinksAreNotDuplicatesOfEachOther is the direct
+// generalisation of TestHardlinksAreNotDuplicatesOfEachOther to N names for
+// one inode: three names, one inode, no other copy anywhere - none of them
+// is a duplicate of either of the others. No special-casing for N > 2: all
+// three elect the same original, and Lookup's SameFile check finds each
+// name to be that original's own file.
+func TestThreeWayHardlinksAreNotDuplicatesOfEachOther(t *testing.T) {
+ d := t.TempDir()
+ a := put(t, d, "a.pdf", []byte("same content"), 0)
+ b := linked(t, d, "b.pdf", a)
+ c := linked(t, d, "c.pdf", a)
+
+ x, errs := NewIndex([]scan.File{a, b, c}, nil)
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ for _, f := range []scan.File{a, b, c} {
+ if orig, dup := lookup(t, x, f); dup {
+ t.Errorf("%s reported as a duplicate (orig=%s)", f.Name, orig)
+ }
+ }
+}
+
+// dupVerdicts looks up every file in fs and returns the paths reported as
+// not a duplicate, and the originals the duplicates were reported against.
+func dupVerdicts(t *testing.T, x *Index, fs ...scan.File) (kept []string, origs map[string]string) {
+ t.Helper()
+ origs = make(map[string]string)
+ for _, f := range fs {
+ orig, dup := lookup(t, x, f)
+ if dup {
+ origs[f.Path] = orig
+ } else {
+ kept = append(kept, f.Path)
+ }
+ }
+ return kept, origs
+}
+
+// TestExtraDirIsTheScannedRoot: (duplicate "DIR") where DIR is the scanned
+// root itself, so every scanned file is also indexed as an extra-directory
+// candidate under the same path. Two identical files are one duplicate and
+// one original, never two duplicates of each other: a (delete) rule must
+// leave one copy.
+func TestExtraDirIsTheScannedRoot(t *testing.T) {
+ d := t.TempDir()
+ a := put(t, d, "a.pdf", []byte("same content"), 0) // older: the original
+ b := put(t, d, "b.pdf", []byte("same content"), 5)
+
+ x, errs := NewIndex([]scan.File{a, b}, []string{d})
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ kept, origs := dupVerdicts(t, x, a, b)
+ if len(kept) != 1 || kept[0] != a.Path {
+ t.Errorf("kept = %v, want exactly %s (duplicates: %v)", kept, a.Path, origs)
+ }
+ if origs[b.Path] != a.Path {
+ t.Errorf("b.pdf: original = %q, want %s", origs[b.Path], a.Path)
+ }
+}
+
+// TestExtraDirInsideRecursiveRoot: DIR is a subdirectory of a recursively
+// scanned root, the "the archive already holds a copy, so the loose one can
+// go" shape. The copy under DIR is the original even though it is newer;
+// only the copy outside DIR is a duplicate.
+func TestExtraDirInsideRecursiveRoot(t *testing.T) {
+ d := t.TempDir()
+ archived := put(t, d, "Archive/x.pdf", []byte("same content"), 5)
+ loose := put(t, d, "x-copy.pdf", []byte("same content"), 0) // older, but not under DIR
+
+ x, errs := NewIndex([]scan.File{archived, loose}, []string{filepath.Join(d, "Archive")})
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ kept, origs := dupVerdicts(t, x, archived, loose)
+ if len(kept) != 1 || kept[0] != archived.Path {
+ t.Errorf("kept = %v, want exactly %s (duplicates: %v)", kept, archived.Path, origs)
+ }
+ if origs[loose.Path] != archived.Path {
+ t.Errorf("x-copy.pdf: original = %q, want %s", origs[loose.Path], archived.Path)
+ }
+}
+
+// TestExtraDirIsAnAncestorOfTheRoot: DIR contains the scanned root, as
+// (duplicate "~") would for a root under the home directory.
+func TestExtraDirIsAnAncestorOfTheRoot(t *testing.T) {
+ parent := t.TempDir()
+ root := filepath.Join(parent, "dl")
+ a := put(t, root, "a.pdf", []byte("same content"), 0) // older: the original
+ b := put(t, root, "b.pdf", []byte("same content"), 5)
+
+ x, errs := NewIndex([]scan.File{a, b}, []string{parent})
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ kept, origs := dupVerdicts(t, x, a, b)
+ if len(kept) != 1 || kept[0] != a.Path {
+ t.Errorf("kept = %v, want exactly %s (duplicates: %v)", kept, a.Path, origs)
+ }
+ if origs[b.Path] != a.Path {
+ t.Errorf("b.pdf: original = %q, want %s", origs[b.Path], a.Path)
+ }
+}
+
+// TestThreeCopiesWithOverlappingExtraDirKeepOne: two copies under DIR and a
+// third outside it, all scanned. Exactly one of the three is not a
+// duplicate, it lies under DIR, and every duplicate names that same file as
+// its original.
+func TestThreeCopiesWithOverlappingExtraDirKeepOne(t *testing.T) {
+ d := t.TempDir()
+ x1 := put(t, d, "Archive/x1.pdf", []byte("same content"), 3)
+ x2 := put(t, d, "Archive/x2.pdf", []byte("same content"), 4)
+ loose := put(t, d, "x-copy.pdf", []byte("same content"), 0)
+
+ x, errs := NewIndex([]scan.File{x1, x2, loose}, []string{filepath.Join(d, "Archive")})
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ kept, origs := dupVerdicts(t, x, x1, x2, loose)
+ if len(kept) != 1 || kept[0] != x1.Path {
+ t.Fatalf("kept = %v, want exactly %s (duplicates: %v)", kept, x1.Path, origs)
+ }
+ for p, orig := range origs {
+ if orig != x1.Path {
+ t.Errorf("%s: original = %q, want %s", p, orig, x1.Path)
+ }
+ }
+}
diff --git a/internal/engine/apply.go b/internal/engine/apply.go
index c74414d..878cb18 100644
--- a/internal/engine/apply.go
+++ b/internal/engine/apply.go
@@ -9,6 +9,7 @@ import (
"io"
"os"
"path/filepath"
+ "sort"
"strings"
"syscall"
"time"
@@ -660,6 +661,50 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string {
// declined - a single pass, not two, so the two kinds of file interleave in
// the log exactly as the run touched them, the same as Apply's own
// approved-and-declined chains do.
+//
+// Task 1 (plan 5): after every file's reversal has been attempted, a second,
+// run-wide pass retries the directory removals that were refused as
+// non-empty. planUndoFile puts the undo-mkdir step for a shared destination
+// on whichever file's chain first created it (spec §9: only the step that
+// actually created a directory logs a "mkdir" entry, so only that file's
+// reversal carries the matching undo-mkdir); when that file reverses first,
+// its siblings are usually still inside, the removal is correctly refused as
+// non-empty (spec §10), and - without this pass - nothing ever retries it,
+// leaving empty directories behind even though every file came back. This
+// mirrors planUndoFile's own undoProjection insight (see its comment) one
+// level up: a removal judged too early is judging the wrong world, whether
+// that "too early" is mid-file (what the projection fixes) or mid-run (what
+// this retry fixes).
+//
+// The retry is a run-level tidy-up, never a re-run of a step: it does not
+// touch what the first undo-mkdir attempt already logged (that entry, ok or
+// failed, stands exactly as it was written), and a directory the retry does
+// manage to remove gets an ADDITIONAL journal entry - never a rewrite - so
+// the log never disagrees with reality (my ruling on the point the brief
+// left open: spec §9 logs every step, and a directory removed while the log
+// still says its removal was refused would be a false record). Because
+// journal.ranAnyUndoStep already excludes "undo-mkdir" from what marks a run
+// "(undone)", this extra "ok" entry cannot change that marking either -
+// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins
+// it rather than assuming it. A retried removal is likewise never folded
+// into ApplyResult: it is
+// collected from candidates whose first attempt already went through
+// tallyFile once (via isFileAffecting's exemption), and counting it again
+// here would double-count a directory that failed once and then quietly
+// tidied itself away.
+//
+// Candidates are collected only from directories this run's own reversal
+// created - by construction, since every candidate comes from an undo-mkdir
+// step, and an undo-mkdir step exists only for a directory the forward run's
+// Made recorded - never a directory the retry merely happens to find empty.
+// They are retried deepest path first (retryDirRemovals), so a nested
+// directory - e.g. Work/Sub under Work - is removed before its
+// now-possibly-empty parent, the same outermost-created/innermost-removed
+// discipline logStep and undoFile already keep within one file's own chain,
+// applied here across files. A directory still non-empty at retry time
+// genuinely holds something else (or the retry runs before every sibling
+// happens to have reversed, on a later undo of a different run) and simply
+// stays, with its original refusal the only record of it.
func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, run string) (*ApplyResult, error) {
result := &ApplyResult{}
@@ -695,6 +740,7 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer,
return result, fmt.Errorf("engine: apply undo: %w", err)
}
+ var retries []dirRetry
for _, f := range actionable {
if err := ctx.Err(); err != nil {
return result, err
@@ -714,6 +760,15 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer,
}
result.Files = append(result.Files, fr)
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})
+ }
+ }
+ }
+
+ if err := e.retryDirRemovals(j, run, retries); err != nil {
+ return result, fmt.Errorf("engine: apply undo: %w", err)
}
if err := j.Append(journal.Entry{Time: e.Now(), Run: run, Action: "run-end", Status: "ok"}); err != nil {
@@ -722,6 +777,69 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer,
return result, nil
}
+// dirRetry names one directory whose undo-mkdir was refused (as non-empty)
+// during ApplyUndo's main pass, kept for the run-wide retry once every
+// file's reversal has been attempted. file and dirName are the file and
+// config directory name that owned the original undo-mkdir step, carried
+// forward so retryDirRemovals's journal entry - if the retry succeeds -
+// names the same file and directory the original refusal did, not an
+// arbitrary one; step is that same step's 1-based index, so the two entries
+// (the original "failed" and, if the retry succeeds, this "ok") read
+// together under the same File/Step in the log.
+type dirRetry struct {
+ dir string
+ dirName string
+ file string
+ step int
+}
+
+// retryDirRemovals is ApplyUndo's run-wide second pass (Task 1, plan 5): once
+// every file's reversal has run, some directories an undo-mkdir step could
+// not remove earlier may now be empty, because a sibling file that shared
+// the directory has since reversed too. candidates is sorted deepest path
+// first (by descending path-segment count) so a nested directory is removed
+// before its parent, exactly the order a real cleanup needs; a directory
+// still non-empty at its turn genuinely holds something else and is left
+// exactly as its first attempt recorded it - no second entry, no error.
+//
+// This never rewrites or removes the original undo-mkdir entry (ok or
+// failed, whichever the first attempt logged): a directory the retry does
+// manage to remove gets one ADDITIONAL entry instead (my ruling on the point
+// the brief left open - see ApplyUndo's comment), so the log always agrees
+// with what is actually on disk. The new entry's own Action is still
+// "undo-mkdir", so journal.ranAnyUndoStep - which excludes that action on
+// principle, not by accident (see its own comment) - continues to treat this
+// exactly like any other undo-mkdir for the purpose of marking a run
+// "(undone)": tidying up an empty directory, on the first attempt or the
+// retry, is still not a restoration.
+func (e *Engine) retryDirRemovals(j *journal.Writer, run string, candidates []dirRetry) error {
+ sort.SliceStable(candidates, func(i, j int) bool {
+ return pathDepth(candidates[i].dir) > pathDepth(candidates[j].dir)
+ })
+ for _, c := range candidates {
+ if err := os.Remove(c.dir); err != nil {
+ // Still not empty (or gone, or otherwise unremovable): the
+ // original refusal already recorded this, and it stands.
+ 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,
+ }); err != nil {
+ return err
+ }
+ }
+ return nil
+}
+
+// pathDepth counts path's separators after cleaning it, so retryDirRemovals
+// can sort deepest first: a nested directory (more separators) is always
+// removed before the parent it sits under, whatever the two paths' common
+// root.
+func pathDepth(path string) int {
+ return strings.Count(filepath.Clean(path), string(filepath.Separator))
+}
+
// declineUndoFile logs f's reversal as declined without carrying out any of
// it - spec §9's "declined files are logged even though nothing happens to
// them", extended to undo (fix round 2026-09-12, item 2 of Task 8's review):
diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go
index 42724a0..a54bffc 100644
--- a/internal/engine/apply_test.go
+++ b/internal/engine/apply_test.go
@@ -1180,3 +1180,246 @@ func TestTallyFileCountsMixedOutcomesOnceEach(t *testing.T) {
t.Errorf("result = %+v, want one of each", result)
}
}
+
+// --- Plan 5, Task 1 ---
+
+// sharedDestUndoFixture builds a downloads directory with three pdf files
+// (a.pdf, b.pdf, c.pdf) and a single rule moving all of them into dest,
+// applies the move, and returns the sandbox home, the loaded engine, the
+// journal's path and the forward run's ID.
+//
+// All three files landing on one destination that this one run creates is
+// the shape that exercises the run-wide directory retry (Task 1, plan 5):
+// whichever file's chain first creates dest carries its undo-mkdir step(s),
+// and that file's own reversal typically runs while its siblings still
+// occupy dest - refusing the removal correctly, at first. dest may name a
+// nested path ("Work/Sub"): apply.mkdirAllTracked then records every
+// directory the move had to create, outermost first, and every one of them
+// still lands on that same first file's chain.
+//
+// Extracted per fix round 1 (Important 2): TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses
+// and TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone
+// used to duplicate this setup verbatim; TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst
+// needed the identical shape with only dest varying, which is what named the
+// parameter rather than hard-coding "Filed" here.
+func sharedDestUndoFixture(t *testing.T, dest string) (h string, e *Engine, logPath string, run string) {
+ t.Helper()
+ h = sandbox(t)
+ dl := filepath.Join(h, "dl")
+ if err := os.MkdirAll(dl, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ // Three files that all move into ONE created destination. The bug this
+ // task fixes is that only the file whose chain first creates it ever
+ // carries the undo-mkdir step, and that step is attempted while its
+ // siblings are still inside.
+ for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} {
+ if err := os.WriteFile(filepath.Join(dl, n), []byte(n), 0o640); err != nil {
+ t.Fatal(err)
+ }
+ }
+ main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": `
+(path "~/dl")
+(min-age 0s)
+(rule "pdfs" (when (type pdf)) (move "` + dest + `"))
+`})
+ loaded, errs := Load(main)
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ e = loaded
+ dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims())
+ if err != nil {
+ t.Fatal(err)
+ }
+ approved := map[string]bool{}
+ for _, c := range dp.Chains {
+ approved[c.File.Rel] = true
+ }
+ logPath = filepath.Join(h, ".local", "state", "krino", "krino.log")
+ j, err := journal.Open(logPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ run = journal.NewRunID(time.Now())
+ if _, err := e.Apply(context.Background(), dp, approved, j, run); err != nil {
+ t.Fatal(err)
+ }
+ j.Close()
+ return h, e, logPath, run
+}
+
+func TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses(t *testing.T) {
+ h, e, logPath, run := sharedDestUndoFixture(t, "Filed")
+ dl := filepath.Join(h, "dl")
+ filed := filepath.Join(dl, "Filed")
+ if _, err := os.Stat(filed); err != nil {
+ t.Fatalf("apply did not create the directory: %v", err)
+ }
+
+ up, err := e.PlanUndo(run)
+ if err != nil {
+ t.Fatal(err)
+ }
+ j2, err := journal.Open(logPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now()))
+ if err != nil {
+ t.Fatal(err)
+ }
+ j2.Close()
+ if res.Failed != 0 {
+ t.Fatalf("undo reported %d failures: %+v", res.Failed, res)
+ }
+ if _, err := os.Stat(filed); !os.IsNotExist(err) {
+ t.Errorf("undo left the created directory behind: %v", err)
+ }
+ for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} {
+ if _, err := os.Stat(filepath.Join(dl, n)); err != nil {
+ t.Errorf("%s did not come back: %v", n, err)
+ }
+ }
+}
+
+// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst pins fix round 1's
+// Important 1: retryDirRemovals must retry deepest path first. All three
+// files move into Work/Sub, so Apply's single mkdirAllTracked call creates
+// both Work and Work/Sub on the FIRST file's own chain (outermost first),
+// which means that one file's reversal carries two undo-mkdir steps, one for
+// each directory - and both are refused on that file's own turn, since the
+// other two files still sit in Work/Sub at that point.
+//
+// Retried deepest first, Work/Sub empties out and is removed, and Work -
+// now itself empty - is removed right after. Retried shallowest first
+// instead, Work is tried while Work/Sub (now empty, but not yet removed)
+// still sits inside it, so Work is refused as non-empty and never retried
+// again in this run; Work/Sub is then removed, leaving the outer Work
+// directory behind. So end state alone - no directory left over - already
+// distinguishes correct (deepest-first) ordering from inverted or dropped
+// ordering; unlike the flat-destination tests above, where only one
+// directory ever entered `retries`, this is the case built to tell the two
+// apart.
+func TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst(t *testing.T) {
+ h, e, logPath, run := sharedDestUndoFixture(t, "Work/Sub")
+ dl := filepath.Join(h, "dl")
+ work := filepath.Join(dl, "Work")
+ sub := filepath.Join(work, "Sub")
+ if _, err := os.Stat(sub); err != nil {
+ t.Fatalf("apply did not create the nested directory: %v", err)
+ }
+
+ up, err := e.PlanUndo(run)
+ if err != nil {
+ t.Fatal(err)
+ }
+ j2, err := journal.Open(logPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now()))
+ if err != nil {
+ t.Fatal(err)
+ }
+ j2.Close()
+ if res.Failed != 0 {
+ t.Fatalf("undo reported %d failures: %+v", res.Failed, res)
+ }
+ if _, err := os.Stat(sub); !os.IsNotExist(err) {
+ t.Errorf("undo left the nested directory behind: %v", err)
+ }
+ if _, err := os.Stat(work); !os.IsNotExist(err) {
+ t.Errorf("undo left the outer directory behind - retryDirRemovals is not retrying deepest path first: %v", err)
+ }
+ for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} {
+ if _, err := os.Stat(filepath.Join(dl, n)); err != nil {
+ t.Errorf("%s did not come back: %v", n, err)
+ }
+ }
+}
+
+// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins
+// the journal half of the ruling on the point Task 1's brief left open: when
+// retryDirRemovals succeeds in removing a directory, it appends an
+// ADDITIONAL journal entry for it - the original "failed" undo-mkdir entry,
+// recorded on whichever file's chain first created the directory, is never
+// rewritten or removed - and, because that entry's Action is still
+// "undo-mkdir" like the first, journal.ranAnyUndoStep continues to exclude
+// it from what marks a run "(undone)" (the brief's constraint: it "cannot
+// change whether a run shows as (undone); confirm that rather than assume
+// it").
+func TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone(t *testing.T) {
+ _, e, logPath, run := sharedDestUndoFixture(t, "Filed")
+
+ up, err := e.PlanUndo(run)
+ if err != nil {
+ t.Fatal(err)
+ }
+ j2, err := journal.Open(logPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ undoRun := journal.NewRunID(time.Now())
+ res, err := e.ApplyUndo(context.Background(), up, j2, undoRun)
+ if err != nil {
+ t.Fatal(err)
+ }
+ j2.Close()
+ if res.Failed != 0 {
+ t.Fatalf("undo reported %d failures: %+v", res.Failed, res)
+ }
+
+ entries, err := journal.Entries(logPath, undoRun)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var failed, ok *journal.Entry
+ var mkdirCount int
+ for i := range entries {
+ en := &entries[i]
+ if en.Action != "undo-mkdir" {
+ continue
+ }
+ mkdirCount++
+ switch en.Status {
+ case "failed":
+ failed = en
+ case "ok":
+ ok = en
+ }
+ }
+ if mkdirCount != 2 {
+ t.Fatalf("undo-mkdir entries = %d, want exactly 2 (the original refusal plus the retry's addition): %+v", mkdirCount, entries)
+ }
+ if failed == nil {
+ t.Fatal("the original refused undo-mkdir entry is missing - it must never be rewritten or removed")
+ }
+ if ok == nil {
+ t.Fatal("no successful undo-mkdir entry was appended for the retry")
+ }
+ if failed.Src != ok.Src {
+ t.Errorf("failed.Src = %q, ok.Src = %q; want the same directory", failed.Src, ok.Src)
+ }
+ if failed.File != ok.File {
+ t.Errorf("failed.File = %q, ok.File = %q; want the retry entry to carry the file that owned the original undo-mkdir", failed.File, ok.File)
+ }
+ if failed.Dir != ok.Dir {
+ t.Errorf("failed.Dir = %q, ok.Dir = %q; want the same directory name", failed.Dir, ok.Dir)
+ }
+
+ runs, err := e.Runs(0)
+ if err != nil {
+ t.Fatal(err)
+ }
+ byID := map[string]bool{}
+ for _, r := range runs {
+ byID[r.ID] = r.Undone
+ }
+ if !byID[run] {
+ t.Errorf("original run %q not marked Undone, though every file came back", run)
+ }
+ if byID[undoRun] {
+ t.Errorf("the undo run %q itself must never read as Undone", undoRun)
+ }
+}
diff --git a/internal/engine/plan_bench_test.go b/internal/engine/plan_bench_test.go
new file mode 100644
index 0000000..a3d08bb
--- /dev/null
+++ b/internal/engine/plan_bench_test.go
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package engine
+
+import (
+ "context"
+ "fmt"
+ "os"
+ "path/filepath"
+ "testing"
+ "time"
+
+ "krino/internal/plan"
+)
+
+// BenchmarkPlan measures krino's own cost of planning: walking a tree,
+// matching rules and building action chains (Engine.Plan, which wraps
+// Engine.Match and plan.Build). It does NOT measure krino's real-world
+// throughput - spec §13 is explicit that there is no performance target
+// for 0.0.1, because a full run's wall time is dominated by the external
+// extractors (pdftotext and friends), not by krino itself. This benchmark
+// therefore uses a config with no (content ...) test, so no extractor
+// ever runs and the result is identical on any machine, with or without
+// poppler installed.
+//
+// The tree is built once in b.TempDir(), before the timer starts; each
+// iteration re-plans the same on-disk tree with a fresh plan.Claims, so
+// iterations are independent and repeatable.
+func BenchmarkPlan(b *testing.B) {
+ root := b.TempDir()
+ scanRoot := filepath.Join(root, "Filed")
+ buildBenchTree(b, scanRoot)
+
+ mainFile := filepath.Join(root, "krino.conf")
+ if err := os.WriteFile(mainFile, []byte(`(include "dl")`), 0o644); err != nil {
+ b.Fatal(err)
+ }
+ dirsDir := filepath.Join(root, "dirs")
+ if err := os.MkdirAll(dirsDir, 0o755); err != nil {
+ b.Fatal(err)
+ }
+ // Type-only rules (no content test), one per group present in the
+ // generated tree, mirroring examples/by-type.conf; "dat" files match
+ // none of them and take the unmatched path through Match.
+ dirConf := fmt.Sprintf(`
+(path %q)
+(recursive yes)
+(min-age 0s)
+(rule "images" (when (type image)) (move "Sorted/Images") (stop))
+(rule "documents" (when (type document)) (move "Sorted/Documents") (stop))
+(rule "spreadsheets" (when (type spreadsheet)) (move "Sorted/Spreadsheets") (stop))
+(rule "archives" (when (type archive)) (move "Sorted/Archives") (stop))
+(rule "media" (when (or (type audio) (type video))) (move "Sorted/Media") (stop))
+`, scanRoot)
+ if err := os.WriteFile(filepath.Join(dirsDir, "dl.conf"), []byte(dirConf), 0o644); err != nil {
+ b.Fatal(err)
+ }
+
+ e, errs := Load(mainFile, "dl")
+ if len(errs) > 0 {
+ b.Fatalf("config errors: %v", errs)
+ }
+
+ ctx := context.Background()
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ if _, err := e.Plan(ctx, e.Dirs[0], plan.NewClaims()); err != nil {
+ b.Fatal(err)
+ }
+ }
+}
+
+// benchTreeDirs * benchFilesPerDir files are generated, spread over
+// nested directories so the walk itself is exercised, not just a single
+// flat directory read.
+const (
+ benchTreeDirs = 12
+ benchFilesPerDir = 150
+)
+
+// buildBenchTree creates a synthetic tree under root for BenchmarkPlan:
+// nested "Sub" directories holding files that cycle through extensions
+// spanning several of Appendix A's type groups, plus one extension
+// ("dat") that matches no rule. Names are neutral (Sub, a<N>.<ext>) -
+// never anything from a real folder, per the leak-check patterns.
+func buildBenchTree(b *testing.B, root string) {
+ b.Helper()
+ exts := []string{"pdf", "jpg", "xlsx", "zip", "mp3", "dat"}
+ old := time.Now().Add(-time.Hour)
+ n := 0
+ for d := 0; d < benchTreeDirs; d++ {
+ dir := filepath.Join(root, fmt.Sprintf("Sub%d", d), "Nested")
+ if err := os.MkdirAll(dir, 0o755); err != nil {
+ b.Fatal(err)
+ }
+ for f := 0; f < benchFilesPerDir; f++ {
+ ext := exts[n%len(exts)]
+ p := filepath.Join(dir, fmt.Sprintf("a%04d.%s", n, ext))
+ if err := os.WriteFile(p, []byte("x"), 0o644); err != nil {
+ b.Fatal(err)
+ }
+ if err := os.Chtimes(p, old, old); err != nil {
+ b.Fatal(err)
+ }
+ n++
+ }
+ }
+}
diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go
index 3ffa14d..2f0f40f 100644
--- a/internal/journal/read_test.go
+++ b/internal/journal/read_test.go
@@ -196,6 +196,220 @@ func TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir(t *testing.T) {
}
}
+// TestEntriesCrashedRunReturnsNilError is item 1: a run-start present,
+// run-end absent, and otherwise clean is exactly the crashed-run shape
+// Entries' own doc comment says it must accept - "to end of file when there
+// is no run-end (a crashed run, which is precisely when corruption is
+// likely)". Pinning it as its own test, rather than leaving it implicit in
+// tests about something else, is the point of the item.
+func TestEntriesCrashedRunReturnsNilError(t *testing.T) {
+ path := filepath.Join(t.TempDir(), "krino.log")
+ w, _ := Open(path)
+ at := time.Date(2026, 9, 12, 8, 0, 0, 0, time.UTC)
+ if err := w.Append(Entry{Time: at, Run: "A", Action: "run-start", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1,
+ Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}); err != nil {
+ t.Fatal(err)
+ }
+ // No run-end: the process crashed right here.
+ if err := w.Close(); err != nil {
+ t.Fatal(err)
+ }
+
+ got, err := Entries(path, "A")
+ if err != nil {
+ t.Fatalf("a crashed but otherwise clean run returned an error: %v", err)
+ }
+ if len(got) != 2 || got[0].Action != "run-start" || got[1].Action != "move" {
+ t.Errorf("entries = %+v, want [run-start, move]", got)
+ }
+}
+
+// TestEntriesIntactRunReturnsNilError is item 2: a complete, clean run -
+// run-start, a step, run-end, nothing corrupt - must read back with a nil
+// error. Every other test in this file needs this to be true along the way,
+// but none of them state it as their own point; this one does.
+func TestEntriesIntactRunReturnsNilError(t *testing.T) {
+ path := filepath.Join(t.TempDir(), "krino.log")
+ w, _ := Open(path)
+ at := time.Date(2026, 9, 12, 8, 0, 0, 0, time.UTC)
+ if err := w.Append(Entry{Time: at, Run: "A", Action: "run-start", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1,
+ Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "A", Action: "run-end", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Close(); err != nil {
+ t.Fatal(err)
+ }
+
+ got, err := Entries(path, "A")
+ if err != nil {
+ t.Fatalf("a fully intact run returned an error: %v", err)
+ }
+ if len(got) != 3 || got[0].Action != "run-start" || got[1].Action != "move" || got[2].Action != "run-end" {
+ t.Errorf("entries = %+v, want [run-start, move, run-end]", got)
+ }
+}
+
+// TestEntriesBothFailureModesReportsBadLineFirst is item 3: a run that both
+// has an unparsable line inside its window AND lacks a readable run-start
+// must surface as the unparsable-line error, not the missing-run-start one -
+// Entries checks badLine before sawRunStart. The run-start line here is
+// destroyed unattributably (as in TestEntriesFailsClosedOnMissingRunStart),
+// and a second, still-attributable line is separately corrupted so badLine
+// is set via the runFieldOf fallback rather than the window check.
+func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) {
+ path := filepath.Join(t.TempDir(), "krino.log")
+ w, _ := Open(path)
+ at := time.Date(2026, 9, 12, 8, 0, 0, 0, time.UTC)
+ if err := w.Append(Entry{Time: at, Run: "A", Action: "run-start", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1,
+ Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "A", Action: "run-end", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Close(); err != nil {
+ t.Fatal(err)
+ }
+
+ raw, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ lines := strings.Split(strings.TrimRight(string(raw), "\n"), "\n")
+ if len(lines) != 3 {
+ t.Fatalf("fixture has %d lines, want 3", len(lines))
+ }
+ // Line 1 (run-start): destroyed unattributably - no tabs at all, so
+ // runFieldOf cannot even recover its Run column.
+ lines[0] = "totally-mangled-no-tabs-here"
+ // Line 2 (move): corrupt its Step column only - it keeps its tabs and
+ // its Run column ("A") stays readable via runFieldOf's fallback.
+ fields := strings.Split(lines[1], "\t")
+ fields[4] = "not-a-number"
+ lines[1] = strings.Join(fields, "\t")
+ if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+
+ got, err := Entries(path, "A")
+ if err == nil {
+ t.Fatal("Entries returned no error with both a bad line and a missing run-start present")
+ }
+ if !strings.Contains(err.Error(), "unparsable line 2") {
+ t.Errorf("error = %q, want it to report the unparsable line (line 2), not the missing run-start", err)
+ }
+ if len(got) != 1 || got[0].Action != "run-end" {
+ t.Errorf("entries = %+v, want just the surviving run-end", got)
+ }
+}
+
+// TestEntriesAdjacentRunStartsOneCorrupted is item 4. Ruling R2: this pins
+// what journal.Entries does TODAY for two runs whose run-start lines are
+// adjacent, one of them corrupted - it does not assert an invented "correct"
+// result, and internal/journal is not touched by this task. The log here is
+// exactly:
+//
+// 1 run-start A (good)
+// 2 run-start B (corrupted: no tabs, unattributable)
+// 3 move A (good)
+// 4 run-end A (good)
+// 5 move B (good)
+// 6 run-end B (good)
+//
+// Observed behaviour, traced by hand against Entries and confirmed by this
+// test: Entries(path, "A") fails closed with the unparsable-line error,
+// because line 2 falls inside A's own window (opened by line 1, not yet
+// closed by a run-end) even though the corrupted line was actually B's
+// run-start, not A's - this is exactly the "residual risk... a false
+// refusal, not a false success" the function's own doc comment already
+// names. Entries(path, "B"), in contrast, never sees line 2 as inside its
+// window (B's window has not opened - its own run-start is the corrupted
+// line), so it reaches the end of the file with no badLine, and instead
+// fails on B's missing run-start.
+//
+// Concern (not fixed here, per R2 - flagged for judgement, not code
+// change): the SAME corrupted line produces two different error shapes
+// depending only on which run asks, which is a surprising inconsistency in
+// the message a caller sees, even though both directions correctly fail
+// closed.
+func TestEntriesAdjacentRunStartsOneCorrupted(t *testing.T) {
+ path := filepath.Join(t.TempDir(), "krino.log")
+ w, _ := Open(path)
+ at := time.Date(2026, 9, 12, 8, 0, 0, 0, time.UTC)
+ if err := w.Append(Entry{Time: at, Run: "A", Action: "run-start", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "B", Action: "run-start", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1,
+ Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "A", Action: "run-end", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "B", Dir: "dl", File: "y.pdf", Step: 1,
+ Action: "move", Status: "ok", Src: "/a/y.pdf", Dst: "/b/y.pdf"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Append(Entry{Time: at, Run: "B", Action: "run-end", Status: "ok"}); err != nil {
+ t.Fatal(err)
+ }
+ if err := w.Close(); err != nil {
+ t.Fatal(err)
+ }
+
+ raw, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ lines := strings.Split(strings.TrimRight(string(raw), "\n"), "\n")
+ if len(lines) != 6 {
+ t.Fatalf("fixture has %d lines, want 6", len(lines))
+ }
+ // Line 2, B's run-start, adjacent to A's on line 1: destroyed
+ // unattributably.
+ lines[1] = "totally-mangled-no-tabs-here"
+ if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+
+ gotA, errA := Entries(path, "A")
+ if errA == nil {
+ t.Fatal("Entries(A) returned no error; pinned behaviour expects one (the corrupted adjacent line falls inside A's window)")
+ }
+ if !strings.Contains(errA.Error(), "unparsable line 2") {
+ t.Errorf("Entries(A) error = %q, want it to name the unparsable line 2", errA)
+ }
+ if len(gotA) != 3 || gotA[0].Action != "run-start" || gotA[1].Action != "move" || gotA[2].Action != "run-end" {
+ t.Errorf("Entries(A) = %+v, want A's own [run-start, move, run-end]", gotA)
+ }
+
+ gotB, errB := Entries(path, "B")
+ if errB == nil {
+ t.Fatal("Entries(B) returned no error; pinned behaviour expects one (B's own run-start is the corrupted line)")
+ }
+ if !strings.Contains(errB.Error(), "no readable run-start") {
+ t.Errorf("Entries(B) error = %q, want it to name the missing run-start", errB)
+ }
+ if len(gotB) != 2 || gotB[0].Action != "move" || gotB[1].Action != "run-end" {
+ t.Errorf("Entries(B) = %+v, want B's surviving [move, run-end]", gotB)
+ }
+}
+
// TestEntriesReportsAMangledLine: a corrupt line that is not the log's
// final line must not be silently dropped by Entries the way Runs drops it
// - PlanUndo needs to know a step went missing so it can refuse the whole
diff --git a/internal/tui/tui.go b/internal/tui/tui.go
index 96bb728..c9f1241 100644
--- a/internal/tui/tui.go
+++ b/internal/tui/tui.go
@@ -36,9 +36,9 @@ func Colour(w io.Writer) bool {
return !noColour
}
-// Height is the terminal's row count, 0 when it is not a terminal or the
+// height is the terminal's row count, 0 when it is not a terminal or the
// size cannot be read.
-func Height(w io.Writer) int {
+func height(w io.Writer) int {
f, ok := w.(*os.File)
if !ok || !isTerminal(int(f.Fd())) {
return 0
@@ -73,7 +73,7 @@ func Page(w io.Writer, text string) error {
// the terminal the pager would inherit, not the writer text is otherwise
// sent to.
func fitsWithoutPaging(text string) bool {
- h := Height(os.Stdout)
+ h := height(os.Stdout)
if h <= 0 {
return true
}
diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go
index 6918859..a3640ac 100644
--- a/internal/tui/tui_test.go
+++ b/internal/tui/tui_test.go
@@ -37,8 +37,8 @@ func TestColourNeedsTerminalAndNoNOCOLOR(t *testing.T) {
func TestHeight(t *testing.T) {
var buf bytes.Buffer
- if h := Height(&buf); h != 0 {
- t.Errorf("Height on a non-terminal writer = %d, want 0", h)
+ if h := height(&buf); h != 0 {
+ t.Errorf("height on a non-terminal writer = %d, want 0", h)
}
oldT, oldS := isTerminal, termSize
@@ -46,8 +46,8 @@ func TestHeight(t *testing.T) {
isTerminal = func(fd int) bool { return true }
termSize = func(fd int) (int, int, error) { return 80, 24, nil }
- if h := Height(os.Stdout); h != 24 {
- t.Errorf("Height = %d, want 24", h)
+ if h := height(os.Stdout); h != 24 {
+ t.Errorf("height = %d, want 24", h)
}
}