diff options
35 files changed, 6958 insertions, 53 deletions
diff --git a/cmd/krino/commands_test.go b/cmd/krino/commands_test.go index 2156da2..b8364e6 100644 --- a/cmd/krino/commands_test.go +++ b/cmd/krino/commands_test.go @@ -9,7 +9,13 @@ import ( "testing" ) -// home gives each test its own HOME with no XDG overrides. +// home gives each test its own HOME with no XDG overrides, and points the +// package's stdin seam at something guaranteed non-terminal (fix round +// 2026-09-12/item 4): every test that reaches cmdSort's terminal check or +// the interactive review must not depend on what the ambient test binary's +// stdin happens to be - if that were ever a real terminal, such a test +// would silently fall through to the interactive prompt and block on a +// keypress instead of failing. func home(t *testing.T) string { t.Helper() h := t.TempDir() @@ -17,9 +23,24 @@ func home(t *testing.T) string { for _, v := range []string{"XDG_CONFIG_HOME", "XDG_STATE_HOME", "XDG_DATA_HOME", "XDG_CACHE_HOME"} { t.Setenv(v, "") } + setNonTerminalStdin(t) return h } +// setNonTerminalStdin points the stdin seam (sort.go) at os.DevNull for the +// duration of the calling test, restoring it afterward. +func setNonTerminalStdin(t *testing.T) { + t.Helper() + f, err := os.Open(os.DevNull) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { f.Close() }) + old := stdin + stdin = f + t.Cleanup(func() { stdin = old }) +} + func TestInitNewCheck(t *testing.T) { h := home(t) if err := os.Mkdir(filepath.Join(h, "dl"), 0o755); err != nil { diff --git a/cmd/krino/history_test.go b/cmd/krino/history_test.go new file mode 100644 index 0000000..2667b63 --- /dev/null +++ b/cmd/krino/history_test.go @@ -0,0 +1,317 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "krino/internal/engine" +) + +func TestLogListsRunsAndUndoReverses(t *testing.T) { + h := matchingFixture(t) + if code, _, errOut := runCLI(t, "-y"); code != 0 { + t.Fatalf("apply: %d %s", code, errOut) + } + filed := filepath.Join(h, "dl", "Work", "Acme", "inv1.txt") + if _, err := os.Stat(filed); err != nil { + t.Fatalf("nothing was filed: %v", err) + } + + code, out, errOut := runCLI(t, "log") + if code != 0 { + t.Fatalf("log: %d %s", code, errOut) + } + if !strings.Contains(out, "moved") || !strings.Contains(out, "dl") { + t.Errorf("log output:\n%s", out) + } + + if code, _, errOut = runCLI(t, "undo", "-y"); code != 0 { + t.Fatalf("undo: %d %s", code, errOut) + } + if _, err := os.Stat(filepath.Join(h, "dl", "inv1.txt")); err != nil { + t.Errorf("undo did not put the file back: %v", err) + } + if _, err := os.Stat(filed); !os.IsNotExist(err) { + t.Error("the filed copy survived the undo") + } + + if _, out, _ = runCLI(t, "log"); !strings.Contains(out, "undone") { + t.Errorf("log does not mark the run undone:\n%s", out) + } + if code, _, errOut = runCLI(t, "undo", "-y"); code == 0 { + t.Errorf("undoing an undo run succeeded: %q", errOut) + } +} + +func TestUndoDryRunChangesNothing(t *testing.T) { + h := matchingFixture(t) + runCLI(t, "-y") + filed := filepath.Join(h, "dl", "Work", "Acme", "inv1.txt") + if code, out, _ := runCLI(t, "undo", "-n"); code != 0 || !strings.Contains(out, "undo-move") { + t.Errorf("undo -n: %d\n%s", code, out) + } + if _, err := os.Stat(filed); err != nil { + t.Error("undo -n moved a file") + } +} + +// TestUndoFailsImmediatelyWithHeldLock is fix round 2026-09-12, item 1: an +// undo moves files just as an apply does, so it needs the same per-directory +// guard sort's TestSecondRunFailsImmediatelyWithYes already pins for the +// forward path (spec §11: "a second krino on the same directory ... fails +// immediately with -y"). The lock file is held under the config NAME "dl", +// not any filesystem path - UndoFile.Dir is the journal's `dir` column, +// which is the directory's name from krino.conf, not its root. +// +// Ruling (fix round 2026-09-12, follow-up): the lock is acquired before the +// plan is even shown, matching cmdSort's own window (acquired before +// Plan/review, held across both) rather than only around ApplyUndo - so +// this also asserts the refusal is noticed before any plan output reaches +// stdout. A version of this test that only checked the exit code and +// stderr would pass equally whether the lock were taken early or late, and +// so would not be pinning the thing this ruling is actually about. +func TestUndoFailsImmediatelyWithHeldLock(t *testing.T) { + h := matchingFixture(t) + if code, _, errOut := runCLI(t, "-y"); code != 0 { + t.Fatalf("apply: %d %s", code, errOut) + } + filed := filepath.Join(h, "dl", "Work", "Acme", "inv1.txt") + if _, err := os.Stat(filed); err != nil { + t.Fatalf("nothing was filed: %v", err) + } + + held := filepath.Join(h, ".local", "state", "krino", "dl.lock") + if err := os.MkdirAll(filepath.Dir(held), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(held, []byte(fmt.Sprintf("pid %d\n", os.Getpid())), 0o644); err != nil { + t.Fatal(err) + } + + code, out, errOut := runCLI(t, "undo", "-y") + if code != 1 || !strings.Contains(errOut, "another krino") { + t.Errorf("undo -y against a held lock: %d %q", code, errOut) + } + if strings.Contains(out, "to reverse") || strings.Contains(out, "undo-move") { + t.Errorf("undo printed the plan before noticing the held lock:\n%s", out) + } + if _, err := os.Stat(filed); err != nil { + t.Errorf("undo reversed the file despite the held lock: %v", err) + } +} + +// undoFiles builds minimal engine.UndoFile fixtures for reviewUndoFiles/ +// reviewUndoPerFile, named by rel path only (Dir left blank - the tests +// below never render it). +func undoFiles(rels ...string) []engine.UndoFile { + out := make([]engine.UndoFile, len(rels)) + for i, r := range rels { + out[i] = engine.UndoFile{File: r, Steps: []engine.UndoStep{{Action: "undo-move", Src: "/t/" + r, Dst: "/s/" + r}}} + } + return out +} + +// TestFinalizeUndoPlanMarksUnapprovedAsDeclined is the wiring point for fix +// round 2026-09-12, item 2: a refused file rides through untouched (its own +// Refused reason is what ApplyUndo checks first), an approved file rides +// through untouched too, and anything else - explicitly declined, or never +// reached because [d]/[q] cut a per-file review short - comes out with +// Declined set rather than being dropped from the plan. +func TestFinalizeUndoPlanMarksUnapprovedAsDeclined(t *testing.T) { + up := &engine.UndoPlan{Run: "r1", Files: []engine.UndoFile{ + {File: "a"}, // index 0: approved + {File: "b"}, // index 1: not approved -> declined + {File: "c", Refused: "gone"}, // index 2: refused, never declined + }} + out := finalizeUndoPlan(up, map[int]bool{0: true}) + if len(out.Files) != 3 { + t.Fatalf("files = %+v, want all three carried through", out.Files) + } + if out.Files[0].Declined || out.Files[0].Refused != "" { + t.Errorf("approved file changed: %+v", out.Files[0]) + } + if !out.Files[1].Declined || out.Files[1].Refused != "" { + t.Errorf("unapproved file not marked declined: %+v", out.Files[1]) + } + if out.Files[2].Declined { + t.Errorf("a refused file must not also be marked declined: %+v", out.Files[2]) + } + if out.Files[2].Refused != "gone" { + t.Errorf("refused file's reason changed: %+v", out.Files[2]) + } +} + +// TestReviewUndoApplyAll: [a] approves every reversible file by index. +func TestReviewUndoApplyAll(t *testing.T) { + approved, action, err := reviewUndoFiles(strings.NewReader("a"), new(strings.Builder), undoFiles("a", "b")) + if err != nil { + t.Fatal(err) + } + if action != 'a' || len(approved) != 2 || !approved[0] || !approved[1] { + t.Errorf("approved = %v action = %q; want both approved", approved, action) + } +} + +// TestReviewUndoSkipAndQuit: [s] and [q] both approve nothing. +func TestReviewUndoSkipAndQuit(t *testing.T) { + approved, action, _ := reviewUndoFiles(strings.NewReader("s"), new(strings.Builder), undoFiles("a", "b")) + if action != 's' || len(approved) != 0 { + t.Errorf("[s] = %q %v; want nothing approved", action, approved) + } + approved, action, _ = reviewUndoFiles(strings.NewReader("q"), new(strings.Builder), undoFiles("a", "b")) + if action != 'q' || len(approved) != 0 { + t.Errorf("[q] = %q %v; want nothing approved", action, approved) + } +} + +// TestReviewUndoChoosePerFile: [c] then per-file y/n, keyed by index. +func TestReviewUndoChoosePerFile(t *testing.T) { + approved, action, err := reviewUndoFiles(strings.NewReader("cyn"), new(strings.Builder), undoFiles("a", "b")) + if err != nil { + t.Fatal(err) + } + if action != 'c' || !approved[0] || approved[1] { + t.Errorf("approved = %v action = %q; want only index 0", approved, action) + } +} + +// TestReviewUndoRefusedFileNotPrompted is spec §10: a file PlanUndo already +// refused is shown (with its reason - covered end to end by +// TestUndoRefusesChangedDestination-style flows through cmdUndo) but never +// asked about, so a [c] session reading one key per file must not stall +// waiting for a key that reviewUndoPerFile never asks for. Three files, one +// key each for the two reversible ones ("y", "n"), none for the refused +// middle one: if it were prompted, the second key ("n") would answer for it +// instead of the third file, and this test would see index 2 approved +// instead of unset. +func TestReviewUndoRefusedFileNotPrompted(t *testing.T) { + files := undoFiles("a", "b", "c") + files[1].Refused = "b changed since the run" + out := new(strings.Builder) + approved, action, err := reviewUndoFiles(strings.NewReader("cyn"), out, files) + if err != nil { + t.Fatal(err) + } + if action != 'c' || !approved[0] || approved[1] || approved[2] { + t.Errorf("approved = %v action = %q; want only index 0 (1 is refused, 2 never reached)", approved, action) + } + if !strings.Contains(out.String(), "b changed since the run") { + t.Errorf("refused file's reason not shown:\n%s", out) + } +} + +// TestPrintUndoPlan is fix wave item 3 (Important), rebuilding fix round +// 2026-09-12's own golden test: that version hand-built its UndoSteps, +// including a Dst on the undo-copy step the real code never sets (Dst is +// deliberately left "" - trash.Put only chooses the entry name at execution +// time), so it was structurally incapable of catching the bug it was meant +// to guard against - an undo-copy row rendering as a bare +// "undo-copy → " with nothing said about what it would do to the +// user's backup copy, the single most destructive step an undo plan takes. +// The same lesson as Task 9's review: a hand-assembled fixture hides a test +// that cannot detect a broken copy-undo. This version runs a REAL forward +// apply (copy then move, so mkdir, copy and move all appear in one file's +// own chain) and a REAL PlanUndo, editing one file's result afterward so +// the plan also carries a genuinely refused row, then renders that. +func TestPrintUndoPlan(t *testing.T) { + h := home(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(filepath.Join(dl, "Work"), 0o755); err != nil { + t.Fatal(err) + } + old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for _, n := range []string{"inv1.pdf", "notes.pdf"} { + p := filepath.Join(dl, n) + if err := os.WriteFile(p, []byte("content of "+n), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(p, old, old); err != nil { + t.Fatal(err) + } + } + if code, _, errOut := runCLI(t, "init"); code != 0 { + t.Fatal(errOut) + } + if code, _, errOut := runCLI(t, "new", "dl", dl); code != 0 { + t.Fatal(errOut) + } + // "keep" mixes all three action kinds a single file's own chain can + // carry: copy needs a fresh ~/backup (one mkdir), move lands in the + // pre-created Work (no mkdir of its own). + rules := "(path \"~/dl\")\n(min-age 0s)\n(rule \"keep\" (when (type pdf)) (copy \"~/backup\") (move \"Work\"))\n" + if err := os.WriteFile(filepath.Join(h, ".config/krino/dirs/dl.conf"), []byte(rules), 0o644); err != nil { + t.Fatal(err) + } + if code, _, errOut := runCLI(t, "-y"); code != 0 { + t.Fatalf("apply: %d %s", code, errOut) + } + + // notes.pdf's moved copy is edited after the run, so PlanUndo genuinely + // refuses its reversal - the row this exercises must still say + // something true, never render blank. + moved := filepath.Join(dl, "Work", "notes.pdf") + if err := os.WriteFile(moved, []byte("edited since the run"), 0o644); err != nil { + t.Fatal(err) + } + + e, errs := engine.Load(filepath.Join(h, ".config", "krino", "krino.conf")) + if len(errs) > 0 { + t.Fatal(errs) + } + runs, err := e.Runs(1) + if err != nil || len(runs) != 1 { + t.Fatalf("runs = %+v, err = %v", runs, err) + } + up, err := e.PlanUndo(runs[0].ID) + if err != nil { + t.Fatal(err) + } + + var buf bytes.Buffer + printUndoPlan(&buf, up) + out := buf.String() + + for _, want := range []string{ + "2 files · 1 to reverse · 1 refused\n", + "undo-move → ~/dl/inv1.pdf\n", + "undo-copy ~/backup/inv1.pdf → trash\n", + "undo-mkdir ~/backup\n", + // Minor 4 / fix wave item 5: the refusal reason must be abbreviated + // against $HOME exactly like every step cell above it, not printed + // as a raw absolute path. + "refused: ~/dl/Work/notes.pdf changed since the run\n", + } { + if !strings.Contains(out, want) { + t.Errorf("output lacks %q:\n%s", want, out) + } + } + if strings.Contains(out, "undo-copy → ") { + t.Errorf("undo-copy rendered a blank destination:\n%s", out) + } + if strings.Contains(out, h) { + t.Errorf("output leaked a raw absolute path instead of abbreviating against $HOME:\n%s", out) + } +} + +// TestReviewUndoInvalidKeyReprompts mirrors review_test.go's +// TestInvalidKeyReprompts for the undo-specific menu. +func TestReviewUndoInvalidKeyReprompts(t *testing.T) { + out := new(strings.Builder) + approved, action, err := reviewUndoFiles(strings.NewReader("zs"), out, undoFiles("a")) + if err != nil { + t.Fatal(err) + } + if action != 's' || len(approved) != 0 { + t.Errorf("approved = %v action = %q; want [s] after the bad key", approved, action) + } + if !strings.Contains(out.String(), "z") { + t.Errorf("no mention of the rejected key:\n%s", out) + } +} diff --git a/cmd/krino/log.go b/cmd/krino/log.go new file mode 100644 index 0000000..6eb15d6 --- /dev/null +++ b/cmd/krino/log.go @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "errors" + "fmt" + "io" + "os" + "strings" + + "krino/internal/engine" + "krino/internal/journal" +) + +func init() { commands["log"] = cmdLog } + +// pastTense renders one of the log's own action words (spec §9's wire +// format - journal.Run.Counts is keyed by these exact strings) as the past +// tense krino log displays. Those strings are never renamed to suit the +// display: krino undo parses them back out of the log, so this map is a +// display-only translation, not a second source of truth. Two families of +// action are deliberately absent, and so never appear in a run's counts +// line at all: run-start/run-end (bookkeeping, not something done to a +// file) and mkdir/undo-mkdir (a side effect of another step, not an +// outcome the user asked for). +var pastTense = map[string]string{ + "copy": "copied", + "move": "moved", + "rename": "renamed", + "trash": "trashed", + "delete": "deleted", + "displace": "displaced", + "undo-copy": "undo-copied", + "undo-move": "undo-moved", + "undo-rename": "undo-renamed", + "undo-trash": "undo-trashed", + "undo-displace": "undo-displaced", +} + +// countOrder is the fixed order krino log renders a run's counts in. +// journal.Run.Counts is a map, whose iteration order is random; a listing +// that reshuffled its own columns between two invocations of the same +// command would be unreadable. +var countOrder = []string{ + "copy", "move", "rename", "trash", "delete", "displace", + "undo-copy", "undo-move", "undo-rename", "undo-trash", "undo-displace", +} + +// cmdLog lists recent runs, newest first (spec §11: "krino log [-n N]"). +func cmdLog(g *globals, args []string, stdout, stderr io.Writer) int { + fs := flagSet("log", g) + n := fs.Int("n", 10, "") + if code, ok := parse(fs, args, stdout, stderr); !ok { + return code + } + if fs.NArg() > 0 { + return usageError(stderr, "usage: krino log [-n N]") + } + + e, errs := engine.Load(mainFile(g)) + if len(errs) > 0 { + printDiags(stderr, errs) + return 2 + } + + runs, err := e.Runs(*n) + if err != nil { + // journal.Runs (via Engine.Runs) fails closed on a genuine read + // error - permissions, a corrupt file - and that must stay + // distinguishable from the ordinary "no log yet" case below rather + // than collapsing into the same message (dispatch notes). + if errors.Is(err, os.ErrNotExist) { + fmt.Fprintln(stdout, "nothing logged yet") + return 0 + } + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + // A log file can exist and still hold no runs (a real run with nothing + // actionable still opens the journal - dispatch notes). That is just as + // ordinary as no log file at all: same message, same exit 0, and no + // table header is printed over zero rows. + if len(runs) == 0 { + fmt.Fprintln(stdout, "nothing logged yet") + return 0 + } + + for _, r := range runs { + fmt.Fprintln(stdout, formatRun(r)) + } + return 0 +} + +// formatRun renders one journal.Run as krino log lists it: id, start time, +// the directories it touched, and its counts (see countsText), with +// "(undone)" appended when a later run has reversed it. +func formatRun(r journal.Run) string { + line := fmt.Sprintf("%s %s %s %s", r.ID, r.Start.Format("2006-01-02 15:04"), strings.Join(r.Dirs, ", "), countsText(r.Counts)) + if r.Undone { + line += " (undone)" + } + return line +} + +// countsText renders a run's counts in countOrder, past tense, skipping any +// action with no "ok" entries. "nothing applied" covers a run every one of +// whose files was declined or failed before anything ran - Counts only +// tallies "ok" entries, so such a run's map holds nothing this function +// would otherwise print. +func countsText(counts map[string]int) string { + var parts []string + for _, action := range countOrder { + if n := counts[action]; n > 0 { + parts = append(parts, fmt.Sprintf("%d %s", n, pastTense[action])) + } + } + if len(parts) == 0 { + return "nothing applied" + } + return strings.Join(parts, " · ") +} diff --git a/cmd/krino/main.go b/cmd/krino/main.go index 9618f6c..e204b5f 100644 --- a/cmd/krino/main.go +++ b/cmd/krino/main.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "os" + "strings" ) // version is stamped by the Makefile with -ldflags "-X main.version=...". @@ -71,6 +72,19 @@ func run(args []string, stdout, stderr io.Writer) int { return cmd(g, rest[1:], stdout, stderr) } } + // Ruling 2026-09-12/4: Go's flag package stops parsing at the first + // non-flag argument, so "krino dl -n" leaves "-n" in rest as a second + // directory name instead of a flag - the dry run is silently never + // honoured. A leftover argument that still looks like a flag is a + // usage error rather than a guess; there is deliberately no second + // pass that re-parses trailing flags, which would make "krino -- + // -weird-dir" ambiguous between the two syntaxes. + for _, a := range rest { + if strings.HasPrefix(a, "-") { + fmt.Fprintf(stderr, "krino: %s: flags must come before directory names\nrun 'krino -h' for help\n", a) + return 2 + } + } return cmdSort(g, rest, stdout, stderr) } diff --git a/cmd/krino/main_test.go b/cmd/krino/main_test.go index 46ceb44..1c01fa0 100644 --- a/cmd/krino/main_test.go +++ b/cmd/krino/main_test.go @@ -51,10 +51,14 @@ func TestCommandsAreReserved(t *testing.T) { } } -func TestSortNotYet(t *testing.T) { +// TestSortNoConfig: apply is implemented as of Task 7, so running with no +// config at all now fails the same way every other command does — "not +// found" from engine.Load, not the old "not implemented yet" hard stop this +// test used to pin (removed as part of Task 7; see cmd/krino/sort.go). +func TestSortNoConfig(t *testing.T) { home(t) code, _, errOut := runCLI(t) - if code != 2 || !strings.Contains(errOut, "not implemented yet") { + if code != 2 || !strings.Contains(errOut, "not found; create it with: krino init") { t.Fatalf("got %d %q", code, errOut) } } diff --git a/cmd/krino/matching_test.go b/cmd/krino/matching_test.go index 614edc2..69c0eca 100644 --- a/cmd/krino/matching_test.go +++ b/cmd/krino/matching_test.go @@ -5,6 +5,7 @@ package main import ( "bytes" "encoding/json" + "fmt" "os" "path/filepath" "strings" @@ -148,8 +149,8 @@ func TestSortFlags(t *testing.T) { want string }{ {[]string{"-y", "-n"}, "-y and -n cannot be used together"}, - {nil, "applying files is not implemented yet; use -n to see what would happen"}, - {[]string{"--json"}, "--json is not implemented yet"}, // --json without -n stays an error + {nil, "not a terminal"}, // no -y, no -n, and the test's stdin is not one + {[]string{"--json"}, "--json is only valid with -n"}, // --json without -n stays an error } for _, tt := range tests { if code, _, errOut := runCLI(t, tt.args...); code != 2 || !strings.Contains(errOut, tt.want) { @@ -356,6 +357,100 @@ func TestDirectoryWarningNotCountedInWarningsField(t *testing.T) { // not stretched further), while a short name alongside it is still padded // out to the full 40-column cap — the layout stays a clean two-column grid // even though one row's first cell overruns it. +// TestApplyWithYesMovesFiles is brief 7's basic apply-path test: -y applies +// the plan with no prompt, the file actually moves, the outcome line says +// so, and the run is logged with both boundaries. +func TestApplyWithYesMovesFiles(t *testing.T) { + h := matchingFixture(t) + code, out, errOut := runCLI(t, "-y") + if code != 0 { + t.Fatalf("exit %d: %s", code, errOut) + } + if _, err := os.Stat(filepath.Join(h, "dl", "Work", "Acme", "inv1.txt")); err != nil { + t.Errorf("inv1.txt was not filed: %v", err) + } + if !strings.Contains(out, "applied") { + t.Errorf("no outcome line:\n%s", out) + } + log := filepath.Join(h, ".local", "state", "krino", "krino.log") + b, err := os.ReadFile(log) + if err != nil { + t.Fatalf("nothing was logged: %v", err) + } + if !strings.Contains(string(b), "run-start") || !strings.Contains(string(b), "run-end") { + t.Errorf("log lacks run boundaries:\n%s", b) + } +} + +// TestDryRunLogsNothing is Ruling 2: journal.Open must never be called at +// all in dry-run mode, since it materialises both the state directory and +// an empty log file as a side effect of merely opening it. +func TestDryRunLogsNothing(t *testing.T) { + h := matchingFixture(t) + if code, _, errOut := runCLI(t, "-n"); code != 0 { + t.Fatalf("exit %d: %s", code, errOut) + } + if _, err := os.Stat(filepath.Join(h, ".local", "state", "krino", "krino.log")); !os.IsNotExist(err) { + t.Error("a dry run wrote to the log") + } +} + +// TestRefusesWithoutTerminalAndWithoutFlags is spec §8.4: with neither -y +// nor -n, and stdin not a terminal (as in every test), krino refuses rather +// than guess. +func TestRefusesWithoutTerminalAndWithoutFlags(t *testing.T) { + matchingFixture(t) + code, _, errOut := runCLI(t) + if code != 2 || !strings.Contains(errOut, "not a terminal") { + t.Errorf("no-flags, no-terminal: %d %q", code, errOut) + } +} + +// TestSecondRunFailsImmediatelyWithYes is spec §11: a second krino on the +// same directory fails immediately with -y (wait = false) rather than +// piling up behind a stuck run, so a cron job never queues silently. +func TestSecondRunFailsImmediatelyWithYes(t *testing.T) { + h := matchingFixture(t) + held := filepath.Join(h, ".local", "state", "krino", "dl.lock") + if err := os.MkdirAll(filepath.Dir(held), 0o755); err != nil { + t.Fatal(err) + } + // A lock held by this very process, so it is not stale. + if err := os.WriteFile(held, []byte(fmt.Sprintf("pid %d\n", os.Getpid())), 0o644); err != nil { + t.Fatal(err) + } + code, _, errOut := runCLI(t, "-y") + if code != 1 || !strings.Contains(errOut, "another krino") { + t.Errorf("-y against a held lock: %d %q", code, errOut) + } +} + +// TestFlagsMustPrecedeDirectoryNames is Ruling 4: Go's flag package stops +// parsing at the first non-flag argument, so "krino dl -n" would otherwise +// silently take "-n" as a second directory name and never honour the dry +// run. A leftover argument starting with "-" is a usage error instead of a +// guess. +func TestFlagsMustPrecedeDirectoryNames(t *testing.T) { + home(t) + code, _, errOut := runCLI(t, "dl", "-n") + if code != 2 || !strings.Contains(errOut, "-n") { + t.Errorf("krino dl -n: %d %q", code, errOut) + } +} + +// TestNoColourEscapeToNonTerminal is Ruling 7's one pinned guarantee: a plan +// piped to a file or read by another tool must be plain text. tui.Colour(w) +// already returns false for anything that is not a terminal *os.File, and +// runCLI's stdout is a bytes.Buffer, so this holds end to end through the +// real command path, not just at the helper that decides it. +func TestNoColourEscapeToNonTerminal(t *testing.T) { + matchingFixture(t) + _, out, _ := runCLI(t, "-n") + if strings.ContainsRune(out, '\x1b') { + t.Errorf("escape byte reached a non-terminal writer:\n%q", out) + } +} + func TestLongNameNotPaddedLayoutIntact(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") diff --git a/cmd/krino/render.go b/cmd/krino/render.go index b27d4d1..de5489a 100644 --- a/cmd/krino/render.go +++ b/cmd/krino/render.go @@ -77,6 +77,25 @@ func printPlan(w io.Writer, dp *engine.DirPlan, verbose bool) { } } +// chainActing reports whether c has at least one step that will actually +// run - the single definition of "actionable" that countActing, +// actionableChains (sort.go) and chainOutcomes (sort.go) all share (fix +// wave item 4 / Minor 5). Before this fix, countActing and actionableChains +// each kept their own copy of this question and disagreed: countActing +// excluded an all-skipped chain (len(Steps) > 0, but every step's Skip is +// set) while actionableChains's own len(Steps) > 0 check included it, so a +// directory could print "N scanned · 0 to act on" and then still ask the +// user to approve a file it had just said there were none of - and on +// approval, log a run-start/run-end pair holding only "skipped" entries. +func chainActing(c plan.Chain) bool { + for _, s := range c.Steps { + if s.Skip == "" { + return true + } + } + return false +} + // countActing reports how many chains have at least one step that will // actually run. C1/ruling 2026-09-12: a rule with no actions is an // exclusion, and a chain every one of whose steps is skipped is not about @@ -84,11 +103,8 @@ func printPlan(w io.Writer, dp *engine.DirPlan, verbose bool) { func countActing(chains []plan.Chain) int { n := 0 for _, c := range chains { - for _, s := range c.Steps { - if s.Skip == "" { - n++ - break - } + if chainActing(c) { + n++ } } return n diff --git a/cmd/krino/review.go b/cmd/krino/review.go new file mode 100644 index 0000000..4f5ad05 --- /dev/null +++ b/cmd/krino/review.go @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "fmt" + "io" + "os" + "strings" + + "krino/internal/plan" + "krino/internal/tui" +) + +// reviewDir drives spec §8.2/§8.3's interactive review over the real +// terminal. It is a thin wrapper: reviewChains holds all the actual +// approval logic, driven here by tui.ReadKey (raw mode while stdin is a +// terminal, a plain single-byte read otherwise) via keyReader, so the exact +// same code path runs whether the input is a real keypress or, in tests, a +// strings.Reader. root is the directory being reviewed, threaded through to +// reviewChains so a per-file destination renders the same way the +// directory-level table does (root-relative inside root, ~-abbreviated +// outside it) instead of always falling back to the abbreviated form. +func reviewDir(out io.Writer, chains []plan.Chain, root string) (map[string]bool, rune, error) { + return reviewChains(keyReader{stdin}, out, chains, root) +} + +// keyReader adapts tui.ReadKey - one key at a time, from a real *os.File - +// to the io.Reader reviewChains expects. tui.ReadKey's own doc comment is +// why this is safe to call once per key: it always reads exactly one byte +// and restores the terminal on every path before returning. +type keyReader struct{ f *os.File } + +func (k keyReader) Read(p []byte) (int, error) { + r, err := tui.ReadKey(k.f) + if err != nil { + return 0, err + } + p[0] = byte(r) + return 1, nil +} + +// reviewChains is spec §8.2/§8.3's approval flow, and the testable core +// reviewDir wraps: the top-level +// +// [a] apply all [c] choose per file [s] skip this directory [q] quit +// +// menu, and, for [c], the per-file +// +// [y] yes [n] no [a] yes to this and all remaining [d] done, apply chosen so far [q] quit, apply nothing +// +// prompt. action is always one of 'a', 'c', 's' or 'q': a [c] session's own +// [q] ("quit, apply nothing") folds into the same 'q' the caller already +// handles for the top-level menu, and approved is emptied to match - even a +// file already marked yes in that session is discarded, per spec §8.3's +// wording ("apply nothing"), unlike [d] ("apply chosen so far"), which +// keeps it. root is the directory being reviewed - passed only to +// reviewPerFile's destination rendering (review finding 1, fix round +// 2026-09-12); nothing here uses it directly. +func reviewChains(in io.Reader, out io.Writer, chains []plan.Chain, root string) (map[string]bool, rune, error) { + fmt.Fprint(out, "\n[a] apply all [c] choose per file [s] skip this directory [q] quit\n") + for { + key, err := readKey(in) + if err != nil { + return nil, 0, err + } + switch key { + case 'a': + return approveAll(chains), 'a', nil + case 's': + return map[string]bool{}, 's', nil + case 'q': + return map[string]bool{}, 'q', nil + case 'c': + approved, quit, err := reviewPerFile(in, out, chains, root) + if err != nil { + return nil, 0, err + } + if quit { + return map[string]bool{}, 'q', nil + } + return approved, 'c', nil + default: + fmt.Fprintf(out, "%q is not a, c, s or q\n", key) + } + } +} + +// reviewPerFile is spec §8.3: one prompt per file, in the order chains +// already carries them (the same order the numbered table above it was +// shown in, per D15 in render.go). [y]/[n] decide just that file; [a] +// approves it and every remaining file without asking again; [d] stops +// asking and applies whatever was already chosen, declining the rest; [q] +// aborts the review entirely, discarding even files already marked yes - +// reported back to reviewChains via quit=true. root is passed to +// actionCell exactly as the directory-level table (render.go's planRows) +// already does, so a destination inside root renders root-relative and one +// outside it renders ~-abbreviated - review finding 1 (fix round +// 2026-09-12): passing "" here always fails filepath.Rel("", dir) and +// silently fell back to the abbreviated form even for a destination inside +// root, which is not what spec §8.3's own worked example shows. +func reviewPerFile(in io.Reader, out io.Writer, chains []plan.Chain, root string) (approved map[string]bool, quit bool, err error) { + approved = map[string]bool{} + yesRest := false + for i, c := range chains { + if yesRest { + approved[c.File.Rel] = true + continue + } + + fmt.Fprintf(out, "\n[%d/%d] %s\n", i+1, len(chains), c.File.Rel) + for _, s := range c.Steps { + fmt.Fprintf(out, " %s\n", actionCell(s, root)) + } + fmt.Fprint(out, " [y] yes [n] no [a] yes to this and all remaining [d] done, apply chosen so far [q] quit, apply nothing\n") + + for { + key, kerr := readKey(in) + if kerr != nil { + return nil, false, kerr + } + switch key { + case 'y': + approved[c.File.Rel] = true + case 'n': + // leave unapproved + case 'a': + approved[c.File.Rel] = true + yesRest = true + case 'd': + return approved, false, nil + case 'q': + return nil, true, nil + default: + fmt.Fprintf(out, "%q is not y, n, a, d or q\n", key) + continue + } + break + } + } + return approved, false, nil +} + +// readKey reads the single byte reviewChains treats as one keypress. Over +// the real terminal that byte already came from tui.ReadKey (via +// keyReader); in a test, it is just the next byte of a strings.Reader. +func readKey(in io.Reader) (rune, error) { + var b [1]byte + if _, err := io.ReadFull(in, b[:]); err != nil { + return 0, err + } + return rune(b[0]), nil +} + +// approveAll approves every one of chains by File.Rel - [a] apply all at +// the top level, and [a] yes to this and all remaining once it fires +// mid per-file review. +func approveAll(chains []plan.Chain) map[string]bool { + approved := make(map[string]bool, len(chains)) + for _, c := range chains { + approved[c.File.Rel] = true + } + return approved +} + +// colourDeletePermanently wraps spec §8.2's "DELETE permanently" marker in +// the terminal's own ANSI red (bold, slot 1 - never hex), the one thing the +// spec singles out for emphasis (Ruling 2026-09-12/7). It is applied to +// text render.go's printPlan already produced, rather than threading a +// colour parameter through the renderer itself: that keeps render.go and +// its golden-file tests exactly as plan 4 built them. colour is always the +// caller's own tui.Colour(stdout) decision - with it false this is a no-op, +// which is what keeps every escape byte out of a plan piped to a file or +// read by another tool. +func colourDeletePermanently(text string, colour bool) string { + if !colour { + return text + } + return strings.ReplaceAll(text, "DELETE permanently", "\x1b[1;31mDELETE permanently\x1b[0m") +} diff --git a/cmd/krino/review_test.go b/cmd/krino/review_test.go new file mode 100644 index 0000000..193a191 --- /dev/null +++ b/cmd/krino/review_test.go @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "strings" + "testing" + + "krino/internal/plan" + "krino/internal/scan" +) + +func chains(rels ...string) []plan.Chain { + out := make([]plan.Chain, len(rels)) + for i, r := range rels { + out[i] = plan.Chain{File: scan.File{Rel: r}, Steps: []plan.Step{{Kind: plan.Move, Dst: "/w/" + r}}} + } + return out +} + +func TestChoosePerFile(t *testing.T) { + // c enters per-file mode, then y n y for three files. + approved, action, err := reviewChains(strings.NewReader("cyny"), new(strings.Builder), chains("a", "b", "c"), "") + if err != nil { + t.Fatal(err) + } + if action != 'c' { + t.Errorf("action = %q", action) + } + if !approved["a"] || approved["b"] || !approved["c"] { + t.Errorf("approved = %v; want a and c only", approved) + } +} + +func TestApplyAllAndSkip(t *testing.T) { + approved, action, _ := reviewChains(strings.NewReader("a"), new(strings.Builder), chains("a", "b"), "") + if action != 'a' || len(approved) != 2 { + t.Errorf("[a] = %q %v; want every file approved", action, approved) + } + approved, action, _ = reviewChains(strings.NewReader("s"), new(strings.Builder), chains("a", "b"), "") + if action != 's' || len(approved) != 0 { + t.Errorf("[s] = %q %v; want nothing approved", action, approved) + } +} + +func TestPerFileDoneStopsAsking(t *testing.T) { + // c, y for the first, then d: apply what was chosen so far. + approved, _, _ := reviewChains(strings.NewReader("cyd"), new(strings.Builder), chains("a", "b", "c"), "") + if !approved["a"] || approved["b"] || approved["c"] { + t.Errorf("approved = %v; want only a", approved) + } +} + +// TestPerFileQuitAppliesNothing: spec §8.3's [q] on the per-file prompt is +// "quit, apply nothing" - stronger than [d], which keeps what was already +// chosen. It folds into the same top-level 'q' the caller already handles +// for the directory-level menu (spec §8.2's [q]), and discards even a file +// already marked yes. +func TestPerFileQuitAppliesNothing(t *testing.T) { + approved, action, err := reviewChains(strings.NewReader("cyq"), new(strings.Builder), chains("a", "b"), "") + if err != nil { + t.Fatal(err) + } + if action != 'q' { + t.Errorf("action = %q, want 'q'", action) + } + if len(approved) != 0 { + t.Errorf("approved = %v; want nothing, even though a was marked yes first", approved) + } +} + +// TestPerFileYesToAllRemaining: spec §8.3's [a] mid-review approves the +// current file and every remaining one without asking again. +func TestPerFileYesToAllRemaining(t *testing.T) { + approved, action, err := reviewChains(strings.NewReader("ca"), new(strings.Builder), chains("a", "b", "c"), "") + if err != nil { + t.Fatal(err) + } + if action != 'c' || len(approved) != 3 { + t.Errorf("approved = %v action = %q; want all three approved", approved, action) + } +} + +// TestInvalidKeyReprompts: an unrecognised key at either the top-level menu +// or the per-file prompt does not abort the review - it is reported and the +// same prompt is read again. +func TestInvalidKeyReprompts(t *testing.T) { + out := new(strings.Builder) + approved, action, err := reviewChains(strings.NewReader("zs"), out, chains("a"), "") + if err != nil { + t.Fatal(err) + } + if action != 's' || len(approved) != 0 { + t.Errorf("approved = %v action = %q; want [s] after the bad key", approved, action) + } + if !strings.Contains(out.String(), "z") { + t.Errorf("no mention of the rejected key:\n%s", out) + } +} + +// TestPerFileDestinationIsRootRelative is review finding 1 (fix round +// 2026-09-12): reviewPerFile must render a destination the same way the +// directory-level table does (render.go's destText) - root-relative for a +// destination inside root, ~-abbreviated for one outside it - not always +// abbreviated because root was never passed through to actionCell at all. +// Both halves are pinned: getting only the "inside" half right would still +// let an outside-root destination silently regress to some other form. +func TestPerFileDestinationIsRootRelative(t *testing.T) { + t.Setenv("HOME", "/home/x") + root := "/home/x/dl" + cs := []plan.Chain{ + {File: scan.File{Rel: "inside.txt"}, Steps: []plan.Step{{Kind: plan.Move, Dst: root + "/Work/Acme/inside.txt"}}}, + {File: scan.File{Rel: "outside.txt"}, Steps: []plan.Step{{Kind: plan.Move, Dst: "/home/x/backup/outside.txt"}}}, + } + out := new(strings.Builder) + if _, _, err := reviewChains(strings.NewReader("cyy"), out, cs, root); err != nil { + t.Fatal(err) + } + text := out.String() + if !strings.Contains(text, "move → Work/Acme/") { + t.Errorf("destination inside root should be root-relative, not ~-abbreviated:\n%s", text) + } + if !strings.Contains(text, "move → ~/backup/") { + t.Errorf("destination outside root should be ~-abbreviated:\n%s", text) + } +} + +// TestNoColourEscapeFromColourFalse pins Ruling 7's safety guarantee at the +// unit that actually decides it: with colour off, colourDeletePermanently +// must not alter the text at all, and with colour on it must add an escape +// around exactly the one marker spec §8.2 singles out for emphasis. +func TestColourDeletePermanently(t *testing.T) { + plain := " 4 setup-1.2.deb DELETE permanently old-pkgs age 94d\n" + if got := colourDeletePermanently(plain, false); got != plain { + t.Errorf("colour=false must leave the text untouched:\n%q", got) + } + got := colourDeletePermanently(plain, true) + if !strings.Contains(got, "\x1b[") { + t.Errorf("colour=true should add an escape sequence:\n%q", got) + } + if !strings.Contains(got, "DELETE permanently") { + t.Errorf("colour=true should not remove the marker text itself:\n%q", got) + } +} diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go index 0ba82bc..68f9eda 100644 --- a/cmd/krino/sort.go +++ b/cmd/krino/sort.go @@ -3,34 +3,59 @@ package main import ( + "bytes" "context" "encoding/json" + "errors" "fmt" "io" "os" + "os/signal" "sort" "strings" + "syscall" "unicode/utf8" + "golang.org/x/term" + "krino/internal/engine" + "krino/internal/journal" + "krino/internal/lock" "krino/internal/plan" "krino/internal/scan" + "krino/internal/tui" "krino/internal/xdg" ) -// cmdSort plans and applies the included directories. Only -n (dry run) is -// implemented; applying arrives in plan 4. +// stdin is os.Stdin, threaded through this seam rather than referenced +// directly: cmdSort's terminal check, installSignalHandler and reviewDir +// all read it, and a test must never depend on what the ambient test +// binary's stdin happens to be (fix round 2026-09-12/item 4). If it were +// ever a real terminal, code that only worked by assuming otherwise would +// fall through to the interactive prompt and block the test suite on a +// keypress - the same kind of hang the lock-cancellation test was built to +// never risk. Tests point this at something guaranteed non-terminal +// (commands_test.go's home helper) instead of relying on a claim about +// what go test does with stdin. +var stdin = os.Stdin + +// zeroOutcome is the per-directory outcome line for a directory that had +// nothing applied to it - either because nothing was actionable, or +// because the user chose [s] or [q] - so the same wording is not retyped +// (and cannot drift) across the three places it applies. +const zeroOutcome = "0 applied · 0 failed · 0 declined" + +// cmdSort plans and, from Task 7, applies the included directories: flags +// are checked before any config is read, a bad config stops the whole run +// before scanning (spec §11), and one journal.Writer, run id and +// plan.Claims cover every directory in the run. See docs/design.md +// §8.2-§8.4 and §11 for the flow this follows. func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { if g.yes && g.dry { return usageError(stderr, "-y and -n cannot be used together") } if g.json && !g.dry { - fmt.Fprintln(stderr, "krino: --json is not implemented yet") - return 2 - } - if !g.dry { - fmt.Fprintln(stderr, "krino: applying files is not implemented yet; use -n to see what would happen") - return 2 + return usageError(stderr, "--json is only valid with -n") } e, errs := engine.Load(mainFile(g), names...) @@ -39,6 +64,50 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { return 2 } + // Spec §8.4: with neither -y nor -n, krino asks; asking a non-terminal + // stdin would just hang (or read garbage), so it refuses instead. + if !g.yes && !g.dry && !term.IsTerminal(int(stdin.Fd())) { + return usageError(stderr, "refusing to prompt: stdin is not a terminal (use -y or -n)") + } + + // Ruling 5: internal/tui deliberately does not trap signals - a package + // that installs process-wide handlers as a side effect of reading one + // key would surprise every caller. It lands here because cmdSort must + // install one anyway: spec §11 says Ctrl-C finishes the current step, + // logs it, and stops, which means the ctx passed to Apply below must be + // cancelled on SIGINT. The same handler also restores the terminal on + // SIGTERM, which - unlike a keyboard Ctrl-C during tui.ReadKey's raw + // read (ISIG is off, so that never even reaches us as a signal) - can + // land mid-read with no defer left to run. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + stopSignals := installSignalHandler(cancel) + defer stopSignals() + + // Ruling 2: journal.Open creates $XDG_STATE_HOME/krino/ and an empty + // krino.log as a side effect of merely being called, so a dry run must + // never call it at all - not open it and clean up afterwards. -n is + // known from the flags before the loop starts, so the gate is exactly + // that, nothing per-directory. One Writer and one run id cover every + // directory in the run (Task 5's undo depends on a single run id + // spanning all of them). + var j *journal.Writer + var run string + if !g.dry { + var err error + j, err = journal.Open(e.Config.LogFile()) + if err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + defer func() { + if cerr := j.Close(); cerr != nil { + fmt.Fprintf(stderr, "krino: %v\n", cerr) + } + }() + run = journal.NewRunID(e.Now()) + } + exit := 0 printed := false jsonDirs := []plan.JSONDir{} // never nil: the document's "dirs" must marshal as [], not null @@ -47,36 +116,154 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { // same destination resolve the collision at planning time instead of // each independently believing it owns that path. claims := plan.NewClaims() + for _, d := range e.Dirs { - if fi, err := os.Stat(d.Root); err != nil || !fi.IsDir() { - fmt.Fprintf(stderr, "krino: skipping %s: %s is not a directory\n", d.Name, xdg.Abbrev(d.Root)) - exit = 1 - continue - } - dp, err := e.Plan(context.Background(), d, claims) + // Spec §3/§11: a second krino on the same directory waits for the + // lock, or fails immediately with -y, so a cron job never piles up + // behind a stuck run. lock.Acquire takes ctx precisely so that wait + // is not unbounded in practice (fix round 2026-09-12/item 1): a + // signal cancels it and Acquire returns ctx.Err() promptly instead + // of polling forever. + l, err := lock.Acquire(ctx, e.Config.LockFile(d.Name), !g.yes) if err != nil { - fmt.Fprintf(stderr, "krino: skipping %s: %v\n", d.Name, err) + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + // Interrupted while waiting for the lock: an interrupt, not + // a failure - the ctx.Err() check at the end of this + // function already turns this into exit 130, and nothing + // further should even be attempted. + break + } + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, err) exit = 1 continue } - if !g.json { - if printed { - fmt.Fprintln(stdout) + + quit := func() bool { + defer func() { + if rerr := l.Release(); rerr != nil { + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, rerr) + } + }() + + if fi, err := os.Stat(d.Root); err != nil || !fi.IsDir() { + fmt.Fprintf(stderr, "krino: skipping %s: %s is not a directory\n", d.Name, xdg.Abbrev(d.Root)) + exit = 1 + return false } - printed = true - fmt.Fprintf(stdout, "krino: %s %s\n", d.Name, xdg.Abbrev(d.Root)) - } - // C3: directory-level warnings go to stderr after the header - // line above, not before it, so on a terminal they read as - // describing the directory just named instead of floating above it. - for _, w := range dp.Result.Warnings { - fmt.Fprintf(stderr, "krino: %s: %s\n", d.Name, w) - } - if g.json { - jsonDirs = append(jsonDirs, plan.NewJSONDir(d.Name, d.Root, dp.Chains, dp.Result.Warnings)) - continue + dp, err := e.Plan(ctx, d, claims) + if err != nil { + fmt.Fprintf(stderr, "krino: skipping %s: %v\n", d.Name, err) + exit = 1 + return false + } + + if !g.json { + if printed { + fmt.Fprintln(stdout) + } + printed = true + fmt.Fprintf(stdout, "krino: %s %s\n", d.Name, xdg.Abbrev(d.Root)) + } + // C3: directory-level warnings go to stderr after the header + // line above, not before it, so on a terminal they read as + // describing the directory just named instead of floating above it. + for _, w := range dp.Result.Warnings { + fmt.Fprintf(stderr, "krino: %s: %s\n", d.Name, w) + } + if g.json { + // --json is only ever reached with -n (checked above), and + // Ruling 7 is explicit that JSON must never be paged, so + // this returns before any of the paging/review code below. + jsonDirs = append(jsonDirs, plan.NewJSONDir(d.Name, d.Root, dp.Chains, dp.Result.Warnings)) + return false + } + + // Ruling 6: the plan goes through tui.Page - taller than the + // terminal, it is shown through $PAGER and the prompt follows + // once the pager exits (spec §8.2) - for -n as much as for the + // interactive and -y paths; a dry run that scrolls 200 files + // off the top of the terminal is exactly the case the pager + // exists for. printPlan is reused as-is (render.go), never + // re-rendered here. + var buf bytes.Buffer + printPlan(&buf, dp, g.verbose) + text := colourDeletePermanently(buf.String(), tui.Colour(stdout)) + if err := tui.Page(stdout, text); err != nil { + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, err) + exit = 1 + return false + } + + if g.dry { + return false + } + + actionable := actionableChains(dp.Chains) + if len(actionable) == 0 { + // Nothing to decide: neither -y nor the interactive menu + // has anything useful to do here, so neither is asked. + fmt.Fprintln(stdout, zeroOutcome) + return false + } + + var approved map[string]bool + var action rune + if g.yes { + approved, action = approveAll(actionable), 'a' + } else { + var rerr error + approved, action, rerr = reviewDir(stdout, actionable, d.Root) + if rerr != nil { + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, rerr) + exit = 1 + return false + } + } + + switch action { + case 's': + // Spec §8.2: [s] applies nothing in this directory and + // moves on - no Apply call at all, so nothing is logged + // for it either (Ruling 1: this is a chosen outcome, not a + // failure, and must not set exit 1). + fmt.Fprintln(stdout, zeroOutcome) + return false + case 'q': + // Spec §8.2: [q] stops krino; directories already applied + // this run stay applied. Nothing is applied here either, + // and no further directory is even planned. + fmt.Fprintln(stdout, zeroOutcome) + return true + } + + res, aerr := e.Apply(ctx, dp, approved, j, run) + if aerr != nil { + if errors.Is(aerr, context.Canceled) || errors.Is(aerr, context.DeadlineExceeded) { + // Interrupted mid-apply (fix round 2026-09-12/item 2): + // treated exactly like the cancelled lock wait above - + // not a failure ("context canceled" is a Go-ism, not + // something to show a user who just pressed Ctrl-C), + // and no further directory is even attempted. The + // ctx.Err() check at the end of this function already + // turns this into exit 130. + return true + } + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, aerr) + exit = 1 + return false + } + fmt.Fprintf(stdout, "%d applied · %d failed · %d declined\n", res.Applied, res.Failed, res.Declined) + // Ruling 1: only an actual step failure makes the run exit 1 + // here - a directory the user declined or skipped must not. + if res.Failed > 0 { + exit = 1 + } + return false + }() + + if quit { + break } - printPlan(stdout, dp, g.verbose) } if g.json { @@ -90,9 +277,107 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { stdout.Write(b) fmt.Fprintln(stdout) } + + // Spec §11: 130 interrupted takes priority over whatever exit already + // accumulated - ctx is only ever cancelled by installSignalHandler, and + // package main's own cancel() (deferred above) has not run yet here. + if ctx.Err() != nil { + return 130 + } return exit } +// actionableChains returns the chains of dp.Chains that have at least one +// step that will actually run (chainActing, render.go) - fix wave item 4 / +// Minor 5: this used to be a separate len(c.Steps) > 0 check, which +// disagreed with render.go's countActing over a chain every one of whose +// steps is skipped, so a directory could report "0 to act on" and then +// still offer such a chain for approval. Converged on chainActing, this is +// now also stricter than the filter engine.Apply's own forward-path loop +// applies (internal/engine/apply.go's Apply, still len(c.Steps) > 0): an +// all-skipped chain is simply never a candidate for approval here, so it +// can never reach Apply with approved == true, and Apply's own loop - +// unchanged - logs it as declined exactly like any other file this review +// never approved (tallyFile then counts it there, not as a fall-through). +func actionableChains(chains []plan.Chain) []plan.Chain { + var out []plan.Chain + for _, c := range chains { + if chainActing(c) { + out = append(out, c) + } + } + return out +} + +// installSignalHandler arranges for SIGINT and SIGTERM to cancel cancel +// and, if stdin is a terminal, restore it to the state it was in when this +// was called (Ruling 5). The returned func stops the handler and must be +// called once the run is over, or its goroutine and signal registration +// outlive cmdSort. +// +// Fix round 2026-09-12/item 2: the handler loops rather than servicing one +// signal and exiting. A single-shot select left signal.Notify's +// registration in place (which suppresses Go's default terminate) with no +// goroutine left reading the channel, so a second Ctrl-C landed in the +// buffered channel unread and a third was dropped outright - together with +// lock.Acquire's own fix, that made a run waiting on a held lock ignore +// every Ctrl-C and every SIGTERM forever, with no escape but another +// shell's kill -9. Looping fixes the common case (ctx cancellation reaches +// something that is actually checking it, e.g. a waiting lock.Acquire or +// Apply between files) and the second signal is also the user's guarantee +// of an exit even when it does not: by then a clean shutdown has already +// been asked for once and not delivered, so it restores the terminal once +// more (harmless if already restored) and exits immediately with the same +// 130 spec §11 already uses for "interrupted". +func installSignalHandler(cancel context.CancelFunc) func() { + fd := int(stdin.Fd()) + var saved *term.State + if term.IsTerminal(fd) { + saved, _ = term.GetState(fd) // best-effort: nothing to restore if this fails + } + + sig := make(chan os.Signal, 1) + signal.Notify(sig, os.Interrupt, syscall.SIGTERM) + done := make(chan struct{}) + go func() { + signals := 0 + for { + select { + case <-sig: + cancel() + if saved != nil { + term.Restore(fd, saved) + } + signals++ + if signals >= 2 { + // Deliberate exception to "the lock is released on + // every path" (fix round 2026-09-12/item 3, by + // design, documented on review): every deferred + // cleanup in cmdSort - including the held directory's + // lock.Release - is skipped here. That is intentional: + // this is the user's escape hatch when a clean + // shutdown was already asked for once (the first + // signal) and not delivered, so trying to unwind + // cleanly a second time is exactly what would make the + // hatch unreliable. It is safe to skip that unwind: + // journal.Append flushes each line as it writes, so + // nothing buffered is lost by exiting immediately, and + // an abandoned lock file is reclaimed automatically by + // Task 4's stale-pid takeover the next time anything + // tries to acquire it (lock.go's tryAcquire). + os.Exit(130) + } + case <-done: + return + } + } + }() + return func() { + signal.Stop(sig) + close(done) + } +} + // warnLine is one file's warning, for the warnings section. type warnLine struct { rel string @@ -235,14 +520,7 @@ func chainOutcomes(chains []plan.Chain) (excluded, allSkipped int) { excluded++ continue } - acting := false - for _, s := range c.Steps { - if s.Skip == "" { - acting = true - break - } - } - if !acting { + if !chainActing(c) { allSkipped++ } } diff --git a/cmd/krino/sort_test.go b/cmd/krino/sort_test.go index a646503..e482f44 100644 --- a/cmd/krino/sort_test.go +++ b/cmd/krino/sort_test.go @@ -2,7 +2,16 @@ package main -import "testing" +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "krino/internal/plan" + "krino/internal/scan" +) // TestRelWidthAndPadCellCountRunes: C4. relWidth and padCell must measure // column width in runes, not bytes, or a name carrying diacritics @@ -21,3 +30,83 @@ func TestRelWidthAndPadCellCountRunes(t *testing.T) { t.Fatalf("padCell(%q, 9) = %q, want %q (already at width: no padding)", "próba.txt", got, want) } } + +// TestActionableChainsAgreesWithCountActing is fix wave item 4 / Minor 5: +// countActing (render.go) and actionableChains used to disagree over a +// chain every one of whose steps is skipped (len(Steps) > 0, but every +// step's own Skip is set) - countActing already excluded it from "to act +// on", while actionableChains's own len(Steps) > 0 check still offered it +// for approval, so a directory could print "N scanned · 0 to act on" and +// then still ask the user to approve a file it had just said there were +// none of. Converged on chainActing (render.go), both must now agree. +func TestActionableChainsAgreesWithCountActing(t *testing.T) { + chains := []plan.Chain{ + {File: scan.File{Rel: "a.txt"}, Steps: []plan.Step{{Kind: plan.Move, Skip: "target exists"}}}, + {File: scan.File{Rel: "b.txt"}, Steps: []plan.Step{{Kind: plan.Move, Dst: "/r/W/b.txt"}}}, + } + if got := countActing(chains); got != 1 { + t.Errorf("countActing = %d, want 1 (a.txt is all-skipped)", got) + } + actionable := actionableChains(chains) + if len(actionable) != 1 || actionable[0].File.Rel != "b.txt" { + t.Errorf("actionableChains = %+v, want only b.txt - an all-skipped chain must never be offered for approval", actionable) + } +} + +// TestAllSkippedDirectoryReportsZeroAndLogsNothing is fix wave item 4 / +// Minor 5 and 6, end to end. Before the fix: a directory whose one file +// matches a rule under (on-conflict skip) - so its single step's own Skip +// is set ("target exists") - printed "0 to act on" (countActing) and then, +// with -y, still ran that chain through Apply anyway (actionableChains' +// own len(Steps) > 0 check approved it regardless), logging a +// run-start/run-end pair holding only a "skipped" entry while the outcome +// line read "0 applied · 0 failed · 0 declined" for a file that had just +// been silently processed. After the fix, the chain is never offered for +// approval, Apply is never even called for this directory, and the journal +// gains nothing at all. +func TestAllSkippedDirectoryReportsZeroAndLogsNothing(t *testing.T) { + h := home(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(filepath.Join(dl, "Out"), 0o755); err != nil { + t.Fatal(err) + } + old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for _, p := range []string{filepath.Join(dl, "a.txt"), filepath.Join(dl, "Out", "a.txt")} { + if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(p, old, old); err != nil { + t.Fatal(err) + } + } + if code, _, errOut := runCLI(t, "init"); code != 0 { + t.Fatal(errOut) + } + if code, _, errOut := runCLI(t, "new", "dl", dl); code != 0 { + t.Fatal(errOut) + } + rules := "(path \"~/dl\")\n(min-age 0s)\n(on-conflict skip)\n(rule \"r\" (when (type text)) (move \"Out\"))\n" + if err := os.WriteFile(filepath.Join(h, ".config", "krino", "dirs", "dl.conf"), []byte(rules), 0o644); err != nil { + t.Fatal(err) + } + + code, out, errOut := runCLI(t, "-y") + if code != 0 { + t.Fatalf("run: %d %s", code, errOut) + } + if !strings.Contains(out, "1 scanned · 0 to act on") { + t.Errorf("output = %q, want \"0 to act on\"", out) + } + if !strings.Contains(out, zeroOutcome) { + t.Errorf("output = %q, want the honest zero outcome %q", out, zeroOutcome) + } + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("reading the journal: %v", err) + } + if len(data) != 0 { + t.Errorf("journal gained entries for a directory with nothing to act on:\n%s", data) + } +} diff --git a/cmd/krino/undo.go b/cmd/krino/undo.go new file mode 100644 index 0000000..7ffe428 --- /dev/null +++ b/cmd/krino/undo.go @@ -0,0 +1,533 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + "sort" + "strconv" + "strings" + + "golang.org/x/term" + + "krino/internal/config" + "krino/internal/engine" + "krino/internal/journal" + "krino/internal/lock" + "krino/internal/tui" + "krino/internal/xdg" +) + +func init() { commands["undo"] = cmdUndo } + +// cmdUndo reverses a run: the one named on the command line, or (spec §10) +// the most recent one otherwise. Since the newest run in the log can never +// itself be marked Undone - that would require a still-later run to have +// reversed it - "the most recent run" and "the most recent run that has not +// been undone" are the same run in every case, including the one this +// task's own test exercises: undoing an undo run a second time with no RUN +// argument targets that very undo run, which PlanUndo then refuses by name. +// +// Undo builds a plan like any other, shown and approved the same way (spec +// §10) - reviewUndoDir/-Files/-PerFile below are undo's own counterpart to +// review.go's reviewChains/reviewPerFile, not a call into them: an undo plan +// is []engine.UndoFile, which can span several directories in one flat +// list, so two files from different directories can share the same Rel and +// approval here is keyed by index rather than by name. +func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int { + fs := flagSet("undo", g) + fs.BoolVar(&g.yes, "y", false, "") + fs.BoolVar(&g.dry, "n", false, "") + if code, ok := parse(fs, args, stdout, stderr); !ok { + return code + } + if g.yes && g.dry { + return usageError(stderr, "-y and -n cannot be used together") + } + rest := fs.Args() + if len(rest) > 1 { + return usageError(stderr, "usage: krino undo [RUN]") + } + + e, errs := engine.Load(mainFile(g)) + if len(errs) > 0 { + printDiags(stderr, errs) + return 2 + } + + // Spec §8.4/§10: with neither -y nor -n, krino asks; a non-terminal + // stdin would just hang, so it refuses instead - the same check + // cmdSort makes before it ever shows a plan. + if !g.yes && !g.dry && !term.IsTerminal(int(stdin.Fd())) { + return usageError(stderr, "refusing to prompt: stdin is not a terminal (use -y or -n)") + } + + // Installed here, before any paging or review, not just around + // ApplyUndo: Ruling 5 (Task 7) is that SIGTERM landing between + // keystrokes or during the pager needs the terminal restored, and that + // window starts as soon as this command might show something on a + // terminal. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + stopSignals := installSignalHandler(cancel) + defer stopSignals() + + runID := "" + if len(rest) == 1 { + runID = rest[0] + } else { + runs, err := e.Runs(1) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + fmt.Fprintln(stdout, "nothing logged yet; nothing to undo") + return 0 + } + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + if len(runs) == 0 { + fmt.Fprintln(stdout, "nothing logged yet; nothing to undo") + return 0 + } + runID = runs[0].ID + } + + // PlanUndo only reads the log; nothing is touched yet (spec §10), which + // is what makes it safe to call before any lock is taken. + up, err := e.PlanUndo(runID) + if err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + + // Fix round 2026-09-12 (widened per the coordinator's follow-up + // ruling): an undo moves files just as an apply does, so it needs + // cmdSort's same per-directory guard (spec §11: a second krino on the + // same directory waits for the lock, or fails immediately with -y), + // held across the SAME window cmdSort holds its own lock across - the + // plan display and the review, not just the apply. Failing before the + // plan is even shown is strictly kinder than making the user review + // (potentially hundreds of files) only to be refused afterward, and it + // means a plan actually reviewed cannot go stale under the reader's + // eyes from another krino moving those same files mid-review. -n never + // reaches this: it changes nothing, so it takes no lock either. One + // undo run can span several directories (UndoFile.Dir is per file), so + // every distinct one up.Files touches is locked, in a fixed (sorted) + // order, and released on every path below - including [s]/[q], every + // early return, and a later ApplyUndo failure - by the single defer + // right after acquisition. + var locks []*lock.Lock + if !g.dry { + locks, err = acquireUndoLocks(ctx, e.Config, undoDirNames(up.Files), !g.yes) + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return 130 + } + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + defer func() { + for _, rerr := range releaseUndoLocks(locks) { + fmt.Fprintf(stderr, "krino: %v\n", rerr) + } + }() + } + + fmt.Fprintf(stdout, "krino: undo %s\n", up.Run) + var buf bytes.Buffer + printUndoPlan(&buf, up) + text := colourRefused(buf.String(), tui.Colour(stdout)) + // Ruling 6 (Task 7), carried over: the plan goes through tui.Page for + // -n as much as for -y and the interactive path. + if err := tui.Page(stdout, text); err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + + if g.dry { + return 0 + } + + if undoActionableCount(up.Files) == 0 { + // Every file was refused, or the run touched none at all: neither + // -y nor the interactive menu has anything useful to do (mirrors + // cmdSort's identical check before it ever asks). + fmt.Fprintln(stdout, zeroOutcome) + return 0 + } + + var approved map[int]bool + var action rune + if g.yes { + approved, action = approveAllUndo(up.Files), 'a' + } else { + var rerr error + approved, action, rerr = reviewUndoDir(stdout, up.Files) + if rerr != nil { + fmt.Fprintf(stderr, "krino: %v\n", rerr) + return 1 + } + } + + if action == 's' || action == 'q' { + fmt.Fprintln(stdout, zeroOutcome) + return 0 + } + + toApply := finalizeUndoPlan(up, approved) + + // Ruling 2 (Task 7), carried over: journal.Open creates the state + // directory and the log file as a side effect of merely being called, + // so it is opened only once we know something will actually be + // applied - never for -n (returned above), and not merely because -y + // or a review session ran, unlike cmdSort's own eager-open (which opens + // before it knows whether anything is actionable, a difference forced + // by cmdSort not yet having a plan to inspect at that point in its + // flow; undo already does, so it opens later, and never opens if + // undoActionableCount was 0 or the user chose [s]/[q] above). + j, err := journal.Open(e.Config.LogFile()) + if err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + defer func() { + if cerr := j.Close(); cerr != nil { + fmt.Fprintf(stderr, "krino: %v\n", cerr) + } + }() + run := journal.NewRunID(e.Now()) + + res, aerr := e.ApplyUndo(ctx, toApply, j, run) + if aerr != nil { + if errors.Is(aerr, context.Canceled) || errors.Is(aerr, context.DeadlineExceeded) { + // Interrupted mid-apply: the ctx.Err() check below turns this + // into exit 130, same as cmdSort. + return 130 + } + fmt.Fprintf(stderr, "krino: %v\n", aerr) + return 1 + } + fmt.Fprintf(stdout, "%d applied · %d failed · %d declined\n", res.Applied, res.Failed, res.Declined) + + if ctx.Err() != nil { + return 130 + } + if res.Failed > 0 { + return 1 + } + return 0 +} + +// undoActionableCount counts the files in files that PlanUndo has not +// already refused - the same "is there anything to even ask about" gate +// cmdSort's actionableChains serves for a sort plan. +func undoActionableCount(files []engine.UndoFile) int { + n := 0 + for _, f := range files { + if f.Refused == "" { + n++ + } + } + return n +} + +// undoDirNames returns the distinct directory names an undo plan's files +// touch, sorted: a fixed order so two processes each locking a plan that +// shares more than one directory always acquire them in the same sequence, +// the standard way to avoid a lock-order deadlock. UndoFile.Dir is the +// journal's own `dir` column - the directory's config NAME (e.g. "dl"), not +// a filesystem path - which is exactly what config.Config.LockFile takes. +func undoDirNames(files []engine.UndoFile) []string { + seen := map[string]bool{} + var out []string + for _, f := range files { + if f.Dir != "" && !seen[f.Dir] { + seen[f.Dir] = true + out = append(out, f.Dir) + } + } + sort.Strings(out) + return out +} + +// acquireUndoLocks takes the lock for every name in dirs, in order, +// mirroring cmdSort's per-directory lock.Acquire call. If any acquisition +// fails - held with wait false, or ctx cancelled while waiting - every lock +// already taken is released before returning, so a partial lock set is +// never left held while the caller reports the error and stops. +func acquireUndoLocks(ctx context.Context, cfg *config.Config, dirs []string, wait bool) ([]*lock.Lock, error) { + locks := make([]*lock.Lock, 0, len(dirs)) + for _, name := range dirs { + l, err := lock.Acquire(ctx, cfg.LockFile(name), wait) + if err != nil { + releaseUndoLocks(locks) + return nil, fmt.Errorf("%s: %w", name, err) + } + locks = append(locks, l) + } + return locks, nil +} + +// releaseUndoLocks releases every lock in locks and returns any release +// errors, one lock's failure never stopping the rest from being released - +// the same "release on every path" guarantee cmdSort gives its own single +// lock, extended to however many an undo plan needed. +func releaseUndoLocks(locks []*lock.Lock) []error { + var errs []error + for _, l := range locks { + if err := l.Release(); err != nil { + errs = append(errs, err) + } + } + return errs +} + +// finalizeUndoPlan builds the *engine.UndoPlan ApplyUndo actually runs, +// preserving up.Files' own order: every refused file rides along unchanged +// (ApplyUndo declines these itself, silently, exactly as it already does +// when handed the unfiltered plan - spec §10's refusal is not this task's +// to make noisier); every actionable file approved marks true rides along +// unchanged too. A file the user said no to, or left unmarked when [d] or +// [q] cut a per-file review short, is not dropped - fix round 2026-09-12, +// item 2 of Task 8's review: spec §9 says a declined file is logged even +// though nothing happens to it, the same as the forward path already does, +// so it is kept with Declined set, which tells ApplyUndo to log its steps +// as declined rather than reverse them. +func finalizeUndoPlan(up *engine.UndoPlan, approved map[int]bool) *engine.UndoPlan { + out := &engine.UndoPlan{Run: up.Run} + for i, f := range up.Files { + if f.Refused == "" && !approved[i] { + f.Declined = true + } + out.Files = append(out.Files, f) + } + return out +} + +// reviewUndoDir drives the interactive review over the real terminal, +// mirroring review.go's reviewDir for the forward path. +func reviewUndoDir(out io.Writer, files []engine.UndoFile) (map[int]bool, rune, error) { + return reviewUndoFiles(keyReader{stdin}, out, files) +} + +// reviewUndoFiles is spec §10's approval flow for an undo plan: the +// top-level +// +// [a] apply all [c] choose per file [s] skip [q] quit +// +// menu, and, for [c], the per-file +// +// [y] yes [n] no [a] yes to this and all remaining [d] done, apply chosen so far [q] quit, apply nothing +// +// prompt - the same shape as review.go's reviewChains/reviewPerFile, over a +// different plan shape (approved is keyed by index into files, not by +// name). action is always one of 'a', 'c', 's' or 'q', with the same [q] +// folding rule reviewChains uses: a [c] session's own [q] becomes the same +// top-level 'q', and approved is emptied to match. +func reviewUndoFiles(in io.Reader, out io.Writer, files []engine.UndoFile) (map[int]bool, rune, error) { + fmt.Fprint(out, "\n[a] apply all [c] choose per file [s] skip [q] quit\n") + for { + key, err := readKey(in) + if err != nil { + return nil, 0, err + } + switch key { + case 'a': + return approveAllUndo(files), 'a', nil + case 's': + return map[int]bool{}, 's', nil + case 'q': + return map[int]bool{}, 'q', nil + case 'c': + approved, quit, err := reviewUndoPerFile(in, out, files) + if err != nil { + return nil, 0, err + } + if quit { + return map[int]bool{}, 'q', nil + } + return approved, 'c', nil + default: + fmt.Fprintf(out, "%q is not a, c, s or q\n", key) + } + } +} + +// reviewUndoPerFile is the per-file half of reviewUndoFiles. A file already +// refused at planning time is never asked about - spec §10 shows it with +// its reason and reverses nothing of it regardless of anything chosen here +// - but it still gets its own [i/N] line, so the numbering accounts for +// every file in the plan, not just the reversible ones. +func reviewUndoPerFile(in io.Reader, out io.Writer, files []engine.UndoFile) (approved map[int]bool, quit bool, err error) { + approved = map[int]bool{} + yesRest := false + for i, f := range files { + fmt.Fprintf(out, "\n[%d/%d] %s/%s\n", i+1, len(files), f.Dir, f.File) + for _, s := range f.Steps { + fmt.Fprintf(out, " %s\n", undoActionCell(s)) + } + if f.Refused != "" { + fmt.Fprintf(out, " refused: %s\n", f.Refused) + continue + } + if yesRest { + approved[i] = true + continue + } + + fmt.Fprint(out, " [y] yes [n] no [a] yes to this and all remaining [d] done, apply chosen so far [q] quit, apply nothing\n") + for { + key, kerr := readKey(in) + if kerr != nil { + return nil, false, kerr + } + switch key { + case 'y': + approved[i] = true + case 'n': + // leave unapproved + case 'a': + approved[i] = true + yesRest = true + case 'd': + return approved, false, nil + case 'q': + return nil, true, nil + default: + fmt.Fprintf(out, "%q is not y, n, a, d or q\n", key) + continue + } + break + } + } + return approved, false, nil +} + +// approveAllUndo approves every reversible file in files by index - [a] +// apply all, at either the top level or mid per-file review. A refused file +// is never marked true: nothing would happen to it anyway (ApplyUndo skips +// it unconditionally), and leaving it unmarked here keeps this function's +// contract simple - "true means ask ApplyUndo to reverse it" - rather than +// also being the thing that decides refused files ride along regardless +// (finalizeUndoPlan does that, independently of this map). +func approveAllUndo(files []engine.UndoFile) map[int]bool { + approved := make(map[int]bool, len(files)) + for i, f := range files { + if f.Refused == "" { + approved[i] = true + } + } + return approved +} + +// undoStepWidth is the widest undo action word (padCell aligns every +// arrow), matching render.go's actionKindWidth for the forward table. +const undoStepWidth = len("undo-displace") + +// undoActionCell renders one undo step: its own refusal reason when it has +// one (a sibling directory not yet empty for undo-mkdir - spec §10's one +// case where a step's own failure does not refuse its whole file), the +// directory removed for undo-mkdir (no destination to show), the file being +// trashed for undo-copy (fix wave item 3: its Dst is deliberately empty - +// trash.Put only chooses the entry name at execution time - so this is the +// one action with no path to point an arrow at; before this fix the cell +// rendered as a bare "undo-copy → ", the plan's one row that said +// nothing about what it would do to the user's file), or an arrow to where +// the step puts the file back, ~-abbreviated - undo has no single root the +// way a sort plan does (one run can span several directories), so there is +// no root-relative form to render here the way actionCell has. +func undoActionCell(s engine.UndoStep) string { + if s.Refused != "" { + return padCell(s.Action, undoStepWidth) + " refused: " + s.Refused + } + switch s.Action { + case "undo-mkdir": + return padCell(s.Action, undoStepWidth) + " " + xdg.Abbrev(s.Src) + case "undo-copy": + return padCell(s.Action, undoStepWidth) + " " + xdg.Abbrev(s.Src) + " → trash" + } + return padCell(s.Action, undoStepWidth) + " → " + xdg.Abbrev(s.Dst) +} + +// printUndoPlan renders an undo plan the way krino undo shows it, below the +// "krino: undo RUN" header line cmdUndo has already written: a counts line, +// then the numbered table, mirroring printPlan's shape for a sort plan +// (spec §10: "shown ... the same way"). +func printUndoPlan(w io.Writer, up *engine.UndoPlan) { + actionable := undoActionableCount(up.Files) + fmt.Fprintf(w, "%d files · %d to reverse · %d refused\n", len(up.Files), actionable, len(up.Files)-actionable) + if len(up.Files) == 0 { + return + } + fmt.Fprintln(w) + printUndoTable(w, up.Files) +} + +// undoRow is one line of the undo table: a file's first step (num and file +// set) or a continuation line (both blank), the same layout planRow uses +// for a sort plan. +type undoRow struct { + num, file, action string +} + +// undoRows turns files into table rows. A refused file gets exactly one +// row - there is nothing to reverse, so no per-step continuation lines - +// showing its reason in place of any step. +func undoRows(files []engine.UndoFile) []undoRow { + var rows []undoRow + for i, f := range files { + label := f.Dir + "/" + f.File + if f.Refused != "" { + rows = append(rows, undoRow{num: strconv.Itoa(i + 1), file: label, action: "refused: " + f.Refused}) + continue + } + for j, s := range f.Steps { + row := undoRow{action: undoActionCell(s)} + if j == 0 { + row.num = strconv.Itoa(i + 1) + row.file = label + } + rows = append(rows, row) + } + } + return rows +} + +// printUndoTable prints rows in the same #, file, action layout +// printPlanTable uses for a sort plan, reusing its column-width helpers +// (relWidth/padCell/padLeft/colWidth, render.go) rather than re-deriving +// them. +func printUndoTable(w io.Writer, files []engine.UndoFile) { + rows := undoRows(files) + nums := make([]string, len(rows)) + fls := make([]string, len(rows)) + for i, r := range rows { + nums[i], fls[i] = r.num, r.file + } + numW := colWidth(nums, 0) + fileW := relWidth(fls) + + fmt.Fprintf(w, " %s %s steps\n", padLeft("#", numW), padCell("file", fileW)) + for _, r := range rows { + fmt.Fprintf(w, " %s %s %s\n", padLeft(r.num, numW), padCell(r.file, fileW), r.action) + } +} + +// colourRefused highlights "refused:" in the terminal's own ANSI red (bold, +// slot 1 - never hex), the undo counterpart of sort.go's +// colourDeletePermanently: the one thing an undo plan singles out for +// attention is the file or step nothing will be reversed for. Same +// no-op-when-plain guarantee: with colour false this never touches the +// text, which is what keeps every escape byte out of a plan piped to a +// file or read by another tool. +func colourRefused(text string, colour bool) string { + if !colour { + return text + } + return strings.ReplaceAll(text, "refused:", "\x1b[1;31mrefused:\x1b[0m") +} diff --git a/docs/design.md b/docs/design.md index 092738a..42c691c 100644 --- a/docs/design.md +++ b/docs/design.md @@ -445,8 +445,10 @@ Reversals, last step first within each file: | delete permanent | none; reported as not undoable | always | If a reversal is refused, the earlier steps of that file's chain are not -reversed either, so no file is left half undone. Undo runs are logged like -any other run. +reversed either, so no file is left half undone. Removing a directory that is +not empty is the exception: it means another file still lives there, not that +the world changed under us, so that refusal is recorded and the rest of the +file's reversal proceeds. Undo runs are logged like any other run. ## 11. Command line @@ -2,4 +2,9 @@ module krino go 1.24.0 -require golang.org/x/text v0.34.0 +require ( + golang.org/x/term v0.38.0 + golang.org/x/text v0.34.0 +) + +require golang.org/x/sys v0.39.0 // indirect @@ -1,2 +1,6 @@ +golang.org/x/sys v0.39.0 h1:CvCKL8MeisomCi6qNZ+wbb0DN9E5AATixKsvNtMoMFk= +golang.org/x/sys v0.39.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= +golang.org/x/term v0.38.0 h1:PQ5pkm/rLO6HnxFR7N2lJHOZX6Kez5Y1gDSJla6jo7Q= +golang.org/x/term v0.38.0/go.mod h1:bSEAKrOT1W+VSu9TSCMtoGEOUcKxOKgl3LE5QEF/xVg= golang.org/x/text v0.34.0 h1:oL/Qq0Kdaqxa1KbNeMKwQq0reLCCaFtqu2eNuSeNHbk= golang.org/x/text v0.34.0/go.mod h1:homfLqTYRFyVYemLBFl5GgL/DWEiH5wcsQ5gSh1yziA= diff --git a/internal/apply/apply.go b/internal/apply/apply.go new file mode 100644 index 0000000..2cd52e4 --- /dev/null +++ b/internal/apply/apply.go @@ -0,0 +1,209 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +// Package apply is the executor: it carries out one file's plan.Chain, +// actually moving, copying, renaming, trashing or permanently deleting real +// files. See docs/design.md §7.2 for the mechanism each action follows and +// §7.4's last paragraph for the execution-time conflict re-check. +package apply + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "time" + + "krino/internal/plan" + "krino/internal/scan" + "krino/internal/trash" +) + +// StepResult is what happened to one step, in the order the executor ran +// them. +type StepResult struct { + Step plan.Step + Status string // "ok" | "failed" | "skipped" + Detail string // the failure, or why it was skipped + Dst string // where the file actually ended up (conflict names can change at execution time) + Size int64 // of the file at Dst afterwards + ModTime time.Time // of the file at Dst afterwards + Entry string // trash entry name, for Trash steps; "" otherwise + // DisplacedEntry is the trash entry name of the file this step + // displaced; "" when none. Entry and DisplacedEntry describe two + // different files: the one being acted on (Entry, only for a Trash-kind + // step), and the one that was in this step's way and had to be trashed + // first (DisplacedEntry, only when Displaces was set). Spec §7.4's + // overwrite policy and §9's "displace" action both depend on this name + // being recoverable — it is chosen inside trash.Put, so nothing + // downstream of Chain could otherwise re-derive it for undo. + DisplacedEntry string + Made []string // directories this step created, outermost first +} + +// Chain runs one file's steps in order and stops at the first failure, +// marking the rest skipped. It never touches a file whose size or mtime no +// longer matches what the plan recorded. +func Chain(c plan.Chain) []StepResult { + results := make([]StepResult, len(c.Steps)) + stopped := false + + for i, step := range c.Steps { + if stopped { + results[i] = StepResult{Step: step, Status: "skipped", Detail: "an earlier step in this chain failed"} + continue + } + if step.Skip != "" { + // Planning already decided this step will not run; it must not + // be attempted, so no pre-step check, no directory creation, no + // touching the file (spec: a step already marked Skip is + // reported, not attempted). + results[i] = StepResult{Step: step, Status: "skipped", Detail: step.Skip} + continue + } + if err := checkUnchanged(step.Src, c.File); err != nil { + results[i] = StepResult{Step: step, Status: "failed", Detail: err.Error()} + stopped = true + continue + } + + res := runStep(step) + results[i] = res + if res.Status == "failed" { + stopped = true + } + } + + return results +} + +// checkUnchanged is the guard that matters most: before every step, the +// source is stat'd and compared against the size and mtime the plan +// recorded for the whole file. A file rewritten or replaced between +// planning and applying must never be acted on. +func checkUnchanged(src string, f scan.File) error { + fi, err := os.Stat(src) + if err != nil { + return fmt.Errorf("changed since plan: %w", err) + } + if fi.Size() != f.Size || !fi.ModTime().Equal(f.ModTime) { + return errors.New("changed since plan") + } + return nil +} + +// runStep dispatches one already-checked, non-skipped step to the code that +// actually carries it out. +func runStep(step plan.Step) StepResult { + switch step.Kind { + case plan.Copy, plan.Move, plan.Rename: + return runFileStep(step) + case plan.Trash: + return runTrashStep(step) + case plan.DeletePermanent: + return runDeleteStep(step) + } + panic(fmt.Sprintf("apply: unknown plan.Kind %d", int(step.Kind))) +} + +// runFileStep carries out copy, move and rename. It re-checks the planned +// destination against the filesystem as it is now (spec §7.4): if something +// with Displaces set claims the file to trash first, that happens before +// anything else, and if the displace fails nothing further is attempted for +// this file. Otherwise, if the planned Dst now exists, the step moves to the +// next free stem_N.ext and records the real name in Dst rather than +// overwriting a file the plan never accounted for. Missing destination +// directories are created and recorded in Made, outermost first, whether or +// not the step that needed them goes on to succeed. +func runFileStep(step plan.Step) StepResult { + dst := step.Dst + var displacedEntry string + + if step.Displaces != "" { + // overwrite policy: the file already at dst must be trashed before + // this step's own destination name is used, so no free-name search + // applies here — the whole point of displacing was to clear this + // exact name. The entry name is captured regardless of what happens + // next in this step: spec §9 logs "displace" as its own action with + // its own line, independent of whether the move/copy/rename that + // needed the name then goes on to succeed, so every return below + // (failure included) carries it once trashing has succeeded. + entry, err := trash.Put(step.Displaces) + if err != nil { + return StepResult{Step: step, Status: "failed", Detail: "displacing the existing file: " + err.Error()} + } + displacedEntry = entry + } else if _, err := os.Lstat(dst); err == nil { + free, err := nextFreeName(dst) + if err != nil { + return StepResult{Step: step, Status: "failed", Detail: err.Error()} + } + dst = free + } else if !os.IsNotExist(err) { + return StepResult{Step: step, Status: "failed", Detail: err.Error()} + } + + made, err := mkdirAllTracked(filepath.Dir(dst)) + if err != nil { + return StepResult{Step: step, Status: "failed", Detail: err.Error(), Made: made, DisplacedEntry: displacedEntry} + } + + switch step.Kind { + case plan.Copy: + err = copyFile(step.Src, dst) + case plan.Move: + err = moveFile(step.Src, dst) + case plan.Rename: + err = os.Rename(step.Src, dst) + } + if err != nil { + return StepResult{Step: step, Status: "failed", Detail: err.Error(), Made: made, DisplacedEntry: displacedEntry} + } + + fi, err := os.Stat(dst) + if err != nil { + return StepResult{Step: step, Status: "failed", Detail: err.Error(), Made: made, DisplacedEntry: displacedEntry} + } + return StepResult{Step: step, Status: "ok", Dst: dst, Size: fi.Size(), ModTime: fi.ModTime(), Made: made, DisplacedEntry: displacedEntry} +} + +// runTrashStep carries out (delete): the file goes to the freedesktop.org +// Trash via trash.Put. A file on a different filesystem from the Trash is +// not trashed at all; the spec requires the failure to name the two ways +// forward, since otherwise the user has no path out of it. +// +// Dst, Size and ModTime describe the file at trash.Dir()/files/<entry>, +// even though plan.Step.Dst is always "" for a delete (there is nothing to +// compute or conflict-check at plan time). That is a deliberate reading of +// §9 rather than an oversight forced by the empty plan.Step.Dst: those +// journal columns describe the file at Dst after the step, and after a +// trash step the file genuinely lives there, so recording it is more +// useful than an empty column and stays greppable. It also cannot confuse +// undo: Task 5's refusal condition for reversing a trash step is "the +// entry is gone, or Src now exists" — it reads Entry and Src, never Dst. +func runTrashStep(step plan.Step) StepResult { + entry, err := trash.Put(step.Src) + if err != nil { + detail := err.Error() + if errors.Is(err, trash.ErrOtherFilesystem) { + detail += "; use (delete permanent) or a move instead" + } + return StepResult{Step: step, Status: "failed", Detail: detail} + } + + dst := filepath.Join(trash.Dir(), "files", entry) + var size int64 + var modTime time.Time + if fi, err := os.Stat(dst); err == nil { + size, modTime = fi.Size(), fi.ModTime() + } + return StepResult{Step: step, Status: "ok", Dst: dst, Size: size, ModTime: modTime, Entry: entry} +} + +// runDeleteStep carries out (delete permanent): a plain unlink, with no +// Trash and no way back. +func runDeleteStep(step plan.Step) StepResult { + if err := os.Remove(step.Src); err != nil { + return StepResult{Step: step, Status: "failed", Detail: err.Error()} + } + return StepResult{Step: step, Status: "ok"} +} diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go new file mode 100644 index 0000000..64b0176 --- /dev/null +++ b/internal/apply/apply_test.go @@ -0,0 +1,252 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package apply + +import ( + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + "krino/internal/plan" + "krino/internal/scan" + "krino/internal/trash" +) + +// chainFor builds a Chain whose File describes path as it is on disk now, so +// the executor's "changed since plan" check passes. +func chainFor(t *testing.T, root, rel string, steps ...plan.Step) plan.Chain { + t.Helper() + p := filepath.Join(root, rel) + fi, err := os.Stat(p) + if err != nil { + t.Fatal(err) + } + return plan.Chain{ + File: scan.File{Path: p, Rel: rel, Name: filepath.Base(rel), Size: fi.Size(), ModTime: fi.ModTime(), Mode: fi.Mode()}, + Steps: steps, + } +} + +func TestChainRunsStepsInOrder(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(root, "x.pdf"), "content", 0o644) + c := chainFor(t, root, "x.pdf", + plan.Step{Kind: plan.Copy, Rule: "backup", Src: filepath.Join(root, "x.pdf"), Dst: filepath.Join(root, "B", "x.pdf")}, + plan.Step{Kind: plan.Move, Rule: "acme", Src: filepath.Join(root, "x.pdf"), Dst: filepath.Join(root, "W", "x.pdf")}, + ) + got := Chain(c) + if len(got) != 2 || got[0].Status != "ok" || got[1].Status != "ok" { + t.Fatalf("results = %+v", got) + } + if b, err := os.ReadFile(filepath.Join(root, "B", "x.pdf")); err != nil || string(b) != "content" { + t.Errorf("the copy is missing: %q %v", b, err) + } + if b, err := os.ReadFile(filepath.Join(root, "W", "x.pdf")); err != nil || string(b) != "content" { + t.Errorf("the move did not arrive: %q %v", b, err) + } + if _, err := os.Stat(filepath.Join(root, "x.pdf")); !os.IsNotExist(err) { + t.Error("the original survived the move") + } + if len(got[0].Made) == 0 { + t.Error("the created directory was not recorded in Made") + } + if got[1].Size != int64(len("content")) { + t.Errorf("Size = %d, want the size at Dst afterwards", got[1].Size) + } +} + +// TestChainStopsWhenFileChanged is the guard that matters most: a file +// rewritten between planning and applying must not be acted on at all. +func TestChainStopsWhenFileChanged(t *testing.T) { + root := t.TempDir() + src := write(t, filepath.Join(root, "x.pdf"), "planned", 0o644) + c := chainFor(t, root, "x.pdf", + plan.Step{Kind: plan.Move, Rule: "a", Src: src, Dst: filepath.Join(root, "W", "x.pdf")}, + plan.Step{Kind: plan.Rename, Rule: "b", Src: filepath.Join(root, "W", "x.pdf"), Dst: filepath.Join(root, "W", "y.pdf")}, + ) + write(t, src, "rewritten since the plan was made", 0o644) + + got := Chain(c) + if got[0].Status != "failed" || !strings.Contains(got[0].Detail, "changed since plan") { + t.Fatalf("first step = %+v; want failed \"changed since plan\"", got[0]) + } + if got[1].Status != "skipped" { + t.Errorf("second step = %+v; want skipped after the failure", got[1]) + } + if b, _ := os.ReadFile(src); string(b) != "rewritten since the plan was made" { + t.Error("the changed file was modified anyway") + } +} + +func TestChainTrashAndPermanentDelete(t *testing.T) { + root := t.TempDir() + t.Setenv("HOME", root) + t.Setenv("XDG_DATA_HOME", filepath.Join(root, "share")) + t.Setenv("XDG_STATE_HOME", "") + t.Setenv("XDG_CONFIG_HOME", "") + t.Setenv("XDG_CACHE_HOME", "") + + gone := write(t, filepath.Join(root, "gone.pdf"), "trash me", 0o644) + c1 := chainFor(t, root, "gone.pdf", plan.Step{Kind: plan.Trash, Rule: "dups", Src: gone}) + r1 := Chain(c1) + if r1[0].Status != "ok" || r1[0].Entry == "" { + t.Fatalf("trash step = %+v; want ok with an Entry name", r1[0]) + } + if r1[0].DisplacedEntry != "" { + t.Errorf("DisplacedEntry = %q, want empty: a plain trash step displaces nothing", r1[0].DisplacedEntry) + } + if _, err := os.Stat(gone); !os.IsNotExist(err) { + t.Error("the trashed file is still in place") + } + + nuked := write(t, filepath.Join(root, "nuked.pdf"), "unlink me", 0o644) + c2 := chainFor(t, root, "nuked.pdf", plan.Step{Kind: plan.DeletePermanent, Rule: "old", Src: nuked}) + if r2 := Chain(c2); r2[0].Status != "ok" { + t.Fatalf("permanent delete = %+v", r2[0]) + } + if _, err := os.Stat(nuked); !os.IsNotExist(err) { + t.Error("the permanently deleted file is still in place") + } +} + +func TestChainReChecksConflictAtExecutionTime(t *testing.T) { + root := t.TempDir() + src := write(t, filepath.Join(root, "x.pdf"), "mine", 0o644) + dst := filepath.Join(root, "W", "x.pdf") + c := chainFor(t, root, "x.pdf", plan.Step{Kind: plan.Move, Rule: "a", Src: src, Dst: dst}) + // Something took the planned name between planning and applying. + write(t, dst, "someone else got here first", 0o644) + + got := Chain(c) + if got[0].Status != "ok" { + t.Fatalf("step = %+v", got[0]) + } + if got[0].Dst == dst { + t.Error("the executor overwrote a name that appeared after planning") + } + if !strings.HasSuffix(got[0].Dst, "x_1.pdf") { + t.Errorf("Dst = %q, want the next free name", got[0].Dst) + } + if b, _ := os.ReadFile(dst); string(b) != "someone else got here first" { + t.Error("the file that took the planned name was overwritten") + } +} + +func TestChainSkippedStepIsNotAttempted(t *testing.T) { + root := t.TempDir() + src := write(t, filepath.Join(root, "x.pdf"), "content", 0o644) + c := chainFor(t, root, "x.pdf", + plan.Step{Kind: plan.Move, Rule: "a", Src: src, Dst: filepath.Join(root, "W", "x.pdf"), Skip: "target exists"}, + ) + if got := Chain(c); got[0].Status != "skipped" || got[0].Detail != "target exists" { + t.Fatalf("result = %+v; want skipped carrying the planning reason", got[0]) + } + if _, err := os.Stat(filepath.Join(root, "W")); !os.IsNotExist(err) { + t.Error("a skipped step created its destination directory") + } + if _, err := os.Stat(src); err != nil { + t.Error("a skipped step moved the file anyway") + } + _ = time.Now +} + +// TestChainDisplacedFileRestoresFromDisplacedEntry is the fix-round-1 test: +// DisplacedEntry must be usable for undo, not merely present. It proves +// that by actually restoring the displaced file from the Trash and checking +// its content, not just that the field is non-empty. The displaced file +// sits at its own path, distinct from the step's own Dst: were the two the +// same (the ordinary overwrite shape), the mover's own file would already +// occupy that name by the time Restore ran, and Restore correctly refuses +// to land on an occupied path — this test isolates DisplacedEntry's own +// round-trip instead of also exercising that refusal. +func TestChainDisplacedFileRestoresFromDisplacedEntry(t *testing.T) { + root := t.TempDir() + t.Setenv("HOME", root) + t.Setenv("XDG_DATA_HOME", filepath.Join(root, "share")) + t.Setenv("XDG_STATE_HOME", "") + t.Setenv("XDG_CONFIG_HOME", "") + t.Setenv("XDG_CACHE_HOME", "") + + src := write(t, filepath.Join(root, "x.pdf"), "mine", 0o644) + displaced := write(t, filepath.Join(root, "old", "y.pdf"), "displaced content", 0o644) + dst := filepath.Join(root, "W", "x.pdf") + + c := chainFor(t, root, "x.pdf", plan.Step{Kind: plan.Move, Rule: "a", Src: src, Dst: dst, Displaces: displaced}) + + got := Chain(c) + if got[0].Status != "ok" { + t.Fatalf("step = %+v", got[0]) + } + if got[0].DisplacedEntry == "" { + t.Fatal("DisplacedEntry is empty; undo has no way to find the displaced file") + } + if got[0].Entry != "" { + t.Errorf("Entry = %q, want empty: this step is a Move, not itself a Trash step", got[0].Entry) + } + if _, err := os.Stat(displaced); !os.IsNotExist(err) { + t.Error("the displaced file is still at its old path") + } + if b, err := os.ReadFile(dst); err != nil || string(b) != "mine" { + t.Errorf("the move's own destination = %q, %v", b, err) + } + + restored, err := trash.Restore(got[0].DisplacedEntry) + if err != nil { + t.Fatalf("Restore(%q): %v", got[0].DisplacedEntry, err) + } + if restored != displaced { + t.Errorf("restored = %q, want %q", restored, displaced) + } + if b, err := os.ReadFile(restored); err != nil || string(b) != "displaced content" { + t.Errorf("restored content = %q, %v; want the displaced file's own content", b, err) + } +} + +// TestChainRunsRenameStep is the fix-round-2 gap: apply_test.go's only other +// Rename (in TestChainStopsWhenFileChanged) is always reported "skipped", +// because the Move before it is made to fail on purpose, so +// "case plan.Rename: err = os.Rename(step.Src, dst)" is never exercised by +// a passing test. A reversed-argument typo there would compile, pass every +// other test, pass make ci, and surface only as live data corruption. +func TestChainRunsRenameStep(t *testing.T) { + root := t.TempDir() + src := write(t, filepath.Join(root, "x.pdf"), "content", 0o644) + dst := filepath.Join(root, "y.pdf") + c := chainFor(t, root, "x.pdf", plan.Step{Kind: plan.Rename, Rule: "a", Src: src, Dst: dst}) + + got := Chain(c) + if got[0].Status != "ok" { + t.Fatalf("step = %+v", got[0]) + } + if _, err := os.Stat(src); !os.IsNotExist(err) { + t.Error("the old name still exists after a successful rename") + } + if b, err := os.ReadFile(dst); err != nil || string(b) != "content" { + t.Errorf("the new name = %q, %v; want the original content at the new name", b, err) + } +} + +// TestChainMadeIsOutermostFirstForNestedDirectories is the fix-round-2 gap: +// every other test creates at most one missing directory level, so +// mkdirAllTracked's outermost-first ordering is correct by trace but +// unpinned by any assertion. Task 5 removes these directories in reverse, +// so a later accidental reordering would break undo while passing +// everything else here. +func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(root, "x.pdf"), "content", 0o644) + dst := filepath.Join(root, "A", "B", "x.pdf") + c := chainFor(t, root, "x.pdf", plan.Step{Kind: plan.Copy, Rule: "a", Src: filepath.Join(root, "x.pdf"), Dst: dst}) + + got := Chain(c) + if got[0].Status != "ok" { + t.Fatalf("step = %+v", got[0]) + } + want := []string{filepath.Join(root, "A"), filepath.Join(root, "A", "B")} + if !slices.Equal(got[0].Made, want) { + t.Errorf("Made = %v, want %v (outermost first)", got[0].Made, want) + } +} diff --git a/internal/apply/fs.go b/internal/apply/fs.go new file mode 100644 index 0000000..205089f --- /dev/null +++ b/internal/apply/fs.go @@ -0,0 +1,197 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package apply + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "syscall" +) + +// copyFile copies src to dst by streaming its content through a temporary +// file created in dst's directory, syncing it, then renaming it into place. +// dst must not already exist. Mode and modification time are preserved from +// src. On any failure the temporary file is removed and neither src nor a +// pre-existing dst is touched. +// +// This is not internal/config's replaceFile reused: that helper resolves a +// destination symlink and overwrites a file that is already there, and it +// takes the whole replacement as a []byte. copy's destination must not exist +// beforehand, and a []byte cannot stand in for a file that may be many +// gigabytes, so this is a separate, streaming equivalent kept local to +// internal/apply rather than shared with internal/config. +func copyFile(src, dst string) error { + in, err := os.Open(src) + if err != nil { + return err + } + defer in.Close() + + fi, err := in.Stat() + if err != nil { + return err + } + + dir := filepath.Dir(dst) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + + tmp, err := os.CreateTemp(dir, ".krino-*") + if err != nil { + return err + } + tmpName := tmp.Name() + done := false + defer func() { + if !done { + os.Remove(tmpName) + } + }() + + if _, err := io.Copy(tmp, in); err != nil { + tmp.Close() + return err + } + if err := tmp.Chmod(fi.Mode().Perm()); err != nil { + tmp.Close() + return err + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Chtimes(tmpName, fi.ModTime(), fi.ModTime()); err != nil { + return err + } + + // Deliberate, not redundant: the executor's own conflict re-check + // (runFileStep, apply.go) already found a name free of anything on disk + // before ever calling copyFile, so dst existing here means something + // else claimed it in the meantime. Refusing is the only safe response — + // silently overwriting it, via os.Rename below, would destroy whatever + // just raced us. + if _, err := os.Lstat(dst); err == nil { + return fmt.Errorf("copy: destination already exists: %s", dst) + } else if !os.IsNotExist(err) { + return err + } + if err := os.Rename(tmpName, dst); err != nil { + return err + } + done = true + return nil +} + +// moveFile moves src to dst. It tries os.Rename first, which is atomic when +// src and dst are on the same filesystem. Only when that fails with EXDEV +// (a different filesystem) does it fall back to copying src to dst through +// copyFile — itself leaving no temporary file and touching neither src nor +// dst on failure — and, only once that copy has landed at dst, removing src. +// A failure at any point before the copy has landed leaves src exactly +// where it was; a failure to remove src afterward leaves both a full copy +// at dst and the original at src rather than risk deleting the only good +// copy. +// +// The filesystem check is done by unwrapping the error for syscall.EXDEV, +// never by comparing a Stat_t's device field: that field's type differs +// across the platforms `make ci` vets (freebsd, openbsd), while syscall.EXDEV +// itself is defined identically on all three. +func moveFile(src, dst string) error { + if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + return err + } + err := os.Rename(src, dst) + if err == nil { + return nil + } + if !errors.Is(err, syscall.EXDEV) { + return err + } + if err := copyFile(src, dst); err != nil { + return err + } + return os.Remove(src) +} + +// 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 +// collision problem and is not sharing code with either. +const maxSuffixAttempts = 10000 + +// nextFreeName finds the first stem_N.ext, N starting at 1, that does not +// currently exist on disk. It is the executor's own conflict re-check (spec +// §7.4, last paragraph): planning already resolved every conflict once +// against the filesystem as it was then, but something else can claim the +// planned name before the executor gets to it, so the executor looks again, +// right before acting, using only the real filesystem — it has no run-wide +// claim set to consult, unlike planning's. +// +// internal/plan's own suffixed() and splitExt are unexported, so they are +// not reachable from here; nextFreeName and splitExt below are a second, +// small implementation, the same shape as internal/plan's and +// internal/trash's for the same reason those two do not share code with +// each other either — each resolves a distinct, independently changing set +// of collisions (planned destinations; entries already in the Trash; names +// that appeared on disk since this plan was made). +func nextFreeName(dst string) (string, error) { + dir, base := filepath.Split(dst) + stem, ext := splitExt(base) + for n := 1; n <= maxSuffixAttempts; n++ { + candidate := filepath.Join(dir, fmt.Sprintf("%s_%d%s", stem, n, ext)) + if _, err := os.Lstat(candidate); os.IsNotExist(err) { + return candidate, nil + } + } + return "", errors.New("too many conflicting names") +} + +// splitExt splits name on its last dot, which does not count when it is the +// first character: "a.tar.gz" -> "a.tar", ".gz"; ".bashrc" -> ".bashrc", "". +// Duplicated from internal/plan/conflict.go and internal/trash/trash.go +// (unexported in both) rather than shared; see nextFreeName's comment. +func splitExt(name string) (stem, ext string) { + i := strings.LastIndexByte(name, '.') + if i <= 0 { + return name, "" + } + return name[:i], name[i:] +} + +// 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) { + dir = filepath.Clean(dir) + if fi, err := os.Stat(dir); err == nil { + if !fi.IsDir() { + return nil, fmt.Errorf("%s exists and is not a directory", dir) + } + return nil, nil + } else if !os.IsNotExist(err) { + return nil, err + } + + parent := filepath.Dir(dir) + var made []string + if parent != dir { + parentMade, err := mkdirAllTracked(parent) + if err != nil { + return nil, err + } + made = parentMade + } + if err := os.Mkdir(dir, 0o755); err != nil && !os.IsExist(err) { + return made, err + } + return append(made, dir), nil +} diff --git a/internal/apply/fs_test.go b/internal/apply/fs_test.go new file mode 100644 index 0000000..87c5824 --- /dev/null +++ b/internal/apply/fs_test.go @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package apply + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func write(t *testing.T, path, content string, mode os.FileMode) string { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), mode); err != nil { + t.Fatal(err) + } + return path +} + +func TestCopyPreservesModeAndModTime(t *testing.T) { + dir := t.TempDir() + src := write(t, filepath.Join(dir, "a", "x.pdf"), "content", 0o640) + old := time.Date(2026, 3, 4, 5, 6, 7, 0, time.UTC) + if err := os.Chtimes(src, old, old); err != nil { + t.Fatal(err) + } + dst := filepath.Join(dir, "b", "x.pdf") + + if err := copyFile(src, dst); err != nil { + t.Fatal(err) + } + fi, err := os.Stat(dst) + if err != nil { + t.Fatal(err) + } + if b, _ := os.ReadFile(dst); string(b) != "content" { + t.Errorf("content = %q", b) + } + if fi.Mode().Perm() != 0o640 { + t.Errorf("mode = %v, want 0640", fi.Mode().Perm()) + } + if !fi.ModTime().Equal(old) { + t.Errorf("mtime = %v, want %v", fi.ModTime(), old) + } + if si, _ := os.Stat(src); si == nil { + t.Error("copy removed its source") + } +} + +func TestCopyLeavesNoTempOnFailure(t *testing.T) { + dir := t.TempDir() + src := write(t, filepath.Join(dir, "x.pdf"), "content", 0o644) + // A destination directory that is really a file: the rename must fail. + blocked := write(t, filepath.Join(dir, "blocked"), "not a directory", 0o644) + if err := copyFile(src, filepath.Join(blocked, "x.pdf")); err == nil { + t.Fatal("copy into a non-directory succeeded") + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if len(e.Name()) > 6 && e.Name()[:7] == ".krino-" { + t.Errorf("a temporary file was left behind: %s", e.Name()) + } + } +} + +// TestMoveAcrossFilesystems exercises the EXDEV fallback. /dev/shm is a +// second filesystem on Linux; the test skips where there is none. +func TestMoveAcrossFilesystems(t *testing.T) { + other, err := os.MkdirTemp("/dev/shm", "krino-apply-") + if err != nil { + t.Skip("no second filesystem available:", err) + } + defer os.RemoveAll(other) + src := write(t, filepath.Join(other, "x.pdf"), "across", 0o644) + dst := filepath.Join(t.TempDir(), "x.pdf") + + if err := moveFile(src, dst); err != nil { + t.Fatal(err) + } + if b, err := os.ReadFile(dst); err != nil || string(b) != "across" { + t.Errorf("destination = %q, %v", b, err) + } + if _, err := os.Stat(src); !os.IsNotExist(err) { + t.Error("the source survived a cross-filesystem move") + } +} diff --git a/internal/config/load.go b/internal/config/load.go index f6eaeae..cd4d97c 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -91,3 +91,9 @@ func (c *Config) LogFile() string { } return filepath.Join(xdg.StateHome(), "krino", "krino.log") } + +// LockFile is where a directory's lock lives while a run is active: +// $XDG_STATE_HOME/krino/<name>.lock, beside the log (spec §3). +func (c *Config) LockFile(name string) string { + return filepath.Join(xdg.StateHome(), "krino", name+".lock") +} diff --git a/internal/config/load_test.go b/internal/config/load_test.go index be72fc0..1cc3ac9 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -104,3 +104,14 @@ func TestDefaultFileAndLogFile(t *testing.T) { t.Errorf("LogFile() = %q", got) } } + +func TestLockFile(t *testing.T) { + h := t.TempDir() + t.Setenv("HOME", h) + t.Setenv("XDG_STATE_HOME", "") + c := &Config{} + want := filepath.Join(h, ".local", "state", "krino", "dl.lock") + if got := c.LockFile("dl"); got != want { + t.Errorf("LockFile = %q, want %q", got, want) + } +} diff --git a/internal/engine/apply.go b/internal/engine/apply.go new file mode 100644 index 0000000..c74414d --- /dev/null +++ b/internal/engine/apply.go @@ -0,0 +1,929 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package engine + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "syscall" + "time" + + "krino/internal/apply" + "krino/internal/journal" + "krino/internal/plan" + "krino/internal/scan" + "krino/internal/trash" + "krino/internal/xdg" +) + +// ApplyResult is what one Apply or ApplyUndo call did. +type ApplyResult struct { + // Dir is the directory the plan came from. ApplyUndo leaves it nil: one + // run's undo can span several directories (each UndoFile carries its + // own Dir name), so there is no single *Dir to attach here the way + // Apply's caller already holds one via its DirPlan. + Dir *Dir + Files []FileResult + Applied int // files with at least one step that ran + Failed int // files with at least one failed step + Declined int +} + +// FileResult is one file's outcome within an ApplyResult. +type FileResult struct { + File scan.File + Steps []apply.StepResult +} + +// Apply carries out dp's plan and logs every event: run-start before the +// first file, run-end after the last, and one entry per step (spec §9). +// approved names the files to act on by Chain.File.Rel; a chain that is not +// named is left alone but still logged, one entry per step, status +// "declined" — spec §9 says declined files are logged even though nothing +// happens to them. +// +// ctx is checked between files, never within one: apply.Chain has no ctx +// parameter and always runs a whole file's chain synchronously, so a file +// already underway always finishes and is logged before Apply looks at ctx +// again (spec §11 — Ctrl-C finishes the current step, logs it, and stops). +// On cancellation, Apply returns what it did so far together with ctx.Err() +// and never writes run-end: the log is left exactly like the crashed-run +// shape journal.Entries already knows how to read back (run-start, no +// run-end), which is what makes an interrupted run still undoable. +func (e *Engine) Apply(ctx context.Context, dp *DirPlan, approved map[string]bool, j *journal.Writer, run string) (*ApplyResult, error) { + result := &ApplyResult{Dir: dp.Dir} + + var actionable []plan.Chain + for _, c := range dp.Chains { + if len(c.Steps) > 0 { + actionable = append(actionable, c) + } + } + if len(actionable) == 0 { + return result, nil + } + if err := ctx.Err(); err != nil { + return result, err + } + + if err := j.Append(journal.Entry{Time: e.Now(), Run: run, Dir: dp.Dir.Name, Action: "run-start", Status: "ok"}); err != nil { + return result, fmt.Errorf("engine: apply: %w", err) + } + + for _, c := range actionable { + if err := ctx.Err(); err != nil { + return result, err + } + fr, err := e.applyFile(dp.Dir.Name, c, approved[c.File.Rel], j, run) + if err != nil { + return result, fmt.Errorf("engine: apply: %w", err) + } + result.Files = append(result.Files, fr) + tallyFile(result, fr.Steps, nil) // every forward action is file-affecting + } + + if err := j.Append(journal.Entry{Time: e.Now(), Run: run, Dir: dp.Dir.Name, Action: "run-end", Status: "ok"}); err != nil { + return result, fmt.Errorf("engine: apply: %w", err) + } + return result, nil +} + +// applyFile carries out (or declines) one file's chain and logs it. +func (e *Engine) applyFile(dirName string, c plan.Chain, approved bool, j *journal.Writer, run string) (FileResult, error) { + rel := c.File.Rel + + if !approved { + steps := make([]apply.StepResult, len(c.Steps)) + for i, step := range c.Steps { + sr := apply.StepResult{Step: step, Status: "declined"} + steps[i] = sr + if err := e.logStep(j, run, dirName, rel, i+1, step, sr); err != nil { + return FileResult{}, err + } + } + return FileResult{File: c.File, Steps: steps}, nil + } + + results := apply.Chain(c) + for i, sr := range results { + if err := e.logStep(j, run, dirName, rel, i+1, c.Steps[i], sr); err != nil { + return FileResult{}, err + } + } + return FileResult{File: c.File, Steps: results}, nil +} + +// tallyFile updates result's Applied/Failed/Declined counters from one +// file's step outcomes. The three are not mutually exclusive: a chain that +// ran one step ok and then failed on the next counts toward both Applied +// and Failed, matching each field's own "at least one step" definition. +// +// failureCounts, when non-nil, is asked before letting a "failed" status at +// index i count toward Failed. This is undo-mkdir's exemption (fix round 2, +// item 3): Task 7 maps ApplyResult to krino undo's exit code, and a file +// whose only failure is an undo-mkdir it correctly declined to remove (a +// shared directory not yet empty - not a hazard, see planUndoFile's and +// undoFile's comments) must not make the whole run look failed. The forward +// path passes nil: every one of its actions is file-affecting, so every +// failure counts. +func tallyFile(result *ApplyResult, steps []apply.StepResult, failureCounts func(i int) bool) { + var ok, failed, declined bool + for i, sr := range steps { + switch sr.Status { + case "ok": + ok = true + case "failed": + if failureCounts == nil || failureCounts(i) { + failed = true + } + case "declined": + declined = true + } + } + // Fix wave item 4 / Minor 6: a file every one of whose steps came back + // "skipped" - the shape an approved all-skipped chain used to take, one + // step for each rule action but every step's own Skip already set - + // left none of ok/failed/declined true above, so it fell out of the + // outcome tally entirely: "0 applied · 0 failed · 0 declined" for a + // file the user was asked about and approved. The converged + // actionableChains/countActing definition (cmd/krino, same fix wave + // item) keeps such a chain from ever reaching here approved in the + // first place, but tallyFile is the shared invariant, not a guarantee + // upheld only by that one caller: every file it is given must land in + // exactly one of the three buckets. Nothing ran and nothing failed, + // which is what "declined" already means to this tally, so an + // otherwise-uncounted file lands there. + if !ok && !failed && !declined && len(steps) > 0 { + declined = true + } + if ok { + result.Applied++ + } + if failed { + result.Failed++ + } + if declined { + result.Declined++ + } +} + +// actionName is the log's action vocabulary (spec §9) for a plan.Kind. +// Trash and DeletePermanent share one Go type (plan.Kind) but two different +// words in the log: "trash" is recoverable (goes to the Trash), "delete" is +// not. +func actionName(k plan.Kind) string { + switch k { + case plan.Copy: + return "copy" + case plan.Move: + return "move" + case plan.Rename: + return "rename" + case plan.Trash: + return "trash" + case plan.DeletePermanent: + return "delete" + } + panic(fmt.Sprintf("engine: unknown plan.Kind %d", int(k))) +} + +// logStep writes every journal line one step produces: a "displace" entry +// when the step trashed a file that was in its way (StepResult.DisplacedEntry +// is the only place that trash entry name exists — Task 3's ruling), a +// "mkdir" entry per directory the step actually created (outermost first, so +// undo can remove them innermost first), and finally the step's own entry. +// All three share stepNum, the step's 1-based position in the chain, so a +// reader can see which step of the plan a mkdir or displace line belongs to; +// PlanUndo does not rely on that number, only on log order and File. +// +// Size and ModTime on the primary entry describe the file at Dst after the +// step (spec §9) only when the step actually ran (Status "ok"): sr.Dst is +// where the file really ended up (conflict resolution can rename it at +// execution time), and sr.Size/sr.ModTime are read from there. For every +// other status nothing happened at a destination, so Dst falls back to the +// step's planned destination (informational only — PlanUndo never reverses +// a non-"ok" entry) and Size/ModTime stay zero. +func (e *Engine) logStep(j *journal.Writer, run, dirName, rel string, stepNum int, step plan.Step, sr apply.StepResult) error { + if sr.DisplacedEntry != "" { + dst := filepath.Join(trash.Dir(), "files", sr.DisplacedEntry) + size, mtime := statSizeModTime(dst) + if err := j.Append(journal.Entry{ + Time: e.Now(), Run: run, Dir: dirName, File: rel, Step: stepNum, + Action: "displace", Status: "ok", Rule: step.Rule, + // Detail carries the trash entry name explicitly (fix round 1, + // item 3): Dst's shape (trash.Dir()/files/<entry>) is + // internal/apply's and this file's own convention, not a + // contract undo may quietly depend on. PlanUndo/ApplyUndo read + // the name from here, never by taking Dst's basename. + Src: step.Displaces, Dst: dst, Size: size, ModTime: mtime, Detail: sr.DisplacedEntry, + }); err != nil { + return err + } + } + + for _, dir := range sr.Made { + size, mtime := statSizeModTime(dir) + if err := j.Append(journal.Entry{ + Time: e.Now(), Run: run, Dir: dirName, File: rel, Step: stepNum, + Action: "mkdir", Status: "ok", Rule: step.Rule, + Dst: dir, Size: size, ModTime: mtime, + }); err != nil { + return err + } + } + + dst := step.Dst + var size int64 + var mtime time.Time + detail := sr.Detail + if sr.Status == "ok" { + dst, size, mtime = sr.Dst, sr.Size, sr.ModTime + if step.Kind == plan.Trash { + // Same reasoning as the displace entry above: sr.Detail is + // always empty on a successful trash (apply.runTrashStep sets + // it only on failure), so this costs nothing and gives undo an + // explicit entry name instead of one derived from Dst. + detail = sr.Entry + } + } + return j.Append(journal.Entry{ + Time: e.Now(), Run: run, Dir: dirName, File: rel, Step: stepNum, + Action: actionName(step.Kind), Status: sr.Status, Rule: step.Rule, + Src: step.Src, Dst: dst, Size: size, ModTime: mtime, Detail: detail, + }) +} + +// statSizeModTime best-effort stats path, returning zero values rather than +// an error: it is used only to make a log line more informative (mkdir and +// displace entries), never to decide anything. +func statSizeModTime(path string) (int64, time.Time) { + fi, err := os.Stat(path) + if err != nil { + return 0, time.Time{} + } + return fi.Size(), fi.ModTime() +} + +// Runs lists recent runs, newest first; n <= 0 means all. +func (e *Engine) Runs(n int) ([]journal.Run, error) { + runs, err := journal.Runs(e.Config.LogFile(), n) + if err != nil { + return nil, fmt.Errorf("engine: runs: %w", err) + } + return runs, nil +} + +// UndoPlan is the reversal of one run, one UndoFile per file the run +// touched, in the order the run first mentioned them. +type UndoPlan struct { + Run string + Files []UndoFile +} + +// UndoFile is the reversal of one file's chain, last original step first. +// Refused set means none of Steps is reversed by ApplyUndo — spec §10: "no +// file is left half undone" — even though individual steps may carry their +// own, informational Refused (see UndoStep). +type UndoFile struct { + File string // the Rel the original run logged + Dir string + Steps []UndoStep // last original step first + Refused string // non-empty: nothing in this file is reversed, and why + + // Declined is never set by PlanUndo - it carries the front end's own + // review decision back into ApplyUndo without widening ApplyUndo's + // signature (fix round 2026-09-12, item 2 of Task 8's review): true + // means the caller chose not to reverse an otherwise-reversible file + // (Refused empty), and ApplyUndo logs it exactly as a declined forward + // chain is logged (spec §9: "declined files are logged even though + // nothing happens to them") - one entry per step, status "declined" - + // rather than silently omitting it the way a Refused file still is. + // Setting this on a file that is also Refused has no effect: Refused's + // own silent-decline path is checked first and wins. + Declined bool +} + +// UndoStep is the reversal of one logged step. +type UndoStep struct { + Original journal.Entry // the step being reversed + Action string // undo-move, undo-rename, undo-copy, undo-trash, undo-displace, undo-mkdir + Src, Dst string // what the reversal will do + Refused string // non-empty: this step cannot be reversed +} + +// PlanUndo builds the reversal of runID, per spec §10's table. It reads the +// log only — no file is touched — so it can be shown and approved before +// anything happens (spec §10: undo is planned and approved like any other +// plan). +// +// journal.Entries returning a nil error is the only signal that runID's +// chain is intact (Task 1's ruling); a non-nil error, meaning a line inside +// the run's window failed to parse or the run has no readable run-start, +// refuses the whole run rather than build a reversal from a chain that might +// be missing steps. A run with no run-end (a crash) is not this case: +// Entries extends the window to end of file and still returns cleanly, so +// PlanUndo treats a crashed run exactly like an intact one. +func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { + entries, err := journal.Entries(e.Config.LogFile(), runID) + if err != nil { + return nil, fmt.Errorf("engine: plan undo: %w", err) + } + if len(entries) == 0 { + return nil, fmt.Errorf("engine: plan undo: run %s not found", runID) + } + if isUndoRun(entries) { + 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{} + for _, en := range entries { + if en.File == "" { // run-start / run-end + continue + } + if _, seen := byFile[en.File]; !seen { + order = append(order, en.File) + } + byFile[en.File] = append(byFile[en.File], en) + } + + up := &UndoPlan{Run: runID} + for _, file := range order { + uf := planUndoFile(file, byFile[file]) + // 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 + // an UndoFile with no Steps and no Refused - not a reversible + // file, and not a refused one either, just a file this run never + // touched. Appending it anyway lied about the plan: it counted + // toward "to reverse" (anything with Refused == "" does) while + // rendering no row and reversing nothing, so the final tally came + // up one short with no message. The condition is deliberately + // "no steps AND no refusal", never "no steps" alone - a + // permanently deleted file also has zero Steps, but planUndoFile + // sets Refused for it (spec §10: it must stay visible, with its + // reason, as not undoable), and that file must still be appended. + if len(uf.Steps) == 0 && uf.Refused == "" { + continue + } + up.Files = append(up.Files, uf) + } + return up, nil +} + +// isUndoRun reports whether every one of entries' file-scoped actions is +// already an "undo-" action, i.e. entries belongs to a run ApplyUndo itself +// produced. Runs cannot themselves be undone (spec §10). This does not read +// journal's own undo-of-run bookkeeping (Detail on the undo run's own +// run-start, unexported to this package): a run ApplyUndo writes never logs +// a plain action, only "undo-" ones, so checking the action vocabulary +// directly is a self-contained equivalent. +func isUndoRun(entries []journal.Entry) bool { + any := false + for _, en := range entries { + if en.Action == "run-start" || en.Action == "run-end" { + continue + } + any = true + if !strings.HasPrefix(en.Action, "undo-") { + return false + } + } + return any +} + +// isFileAffecting reports whether an undo action's own success or failure +// bears on the FILE's data - as opposed to "undo-mkdir", whose refusal +// gates neither planning nor execution the way every other action's does. +// See the comment on planUndoFile for why the two are treated differently. +func isFileAffecting(action string) bool { + return action != "undo-mkdir" +} + +// planUndoFile builds one file's UndoFile from its log entries (file's own +// order, chronological). Only "ok" entries ever happened and so are +// candidates for reversal; declined, skipped and failed ones are ignored. +// Entries are walked from last to first, per spec §10 ("last step first +// within each file"), which also puts a step's own mkdir/displace +// sub-entries in the right place relative to it: logStep always writes them +// before the step's own primary entry, so reversed order visits the primary +// entry first (undo it) and its mkdir/displace satellites after (clean up, +// innermost mkdir first; restore what it displaced last) — exactly the +// order a real reversal needs. +// +// Fix round 1, ruling on item 1: "undo-mkdir" is the one action excluded +// from the whole-file refusal gate, and the distinction is deliberate, not +// an inconsistency. Every other reversal's refusal condition - dst +// missing/changed, src now occupied, the trash entry gone - means the same +// thing: the world changed under us since the run, and reversing anyway +// could lose data. That is what spec §10's "no file is left half undone" +// exists to prevent, so it correctly gates the whole file. "Directory not +// empty" is not that kind of condition: it means a SIBLING file still lives +// there, which is not a hazard to anything, and a directory two files share +// is only actually empty once every file that used it has been reversed - +// checking it once at planning time, before any of those reversals have +// run, would refuse it (and, by the whole-file rule, the entire owning +// file, including its otherwise-safe undo-move) essentially every time two +// files share a destination directory, which is the common case. So an +// undo-mkdir reversal is never refused at planning time, and a failed one +// at execution time (ApplyUndo/undoFile) leaves the rest of that file's +// steps to run rather than aborting the file — the same "the substantive +// act's result is what is reported, cleanup is best-effort" shape as +// Task 2's .trashinfo ruling. isFileAffecting is the one predicate both +// this function and undoFile's stop-on-failure check share, so the two +// places this distinction matters cannot drift apart. +// +// Fix wave item 1 (Critical): this loop owns an undoProjection, built up as +// it appends steps in the order they will actually execute. resolveConflict +// (internal/plan/conflict.go) can make one contested path both a step's own +// Dst and its Displaces - deliberately, and correct for the forward run - +// which means the reversal that puts the incoming file back where it came +// from (freeing the contested path) and the reversal that restores the +// displaced original to that same path are two steps of ONE file's chain +// that genuinely contend for it. reverseStep alone cannot see that: it is a +// pure function of one journal entry. So the src-exists occupancy check +// (refuseIfSrcExists) is no longer decided there; it is decided here, after +// reverseStep returns, with the projection recording what every +// earlier-executing step (already appended) will do to the filesystem once +// it runs. A path a predecessor will vacate does not count as occupied for +// a step that runs after it - the previous ordering assumption ("the world +// exactly as it is now, before ANY reversal has run") was simply false for +// 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)} + proj := newUndoProjection() + for i := len(ents) - 1; i >= 0; i-- { + en := ents[i] + if en.Status != "ok" { + continue + } + if en.Action == "delete" { + // Permanent delete is terminal: nothing can follow it for this + // file, and it is never reversible (spec §10). No UndoStep is + // built for it - there is no undo- action for a permanent + // delete - the file is simply refused outright. + if uf.Refused == "" { + uf.Refused = "permanent delete cannot be undone" + } + break + } + step := reverseStep(en) + if step.Refused == "" { + step.Refused = refuseIfSrcExists(step, proj) + } + proj.record(step) + uf.Steps = append(uf.Steps, step) + if step.Refused != "" && isFileAffecting(step.Action) && uf.Refused == "" { + uf.Refused = step.Refused + } + } + return uf +} + +// undoProjection tracks what the reversal steps planUndoFile has already +// queued (in the order they will execute) will do to the filesystem, so a +// later step's occupancy check can tell a real, external occupant from a +// path one of this same file's own earlier-executing steps is about to +// vacate. It never touches the filesystem itself - it is a bookkeeping +// overlay purely for the offer planUndoFile builds; the execution-time +// guards (trash.Restore's own occupancy refusal, renameOrCopy's Lstat) are +// what actually protects a file once ApplyUndo runs, regardless of whether +// this projection turns out right. +type undoProjection struct { + vacated map[string]bool // paths a queued step will free once it runs + occupied map[string]bool // paths a queued step will place a file at once it runs +} + +func newUndoProjection() *undoProjection { + return &undoProjection{vacated: map[string]bool{}, occupied: map[string]bool{}} +} + +// record updates the projection with one step's effect, once it has already +// been queued: its own Src becomes free (every reversal action vacates the +// path it reads from), and, for the actions that put a file back at a fixed +// path (undo-move, undo-rename, undo-trash, undo-displace - never +// undo-copy, whose destination is chosen by trash.Put at execution time, and +// never undo-mkdir, which only ever frees a path), its Dst becomes occupied. +// A path cannot be both at once, so whichever happens second here wins. +func (p *undoProjection) record(us UndoStep) { + delete(p.occupied, us.Src) + p.vacated[us.Src] = true + if needsOccupancyCheck(us.Action) { + delete(p.vacated, us.Dst) + p.occupied[us.Dst] = true + } +} + +// occupiedNow reports whether path is spoken for, from the projection's +// point of view: really on disk and not about to be vacated by an +// earlier-queued step, or not on disk yet but about to be occupied by one +// anyway (two of this file's own steps landing on the same path, which +// would be a real, if so-far unseen, contention). +func (p *undoProjection) occupiedNow(path string) bool { + if p.occupied[path] { + return true + } + if p.vacated[path] { + return false + } + _, err := os.Lstat(path) + return err == nil +} + +// needsOccupancyCheck reports whether action restores a file to a fixed +// path - the only actions refuseIfSrcExists ever needs to check, and +// therefore the only ones record above tracks as occupying their Dst. +func needsOccupancyCheck(action string) bool { + switch action { + case "undo-move", "undo-rename", "undo-trash", "undo-displace": + return true + } + 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 +// anything, so PlanUndo stays read-only. +// +// Fix wave item 1: it deliberately does NOT decide the "src now exists" +// occupancy refusal any more - that is refuseIfSrcExists, called by +// planUndoFile's loop instead of from here. reverseStep is a pure function +// of one journal entry: it has no way to see the rest of the file's chain, +// so it cannot tell a real occupant from a path an earlier-executing step +// of this same chain is about to vacate. planUndoFile owns the projection +// that can. +func reverseStep(en journal.Entry) UndoStep { + us := UndoStep{Original: en, Action: "undo-" + en.Action} + switch en.Action { + case "move", "rename": + us.Src, us.Dst = en.Dst, en.Src + us.Refused = refuseIfChanged(en) + case "copy": + // The reversal moves the copy to the Trash; where it lands there is + // decided at execution time (trash.Put chooses the entry name), the + // same reason plan.Step.Dst is always "" for a Trash-kind step. + us.Src = en.Dst + 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" + } + case "mkdir": + us.Src = en.Dst + } + return us +} + +// refuseIfChanged is the move/rename/copy refusal check: the file at +// en.Dst, as it is now, must still match the size and mtime the run logged +// for it (spec §9: those columns describe the file at Dst after the step, +// precisely so undo can tell whether it has been touched since). +// +// The mtime comparison is exact (time.Time.Equal), not truncated to whole +// seconds: journal entries now round-trip through RFC3339Nano +// (journal.Writer.Append, fix round 1 item 4), which preserves the +// sub-second precision a fresh os.Stat also has. A whole-second comparison +// would let a file rewritten within the same second as the recorded mtime +// read as untouched, and undo would move it back believing it had not +// changed - the one guard that decides whether to overwrite the user's +// file, so it must not have that gap. +// Both refusal messages below go through xdg.Abbrev (Minor 4 / fix wave +// item 5): every step cell in the printed plan already abbreviates its path +// against $HOME (cmd/krino/undo.go's undoActionCell, via xdg.Abbrev), and a +// refusal reason sitting two lines under a "→ ~/dl/a.pdf" row in raw +// "/tmp/.../sbx/home/dl/a.pdf" form was the one cell that did not match. +func refuseIfChanged(en journal.Entry) string { + fi, err := os.Stat(en.Dst) + if err != nil { + return fmt.Sprintf("%s is missing", xdg.Abbrev(en.Dst)) + } + if fi.Size() != en.Size || !fi.ModTime().Equal(en.ModTime) { + return fmt.Sprintf("%s changed since the run", xdg.Abbrev(en.Dst)) + } + 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 +// call (see reverseStep's comment) - planUndoFile calls it after reverseStep +// returns, passing the projection built from every reversal step already +// queued ahead of us in this same file's chain, so a path a predecessor is +// about to vacate does not read as occupied. needsOccupancyCheck excludes +// undo-copy (destination chosen by trash.Put at execution time) and +// undo-mkdir (its own, execution-time-only refusal), the two actions whose +// UndoStep.Dst is not a fixed path this check would even make sense against. +func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { + if !needsOccupancyCheck(us.Action) { + return "" + } + if proj.occupiedNow(us.Dst) { + return fmt.Sprintf("%s already exists", xdg.Abbrev(us.Dst)) + } + return "" +} + +// ApplyUndo reverses up, skipping every file whose Refused is set and +// logging a declined file's steps without reversing them (see +// declineUndoFile), and logs the reversal as a run of its own: a run-start +// whose Detail records which run this undoes (journal's own convention - +// Task 1 - so Runs can mark the original run Undone), one entry per undo +// step, and a run-end. +// +// Dir is left blank on the run-start/run-end entries: unlike Apply, which is +// always scoped to one directory's DirPlan, one undo run can span several +// directories, so there is no single name to put there; each step's own +// entry still carries its own file's real directory name from UndoFile.Dir. +// +// actionable preserves up.Files' own order (the order the original run first +// mentioned them), whether a file is actually reversed or only logged as +// 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. +func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, run string) (*ApplyResult, error) { + result := &ApplyResult{} + + var actionable []UndoFile + for _, f := range up.Files { + if f.Refused != "" { + result.Declined++ + continue + } + // Fix wave item 4 / final-wave item 24: the same invariant + // PlanUndo's own "no steps AND no refusal" guard states explicitly + // (see its comment) - a file with no steps and no Refused is one + // this run never touched, not a reversible one - given the same + // two-condition form here, rather than relying on the Refused + // branch above to have already made f.Refused == "" true by the + // time this runs. Written this way, the guard is correct on its + // own, independent of that branch's order or presence, rather than + // unreachable-by-construction the way the parked ruling on this + // line described it before Minor 6 showed the same error live. + if len(f.Steps) == 0 && f.Refused == "" { + continue + } + actionable = append(actionable, f) + } + if len(actionable) == 0 { + return result, nil + } + if err := ctx.Err(); err != nil { + return result, err + } + + if err := j.Append(journal.Entry{Time: e.Now(), Run: run, Action: "run-start", Status: "ok", Detail: journal.UndoOf(up.Run)}); err != nil { + return result, fmt.Errorf("engine: apply undo: %w", err) + } + + for _, f := range actionable { + if err := ctx.Err(); err != nil { + return result, err + } + if f.Declined { + fr, err := e.declineUndoFile(f, j, run) + if err != nil { + return result, fmt.Errorf("engine: apply undo: %w", err) + } + result.Files = append(result.Files, fr) + result.Declined++ + continue + } + fr, err := e.undoFile(f, j, run) + if err != nil { + return result, fmt.Errorf("engine: apply undo: %w", err) + } + result.Files = append(result.Files, fr) + tallyFile(result, fr.Steps, func(i int) bool { return isFileAffecting(f.Steps[i].Action) }) + } + + if err := j.Append(journal.Entry{Time: e.Now(), Run: run, Action: "run-end", Status: "ok"}); err != nil { + return result, fmt.Errorf("engine: apply undo: %w", err) + } + return result, nil +} + +// 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): +// a file the front end's own review chose not to reverse still gets one +// entry per step, status "declined", the same shape applyFile already gives +// a declined forward chain. Every step is declined, not just the first: an +// undo file whose reversal was never started needs the same per-step record +// a partially-run one would have, so a reader scanning the log by step +// number sees a complete, if inert, chain rather than a gap. +func (e *Engine) declineUndoFile(f UndoFile, j *journal.Writer, run string) (FileResult, error) { + steps := make([]apply.StepResult, len(f.Steps)) + for i, step := range f.Steps { + sr := apply.StepResult{Status: "declined"} + steps[i] = sr + if err := j.Append(journal.Entry{ + Time: e.Now(), Run: run, Dir: f.Dir, File: f.File, Step: i + 1, + Action: step.Action, Status: "declined", Src: step.Src, Dst: step.Dst, + }); err != nil { + return FileResult{}, err + } + } + return FileResult{Steps: steps}, nil +} + +// undoFile executes every step of f in order (already last-original-step +// first from PlanUndo) and logs each. +// +// Fix round 1, ruling on item 2: a failed FILE-AFFECTING step (everything +// but undo-mkdir - see isFileAffecting) stops the rest of the file's steps, +// matching apply.Chain's forward model, exactly because continuing past it +// is the half-undone state spec §10 forbids: if undo-move fails, reversing +// this file's still-earlier steps anyway would leave it in a state that was +// never real. A failed undo-mkdir does not stop anything: a sibling file +// still occupying that directory is not a hazard (see planUndoFile's +// comment), so the remaining steps - which may include another file's +// still-untouched undo-copy or undo-trash - keep running. +func (e *Engine) undoFile(f UndoFile, j *journal.Writer, run string) (FileResult, error) { + steps := make([]apply.StepResult, len(f.Steps)) + stopped := false + for i, step := range f.Steps { + var sr apply.StepResult + if stopped { + sr = apply.StepResult{Status: "skipped", Detail: "an earlier step in this file's reversal failed"} + } else { + sr = runUndoStep(step) + if sr.Status == "failed" && isFileAffecting(step.Action) { + stopped = true + } + } + steps[i] = sr + // dst falls back to the reversal's planned destination when nothing + // actually happened (failed or skipped), the same informational + // convention logStep uses for a forward step that did not run. + dst := step.Dst + if sr.Status == "ok" { + dst = sr.Dst + } + if err := j.Append(journal.Entry{ + Time: e.Now(), Run: run, Dir: f.Dir, File: f.File, Step: i + 1, + Action: step.Action, Status: sr.Status, Src: step.Src, Dst: dst, + Size: sr.Size, ModTime: sr.ModTime, Detail: sr.Detail, + }); err != nil { + return FileResult{}, err + } + } + return FileResult{Steps: steps}, nil +} + +// runUndoStep actually carries out one reversal. It reuses apply.StepResult +// as a convenient result shape (Status, Detail, Dst, Size, ModTime); its +// Step field does not apply here (there is no plan.Kind for an undo) and is +// left zero. +func runUndoStep(step UndoStep) apply.StepResult { + 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()} + } + if err := renameOrCopy(step.Src, step.Dst); err != nil { + return apply.StepResult{Status: "failed", Detail: err.Error()} + } + size, mtime := statSizeModTime(step.Dst) + return apply.StepResult{Status: "ok", Dst: step.Dst, Size: size, ModTime: mtime} + + case "undo-copy": + entry, err := trash.Put(step.Src) + if err != nil { + return apply.StepResult{Status: "failed", Detail: err.Error()} + } + dst := filepath.Join(trash.Dir(), "files", entry) + size, mtime := statSizeModTime(dst) + return apply.StepResult{Status: "ok", Dst: dst, Size: size, ModTime: mtime} + + case "undo-trash", "undo-displace": + // The trash entry name is read from Original.Detail, where logStep + // put it explicitly (fix round 1, item 3) - never re-derived from + // Src or Dst's shape, which belong to internal/apply's and this + // file's own conventions and must stay free to change independently. + restored, err := trash.Restore(step.Original.Detail) + if err != nil { + return apply.StepResult{Status: "failed", Detail: err.Error()} + } + size, mtime := statSizeModTime(restored) + return apply.StepResult{Status: "ok", Dst: restored, Size: size, ModTime: mtime} + + case "undo-mkdir": + if err := os.Remove(step.Src); err != nil { + return apply.StepResult{Status: "failed", Detail: err.Error()} + } + return apply.StepResult{Status: "ok"} + } + return apply.StepResult{Status: "failed", Detail: "engine: unknown undo action " + step.Action} +} + +// renameOrCopy moves src to dst, falling back to a copy-then-remove when +// they are on different filesystems. It is engine's own minimal equivalent +// of internal/apply's unexported moveFile: that package exports only +// Chain, so undo cannot reach its careful temp-file machinery and carries a +// small, independent implementation instead. +// +// The Lstat guard below is not optional (fix round 2, item 1, Critical): +// POSIX rename(2) replaces an existing regular file at dst without error, +// and PlanUndo's own "src now exists" check ran at planning time, not now - +// spec §10 has an undo plan "shown and approved the same way" as any other, +// a real human-length window in which something can create a file at dst +// before ApplyUndo gets here. Every other reversal path in this file +// already re-checks at execution time (the forward executor re-Lstats its +// destination, trash.Restore refuses "already exists" at call time, +// copyThenRemove below does its own check for the EXDEV fallback); only +// this, the common same-filesystem path, was missing it. +func renameOrCopy(src, dst string) error { + if _, err := os.Lstat(dst); err == nil { + return fmt.Errorf("engine: undo: %s already exists", dst) + } else if !os.IsNotExist(err) { + return err + } + if err := os.Rename(src, dst); err == nil { + return nil + } else if !errors.Is(err, syscall.EXDEV) { + return err + } + return copyThenRemove(src, dst) +} + +// copyThenRemove copies src to dst (which must not exist) and, only once +// that copy has landed, removes src - the same failure direction as +// internal/apply's moveFile: a failure before the copy lands leaves src +// untouched, and dst is never partially written where something might read +// it (the temporary is removed on any failure before rename). +func copyThenRemove(src, dst string) error { + in, err := os.Open(src) + if err != nil { + return err + } + defer in.Close() + fi, err := in.Stat() + if err != nil { + return err + } + + dir := filepath.Dir(dst) + tmp, err := os.CreateTemp(dir, ".krino-undo-*") + if err != nil { + return err + } + tmpName := tmp.Name() + done := false + defer func() { + if !done { + os.Remove(tmpName) + } + }() + + if _, err := io.Copy(tmp, in); err != nil { + tmp.Close() + return err + } + if err := tmp.Chmod(fi.Mode().Perm()); err != nil { + tmp.Close() + return err + } + // Fix round 2, item 2 (Important): sync before close, matching + // internal/apply's copyFile (fs.go), which this was modelled on - same + // durability requirement, same reason. + if err := tmp.Sync(); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Chtimes(tmpName, fi.ModTime(), fi.ModTime()); err != nil { + return err + } + if _, err := os.Lstat(dst); err == nil { + return fmt.Errorf("engine: undo: destination already exists: %s", dst) + } else if !os.IsNotExist(err) { + return err + } + if err := os.Rename(tmpName, dst); err != nil { + return err + } + done = true + return os.Remove(src) +} diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go new file mode 100644 index 0000000..42724a0 --- /dev/null +++ b/internal/engine/apply_test.go @@ -0,0 +1,1182 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package engine + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "krino/internal/apply" + "krino/internal/journal" + "krino/internal/plan" + "krino/internal/trash" +) + +// applyFixture builds a directory with two files and a rule moving pdfs into +// Work, then plans it. It returns the home, the plan and an open journal. +// +// Adapted from the brief to this package's actual writeConfig helper, which +// takes a main-file body and a dirs map keyed by name (see +// TestLoadRejectsUnsuppliedCaptures's comment in engine_test.go for the same +// adaptation elsewhere in this package): the brief's fixture wrote +// `(path ...)` and `(rule ...)` straight into what it called the main file, +// but the real config language (docs/design.md §4.2-4.3) requires those in a +// directory file reached through `(include ...)`. Every assertion below is +// unchanged from the brief; only this setup plumbing differs. +func applyFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, string) { + t.Helper() + h := sandbox(t) + dl := filepath.Join(h, "dl") + for name, body := range map[string]string{"a.pdf": "one", "b.txt": "two"} { + if err := os.MkdirAll(dl, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dl, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + old := time.Now().Add(-time.Hour) + os.Chtimes(filepath.Join(dl, name), old, old) + } + main := writeConfig(t, h, `(include "dl")`, map[string]string{ + "dl": `(path "~/dl")` + "\n" + `(rule "pdfs" (when (type pdf)) (move "Work"))`, + }) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims()) + if err != nil { + t.Fatal(err) + } + j, err := journal.Open(filepath.Join(h, ".local", "state", "krino", "krino.log")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { j.Close() }) + return h, e, dp, j, journal.NewRunID(time.Now()) +} + +func TestApplyMovesApprovedAndDeclinesTheRest(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + res, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run) + if err != nil { + t.Fatal(err) + } + if res.Applied != 1 || res.Failed != 0 { + t.Errorf("result = %+v; want one applied, none failed", res) + } + if _, err := os.Stat(filepath.Join(h, "dl", "Work", "a.pdf")); err != nil { + t.Errorf("the approved file did not move: %v", err) + } + if _, err := os.Stat(filepath.Join(h, "dl", "a.pdf")); !os.IsNotExist(err) { + t.Error("the original survived the move") + } + if _, err := os.Stat(filepath.Join(h, "dl", "b.txt")); err != nil { + t.Error("a file that matched no rule was touched") + } +} + +func TestApplyLogsRunBoundariesAndSteps(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + entries, err := journal.Entries(filepath.Join(h, ".local", "state", "krino", "krino.log"), run) + if err != nil { + t.Fatal(err) + } + if len(entries) < 3 { + t.Fatalf("logged %d entries, want run-start, at least one step and run-end", len(entries)) + } + if entries[0].Action != "run-start" || entries[len(entries)-1].Action != "run-end" { + t.Errorf("boundaries = %q .. %q", entries[0].Action, entries[len(entries)-1].Action) + } + var moved *journal.Entry + for i := range entries { + if entries[i].Action == "move" { + moved = &entries[i] + } + } + if moved == nil { + t.Fatal("no move entry was logged") + } + if moved.Status != "ok" || moved.File != "a.pdf" || moved.Rule != "pdfs" { + t.Errorf("move entry = %+v", *moved) + } + if moved.Size != int64(len("one")) { + t.Errorf("Size = %d; want the size at Dst after the step", moved.Size) + } + if !strings.HasSuffix(moved.Dst, filepath.Join("Work", "a.pdf")) { + t.Errorf("Dst = %q", moved.Dst) + } +} + +func TestPlanUndoReversesLastStepFirst(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if len(up.Files) != 1 { + t.Fatalf("undo plan covers %d files, want 1", len(up.Files)) + } + f := up.Files[0] + if f.Refused != "" { + t.Fatalf("undo refused: %s", f.Refused) + } + if len(f.Steps) == 0 || f.Steps[0].Action != "undo-move" { + t.Fatalf("steps = %+v; want undo-move first", f.Steps) + } + if f.Steps[0].Dst != filepath.Join(h, "dl", "a.pdf") { + t.Errorf("undo-move puts the file at %q, want its original path", f.Steps[0].Dst) + } +} + +// TestPlanUndoRefusesWholeFileWhenOneStepCannotBeReversed: spec §10 - no file +// is left half undone, so one refused step refuses the file. +func TestPlanUndoRefusesWholeFileWhenOneStepCannotBeReversed(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + // Someone edited the moved file, so the reversal is no longer safe. + moved := filepath.Join(h, "dl", "Work", "a.pdf") + if err := os.WriteFile(moved, []byte("edited since the run"), 0o644); err != nil { + t.Fatal(err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + f := up.Files[0] + if f.Refused == "" { + t.Fatal("undo did not refuse a file that changed since the run") + } + if !strings.Contains(f.Refused, "changed") { + t.Errorf("Refused = %q; want it to say the file changed", f.Refused) + } +} + +func TestPlanUndoRefusesPermanentDelete(t *testing.T) { + h := sandbox(t) + dl := filepath.Join(h, "dl") + os.MkdirAll(dl, 0o755) + os.WriteFile(filepath.Join(dl, "old.iso"), []byte("gone"), 0o644) + old := time.Now().Add(-time.Hour) + os.Chtimes(filepath.Join(dl, "old.iso"), old, old) + main := writeConfig(t, h, `(include "dl")`, map[string]string{ + "dl": `(path "~/dl")` + "\n" + `(rule "purge" (when (type iso)) (delete permanent))`, + }) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims()) + if err != nil { + t.Fatal(err) + } + j, _ := journal.Open(filepath.Join(h, ".local", "state", "krino", "krino.log")) + run := journal.NewRunID(time.Now()) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"old.iso": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused == "" || !strings.Contains(up.Files[0].Refused, "permanent") { + t.Errorf("Refused = %q; want it to name the permanent delete", up.Files[0].Refused) + } +} + +// TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete is the Critical +// finding from Task 8's review: a file the ORIGINAL forward run declined +// (spec §9: its steps are still logged, status "declined") has no "ok" +// entries at all, so planUndoFile's last-to-first walk skips every one of +// them and returns an UndoFile with Steps == nil and Refused == "" - a file +// that was never touched, not a reversible one. Before the fix, PlanUndo +// appended that empty UndoFile anyway, and undoActionableCount (cmd/krino) +// counts every Refused == "" file as "to reverse" regardless of whether it +// has any steps - inflating the header's count while the table renders no +// row for it and the final tally comes up one short, silently, at exit 0. +// +// The two halves in one test, deliberately, per the review: a fix that +// dropped every zero-step UndoFile instead of the correct +// "len(Steps) == 0 && Refused == \"\"" condition would also drop a +// permanently deleted file (zero steps, but Refused IS set - spec §10 +// requires it to stay visible with its reason) - so both conditions live +// in the same test, and a future "simplification" that breaks either one +// fails this one test immediately rather than needing two separate reviews +// to notice. +func TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete(t *testing.T) { + h := sandbox(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(dl, 0o755); err != nil { + t.Fatal(err) + } + files := map[string]string{"moved.pdf": "one", "declined.pdf": "two", "old.iso": "gone"} + old := time.Now().Add(-time.Hour) + for name, body := range files { + p := filepath.Join(dl, name) + 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) + } + } + main := writeConfig(t, h, `(include "dl")`, map[string]string{ + "dl": `(path "~/dl")` + "\n" + + `(rule "pdfs" (when (type pdf)) (move "Work"))` + "\n" + + `(rule "purge" (when (type iso)) (delete permanent))`, + }) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims()) + if err != nil { + t.Fatal(err) + } + j, err := journal.Open(filepath.Join(h, ".local", "state", "krino", "krino.log")) + if err != nil { + t.Fatal(err) + } + run := journal.NewRunID(time.Now()) + // declined.pdf is deliberately left out of approved: spec §9 still logs + // its step, status "declined" - it was never touched. + if _, err := e.Apply(context.Background(), dp, map[string]bool{"moved.pdf": true, "old.iso": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + + byFile := map[string]UndoFile{} + for _, f := range up.Files { + byFile[f.File] = f + } + + if _, ok := byFile["declined.pdf"]; ok { + t.Errorf("a file with no \"ok\" entries (declined in the original run) must not appear in the undo plan at all: %+v", up.Files) + } + if got := byFile["moved.pdf"]; len(got.Steps) == 0 { + t.Errorf("the actually-reversed file lost its steps: %+v", got) + } + permDel, ok := byFile["old.iso"] + if !ok { + t.Fatal("the permanently deleted file was dropped too - a zero-step file is not always an untouched one, and this one must stay visible with its refusal reason") + } + if permDel.Refused == "" || !strings.Contains(permDel.Refused, "permanent") { + t.Errorf("Refused = %q; want it to still name the permanent delete", permDel.Refused) + } + if len(up.Files) != 2 { + t.Errorf("undo plan has %d files, want exactly 2 (moved.pdf and old.iso); declined.pdf must be omitted, not merely empty: %+v", len(up.Files), up.Files) + } +} + +// TestPlanUndoAcceptsIntactRun pins the trust Task 1 established but never +// itself exercised through PlanUndo: journal.Entries returning a nil error +// for a run whose run-start and run-end both parsed cleanly is the signal +// that the chain is intact, and PlanUndo must build a usable plan from it +// rather than refuse. +func TestPlanUndoAcceptsIntactRun(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + entries, err := journal.Entries(logPath, run) + if err != nil { + t.Fatalf("Entries refused a fully intact run: %v", err) + } + if entries[len(entries)-1].Action != "run-end" { + t.Fatalf("fixture run is not intact: last action %q", entries[len(entries)-1].Action) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatalf("PlanUndo refused an intact run: %v", err) + } + if len(up.Files) != 1 || up.Files[0].Refused != "" { + t.Fatalf("intact run did not yield a usable undo plan: %+v", up) + } +} + +// TestPlanUndoAcceptsCrashedRun: a run-start with no run-end (the process +// died mid-run) must still yield a usable undo plan, per Entries' documented +// window-to-EOF behaviour. If this refused, Task 1's contract and this +// task's assumption would disagree - worth a ruling, not a workaround. +func TestPlanUndoAcceptsCrashedRun(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatal(err) + } + lines := strings.Split(strings.TrimRight(string(data), "\n"), "\n") + if !strings.Contains(lines[len(lines)-1], "\trun-end\t") { + t.Fatalf("fixture's last line is not run-end: %q", lines[len(lines)-1]) + } + // Simulate a crash: the process died before writing run-end. + truncated := strings.Join(lines[:len(lines)-1], "\n") + "\n" + if err := os.WriteFile(logPath, []byte(truncated), 0o644); err != nil { + t.Fatal(err) + } + + entries, err := journal.Entries(logPath, run) + if err != nil { + t.Fatalf("Entries refused a crashed-but-clean run: %v", err) + } + if len(entries) == 0 { + t.Fatal("no entries survived truncation") + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatalf("PlanUndo refused a crashed run: %v", err) + } + if len(up.Files) != 1 || up.Files[0].Refused != "" { + t.Fatalf("crashed run did not yield a usable undo plan: %+v", up) + } +} + +// TestApplyDeclinesLogEachStepAndTouchNothing: a chain that is not named in +// approved is left completely alone, but still logged (spec §9: "declined +// files are [logged]"), one entry per step, status "declined". +func TestApplyDeclinesLogEachStepAndTouchNothing(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + res, err := e.Apply(context.Background(), dp, map[string]bool{}, j, run) + if err != nil { + t.Fatal(err) + } + if res.Declined != 1 || res.Applied != 0 { + t.Errorf("result = %+v; want one declined, none applied", res) + } + if _, err := os.Stat(filepath.Join(h, "dl", "a.pdf")); err != nil { + t.Errorf("a declined file was touched: %v", err) + } + j.Close() + + entries, err := journal.Entries(filepath.Join(h, ".local", "state", "krino", "krino.log"), run) + if err != nil { + t.Fatal(err) + } + var declined *journal.Entry + for i := range entries { + if entries[i].Status == "declined" { + declined = &entries[i] + } + } + if declined == nil { + t.Fatal("no declined entry was logged") + } + if declined.Action != "move" || declined.File != "a.pdf" { + t.Errorf("declined entry = %+v", *declined) + } +} + +// TestApplyChecksContextBetweenFilesNotWithinOne: Ctrl-C finishes the +// current file's chain, logs it, and stops before the next one - spec §11. +// The context is already cancelled before Apply is even called, so the +// boundary check must fire before the first (only actionable) file, proving +// cancellation is honoured rather than ignored. +func TestApplyChecksContextBetweenFilesNotWithinOne(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + res, err := e.Apply(ctx, dp, map[string]bool{"a.pdf": true}, j, run) + if err == nil { + t.Fatal("Apply did not report the cancellation") + } + if len(res.Files) != 0 || res.Applied != 0 { + t.Errorf("result = %+v; want nothing done once already cancelled", res) + } + if _, err := os.Stat(filepath.Join(h, "dl", "a.pdf")); err != nil { + t.Error("a cancelled Apply touched a file") + } +} + +// TestApplyUndoRestoresMovedFile: the smallest possible round trip through +// ApplyUndo, since Task 9's is the only other test that exercises it. +func TestApplyUndoRestoresMovedFile(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused != "" { + t.Fatalf("undo refused: %s", up.Files[0].Refused) + } + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + defer j2.Close() + undoRun := journal.NewRunID(time.Now()) + res, err := e.ApplyUndo(context.Background(), up, j2, undoRun) + if err != nil { + t.Fatal(err) + } + if res.Applied != 1 || res.Failed != 0 { + t.Errorf("undo result = %+v; want one applied, none failed", res) + } + if _, err := os.Stat(filepath.Join(h, "dl", "a.pdf")); err != nil { + t.Errorf("undo did not restore the file: %v", err) + } + if _, err := os.Stat(filepath.Join(h, "dl", "Work", "a.pdf")); !os.IsNotExist(err) { + t.Error("undo left a copy at the moved-to location") + } +} + +// TestApplyUndoSkipsRefusedFiles: rule 4 enforced at execution time too - a +// refused file must come back from ApplyUndo untouched. +func TestApplyUndoSkipsRefusedFiles(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + moved := filepath.Join(h, "dl", "Work", "a.pdf") + if err := os.WriteFile(moved, []byte("edited since the run"), 0o644); err != nil { + t.Fatal(err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused == "" { + t.Fatal("expected the file to be refused") + } + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + defer j2.Close() + res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + if res.Declined != 1 || res.Applied != 0 { + t.Errorf("undo result = %+v; want the refused file declined, nothing applied", res) + } + if got, err := os.ReadFile(moved); err != nil || string(got) != "edited since the run" { + t.Errorf("a refused file was touched: content=%q err=%v", got, err) + } +} + +// TestApplyUndoLogsDeclinedFile is fix round 2026-09-12, item 2 of Task 8's +// review: a file the front end's own review chose not to reverse (Refused +// empty, Declined set by the caller - PlanUndo itself never sets it) must +// still be logged, spec §9's "declined files are logged even though nothing +// happens to them" extended to undo. The file must come back untouched, the +// run must still get its run-start/run-end boundaries even though nothing +// was actually reversed, and the logged entry's status must read "declined", +// never "refused" - which spec §9/§10 give a different meaning (the world +// changed under us). +func TestApplyUndoLogsDeclinedFile(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused != "" { + t.Fatalf("expected the file to be reversible, got refused: %s", up.Files[0].Refused) + } + up.Files[0].Declined = true + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + defer j2.Close() + undoRun := journal.NewRunID(time.Now()) + res, err := e.ApplyUndo(context.Background(), up, j2, undoRun) + if err != nil { + t.Fatal(err) + } + if res.Declined != 1 || res.Applied != 0 { + t.Errorf("undo result = %+v; want the declined file counted, nothing applied", res) + } + if _, err := os.Stat(filepath.Join(h, "dl", "Work", "a.pdf")); err != nil { + t.Errorf("the declined file was moved: %v", err) + } + if _, err := os.Stat(filepath.Join(h, "dl", "a.pdf")); !os.IsNotExist(err) { + t.Error("the declined file's reversal ran anyway") + } + + entries, err := journal.Entries(logPath, undoRun) + if err != nil { + t.Fatal(err) + } + if entries[0].Action != "run-start" || entries[len(entries)-1].Action != "run-end" { + t.Errorf("boundaries = %q .. %q; a run with only a declined file must still get both", entries[0].Action, entries[len(entries)-1].Action) + } + // a.pdf's chain moved it into a directory Apply had to create (spec + // §10: last-original-step-first means undo-move is logged before its + // own undo-mkdir), so more than one entry carries File "a.pdf" - + // every one of them must read "declined", and the first must be the + // file's own undo-move. + var fileEntries []journal.Entry + for _, en := range entries { + if en.File == "a.pdf" { + fileEntries = append(fileEntries, en) + } + } + if len(fileEntries) == 0 { + t.Fatal("no entry was logged for the declined file") + } + if fileEntries[0].Action != "undo-move" { + t.Errorf("first step's action = %q, want the file's own undo-move", fileEntries[0].Action) + } + for _, en := range fileEntries { + if en.Status != "declined" { + t.Errorf("entry %+v: status = %q, want %q (never \"refused\", which means something else)", en, en.Status, "declined") + } + } +} + +// TestApplyUndoDecliningEveryFileDoesNotMarkOriginalRunUndone is fix wave +// item 2 (Important): reproduced by the reviewer via pty as `1 moved +// (undone)` with the file still filed. The mechanism is +// journal.Runs' own (see TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined +// for that unit-level pin); this is the same defect exercised end to end +// through a real forward run, a real declined undo, and e.Runs() itself - +// the exact call `krino log` makes - rather than a hand-built log. +func TestApplyUndoDecliningEveryFileDoesNotMarkOriginalRunUndone(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused != "" { + t.Fatalf("expected the file to be reversible, got refused: %s", up.Files[0].Refused) + } + up.Files[0].Declined = true // the front end's own review declined it + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + 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.Declined != 1 || res.Applied != 0 { + t.Fatalf("undo result = %+v; want the declined file counted, nothing applied", res) + } + if _, err := os.Stat(filepath.Join(h, "dl", "Work", "a.pdf")); err != nil { + t.Fatalf("the declined file was moved: %v", err) + } + + 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 marked Undone, but every file's reversal was declined and nothing moved", run) + } + if byID[undoRun] { + t.Errorf("the undo run %q itself must never read as Undone", undoRun) + } +} + +func TestRunsDelegatesToJournal(t *testing.T) { + _, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + runs, err := e.Runs(0) + if err != nil { + t.Fatal(err) + } + if len(runs) != 1 || runs[0].ID != run { + t.Errorf("runs = %+v, want one run %q", runs, run) + } +} + +// --- Fix round 1 --- + +// TestApplyLogsTrashEntryNameInDetail: fix round 1, item 3. The trash entry +// name must be logged explicitly (Detail), not left to be re-derived from +// Dst's basename - Dst's shape is internal/apply's contract, not undo's, and +// the two must not be secretly coupled. +func TestApplyLogsTrashEntryNameInDetail(t *testing.T) { + h := sandbox(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(dl, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dl, "old.log"), []byte("stale"), 0o644); err != nil { + t.Fatal(err) + } + old := time.Now().Add(-time.Hour) + os.Chtimes(filepath.Join(dl, "old.log"), old, old) + main := writeConfig(t, h, `(include "dl")`, map[string]string{ + "dl": `(path "~/dl")` + "\n" + `(rule "trash-logs" (when (type log)) (delete))`, + }) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims()) + if err != nil { + t.Fatal(err) + } + 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, map[string]bool{"old.log": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + entries, err := journal.Entries(logPath, run) + if err != nil { + t.Fatal(err) + } + var trashEntry *journal.Entry + for i := range entries { + if entries[i].Action == "trash" { + trashEntry = &entries[i] + } + } + if trashEntry == nil { + t.Fatal("no trash entry was logged") + } + if trashEntry.Detail == "" { + t.Fatal("trash entry's Detail does not carry the trash entry name") + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused != "" { + t.Fatalf("undo refused: %s", up.Files[0].Refused) + } + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + defer j2.Close() + res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + if res.Applied != 1 { + t.Errorf("undo result = %+v; want the trashed file restored", res) + } + if _, err := os.Stat(filepath.Join(h, "dl", "old.log")); err != nil { + t.Errorf("undo did not restore the trashed file: %v", err) + } +} + +// TestRunUndoStepTrashReadsEntryNameFromDetailNotDst: fix round 1, item 3, +// isolated. Src is deliberately a path whose basename names no real trash +// entry; only Original.Detail names the real one. If runUndoStep ever goes +// back to deriving the name from Dst (or Src), this fails. +func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { + h := sandbox(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(dl, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(dl, "gone.txt") + if err := os.WriteFile(target, []byte("data"), 0o644); err != nil { + t.Fatal(err) + } + entry, err := trash.Put(target) + 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}, + } + sr := runUndoStep(step) + if sr.Status != "ok" { + t.Fatalf("runUndoStep = %+v; want ok, using Original.Detail's entry name", sr) + } + if _, err := os.Stat(target); err != nil { + t.Errorf("file was not restored: %v", err) + } +} + +// TestPlanUndoRefusesFileModifiedWithinSameSecond: fix round 1, item 4. The +// journal now records ModTime with sub-second precision (RFC3339Nano), so a +// file rewritten within the same whole second as the run must still be +// detected as changed - a .Unix()-granularity comparison would miss this +// and undo would silently move the edited file back over the user's data. +func TestPlanUndoRefusesFileModifiedWithinSameSecond(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + moved := filepath.Join(h, "dl", "Work", "a.pdf") + fi, err := os.Stat(moved) + if err != nil { + t.Fatal(err) + } + sec := fi.ModTime().Truncate(time.Second) + nudge := 100 * time.Millisecond + if sec.Add(nudge).Equal(fi.ModTime()) { + nudge = 700 * time.Millisecond // guaranteed different sub-second offset + } + nudged := sec.Add(nudge) + if err := os.Chtimes(moved, nudged, nudged); err != nil { + t.Fatal(err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused == "" { + t.Fatal("undo did not refuse a file whose mtime changed within the same second") + } +} + +// TestUndoFileStopsAfterFailedFileAffectingStep: fix round 1, item 2. A +// failed undo-move must stop the rest of that file's reversal - continuing +// would leave it half undone (spec §10), even though the later step +// (undo-copy) would, in isolation, have succeeded. +func TestUndoFileStopsAfterFailedFileAffectingStep(t *testing.T) { + h := sandbox(t) + keep := filepath.Join(h, "keep.txt") + if err := os.WriteFile(keep, []byte("do not trash me"), 0o644); err != nil { + t.Fatal(err) + } + j, err := journal.Open(filepath.Join(h, "state", "krino.log")) + if err != nil { + t.Fatal(err) + } + defer j.Close() + e := &Engine{Now: time.Now} + + uf := UndoFile{ + File: "f", Dir: "d", + Steps: []UndoStep{ + // Src does not exist, so the rename underneath fails. + {Action: "undo-move", Src: filepath.Join(h, "no-such-source"), Dst: filepath.Join(h, "sub", "dst.txt")}, + {Action: "undo-copy", Src: keep}, + }, + } + fr, err := e.undoFile(uf, j, "run1") + if err != nil { + t.Fatal(err) + } + if fr.Steps[0].Status != "failed" { + t.Fatalf("step 0 = %+v, want failed", fr.Steps[0]) + } + if fr.Steps[1].Status != "skipped" { + t.Fatalf("step 1 = %+v, want skipped after the file-affecting failure", fr.Steps[1]) + } + if _, err := os.Stat(keep); err != nil { + t.Errorf("the skipped undo-copy still touched its file: %v", err) + } +} + +// TestUndoFileContinuesPastFailedMkdir: fix round 1, item 2's other half - +// a failed undo-mkdir (directory not empty) must NOT stop the rest of the +// file's reversal, unlike every other action. +func TestUndoFileContinuesPastFailedMkdir(t *testing.T) { + h := sandbox(t) + nonEmpty := filepath.Join(h, "nonempty") + if err := os.MkdirAll(nonEmpty, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(nonEmpty, "still-here.txt"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + keep := filepath.Join(h, "keep.txt") + if err := os.WriteFile(keep, []byte("trash me, that's fine"), 0o644); err != nil { + t.Fatal(err) + } + j, err := journal.Open(filepath.Join(h, "state", "krino.log")) + if err != nil { + t.Fatal(err) + } + defer j.Close() + e := &Engine{Now: time.Now} + + uf := UndoFile{ + File: "f", Dir: "d", + Steps: []UndoStep{ + {Action: "undo-mkdir", Src: nonEmpty}, + {Action: "undo-copy", Src: keep}, + }, + } + fr, err := e.undoFile(uf, j, "run2") + if err != nil { + t.Fatal(err) + } + if fr.Steps[0].Status != "failed" { + t.Fatalf("step 0 = %+v, want failed (not empty)", fr.Steps[0]) + } + if fr.Steps[1].Status != "ok" { + t.Fatalf("step 1 = %+v, want ok - a failed undo-mkdir must not stop the rest of the file", fr.Steps[1]) + } + if _, err := os.Stat(keep); !os.IsNotExist(err) { + t.Error("undo-copy after the failed mkdir did not run") + } +} + +// --- Fix round 2 --- + +// TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution: fix round 2, +// item 1 (Critical). Spec §10 says an undo plan is shown and approved like +// any other, so there is a real, human-length window between PlanUndo's +// refuseIfSrcExists check and ApplyUndo actually running - long enough for +// something else to create a file at the reversal's destination in between. +// undo-move/undo-rename must re-check at execution time rather than let a +// bare os.Rename silently replace it and report the step "ok". +func TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution(t *testing.T) { + h, e, dp, j, run := applyFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused != "" { + t.Fatalf("undo refused at planning time: %s", up.Files[0].Refused) + } + + // The window spec §10 describes: something creates a file at the + // reversal's destination after planning, before execution. + reappeared := filepath.Join(h, "dl", "a.pdf") + if err := os.WriteFile(reappeared, []byte("someone else's file"), 0o644); err != nil { + t.Fatal(err) + } + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + defer j2.Close() + res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + if res.Failed != 1 || res.Applied != 0 { + t.Errorf("undo result = %+v; want the step to fail rather than silently overwrite", res) + } + if got, err := os.ReadFile(reappeared); err != nil || string(got) != "someone else's file" { + t.Errorf("the reappeared file was overwritten: content=%q err=%v", got, err) + } + moved := filepath.Join(h, "dl", "Work", "a.pdf") + if got, err := os.ReadFile(moved); err != nil || string(got) != "one" { + t.Errorf("the moved file did not stay where it was: content=%q err=%v", got, err) + } +} + +// TestApplyUndoDoesNotCountFailedMkdirAsFailed: fix round 2, item 3. A file +// whose only failure is an undo-mkdir (a shared directory not yet empty) +// must not flip ApplyResult.Failed - Task 7 maps that to krino undo's exit +// code, and ruling 4 (fix round 1, item 1) established that this specific +// refusal is tidiness, not a hazard. +func TestApplyUndoDoesNotCountFailedMkdirAsFailed(t *testing.T) { + h := sandbox(t) + dir := filepath.Join(h, "Work") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + dst := filepath.Join(dir, "a.pdf") + if err := os.WriteFile(dst, []byte("moved"), 0o644); err != nil { + t.Fatal(err) + } + // A sibling file still occupies the directory, so its undo-mkdir must + // fail with "not empty" once undo-move has already vacated dst. + if err := os.WriteFile(filepath.Join(dir, "sibling.pdf"), []byte("still here"), 0o644); err != nil { + t.Fatal(err) + } + src := filepath.Join(h, "a.pdf") + + j, err := journal.Open(filepath.Join(h, "state", "krino.log")) + if err != nil { + t.Fatal(err) + } + defer j.Close() + e := &Engine{Now: time.Now} + + up := &UndoPlan{Run: "r", Files: []UndoFile{ + {File: "a.pdf", Dir: "d", Steps: []UndoStep{ + {Action: "undo-move", Src: dst, Dst: src}, + {Action: "undo-mkdir", Src: dir}, + }}, + }} + res, err := e.ApplyUndo(context.Background(), up, j, "run1") + if err != nil { + t.Fatal(err) + } + if res.Applied != 1 { + t.Errorf("Applied = %d, want 1 (the move succeeded)", res.Applied) + } + if res.Failed != 0 { + t.Errorf("Failed = %d, want 0 - a failed undo-mkdir alone must not count as a failure", res.Failed) + } +} + +// --- Fix wave (2026-09-12) --- + +// overwriteFixture builds a directory where a forward move under +// (on-conflict overwrite) will displace a pre-existing file at its +// destination: dl/incoming.pdf moves to dl/Work/incoming.pdf, which already +// holds a different file (the "victim") the move must trash first. This is +// the one shape that makes a step's Displaces and another step's Dst name +// the exact same path (internal/plan/conflict.go's resolveConflict, +// deliberately), which is what fix wave item 1 (Critical) is about. +func overwriteFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, string) { + t.Helper() + h := sandbox(t) + dl := filepath.Join(h, "dl") + work := filepath.Join(dl, "Work") + if err := os.MkdirAll(work, 0o755); err != nil { + t.Fatal(err) + } + old := time.Now().Add(-time.Hour) + + incoming := filepath.Join(dl, "incoming.pdf") + if err := os.WriteFile(incoming, []byte("incoming content"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(incoming, old, old); err != nil { + t.Fatal(err) + } + victim := filepath.Join(work, "incoming.pdf") + if err := os.WriteFile(victim, []byte("original victim content"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(victim, old, old); err != nil { + t.Fatal(err) + } + + main := writeConfig(t, h, `(include "dl")`, map[string]string{ + "dl": `(path "~/dl")` + "\n" + `(on-conflict overwrite)` + "\n" + `(rule "pdfs" (when (type pdf)) (move "Work"))`, + }) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims()) + if err != nil { + t.Fatal(err) + } + j, err := journal.Open(filepath.Join(h, ".local", "state", "krino", "krino.log")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { j.Close() }) + return h, e, dp, j, journal.NewRunID(time.Now()) +} + +// TestApplyUndoReversesOverwriteRoundTrip is fix wave item 1 (CRITICAL): the +// end-to-end reproduction of the defect the final-plan review found - +// `krino undo` could not reverse a run that used (on-conflict overwrite) at +// all, by construction. reverseStep's planning-time occupancy check judged +// the displace reversal against the world exactly as it stood before any +// reversal had run, while the move-back that frees the contested path is +// ordered to execute first (reversal is last-original-step-first), so the +// displace reversal was refused every time and, being file-affecting, +// aborted the whole file's reversal - including the otherwise-safe +// move-back. This is the first coverage of undo-displace anywhere in the +// repo (grep undo-displace across every prior test returns nothing), and it +// is built from a REAL forward run through overwriteFixture's real +// displacing apply, per the brief: a hand-assembled journal.Entry is +// exactly what would let a narrower, wrong fix pass while still being +// wrong. +func TestApplyUndoReversesOverwriteRoundTrip(t *testing.T) { + h, e, dp, j, run := overwriteFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"incoming.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + dest := filepath.Join(h, "dl", "Work", "incoming.pdf") + if got, err := os.ReadFile(dest); err != nil || string(got) != "incoming content" { + t.Fatalf("forward run did not land as expected: content=%q err=%v", got, err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if len(up.Files) != 1 { + t.Fatalf("undo plan covers %d files, want 1", len(up.Files)) + } + if up.Files[0].Refused != "" { + t.Fatalf("undo refused an (on-conflict overwrite) round trip that should be fully reversible: %s", up.Files[0].Refused) + } + var sawDisplace bool + for _, s := range up.Files[0].Steps { + if s.Action == "undo-displace" { + sawDisplace = true + if s.Refused != "" { + t.Errorf("undo-displace step itself refused: %s", s.Refused) + } + } + } + if !sawDisplace { + t.Fatal("no undo-displace step in the plan; the fixture did not exercise the displace path") + } + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + defer j2.Close() + res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + if res.Applied != 1 || res.Failed != 0 || res.Declined != 0 { + t.Fatalf("undo result = %+v; want the one file fully reversed", res) + } + + orig := filepath.Join(h, "dl", "incoming.pdf") + if got, err := os.ReadFile(orig); err != nil || string(got) != "incoming content" { + t.Errorf("the incoming file did not come back to its original path: content=%q err=%v", got, err) + } + if got, err := os.ReadFile(dest); err != nil || string(got) != "original victim content" { + t.Errorf("the displaced original was not restored from the Trash: content=%q err=%v", got, err) + } +} + +// TestApplyUndoStillRefusesGenuineOccupant is fix wave item 1's second +// required test: the projection must only excuse a path an earlier step of +// THIS SAME chain is about to vacate, never turn every occupancy refusal +// into a pass. Here something outside the chain entirely - not the +// displaced original, not the incoming file itself - now occupies the +// path the move-back needs, and no step of this file's reversal will ever +// free it. +func TestApplyUndoStillRefusesGenuineOccupant(t *testing.T) { + h, e, dp, j, run := overwriteFixture(t) + if _, err := e.Apply(context.Background(), dp, map[string]bool{"incoming.pdf": true}, j, run); err != nil { + t.Fatal(err) + } + j.Close() + + reappeared := filepath.Join(h, "dl", "incoming.pdf") + if err := os.WriteFile(reappeared, []byte("someone else's file"), 0o644); err != nil { + t.Fatal(err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + if up.Files[0].Refused == "" { + t.Fatal("undo did not refuse a path genuinely occupied by something outside this file's own chain") + } + if !strings.Contains(up.Files[0].Refused, "already exists") { + t.Errorf("Refused = %q, want it to say the path already exists", up.Files[0].Refused) + } + if got, err := os.ReadFile(reappeared); err != nil || string(got) != "someone else's file" { + t.Errorf("the genuine occupant was disturbed just by planning: content=%q err=%v", got, err) + } +} + +// TestTallyFileCountsAnAllSkippedFileAsDeclined is fix wave item 4 / Minor +// 6: a file every one of whose steps came back "skipped" - the shape an +// approved all-skipped chain used to take - set none of ok/failed/declined +// in tallyFile, so it fell out of the outcome tally entirely: "0 applied · +// 0 failed · 0 declined" for a file the user was asked about and approved. +// tallyFile must land every file it is given in exactly one bucket; nothing +// ran and nothing failed, so it belongs in Declined. +func TestTallyFileCountsAnAllSkippedFileAsDeclined(t *testing.T) { + result := &ApplyResult{} + steps := []apply.StepResult{ + {Status: "skipped", Detail: "target exists"}, + } + tallyFile(result, steps, nil) + if result.Applied != 0 || result.Failed != 0 || result.Declined != 1 { + t.Errorf("result = %+v, want the all-skipped file counted once, as declined", result) + } +} + +// TestTallyFileCountsMixedOutcomesOnceEach pins the existing "not mutually +// exclusive" contract alongside the new all-skipped fallback: a file with +// one ok, one failed and one declined step must still count toward all +// three (unchanged behaviour), and the fallback added for the all-skipped +// case must never fire when any real status is present. +func TestTallyFileCountsMixedOutcomesOnceEach(t *testing.T) { + result := &ApplyResult{} + steps := []apply.StepResult{ + {Status: "ok"}, + {Status: "failed"}, + {Status: "declined"}, + } + tallyFile(result, steps, nil) + if result.Applied != 1 || result.Failed != 1 || result.Declined != 1 { + t.Errorf("result = %+v, want one of each", result) + } +} diff --git a/internal/engine/roundtrip_test.go b/internal/engine/roundtrip_test.go new file mode 100644 index 0000000..425dc1a --- /dev/null +++ b/internal/engine/roundtrip_test.go @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package engine + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "io/fs" + "os" + "path/filepath" + "reflect" + "testing" + "time" + + "krino/internal/journal" + "krino/internal/plan" +) + +// snapshot records every file under root: path, content hash, mode and mtime. +func snapshot(t *testing.T, root string) map[string]string { + t.Helper() + out := map[string]string{} + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + b, err := os.ReadFile(p) + if err != nil { + return err + } + fi, err := d.Info() + if err != nil { + return err + } + rel, _ := filepath.Rel(root, p) + sum := sha256.Sum256(b) + out[rel] = hex.EncodeToString(sum[:]) + " " + fi.Mode().String() + " " + fi.ModTime().UTC().Format(time.RFC3339Nano) + return nil + }) + if err != nil { + t.Fatal(err) + } + return out +} + +func TestApplyThenUndoRestoresTheTree(t *testing.T) { + h := sandbox(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(filepath.Join(dl, "sub"), 0o755); err != nil { + t.Fatal(err) + } + files := map[string]string{ + "inv1.pdf": "invoice one", + "inv2.pdf": "invoice two", + "notes.txt": "not a pdf", + "sub/deep.pdf": "nested", + } + old := time.Now().Add(-2 * time.Hour) + for rel, body := range files { + p := filepath.Join(dl, rel) + if err := os.WriteFile(p, []byte(body), 0o640); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(p, old, old); err != nil { + t.Fatal(err) + } + } + before := snapshot(t, dl) + + main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` +(path "~/dl") +(recursive yes) +(rule "pdfs" (when (type pdf)) (copy "~/backup") (move "Work/{mtime:%Y}")) +`}) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + 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()) + res, err := e.Apply(context.Background(), dp, approved, j, run) + if err != nil { + t.Fatal(err) + } + j.Close() + if res.Failed != 0 { + t.Fatalf("%d files failed: %+v", res.Failed, res) + } + if reflect.DeepEqual(snapshot(t, dl), before) { + t.Fatal("apply changed nothing") + } + if _, err := os.Stat(filepath.Join(h, "backup", "inv1.pdf")); err != nil { + t.Errorf("the copy did not land: %v", err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + for _, f := range up.Files { + if f.Refused != "" { + t.Fatalf("undo refused %s: %s", f.File, f.Refused) + } + } + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + if _, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())); err != nil { + t.Fatal(err) + } + j2.Close() + + // Verify the move cycle within dl was undone completely. + got := snapshot(t, dl) + for rel, want := range before { + if got[rel] != want { + t.Errorf("%s after undo:\n got %s\nwant %s", rel, got[rel], want) + } + } + for rel := range got { + if _, ok := before[rel]; !ok { + t.Errorf("%s exists after undo but did not before", rel) + } + } + + // Verify the copy-undo removed all copies from ~/backup. + // undo-copy sends them to trash, so backup should be gone (or exist but + // contain none of inv1.pdf, inv2.pdf, deep.pdf). + backupDir := filepath.Join(h, "backup") + copied := []string{"inv1.pdf", "inv2.pdf", "deep.pdf"} + for _, name := range copied { + p := filepath.Join(backupDir, name) + if _, err := os.Stat(p); err == nil { + t.Errorf("copy %s still exists after undo", name) + } else if !os.IsNotExist(err) { + t.Errorf("checking %s after undo: %v", name, err) + } + } + // Also check that if backupDir exists, it is empty (no copies remain). + if entries, err := os.ReadDir(backupDir); err == nil { + if len(entries) > 0 { + t.Errorf("backup dir not empty after undo: %v", entries) + } + } else if !os.IsNotExist(err) { + t.Errorf("reading backup dir after undo: %v", err) + } +} diff --git a/internal/journal/journal.go b/internal/journal/journal.go new file mode 100644 index 0000000..465cd51 --- /dev/null +++ b/internal/journal/journal.go @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +// Package journal is the append-only log every krino action is recorded in, +// and the only record krino undo reads back. See docs/design.md §9. +package journal + +import ( + "crypto/rand" + "encoding/hex" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "time" + "unicode/utf8" +) + +// Entry is one logged event. Field order here IS the column order in the +// file; never reorder it. +type Entry struct { + Time time.Time // RFC 3339 with offset + Run string // e.g. "20260911T100203-4f2a" + Dir string // the directory's name from krino.conf + File string // the file's Rel within that directory + Step int // 1-based index within the file's chain; 0 for run-start/run-end + Action string // run-start mkdir copy move rename trash delete displace run-end, and undo- forms + Status string // ok failed skipped declined + Rule string + Src string + Dst string + Size int64 // of the file at Dst after the step + ModTime time.Time // of the file at Dst after the step + Detail string +} + +// NewRunID returns "<t as 20060102T150405>-<4 hex>": a run identifier that +// sorts lexically by start time and does not collide across runs started in +// the same second. +func NewRunID(t time.Time) string { + var b [2]byte + _, _ = rand.Read(b[:]) // crypto/rand.Read never fails on supported platforms + return t.Format("20060102T150405") + "-" + hex.EncodeToString(b[:]) +} + +// Writer appends entries to a log file, one line per Append call. +type Writer struct { + f *os.File +} + +// Open opens the log at path for appending, creating its parent directories +// and the file itself if necessary. It never truncates an existing log. +func Open(path string) (*Writer, error) { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return nil, fmt.Errorf("journal: %w", err) + } + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if err != nil { + return nil, fmt.Errorf("journal: %w", err) + } + return &Writer{f: f}, nil +} + +// Append writes e as one line and flushes it before returning. The whole +// line is written with a single Write call so that two concurrent runs +// appending to the same file cannot interleave a partial line. +// +// Time and ModTime are formatted with RFC3339Nano, not RFC3339: spec §9 +// asks for "RFC 3339 with offset", which RFC3339Nano still is (it only adds +// an optional fractional-second field; a zero-nanosecond time formats +// identically under both). Task 5's undo needs the fractional seconds: a +// refusal check comparing a file's current mtime against the mtime this +// line records must not be fooled by a file rewritten within the same +// whole second. read.go's parser already accepts fractional seconds under +// either constant (a documented time.Parse special case for RFC3339), so +// only this side needed to change. +func (w *Writer) Append(e Entry) error { + line := strings.Join([]string{ + e.Time.Format(time.RFC3339Nano), + escape(e.Run), + escape(e.Dir), + escape(e.File), + strconv.Itoa(e.Step), + escape(e.Action), + escape(e.Status), + escape(e.Rule), + escape(e.Src), + escape(e.Dst), + strconv.FormatInt(e.Size, 10), + e.ModTime.Format(time.RFC3339Nano), + escape(e.Detail), + }, "\t") + "\n" + if _, err := w.f.Write([]byte(line)); err != nil { + return fmt.Errorf("journal: %w", err) + } + return nil +} + +// Close closes the underlying file. +func (w *Writer) Close() error { + if err := w.f.Close(); err != nil { + return fmt.Errorf("journal: %w", err) + } + return nil +} + +// escape encodes s so it can never contain a tab or a newline, and so every +// byte round-trips exactly: \t, \n, \\ are backslash-escaped, and every +// other control byte or byte that is not part of valid UTF-8 becomes \xNN. +// A byte loop is used rather than strconv.Quote, which would also escape +// non-ASCII text and make names like "zażółć" unreadable in the log. +func escape(s string) string { + if !needsEscape(s) { + return s + } + var b strings.Builder + b.Grow(len(s) + 8) + i := 0 + for i < len(s) { + c := s[i] + switch c { + case '\t': + b.WriteString(`\t`) + i++ + continue + case '\n': + b.WriteString(`\n`) + i++ + continue + case '\\': + b.WriteString(`\\`) + i++ + continue + } + if c < 0x20 || c == 0x7f { + fmt.Fprintf(&b, `\x%02x`, c) + i++ + continue + } + r, size := utf8.DecodeRuneInString(s[i:]) + if r == utf8.RuneError && size <= 1 { + fmt.Fprintf(&b, `\x%02x`, c) + i++ + continue + } + b.WriteString(s[i : i+size]) + i += size + } + return b.String() +} + +// needsEscape reports whether s contains anything escape would change, so +// the common case (a plain name) avoids allocating a builder. +func needsEscape(s string) bool { + for i := 0; i < len(s); i++ { + c := s[i] + if c == '\t' || c == '\n' || c == '\\' || c < 0x20 || c == 0x7f || c >= 0x80 { + return true + } + } + return false +} diff --git a/internal/journal/journal_test.go b/internal/journal/journal_test.go new file mode 100644 index 0000000..a95bb05 --- /dev/null +++ b/internal/journal/journal_test.go @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package journal + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestNewRunID(t *testing.T) { + at := time.Date(2026, 9, 11, 10, 2, 3, 0, time.UTC) + id := NewRunID(at) + if !strings.HasPrefix(id, "20260911T100203-") || len(id) != len("20260911T100203-")+4 { + t.Fatalf("run id = %q", id) + } + if NewRunID(at) == id { + t.Error("two run ids for the same instant collided; the suffix is not random") + } +} + +// TestRoundTripsAwkwardNames is the point of the escaping: a file name with a +// tab, a newline, a backslash, a control byte or invalid UTF-8 must come back +// byte-for-byte, and must not break the line or column structure awk sees. +func TestRoundTripsAwkwardNames(t *testing.T) { + names := []string{ + "plain.pdf", + "with\ttab.pdf", + "with\nnewline.pdf", + "back\\slash.pdf", + "bell\a.pdf", + "invalid\xff\xfeutf8.pdf", + "zażółć gęślą jaźń.pdf", + } + path := filepath.Join(t.TempDir(), "state", "krino.log") + w, err := Open(path) + if err != nil { + t.Fatal(err) + } + at := time.Date(2026, 9, 11, 10, 2, 3, 0, time.FixedZone("CEST", 2*3600)) + if err := w.Append(Entry{Time: at, Run: "R", Action: "run-start", Status: "ok"}); err != nil { + t.Fatal(err) + } + for i, n := range names { + e := Entry{Time: at, Run: "R", Dir: "dl", File: n, Step: i + 1, + Action: "move", Status: "ok", Rule: "acme", Src: "/a/" + n, Dst: "/b/" + n, + Size: int64(i), ModTime: at, Detail: ""} + if err := w.Append(e); 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) + } + if got := strings.Count(string(raw), "\n"); got != len(names)+1 { + t.Errorf("file has %d lines, want %d: a field broke the line structure", got, len(names)+1) + } + for _, line := range strings.Split(strings.TrimRight(string(raw), "\n"), "\n") { + if n := strings.Count(line, "\t"); n != 12 { + t.Errorf("line has %d tabs, want 12 (13 columns): %q", n, line) + } + } + + got, err := Entries(path, "R") + if err != nil { + t.Fatal(err) + } + if len(got) != len(names)+1 { + t.Fatalf("read %d entries, want %d", len(got), len(names)+1) + } + for i, n := range names { + if got[i+1].File != n { + t.Errorf("entry %d: File = %q, want %q", i, got[i+1].File, n) + } + if got[i+1].Src != "/a/"+n || got[i+1].Dst != "/b/"+n { + t.Errorf("entry %d: paths did not round-trip: %q %q", i, got[i+1].Src, got[i+1].Dst) + } + if !got[i+1].Time.Equal(at) { + t.Errorf("entry %d: Time = %v, want %v", i, got[i+1].Time, at) + } + } +} + +// TestAppendWritesRFC3339WithOffsetAndKeepsNanoseconds is item 13, promoted +// to before-commit by the plan 4 final review: nothing anywhere pinned the +// journal's on-disk time format - RFC3339Nano appears in no test file, and +// every timestamp assertion round-trips through krino's own Writer and +// Entries, so a change to something no other tool could parse would pass +// silently. The journal is the only record undo has. This reads the RAW +// bytes of a written line - not Entries, which would launder the format +// through krino's own parser - and asserts column 1 parses as RFC 3339 with +// a real numeric offset (not just "Z"), and that a time carrying +// nanoseconds keeps them. +func TestAppendWritesRFC3339WithOffsetAndKeepsNanoseconds(t *testing.T) { + path := filepath.Join(t.TempDir(), "state", "krino.log") + w, err := Open(path) + if err != nil { + t.Fatal(err) + } + at := time.Date(2026, 9, 11, 10, 2, 3, 123456789, time.FixedZone("", 2*3600)) + if err := w.Append(Entry{Time: at, Run: "R", Action: "run-start", 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) + } + line := strings.TrimSuffix(string(raw), "\n") + col1 := strings.SplitN(line, "\t", 2)[0] + + parsed, err := time.Parse(time.RFC3339, col1) + if err != nil { + t.Fatalf("column 1 %q does not parse as RFC 3339: %v", col1, err) + } + if _, offset := parsed.Zone(); offset != 2*3600 { + t.Errorf("offset = %ds, want %ds: the written column must carry a real offset, not just a bare local time", offset, 2*3600) + } + if parsed.Nanosecond() != 123456789 { + t.Errorf("nanoseconds = %d, want 123456789: a nanosecond-precision time must not be truncated on the wire", parsed.Nanosecond()) + } +} + +func TestAppendIsAppendOnly(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + for i := 0; i < 2; i++ { + w, err := Open(path) + if err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: time.Now(), Run: "R", Action: "run-start", Status: "ok"}); err != nil { + t.Fatal(err) + } + w.Close() + } + raw, _ := os.ReadFile(path) + if got := strings.Count(string(raw), "\n"); got != 2 { + t.Errorf("%d lines after two Open/Append/Close cycles, want 2: the second Open truncated", got) + } +} diff --git a/internal/journal/read.go b/internal/journal/read.go new file mode 100644 index 0000000..3de15d2 --- /dev/null +++ b/internal/journal/read.go @@ -0,0 +1,344 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package journal + +import ( + "fmt" + "os" + "sort" + "strconv" + "strings" + "time" +) + +// wantFields is the number of tab-separated columns a well-formed line has. +const wantFields = 13 + +// undoOfPrefix is the exact, single-source convention linking an undo run +// back to the run it reverses: an undo run's run-start entry carries +// Detail = undoOfPrefix + the original run's ID, verbatim, and nowhere +// else - never repeated on the undo-* step entries, so there is only one +// place to write it and one place that can drift. A log truncated before +// its run-start line loses the link; the accepted cost is that `krino undo` +// may then re-offer an already-reversed run, whose per-file refusal checks +// decline every file because they are already back in place. +const undoOfPrefix = "undo of " + +// UndoOf returns the Detail value an undo run's run-start entry carries to +// record which run it reverses (see undoOfPrefix). Fix wave item 2 / +// final-wave item 17: before this, internal/engine wrote the same text as +// a bare string literal with nothing tying it to undoOfPrefix, so a typo in +// either would silently break Runs' Undone marking while every test stayed +// green. This is the one place that string is built; internal/engine calls +// it rather than keeping its own copy. +func UndoOf(run string) string { + return undoOfPrefix + run +} + +// Run summarises one logged run, for `krino log` and for choosing what +// `krino undo` reverses. +type Run struct { + ID string + Start time.Time + Dirs []string + Counts map[string]int // action -> count of status "ok" + Undone bool // a later run reversed this one +} + +// Entries returns every entry belonging to runID, in file order. A line +// that fails to parse is skipped, but Entries fails closed within the run's +// own window - from its run-start line to its run-end line, or to end of +// file when there is no run-end (a crashed run, which is precisely when +// corruption is likely): any unparsable line found inside that window sets +// the returned error, whether or not the line's own Run column can still be +// read back. The mere possibility that it belonged to this run is enough, +// because an incomplete chain must refuse the whole run rather than let an +// undo reverse it partway (spec §10). A line outside the window is ignored +// even when unparsable, since it cannot belong to this run. +// +// The residual risk this leaves is a false refusal, not a false success: +// krino's lock is per directory, not global, so two processes could in +// principle write to the log at once, and an unattributable corrupt line +// that falls inside this run's window might really belong to the other +// run - this run would then be refused unnecessarily. That is the safe +// direction, and it is rare. A nil error is what proves the run's chain +// parsed completely; a non-nil error is proof only that it cannot be +// trusted as complete, not that the run itself is corrupt. +// +// A run also fails closed if it has no readable run-start: every run Apply +// writes begins with one, so once at least one entry for the run has +// parsed, a missing run-start means either corruption or a log truncated +// at the front, and either way the chain cannot be trusted. The same rule +// does not apply to run-end - a crashed run legitimately has none, and the +// window rule above already covers that case correctly. The cost is +// symmetric with the one above: if the log's front were ever trimmed, the +// oldest surviving run would refuse to undo. krino never trims the log - +// it is append-only with no rotation - so this only bites a hand-edited +// file, which is exactly the case where refusing is right. +func Entries(path, runID string) ([]Entry, error) { + lines, err := readLines(path) + if err != nil { + return nil, err + } + var out []Entry + badLine := 0 + inWindow := false + sawRunStart := false + for i, line := range lines { + e, ok := parseLine(line) + if ok { + if e.Run != runID { + continue + } + out = append(out, e) + switch e.Action { + case "run-start": + inWindow = true + sawRunStart = true + case "run-end": + inWindow = false + } + continue + } + if badLine != 0 { + continue + } + if inWindow { + badLine = i + 1 + continue + } + if run, found := runFieldOf(line); found && run == runID { + badLine = i + 1 + } + } + if badLine != 0 { + return out, fmt.Errorf("journal: entries: run %s: unparsable line %d", runID, badLine) + } + if len(out) > 0 && !sawRunStart { + return out, fmt.Errorf("journal: entries: run %s: no readable run-start", runID) + } + return out, nil +} + +// runFieldOf best-effort extracts a line's Run column even when the line +// otherwise fails to parse, so Entries can tell whether an unparsable line +// belonged to the run it was asked for. +func runFieldOf(line string) (string, bool) { + f := strings.SplitN(line, "\t", 3) + if len(f) < 2 { + return "", false + } + return unescape(f[1]), true +} + +// Runs summarises every run found in the log, newest first. n <= 0 means +// all. As with Entries, an unparsable line is skipped rather than failing +// the read - here silently and always, even when it belonged to the run +// being summarised: a listing that refuses to print anything because one +// old line is corrupt is worse than one that just omits it. +// +// A run is marked Undone when a later run's run-start entry's Detail is +// undoOfPrefix followed by this run's ID, AND that later run actually +// reversed something (fix wave item 2): a fully declined undo - every file +// the reviewer chose not to reverse - still opens with that same run-start +// (ApplyUndo logs a declined file exactly as spec §9 asks the forward path +// to), so the Detail alone is not proof anything happened. Reproduced by +// the reviewer: `krino undo` with every file declined left `krino log` +// reporting the original run "(undone)" regardless. What actually happened +// is provable from the same file: at least one "ok" undo-* entry. +func Runs(path string, n int) ([]Run, error) { + lines, err := readLines(path) + if err != nil { + return nil, err + } + + order := make([]string, 0) + byID := make(map[string]*Run) + pendingUndo := make(map[string]string) // undo run ID -> the run ID it claims to undo + + for _, line := range lines { + e, ok := parseLine(line) + if !ok { + continue + } + r, seen := byID[e.Run] + if !seen { + r = &Run{ID: e.Run, Start: e.Time, Counts: make(map[string]int)} + byID[e.Run] = r + order = append(order, e.Run) + } + if e.Dir != "" && !contains(r.Dirs, e.Dir) { + r.Dirs = append(r.Dirs, e.Dir) + } + if e.Status == "ok" { + r.Counts[e.Action]++ + } + if e.Action == "run-start" { + if orig, ok := strings.CutPrefix(e.Detail, undoOfPrefix); ok && orig != "" { + pendingUndo[e.Run] = orig + } + } + } + + // Resolved only once the whole file has been scanned: an undo run's + // run-start line - and therefore its claim on pendingUndo - is always + // written before its own step entries, so whether it actually reversed + // anything cannot be known until its Counts are complete. + undoes := make(map[string]bool) // run IDs actually reversed by some later run + for undoRun, orig := range pendingUndo { + if r, ok := byID[undoRun]; ok && ranAnyUndoStep(r.Counts) { + undoes[orig] = true + } + } + + runs := make([]Run, len(order)) + for i, id := range order { + runs[i] = *byID[id] + } + sort.SliceStable(runs, func(i, j int) bool { return runs[i].Start.After(runs[j].Start) }) + for i := range runs { + runs[i].Undone = undoes[runs[i].ID] + } + + if n > 0 && n < len(runs) { + runs = runs[:n] + } + return runs, nil +} + +// ranAnyUndoStep reports whether counts - a run's own tally of "ok" actions, +// by action name - includes at least one undo- action that actually +// restored something, as opposed to merely having been started and then +// declining every file (Important 2), or having failed to restore anything +// while a wholly unrelated undo-mkdir still happened to succeed (the +// coordinator's tightening of that same fix): "undo-mkdir" is deliberately +// excluded, the one undo- action package journal cannot help but name +// directly (this package must not import internal/engine to reuse its +// isFileAffecting predicate - journal is the lower layer), but which draws +// exactly the same line that predicate does. Removing a directory once it +// turns out empty is tidiness, not a restoration: a file's own chain stops +// after a failed file-affecting reversal, but a failed or refused +// undo-mkdir never stops anything (see internal/engine's isFileAffecting +// and undoFile), so it can succeed for one file while every file-affecting +// reversal in the whole run failed - and marking the original run Undone +// from that alone would be Important 2's bug again, by a narrower route. +func ranAnyUndoStep(counts map[string]int) bool { + for action, n := range counts { + if n > 0 && action != "undo-mkdir" && strings.HasPrefix(action, "undo-") { + return true + } + } + return false +} + +func contains(ss []string, s string) bool { + for _, x := range ss { + if x == s { + return true + } + } + return false +} + +// readLines reads path and splits it into lines, dropping the single +// trailing empty element a final newline produces. It does not itself +// validate line structure; parseLine does that per line. +func readLines(path string) ([]string, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("journal: %w", err) + } + if len(data) == 0 { + return nil, nil + } + lines := strings.Split(string(data), "\n") + if lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + return lines, nil +} + +// parseLine parses one log line into an Entry. It reports false for +// anything that does not look like a complete, well-formed line: wrong +// column count, or a time/step/size column that does not parse. That is the +// only contract a crash mid-write needs: the truncated final line always +// fails one of these checks, and everything before it still parses. +func parseLine(line string) (Entry, bool) { + f := strings.Split(line, "\t") + if len(f) != wantFields { + return Entry{}, false + } + t, err := time.Parse(time.RFC3339, f[0]) + if err != nil { + return Entry{}, false + } + step, err := strconv.Atoi(f[4]) + if err != nil { + return Entry{}, false + } + size, err := strconv.ParseInt(f[10], 10, 64) + if err != nil { + return Entry{}, false + } + mtime, err := time.Parse(time.RFC3339, f[11]) + if err != nil { + return Entry{}, false + } + return Entry{ + Time: t, + Run: unescape(f[1]), + Dir: unescape(f[2]), + File: unescape(f[3]), + Step: step, + Action: unescape(f[5]), + Status: unescape(f[6]), + Rule: unescape(f[7]), + Src: unescape(f[8]), + Dst: unescape(f[9]), + Size: size, + ModTime: mtime, + Detail: unescape(f[12]), + }, true +} + +// unescape reverses escape: \t, \n, \\ and \xNN. Any other backslash +// sequence - which a well-formed log never contains - is left as a literal +// backslash rather than silently eaten, so a hand-edited line does not lose +// data. +func unescape(s string) string { + if !strings.Contains(s, `\`) { + return s + } + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); i++ { + c := s[i] + if c != '\\' || i+1 >= len(s) { + b.WriteByte(c) + continue + } + switch s[i+1] { + case 't': + b.WriteByte('\t') + i++ + case 'n': + b.WriteByte('\n') + i++ + case '\\': + b.WriteByte('\\') + i++ + case 'x': + if i+3 < len(s) { + if v, err := strconv.ParseUint(s[i+2:i+4], 16, 8); err == nil { + b.WriteByte(byte(v)) + i += 3 + continue + } + } + b.WriteByte(c) + default: + b.WriteByte(c) + } + } + return b.String() +} diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go new file mode 100644 index 0000000..3ffa14d --- /dev/null +++ b/internal/journal/read_test.go @@ -0,0 +1,413 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package journal + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestRunsListsNewestFirst(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + t0 := time.Date(2026, 9, 11, 9, 0, 0, 0, time.UTC) + t1 := t0.Add(time.Hour) + for _, r := range []struct { + id string + at time.Time + dirs []string + }{{"A", t0, []string{"dl"}}, {"B", t1, []string{"dl", "docs"}}} { + w.Append(Entry{Time: r.at, Run: r.id, Action: "run-start", Status: "ok"}) + for _, d := range r.dirs { + w.Append(Entry{Time: r.at, Run: r.id, Dir: d, File: "x.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}) + } + w.Append(Entry{Time: r.at, Run: r.id, Action: "run-end", Status: "ok"}) + } + w.Close() + + runs, err := Runs(path, 0) + if err != nil { + t.Fatal(err) + } + if len(runs) != 2 || runs[0].ID != "B" || runs[1].ID != "A" { + t.Fatalf("runs = %+v; want B then A", runs) + } + if len(runs[0].Dirs) != 2 || runs[0].Dirs[0] != "dl" || runs[0].Dirs[1] != "docs" { + t.Errorf("run B dirs = %v, want [dl docs] in first-seen order", runs[0].Dirs) + } + if runs[0].Counts["move"] != 2 { + t.Errorf("run B move count = %d, want 2", runs[0].Counts["move"]) + } + if !runs[0].Start.Equal(t1) { + t.Errorf("run B start = %v, want %v", runs[0].Start, t1) + } + if runs, err = Runs(path, 1); err != nil || len(runs) != 1 || runs[0].ID != "B" { + t.Errorf("Runs(path, 1) = %+v, %v", runs, err) + } +} + +// TestTruncatedLastLineIsSkipped: a crash mid-write must not make the log +// unreadable - everything before the broken line still parses. +func TestTruncatedLastLineIsSkipped(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + w.Append(Entry{Time: time.Now(), Run: "A", Action: "run-start", Status: "ok"}) + w.Close() + f, _ := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0o644) + f.WriteString("2026-09-11T10:02:03+02:00\tA\tdl\thalf-written") + f.Close() + + runs, err := Runs(path, 0) + if err != nil { + t.Fatalf("a truncated final line made the whole log unreadable: %v", err) + } + if len(runs) != 1 || runs[0].ID != "A" { + t.Errorf("runs = %+v; want the complete run A", runs) + } +} + +// TestRunsMarksAnUndoneRun: an undo run's run-start Detail names the run it +// reverses, in the exact format "undo of <run id>". Runs must mark that +// earlier run Undone, and must not mark the undo run itself. +func TestRunsMarksAnUndoneRun(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + t0 := time.Date(2026, 9, 11, 9, 0, 0, 0, time.UTC) + t1 := t0.Add(time.Hour) + w.Append(Entry{Time: t0, Run: "A", Action: "run-start", Status: "ok"}) + w.Append(Entry{Time: t0, Run: "A", Dir: "dl", File: "x.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}) + w.Append(Entry{Time: t0, Run: "A", Action: "run-end", Status: "ok"}) + w.Append(Entry{Time: t1, Run: "B", Action: "run-start", Status: "ok", Detail: "undo of A"}) + w.Append(Entry{Time: t1, Run: "B", Dir: "dl", File: "x.pdf", Step: 1, + Action: "undo-move", Status: "ok", Src: "/b/x.pdf", Dst: "/a/x.pdf"}) + w.Append(Entry{Time: t1, Run: "B", Action: "run-end", Status: "ok"}) + w.Close() + + runs, err := Runs(path, 0) + if err != nil { + t.Fatal(err) + } + byID := map[string]Run{} + for _, r := range runs { + byID[r.ID] = r + } + if !byID["A"].Undone { + t.Errorf("run A = %+v, want Undone", byID["A"]) + } + if byID["B"].Undone { + t.Errorf("run B (the undo run itself) = %+v, want not Undone", byID["B"]) + } +} + +// TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined is fix wave item 2 +// (Important) / final-wave item 17: an undo run's run-start Detail alone +// used to be enough for Runs to mark the original run Undone, even when the +// undo run went on to decline every file (spec §9's "declined files are +// logged even though nothing happens to them", extended to undo) and +// reversed nothing at all. Reproduced by the reviewer via pty: `krino log` +// told the user a run had been undone when the file was still filed. Run B +// here carries the same run-start Detail as TestRunsMarksAnUndoneRun's, but +// every one of its file-scoped entries is "declined", never "ok" - the +// shape ApplyUndo logs when the front end's own review declines everything +// - so run A must come back exactly as untouched. +func TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + t0 := time.Date(2026, 9, 11, 9, 0, 0, 0, time.UTC) + t1 := t0.Add(time.Hour) + w.Append(Entry{Time: t0, Run: "A", Action: "run-start", Status: "ok"}) + w.Append(Entry{Time: t0, Run: "A", Dir: "dl", File: "x.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}) + w.Append(Entry{Time: t0, Run: "A", Action: "run-end", Status: "ok"}) + w.Append(Entry{Time: t1, Run: "B", Action: "run-start", Status: "ok", Detail: "undo of A"}) + w.Append(Entry{Time: t1, Run: "B", Dir: "dl", File: "x.pdf", Step: 1, + Action: "undo-move", Status: "declined", Src: "/b/x.pdf", Dst: "/a/x.pdf"}) + w.Append(Entry{Time: t1, Run: "B", Action: "run-end", Status: "ok"}) + w.Close() + + runs, err := Runs(path, 0) + if err != nil { + t.Fatal(err) + } + byID := map[string]Run{} + for _, r := range runs { + byID[r.ID] = r + } + if byID["A"].Undone { + t.Errorf("run A = %+v, want NOT Undone - the undo run declined every file and reversed nothing", byID["A"]) + } + if byID["B"].Undone { + t.Errorf("run B (the undo run itself) = %+v, want not Undone", byID["B"]) + } +} + +// TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir is the coordinator's +// tightening of fix wave item 2: "at least one ok undo-* entry" is still +// too loose, by the same shape as the bug it fixes. A file's own chain +// stops after a failed file-affecting reversal, but a failed or refused +// undo-mkdir deliberately does not stop anything (internal/engine's +// isFileAffecting draws exactly this line, and undoFile's stop-on-failure +// check shares it) - so an undo-mkdir belonging to one file can still +// succeed even though every file-affecting reversal in the whole run +// failed. Here x.pdf's own undo-move fails, y.pdf's own undo-move also +// fails, and z.pdf's undo-mkdir - tidying up a directory that turned out +// empty, not restoring anything - is the run's only "ok" entry. Marking +// the original run Undone from that alone would be exactly Important 2's +// bug again, by a narrower route. +func TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + t0 := time.Date(2026, 9, 11, 9, 0, 0, 0, time.UTC) + t1 := t0.Add(time.Hour) + w.Append(Entry{Time: t0, Run: "A", Action: "run-start", Status: "ok"}) + w.Append(Entry{Time: t0, Run: "A", Dir: "dl", File: "x.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}) + w.Append(Entry{Time: t0, Run: "A", Dir: "dl", File: "y.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/y.pdf", Dst: "/b/y.pdf"}) + w.Append(Entry{Time: t0, Run: "A", Action: "run-end", Status: "ok"}) + w.Append(Entry{Time: t1, Run: "B", Action: "run-start", Status: "ok", Detail: "undo of A"}) + w.Append(Entry{Time: t1, Run: "B", Dir: "dl", File: "x.pdf", Step: 1, + Action: "undo-move", Status: "failed", Src: "/b/x.pdf", Dst: "/a/x.pdf"}) + w.Append(Entry{Time: t1, Run: "B", Dir: "dl", File: "y.pdf", Step: 1, + Action: "undo-move", Status: "failed", Src: "/b/y.pdf", Dst: "/a/y.pdf"}) + // z.pdf's own file-affecting reversal is unrelated to x.pdf/y.pdf's + // failures; only its cleanup mkdir is shown here, since that mkdir is + // the one entry this test is about - the run's only "ok" line. + w.Append(Entry{Time: t1, Run: "B", Dir: "dl", File: "z.pdf", Step: 2, + Action: "undo-mkdir", Status: "ok", Src: "/a/Work"}) + w.Append(Entry{Time: t1, Run: "B", Action: "run-end", Status: "ok"}) + w.Close() + + runs, err := Runs(path, 0) + if err != nil { + t.Fatal(err) + } + byID := map[string]Run{} + for _, r := range runs { + byID[r.ID] = r + } + if byID["A"].Undone { + t.Errorf("run A = %+v, want NOT Undone - the run's only ok entry is an undo-mkdir (tidiness, not a restoration), and every file-affecting reversal failed", byID["A"]) + } +} + +// 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 +// run rather than half-undo a file (spec §10). +func TestEntriesReportsAMangledLine(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 11, 10, 2, 3, 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) + } + + // Mangle line 2 (the move step) in place: corrupt its Step column so it + // fails to parse, without touching the line or column count of the + // file otherwise - the point is a bad line in the middle, not at EOF. + 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)) + } + fields := strings.Split(lines[1], "\t") + fields[4] = "not-a-number" // the step column + 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 did not report the mangled line") + } + if !strings.Contains(err.Error(), "line 2") { + t.Errorf("error %q does not name line 2", err) + } + if len(got) != 2 { + t.Fatalf("got %d entries, want the 2 surviving (run-start, run-end): %+v", len(got), got) + } + if got[0].Action != "run-start" || got[1].Action != "run-end" { + t.Errorf("entries = %+v", got) + } +} + +// TestEntriesFailsClosedOnUnattributableCorruptionInsideWindow: when a +// line's own Run column is destroyed, Entries cannot attribute it by +// content - but if it falls inside runID's own window (between its +// run-start and run-end), that possibility alone must be enough to refuse +// rather than silently return an incomplete chain (spec §10). +func TestEntriesFailsClosedOnUnattributableCorruptionInsideWindow(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 11, 10, 2, 3, 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) + } + + // Insert a line with no tabs at all - its Run column is unrecoverable - + // between the move step and run-end, i.e. inside A's window. + 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)) + } + inserted := make([]string, 0, len(lines)+1) + inserted = append(inserted, lines[:2]...) + inserted = append(inserted, "totally-mangled-no-tabs-here") + inserted = append(inserted, lines[2:]...) + if err := os.WriteFile(path, []byte(strings.Join(inserted, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } + + got, err := Entries(path, "A") + if err == nil { + t.Fatal("Entries did not fail closed on unattributable corruption inside the run's window") + } + if !strings.Contains(err.Error(), "line 3") { + t.Errorf("error %q does not name line 3", err) + } + if len(got) != 3 { + t.Fatalf("got %d entries, want the 3 surviving (run-start, move, run-end): %+v", len(got), got) + } +} + +// TestEntriesIgnoresUnattributableCorruptionOutsideWindow: the same +// corruption shape, placed after A's run-end inside a later run B's own +// window, must not poison A - the window scoping keeps it out. +func TestEntriesIgnoresUnattributableCorruptionOutsideWindow(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 11, 10, 2, 3, 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.Append(Entry{Time: at, Run: "B", Action: "run-start", Status: "ok"}); 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) + } + + // Insert the same unattributable corruption, now inside B's window + // (between B's run-start and run-end), not A's. + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + lines := strings.Split(strings.TrimRight(string(raw), "\n"), "\n") + if len(lines) != 5 { + t.Fatalf("fixture has %d lines, want 5", len(lines)) + } + inserted := make([]string, 0, len(lines)+1) + inserted = append(inserted, lines[:4]...) + inserted = append(inserted, "totally-mangled-no-tabs-here") + inserted = append(inserted, lines[4:]...) + if err := os.WriteFile(path, []byte(strings.Join(inserted, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } + + got, err := Entries(path, "A") + if err != nil { + t.Fatalf("corruption outside A's window poisoned A: %v", err) + } + if len(got) != 3 { + t.Fatalf("got %d entries, want A's 3: %+v", len(got), got) + } +} + +// TestEntriesFailsClosedOnMissingRunStart: run-start is not an optional +// marker - every run Apply writes begins with one, so its absence, once +// other entries for the run did parse, means either corruption or a log +// truncated at the front. Either way the chain cannot be trusted, even +// though the window logic alone sees nothing wrong (it never opens without +// a parsed run-start, so it never flags anything inside the gap). +func TestEntriesFailsClosedOnMissingRunStart(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 11, 10, 2, 3, 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) + } + + // Replace A's run-start line (line 1) with a line with no tabs at all - + // unrecoverable, like the round-1 fixtures. + 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)) + } + lines[0] = "totally-mangled-no-tabs-here" + 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 did not fail closed on a missing run-start") + } + if !strings.Contains(err.Error(), "run-start") { + t.Errorf("error %q does not name the missing run-start", err) + } + if len(got) != 2 { + t.Fatalf("got %d entries, want the 2 surviving (move, run-end): %+v", len(got), got) + } + if got[0].Action != "move" || got[1].Action != "run-end" { + t.Errorf("entries = %+v", got) + } +} diff --git a/internal/lock/lock.go b/internal/lock/lock.go new file mode 100644 index 0000000..462b26e --- /dev/null +++ b/internal/lock/lock.go @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +// Package lock keeps two krino runs from acting on the same directory at +// once: Acquire takes an exclusive lock file, waiting or failing depending +// on the caller, and Release lets it go. See docs/design.md §3, §11. +package lock + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "time" +) + +// ErrHeld is returned by Acquire when the lock is already held by a running +// krino and wait is false. +var ErrHeld = errors.New("another krino is working in this directory") + +// pollInterval is how often a waiting Acquire retries the lock. +const pollInterval = 100 * time.Millisecond + +// Lock is a held lock file. The zero Lock holds nothing; Release on it, or +// on a nil *Lock, is a no-op. +type Lock struct { + // Path is where the lock file lives. + Path string + + // TookOverStale reports whether Acquire found a lock naming a pid that + // was no longer running, and took the lock over. The caller should + // mention this rather than stay silent about it. + TookOverStale bool + + held bool +} + +// Acquire takes the lock at path: an O_CREATE|O_EXCL file naming the +// holder's pid and start time, so a human can see who holds it. Parent +// directories are created as needed. +// +// When wait is false, Acquire fails immediately with ErrHeld if the lock is +// already held, so a cron job never piles up behind a stuck run. When wait +// is true, Acquire polls every 100ms, with no fixed timeout - but it does +// not poll forever regardless of ctx: a cancelled or expired ctx makes a +// waiting Acquire return ctx.Err() promptly instead of ignoring it (fix +// round 2026-09-12/item 3 - a run blocked waiting for a held lock must +// still notice Ctrl-C). ctx is not consulted at all when wait is false or +// the lock is free on the first try, so -y's non-waiting callers are +// unaffected. +// +// A lock naming a pid that is not running is stale — the machine may have +// lost power mid-run. Acquire removes a stale lock and retries the O_EXCL +// create once; if that retry also loses, another process has reached the +// same conclusion first and Acquire treats the lock as held. A takeover is +// reported via the returned Lock's TookOverStale field. +func Acquire(ctx context.Context, path string, wait bool) (*Lock, error) { + for { + l, err := tryAcquire(path) + if err == nil { + return l, nil + } + if !errors.Is(err, ErrHeld) || !wait { + return nil, err + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(pollInterval): + } + } +} + +// tryAcquire makes one attempt at the lock: create it, or if it is held, +// decide whether the holder is stale and take it over. +func tryAcquire(path string) (*Lock, error) { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return nil, fmt.Errorf("lock %s: %w", path, err) + } + + if err := create(path); err == nil { + return &Lock{Path: path, held: true}, nil + } else if !errors.Is(err, fs.ErrExist) { + return nil, fmt.Errorf("lock %s: %w", path, err) + } + + pid, ok := readHolderPid(path) + if !ok || running(pid) { + return nil, ErrHeld + } + + // Stale: the recorded pid is not running. Take the lock over by + // removing it and retrying the create once. If that retry also loses, + // another process beat us to the same conclusion — treat it as held. + os.Remove(path) + if err := create(path); err != nil { + if errors.Is(err, fs.ErrExist) { + return nil, ErrHeld + } + return nil, fmt.Errorf("lock %s: %w", path, err) + } + return &Lock{Path: path, held: true, TookOverStale: true}, nil +} + +// create makes path with O_CREATE|O_EXCL and writes the holder's pid and +// start time into it. If the write or close fails after the file was +// created, the file is removed so no half-written lock is left behind. +func create(path string) error { + f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o644) + if err != nil { + return err + } + _, werr := fmt.Fprintf(f, "pid %d\nstarted %s\n", os.Getpid(), time.Now().UTC().Format(time.RFC3339)) + cerr := f.Close() + if werr != nil || cerr != nil { + os.Remove(path) + if werr != nil { + return werr + } + return cerr + } + return nil +} + +// readHolderPid reads the pid recorded in the lock file at path. ok is +// false when the file cannot be read or does not name a pid — in which +// case the caller must not treat the lock as stale. +func readHolderPid(path string) (pid int, ok bool) { + b, err := os.ReadFile(path) + if err != nil { + return 0, false + } + return parsePid(string(b)) +} + +// parsePid extracts the pid from a lock file's "pid N" line. +func parsePid(s string) (int, bool) { + const prefix = "pid " + i := strings.Index(s, prefix) + if i < 0 { + return 0, false + } + s = s[i+len(prefix):] + if j := strings.IndexAny(s, "\n\r \t"); j >= 0 { + s = s[:j] + } + n, err := strconv.Atoi(s) + if err != nil || n <= 0 { + return 0, false + } + return n, true +} + +// running reports whether pid names a process that is currently running. +func running(pid int) bool { + if pid <= 0 { + return false + } + proc, err := os.FindProcess(pid) + if err != nil { + return false + } + return proc.Signal(syscall.Signal(0)) == nil +} + +// Release removes the lock file. Release on a Lock that was never acquired +// (the zero Lock, a nil *Lock, or one already released) is harmless. +func (l *Lock) Release() error { + if l == nil || !l.held { + return nil + } + l.held = false + if err := os.Remove(l.Path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("release lock %s: %w", l.Path, err) + } + return nil +} diff --git a/internal/lock/lock_test.go b/internal/lock/lock_test.go new file mode 100644 index 0000000..fca4d76 --- /dev/null +++ b/internal/lock/lock_test.go @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package lock + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestAcquireAndRelease(t *testing.T) { + path := filepath.Join(t.TempDir(), "state", "dl.lock") + l, err := Acquire(context.Background(), path, false) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(path); err != nil { + t.Errorf("lock file missing: %v", err) + } + if b, _ := os.ReadFile(path); !strings.Contains(string(b), fmt.Sprint(os.Getpid())) { + t.Errorf("lock file does not name the holder's pid: %q", b) + } + if err := l.Release(); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Error("Release left the lock file behind") + } + if err := l.Release(); err != nil { + t.Errorf("a second Release must be harmless: %v", err) + } +} + +func TestAcquireFailsWhenHeldAndNotWaiting(t *testing.T) { + path := filepath.Join(t.TempDir(), "dl.lock") + first, err := Acquire(context.Background(), path, false) + if err != nil { + t.Fatal(err) + } + defer first.Release() + if _, err := Acquire(context.Background(), path, false); !errors.Is(err, ErrHeld) { + t.Fatalf("second Acquire err = %v, want ErrHeld", err) + } +} + +func TestAcquireWaitsUntilReleased(t *testing.T) { + path := filepath.Join(t.TempDir(), "dl.lock") + first, err := Acquire(context.Background(), path, false) + if err != nil { + t.Fatal(err) + } + go func() { + time.Sleep(150 * time.Millisecond) + first.Release() + }() + start := time.Now() + second, err := Acquire(context.Background(), path, true) + if err != nil { + t.Fatalf("waiting Acquire failed: %v", err) + } + defer second.Release() + if time.Since(start) < 100*time.Millisecond { + t.Error("Acquire returned before the first holder released") + } +} + +// TestAcquireRespectsContextCancellation is fix round 2026-09-12/item 3: a +// waiting Acquire must not ignore an interrupt - a cancelled ctx must return +// promptly with ctx.Err(), not poll forever. The unfixed code HANGS rather +// than fails here, so the wait for Acquire's result is itself bounded with +// its own hard timeout: a regression must fail this test, not hang the +// whole suite. +func TestAcquireRespectsContextCancellation(t *testing.T) { + path := filepath.Join(t.TempDir(), "dl.lock") + held, err := Acquire(context.Background(), path, false) + if err != nil { + t.Fatal(err) + } + defer held.Release() + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + result := make(chan error, 1) + start := time.Now() + go func() { + _, err := Acquire(ctx, path, true) + result <- err + }() + + select { + case err := <-result: + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Acquire err = %v, want context.DeadlineExceeded", err) + } + if elapsed := time.Since(start); elapsed > 500*time.Millisecond { + t.Errorf("Acquire took %v to notice cancellation, want well under a second", elapsed) + } + case <-time.After(2 * time.Second): + t.Fatal("Acquire ignored context cancellation and is still blocked") + } +} + +// TestStaleLockIsTakenOver: a lock naming a pid that is not running must not +// wedge krino - a machine that lost power mid-run would need manual cleanup. +func TestStaleLockIsTakenOver(t *testing.T) { + path := filepath.Join(t.TempDir(), "dl.lock") + if err := os.WriteFile(path, []byte("pid 4294967000\nstarted 2020-01-01T00:00:00Z\n"), 0o644); err != nil { + t.Fatal(err) + } + l, err := Acquire(context.Background(), path, false) + if err != nil { + t.Fatalf("a stale lock blocked Acquire: %v", err) + } + defer l.Release() + if !l.TookOverStale { + t.Error("the takeover was not reported to the caller") + } +} diff --git a/internal/trash/trash.go b/internal/trash/trash.go new file mode 100644 index 0000000..cc9bc23 --- /dev/null +++ b/internal/trash/trash.go @@ -0,0 +1,222 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +// Package trash implements the freedesktop.org Trash specification well +// enough for krino's (delete) action to be recoverable: Put moves a file +// into $XDG_DATA_HOME/Trash and records where it came from, and Restore +// undoes that. See docs/design.md §7.2. +package trash + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" + "time" + + "krino/internal/xdg" +) + +// ErrOtherFilesystem is returned by Put when path is not on the same +// filesystem as the Trash: the file is left exactly where it was. +var ErrOtherFilesystem = errors.New("not on the same filesystem as the trash") + +// maxSuffixAttempts bounds the collision loop in claimName. internal/plan +// has its own suffixed() with the same cap (internal/plan/conflict.go), but +// the two solve different problems and are free to diverge independently: +// plan's avoids collisions with other planned destinations, this one avoids +// collisions among entries already inside the Trash. They are not shared +// because internal/trash's dependencies are stdlib plus internal/xdg only — +// importing internal/plan for its three-line suffix logic would pull in +// config, scan and dup transitively for that. +const maxSuffixAttempts = 10000 + +// Dir is $XDG_DATA_HOME/Trash, with its files/ and info/ subdirectories. +func Dir() string { return filepath.Join(xdg.DataHome(), "Trash") } + +func filesDir() string { return filepath.Join(Dir(), "files") } +func infoDir() string { return filepath.Join(Dir(), "info") } + +// Put moves path into the Trash and writes its .trashinfo. It returns the +// entry name (the base name inside files/), which the log records so undo +// can find it again. +// +// The name is claimed first: info/<entry>.trashinfo is created with +// O_CREATE|O_EXCL before anything is moved, so two trash clients racing for +// the same name cannot collide. If the subsequent move fails, the info file +// is removed so no orphan is left. +func Put(path string) (entry string, err error) { + abs, err := filepath.Abs(path) + if err != nil { + return "", fmt.Errorf("trash: %w", err) + } + if err := os.MkdirAll(filesDir(), 0o700); err != nil { + return "", fmt.Errorf("trash: %w", err) + } + if err := os.MkdirAll(infoDir(), 0o700); err != nil { + return "", fmt.Errorf("trash: %w", err) + } + + entry, infoPath, f, err := claimName(filepath.Base(abs)) + if err != nil { + return "", fmt.Errorf("trash: %w", err) + } + + info := "[Trash Info]\n" + + "Path=" + percentEncode(abs) + "\n" + + "DeletionDate=" + time.Now().Format("2006-01-02T15:04:05") + "\n" + if _, err := f.WriteString(info); err != nil { + f.Close() + os.Remove(infoPath) + return "", fmt.Errorf("trash: %w", err) + } + if err := f.Close(); err != nil { + os.Remove(infoPath) + return "", fmt.Errorf("trash: %w", err) + } + + dst := filepath.Join(filesDir(), entry) + if err := os.Rename(abs, dst); err != nil { + os.Remove(infoPath) + if errors.Is(err, syscall.EXDEV) { + return "", ErrOtherFilesystem + } + return "", fmt.Errorf("trash: %w", err) + } + + return entry, nil +} + +// claimName finds a free entry name derived from base and atomically creates +// its .trashinfo, so the name is reserved before anything is moved. On +// collision it tries stem_1.ext, stem_2.ext, ... — the same shape as +// internal/plan's suffixing, but resolving a different, unrelated set of +// collisions; see maxSuffixAttempts for why the two are not shared code. +func claimName(base string) (entry, infoPath string, f *os.File, err error) { + stem, ext := splitExt(base) + for n := 0; n <= maxSuffixAttempts; n++ { + candidate := base + if n > 0 { + candidate = stem + "_" + strconv.Itoa(n) + ext + } + path := filepath.Join(infoDir(), candidate+".trashinfo") + f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err == nil { + return candidate, path, f, nil + } + if !os.IsExist(err) { + return "", "", nil, err + } + } + return "", "", nil, errors.New("too many conflicting names") +} + +// splitExt splits name on its last dot, which does not count when it is the +// first character: "a.tar.gz" -> "a.tar", ".gz"; ".bashrc" -> ".bashrc", "". +func splitExt(name string) (stem, ext string) { + i := strings.LastIndexByte(name, '.') + if i <= 0 { + return name, "" + } + return name[:i], name[i:] +} + +// percentEncode RFC-2396-encodes s, leaving unreserved characters and '/' +// literal. A byte loop is used rather than url.PathEscape, which also +// escapes '/' and would produce a Path no other trash implementation can +// read. +func percentEncode(s string) string { + const hex = "0123456789ABCDEF" + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); i++ { + c := s[i] + if isUnreserved(c) || c == '/' { + b.WriteByte(c) + continue + } + b.WriteByte('%') + b.WriteByte(hex[c>>4]) + b.WriteByte(hex[c&0xf]) + } + return b.String() +} + +// isUnreserved reports whether c is unreserved under RFC 2396: letters, +// digits, and -_.~, which percentEncode passes through unchanged. +func isUnreserved(c byte) bool { + switch { + case c >= 'A' && c <= 'Z', c >= 'a' && c <= 'z', c >= '0' && c <= '9': + return true + case c == '-' || c == '_' || c == '.' || c == '~': + return true + } + return false +} + +// percentDecode reverses percentEncode. +func percentDecode(s string) string { + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); i++ { + if s[i] == '%' && i+2 < len(s) { + if v, err := strconv.ParseUint(s[i+1:i+3], 16, 8); err == nil { + b.WriteByte(byte(v)) + i += 2 + continue + } + } + b.WriteByte(s[i]) + } + return b.String() +} + +// Restore moves an entry back to the Path recorded in its .trashinfo and +// removes the .trashinfo. It refuses when that path already exists. +// +// Once the rename back to the original path has succeeded, removing the +// .trashinfo is best-effort: that file back in place is the substantive +// result, and a caller must be able to trust a non-error return means the +// restore happened. So a failure to remove the .trashinfo is not reported +// as an error — Restore returns (path, nil) regardless — and the +// .trashinfo may survive as a stale, otherwise-harmless record. +func Restore(entry string) (restored string, err error) { + infoPath := filepath.Join(infoDir(), entry+".trashinfo") + b, err := os.ReadFile(infoPath) + if err != nil { + return "", fmt.Errorf("trash: %w", err) + } + path, err := parsePath(string(b)) + if err != nil { + return "", fmt.Errorf("trash: %s: %w", entry, err) + } + if _, err := os.Lstat(path); err == nil { + return "", fmt.Errorf("trash: %s: already exists", path) + } else if !os.IsNotExist(err) { + return "", fmt.Errorf("trash: %w", err) + } + + src := filepath.Join(filesDir(), entry) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return "", fmt.Errorf("trash: %w", err) + } + if err := os.Rename(src, path); err != nil { + return "", fmt.Errorf("trash: %w", err) + } + // The file is back; removing its bookkeeping is best-effort from here + // (see the doc comment above). + _ = os.Remove(infoPath) + return path, nil +} + +// parsePath extracts and decodes the Path= line of a .trashinfo file. +func parsePath(info string) (string, error) { + for _, line := range strings.Split(info, "\n") { + if v, ok := strings.CutPrefix(line, "Path="); ok { + return percentDecode(v), nil + } + } + return "", errors.New("trashinfo has no path") +} diff --git a/internal/trash/trash_test.go b/internal/trash/trash_test.go new file mode 100644 index 0000000..24f4ea8 --- /dev/null +++ b/internal/trash/trash_test.go @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package trash + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// sandbox points XDG_DATA_HOME at a temporary tree, so the real Trash is +// never touched. +func sandbox(t *testing.T) string { + t.Helper() + h := t.TempDir() + t.Setenv("HOME", h) + t.Setenv("XDG_DATA_HOME", filepath.Join(h, "share")) + t.Setenv("XDG_CONFIG_HOME", "") + t.Setenv("XDG_STATE_HOME", "") + t.Setenv("XDG_CACHE_HOME", "") + return h +} + +func write(t *testing.T, path, content string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func TestPutWritesBothParts(t *testing.T) { + h := sandbox(t) + src := filepath.Join(h, "dl", "old report.pdf") + write(t, src, "pdf") + + entry, err := Put(src) + if err != nil { + t.Fatal(err) + } + if entry != "old report.pdf" { + t.Errorf("entry = %q, want the base name", entry) + } + if _, err := os.Stat(src); !os.IsNotExist(err) { + t.Error("the original is still in place") + } + if b, err := os.ReadFile(filepath.Join(Dir(), "files", entry)); err != nil || string(b) != "pdf" { + t.Errorf("trashed content = %q, %v", b, err) + } + info, err := os.ReadFile(filepath.Join(Dir(), "info", entry+".trashinfo")) + if err != nil { + t.Fatal(err) + } + got := string(info) + if !strings.HasPrefix(got, "[Trash Info]\n") { + t.Errorf("trashinfo lacks its header:\n%s", got) + } + if !strings.Contains(got, "Path="+strings.ReplaceAll(src, " ", "%20")+"\n") { + t.Errorf("Path is not the absolute percent-encoded original:\n%s", got) + } + // DeletionDate is local time with no offset: 19 characters, no Z, no +. + for _, line := range strings.Split(got, "\n") { + if v, ok := strings.CutPrefix(line, "DeletionDate="); ok { + if len(v) != 19 || strings.ContainsAny(v, "Z+") { + t.Errorf("DeletionDate = %q; want local time like 2026-04-23T16:04:23", v) + } + } + } +} + +func TestPutSuffixesOnCollision(t *testing.T) { + h := sandbox(t) + first := filepath.Join(h, "a", "x.pdf") + second := filepath.Join(h, "b", "x.pdf") + write(t, first, "one") + write(t, second, "two") + + if _, err := Put(first); err != nil { + t.Fatal(err) + } + entry, err := Put(second) + if err != nil { + t.Fatal(err) + } + if entry != "x_1.pdf" { + t.Fatalf("second entry = %q, want x_1.pdf", entry) + } + if b, _ := os.ReadFile(filepath.Join(Dir(), "files", "x.pdf")); string(b) != "one" { + t.Error("the first entry was overwritten") + } + if b, _ := os.ReadFile(filepath.Join(Dir(), "files", "x_1.pdf")); string(b) != "two" { + t.Error("the second entry holds the wrong content") + } + info, err := os.ReadFile(filepath.Join(Dir(), "info", "x_1.pdf.trashinfo")) + if err != nil { + t.Fatal(err) + } + wantPath := "Path=" + strings.ReplaceAll(second, " ", "%20") + "\n" + if !strings.Contains(string(info), wantPath) { + t.Errorf("x_1.pdf.trashinfo does not point at its own original %q:\n%s", second, info) + } +} + +func TestRestoreRoundTrips(t *testing.T) { + h := sandbox(t) + src := filepath.Join(h, "dl", "zażółć gęślą.pdf") + write(t, src, "polish") + + entry, err := Put(src) + if err != nil { + t.Fatal(err) + } + restored, err := Restore(entry) + if err != nil { + t.Fatal(err) + } + if restored != src { + t.Errorf("restored to %q, want %q", restored, src) + } + if b, err := os.ReadFile(src); err != nil || string(b) != "polish" { + t.Errorf("content after restore = %q, %v", b, err) + } + if _, err := os.Stat(filepath.Join(Dir(), "info", entry+".trashinfo")); !os.IsNotExist(err) { + t.Error("the .trashinfo was left behind") + } +} + +func TestRestoreRefusesWhenTargetExists(t *testing.T) { + h := sandbox(t) + src := filepath.Join(h, "dl", "x.pdf") + write(t, src, "one") + entry, err := Put(src) + if err != nil { + t.Fatal(err) + } + write(t, src, "something new") + if _, err := Restore(entry); err == nil { + t.Fatal("Restore overwrote a file that had taken the original path") + } + if b, _ := os.ReadFile(src); string(b) != "something new" { + t.Error("the file at the original path was modified") + } + if _, err := os.Stat(filepath.Join(Dir(), "files", entry)); err != nil { + t.Error("the trash entry was consumed by a refused restore") + } +} + +// TestPutRefusesOtherFilesystem needs a second filesystem. /dev/shm is one on +// Linux; the test skips where there is none. +func TestPutRefusesOtherFilesystem(t *testing.T) { + sandbox(t) + other, err := os.MkdirTemp("/dev/shm", "krino-trash-") + if err != nil { + t.Skip("no second filesystem available:", err) + } + defer os.RemoveAll(other) + src := filepath.Join(other, "x.pdf") + write(t, src, "elsewhere") + + if _, err := Put(src); !errors.Is(err, ErrOtherFilesystem) { + t.Fatalf("Put across filesystems: err = %v, want ErrOtherFilesystem", err) + } + if b, err := os.ReadFile(src); err != nil || string(b) != "elsewhere" { + t.Errorf("the file was disturbed by a refused Put: %q, %v", b, err) + } + if entries, _ := os.ReadDir(filepath.Join(Dir(), "info")); len(entries) != 0 { + t.Errorf("a refused Put left %d orphaned info files", len(entries)) + } +} diff --git a/internal/tui/keys.go b/internal/tui/keys.go new file mode 100644 index 0000000..e88a326 --- /dev/null +++ b/internal/tui/keys.go @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package tui + +import ( + "fmt" + "os" + + "golang.org/x/term" +) + +// ReadKey reads one keypress without Enter, restoring the terminal before +// it returns - on every path, including an error. It always reads a +// single byte; only a terminal is first put into raw mode, so a +// non-terminal (a pipe, in tests) needs no pty to exercise it. +func ReadKey(in *os.File) (rune, error) { + fd := int(in.Fd()) + if !isTerminal(fd) { + return readByte(in) + } + + state, err := term.MakeRaw(fd) + if err != nil { + return 0, fmt.Errorf("tui: %w", err) + } + defer term.Restore(fd, state) + + return readByte(in) +} + +func readByte(in *os.File) (rune, error) { + var b [1]byte + if _, err := in.Read(b[:]); err != nil { + return 0, err + } + return rune(b[0]), nil +} diff --git a/internal/tui/tui.go b/internal/tui/tui.go new file mode 100644 index 0000000..96bb728 --- /dev/null +++ b/internal/tui/tui.go @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +// Package tui is krino's terminal layer: colour policy, paging a plan +// through $PAGER when it does not fit the screen, and single-key input for +// the interactive review prompt. See docs/design.md §8.2. +package tui + +import ( + "io" + "os" + "os/exec" + "strings" + + "golang.org/x/term" +) + +// isTerminal and termSize hold term.IsTerminal and term.GetSize so tests +// can replace them; that is the only way to test this package without a +// pty. +var ( + isTerminal = term.IsTerminal + termSize = term.GetSize +) + +// defaultPager is used when $PAGER is unset. +const defaultPager = "less -FRX" + +// Colour reports whether to emit ANSI colour: w is a terminal and NO_COLOR +// is unset (spec §8.2). +func Colour(w io.Writer) bool { + f, ok := w.(*os.File) + if !ok || !isTerminal(int(f.Fd())) { + return false + } + _, noColour := os.LookupEnv("NO_COLOR") + return !noColour +} + +// 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 { + f, ok := w.(*os.File) + if !ok || !isTerminal(int(f.Fd())) { + return 0 + } + _, h, err := termSize(int(f.Fd())) + if err != nil { + return 0 + } + return h +} + +// Page writes text through $PAGER (default "less -FRX") when it is taller +// than the terminal, and directly otherwise. Height is judged from +// os.Stdout regardless of which writer w is: that is the terminal a +// spawned pager would inherit, not necessarily w. A missing or broken +// pager never loses the plan: Page falls back to writing directly when +// the pager cannot start. +func Page(w io.Writer, text string) error { + if fitsWithoutPaging(text) { + _, err := io.WriteString(w, text) + return err + } + if runPager(text) { + return nil + } + _, err := io.WriteString(w, text) + return err +} + +// fitsWithoutPaging reports whether text has no more lines than the +// terminal's height. It looks at the process's own stdout, since that is +// the terminal the pager would inherit, not the writer text is otherwise +// sent to. +func fitsWithoutPaging(text string) bool { + h := Height(os.Stdout) + if h <= 0 { + return true + } + lines := strings.Count(text, "\n") + if text != "" && !strings.HasSuffix(text, "\n") { + lines++ // the final, unterminated line still occupies a row + } + return lines <= h +} + +// runPager sends text through $PAGER (default "less -FRX") and reports +// whether it started. $PAGER is split with strings.Fields, not a shell. +func runPager(text string) bool { + spec := os.Getenv("PAGER") + if spec == "" { + spec = defaultPager + } + fields := strings.Fields(spec) + if len(fields) == 0 { + return false + } + cmd := exec.Command(fields[0], fields[1:]...) + cmd.Stdin = strings.NewReader(text) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + if err := cmd.Start(); err != nil { + return false + } + _ = cmd.Wait() + return true +} diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go new file mode 100644 index 0000000..6918859 --- /dev/null +++ b/internal/tui/tui_test.go @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package tui + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestColourNeedsTerminalAndNoNOCOLOR(t *testing.T) { + var buf bytes.Buffer + if Colour(&buf) { + t.Error("colour on a non-terminal writer") + } + + old := isTerminal + t.Cleanup(func() { isTerminal = old }) + isTerminal = func(fd int) bool { return true } + + t.Setenv("NO_COLOR", "") + os.Unsetenv("NO_COLOR") + if !Colour(os.Stdout) { + t.Error("no colour on a terminal with NO_COLOR unset") + } + t.Setenv("NO_COLOR", "1") + if Colour(os.Stdout) { + t.Error("colour emitted with NO_COLOR set") + } + t.Setenv("NO_COLOR", "") + if Colour(os.Stdout) { + t.Error("NO_COLOR set to the empty string must still disable colour") + } +} + +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) + } + + oldT, oldS := isTerminal, termSize + t.Cleanup(func() { isTerminal, termSize = oldT, oldS }) + 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) + } +} + +func TestPageUsesPagerOnlyWhenTaller(t *testing.T) { + dir := t.TempDir() + marker := filepath.Join(dir, "paged") + // A pager that records what it was given. + t.Setenv("PAGER", "tee "+marker) + + oldT, oldS := isTerminal, termSize + t.Cleanup(func() { isTerminal, termSize = oldT, oldS }) + isTerminal = func(fd int) bool { return true } + termSize = func(fd int) (int, int, error) { return 80, 5, nil } + + var buf bytes.Buffer + if err := Page(&buf, "one\ntwo\n"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Error("short text went through the pager") + } + if buf.String() != "one\ntwo\n" { + t.Errorf("short text = %q", buf.String()) + } + + tall := strings.Repeat("line\n", 20) + if err := Page(&buf, tall); err != nil { + t.Fatal(err) + } + b, err := os.ReadFile(marker) + if err != nil { + t.Fatalf("tall text did not reach the pager: %v", err) + } + if string(b) != tall { + t.Errorf("the pager received %q", b) + } +} + +func TestPageCountsFinalLineWithoutTrailingNewline(t *testing.T) { + dir := t.TempDir() + marker := filepath.Join(dir, "paged") + t.Setenv("PAGER", "tee "+marker) + + oldT, oldS := isTerminal, termSize + t.Cleanup(func() { isTerminal, termSize = oldT, oldS }) + isTerminal = func(fd int) bool { return true } + termSize = func(fd int) (int, int, error) { return 80, 5, nil } + + // Six lines but only five newlines: one more line than the terminal's + // height, with no trailing newline after the last one. + text := "one\ntwo\nthree\nfour\nfive\nsix" + + var buf bytes.Buffer + if err := Page(&buf, text); err != nil { + t.Fatal(err) + } + b, err := os.ReadFile(marker) + if err != nil { + t.Fatalf("six lines with no trailing newline, one more than the terminal's height, did not reach the pager: %v", err) + } + if string(b) != text { + t.Errorf("the pager received %q", b) + } +} + +func TestReadKeyOnAPipeReadsOneByte(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + go func() { w.WriteString("ay"); w.Close() }() + got, err := ReadKey(r) + if err != nil { + t.Fatal(err) + } + if got != 'a' { + t.Errorf("key = %q, want 'a'", got) + } +} |