diff options
Diffstat (limited to 'internal/engine')
| -rw-r--r-- | internal/engine/apply.go | 929 | ||||
| -rw-r--r-- | internal/engine/apply_test.go | 1182 | ||||
| -rw-r--r-- | internal/engine/roundtrip_test.go | 161 |
3 files changed, 2272 insertions, 0 deletions
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) + } +} |