diff options
Diffstat (limited to 'internal')
43 files changed, 644 insertions, 704 deletions
diff --git a/internal/apply/apply.go b/internal/apply/apply.go index a93aad4..d7bf71e 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -52,13 +52,13 @@ func Chain(c plan.Chain) []StepResult { // ChainLogged is Chain, calling done with each step's result as soon as // that step has run or been skipped, before the next one starts - so a // caller that logs from done never has a completed step missing from the -// log when the process dies mid-chain (review M9). An error from done stops -// the chain at once and is returned with the results so far. +// log when the process dies mid-chain. An error from done stops the chain +// at once and is returned with the results so far. // // A move or rename that had to take a free name at apply time, because its // planned destination was taken since planning, stops the chain too: every // later step was planned against the name the file did not get, and must -// not act on whatever is at that path (review M3). +// not act on whatever is at that path. // // Once ctx is cancelled (an interrupt), the step already under way finishes // and every later step is skipped as "interrupted": an interrupt stops after @@ -160,8 +160,8 @@ func runFileStep(step plan.Step) StepResult { var displacedEntry string if step.Displaces != "" { - // Re-checked at apply time (review M4): only a regular file may be - // trashed to make room, never a directory or link put there since. + // Re-checked at apply time: only a regular file may be trashed to + // make room, never a directory or link put there since. if fi, err := os.Lstat(step.Displaces); err != nil || !fi.Mode().IsRegular() { return StepResult{Step: step, Status: "failed", Detail: "the file to replace is gone or no longer a regular file"} } @@ -224,8 +224,8 @@ func runFileStep(step plan.Step) StepResult { // 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. +// undo: the 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 { diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index 4bf6c30..6134988 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -153,15 +153,15 @@ func TestChainSkippedStepIsNotAttempted(t *testing.T) { _ = 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. +// TestChainDisplacedFileRestoresFromDisplacedEntry: 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) @@ -205,12 +205,12 @@ func TestChainDisplacedFileRestoresFromDisplacedEntry(t *testing.T) { } } -// 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. +// TestChainRunsRenameStep: 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) @@ -229,12 +229,12 @@ func TestChainRunsRenameStep(t *testing.T) { } } -// 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. +// TestChainMadeIsOutermostFirstForNestedDirectories: 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. Undo removes these directories in reverse, so a later +// accidental reordering would break it while passing everything else +// here. func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) { root := t.TempDir() write(t, filepath.Join(root, "x.pdf"), "content", 0o644) @@ -251,12 +251,11 @@ func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) { } } -// TestMoveFileRefusesOccupiedDestination is item 16 (fix round 2026-09-12, -// plan 5 Task 2): moveFile must refuse an occupied destination on its own, -// not merely rely on runFileStep having already checked - the exact -// arrangement that produced plan 4's Task 5 Critical, where a helper that -// replaced silently was trusted because some caller had checked. Called -// directly, bypassing runFileStep's own pre-check entirely. +// TestMoveFileRefusesOccupiedDestination: moveFile must refuse an occupied +// destination on its own, not merely rely on runFileStep having already +// checked - trusting that some caller had checked is exactly what let a +// silently replacing helper cause harm. Called directly, bypassing +// runFileStep's own pre-check entirely. func TestMoveFileRefusesOccupiedDestination(t *testing.T) { dir := t.TempDir() src := write(t, filepath.Join(dir, "x.pdf"), "source", 0o644) @@ -273,10 +272,10 @@ func TestMoveFileRefusesOccupiedDestination(t *testing.T) { } } -// TestRenameFileRefusesOccupiedDestination is item 16's other half: -// runFileStep's bare os.Rename call for the Rename kind was just as -// unguarded in itself as moveFile was. renameFile is the helper that now -// carries the same independent guard, called directly here. +// TestRenameFileRefusesOccupiedDestination: runFileStep's bare os.Rename +// call for the Rename kind was just as unguarded in itself as moveFile +// was. renameFile is the helper that now carries the same independent +// guard, called directly here. func TestRenameFileRefusesOccupiedDestination(t *testing.T) { dir := t.TempDir() src := write(t, filepath.Join(dir, "x.pdf"), "source", 0o644) diff --git a/internal/apply/fs.go b/internal/apply/fs.go index 56737e8..a8feccc 100644 --- a/internal/apply/fs.go +++ b/internal/apply/fs.go @@ -108,13 +108,12 @@ func moveFile(src, dst string) error { if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { return err } - // Item 16 (fix round 2026-09-12, plan 5 Task 2): this guard must hold - // independently of runFileStep's own pre-check, layered rather than - // moved - the exact arrangement that produced plan 4's Task 5 Critical, - // where a helper that replaced silently was trusted because some caller - // had checked. Placed immediately before the operation that would - // otherwise clobber dst, the same way copyFile's own guard sits right - // before its rename into place. + // This guard must hold independently of runFileStep's own pre-check, + // layered rather than moved: trusting that some caller already checked + // is exactly what lets a silently replacing helper cause harm. Placed + // immediately before the operation that would otherwise clobber dst, + // the same way copyFile's own guard sits right before its rename into + // place. if err := refuseIfExists(dst); err != nil { return err } @@ -132,7 +131,7 @@ func moveFile(src, dst string) error { } // renameFile renames src to dst, refusing on its own when dst already -// exists rather than trusting that a caller checked first (item 16, same +// exists rather than trusting that a caller checked first (the same // reasoning as moveFile's guard above): a bare os.Rename silently replaces // an occupied destination, and runFileStep's own pre-check must not be the // only thing standing between a rename step and that. diff --git a/internal/cond/compile.go b/internal/cond/compile.go index 94118ac..6580ee9 100644 --- a/internal/cond/compile.go +++ b/internal/cond/compile.go @@ -32,8 +32,8 @@ func Compile(file string, when []*sexp.Node, opt Options) (*Cond, []*config.Diag return c, errs } -// compile is Compile with cost reordering switchable, for the property test -// in Task 8. +// compile is Compile with cost reordering switchable, for the property +// test. func compile(file string, when []*sexp.Node, opt Options, reorder bool) (*Cond, []*config.Diag) { c := &compiler{file: file, opt: opt, reorder: reorder, cond: &Cond{opt: opt}} var roots []*node diff --git a/internal/cond/eval.go b/internal/cond/eval.go index 1147c05..2c35dba 100644 --- a/internal/cond/eval.go +++ b/internal/cond/eval.go @@ -26,8 +26,7 @@ type Facts interface { Duplicate(dirs []string) (original string, ok bool, err error) // Matched reports whether an earlier rule matched this file, and whether // an earlier rule could not be decided (its condition was unknown): with - // no match and an undecided rule, (matched) is unknown (plan 11 review - // L6). + // no match and an undecided rule, (matched) is unknown. Matched() (matched, undecided bool) } @@ -40,12 +39,11 @@ type Result struct { // Unreadable is true when the condition's value is unknown: it depends // on a content test that could not read the file. Match is then false; - // an exclude holds anyway (review M11). A condition decided whatever the - // text holds - (and (content "x") (type txt)) on a pdf - is not - // unknown (plan 11). + // an exclude holds anyway. A condition decided whatever the text holds + // - (and (content "x") (type txt)) on a pdf - is not unknown. Unreadable bool // Undecided says what made the value unknown: "content unreadable", - // "duplicate check failed", or both, comma-separated (plan 12). + // "duplicate check failed", or both, comma-separated. Undecided string } @@ -95,8 +93,7 @@ func (ctx *evalCtx) undecidedLabel() string { } // undecidable reports whether a leaf whose fact could not be read is -// unknown rather than false: a content test (review M11) or a duplicate -// test (plan 12). +// unknown rather than false: a content test or a duplicate test. func undecidable(k kind) bool { return k == kContent || k == kDuplicate } diff --git a/internal/cond/eval_test.go b/internal/cond/eval_test.go index 7ebe038..73e0ea6 100644 --- a/internal/cond/eval_test.go +++ b/internal/cond/eval_test.go @@ -231,8 +231,8 @@ func TestNegatedCombinatorReason(t *testing.T) { // TestEvalReportsUnreadableContent: Result.Unreadable says a content test // was reached and could not read the file - what lets an exclude fail -// closed (review M11) - and stays false when evaluation never reached the -// content test. +// closed - and stays false when evaluation never reached the content +// test. func TestEvalReportsUnreadableContent(t *testing.T) { f := &fake{name: "a.pdf", rawErr: errors.New("larger than max-read")} if r := eval(t, `(and (type pdf) (content "x"))`, Options{}, f); r.Match || !r.Unreadable { @@ -264,8 +264,8 @@ func TestCapturesKeepDiacritics(t *testing.T) { // is unknown, not false, and and/or/not combine unknowns the way Kleene's // three-valued logic does: a condition certainly false (or true) whatever // the text holds is decided, and only one that depends on the text is -// unknown (plan 11, re-review cache F2). A rule matches only a true -// condition; an exclude holds on true or unknown. +// unknown. A rule matches only a true condition; an exclude holds on true +// or unknown. func TestUnreadableContentIsUnknown(t *testing.T) { f := &fake{name: "a.pdf", rawErr: errors.New("larger than max-read")} cases := []struct { @@ -289,8 +289,7 @@ func TestUnreadableContentIsUnknown(t *testing.T) { // TestMatchedIsUnknownAfterAnUnknownRule: when no earlier rule matched but // one could not be decided, (matched) is unknown, so a later -// (not (matched)) does not act on a file krino could not read (plan 11 -// review L6). +// (not (matched)) does not act on a file krino could not read. func TestMatchedIsUnknownAfterAnUnknownRule(t *testing.T) { f := &fake{name: "a.docx", matchedUnknown: true} if r := eval(t, `(not (matched))`, Options{}, f); r.Match || !r.Unreadable { @@ -303,7 +302,7 @@ func TestMatchedIsUnknownAfterAnUnknownRule(t *testing.T) { } // TestFailedDuplicateLookupIsUnknown: a duplicate test whose lookup fails is -// unknown, like unreadable content, and says why (plan 12). +// unknown, like unreadable content, and says why. func TestFailedDuplicateLookupIsUnknown(t *testing.T) { f := &fake{name: "a.pdf", dupErr: errors.New("~/dl/a.pdf: open: permission denied")} for _, c := range []struct { diff --git a/internal/cond/types.go b/internal/cond/types.go index 62a6640..97e761a 100644 --- a/internal/cond/types.go +++ b/internal/cond/types.go @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later // Package cond compiles the s-expression conditions of a rule's (when ...) -// into a tree that Task 8's evaluator walks against one file's facts. +// into a tree that the evaluator walks against one file's facts. package cond import ( @@ -21,7 +21,7 @@ type Options struct { // without when) is always true. type Cond struct { root *node // nil: always true - opt Options // the case/fold settings conditions were compiled with; Task 8 needs them again at eval time + opt Options // the case/fold settings conditions were compiled with; needed again at eval time UsesContent bool // some content test exists DupDirs [][]string // the raw directory arguments of each duplicate test, in order Keywords []Keyword // every content keyword, as compiled, in order @@ -91,7 +91,7 @@ type keyword struct { } // node is one compiled condition: a leaf test, or an and/or/not combinator -// over other nodes. Task 8 evaluates this tree. +// over other nodes. The evaluator walks this tree. type node struct { kind kind pos sexp.Pos // the position of the node as written, for diagnostics diff --git a/internal/config/enum_test.go b/internal/config/enum_test.go index 2e5b718..06f5640 100644 --- a/internal/config/enum_test.go +++ b/internal/config/enum_test.go @@ -13,7 +13,7 @@ import ( // with their String(), so every Conflict and CaseMode value must print as // the word a configuration uses, and parse back as that same value. A value // added later without its branch would otherwise print as "Conflict(3)", -// which krino check refuses (plan 13 review F5). +// which krino check refuses. func TestEverySettingValuePrintsAsItselfInAConfig(t *testing.T) { for _, c := range []struct { typ, form string diff --git a/internal/config/load.go b/internal/config/load.go index c75d000..3a1cbeb 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -24,9 +24,8 @@ type Config struct { // unusedOverrides reports text for a file no part of this configuration // names: it would otherwise pass as checked while the file on disk was read -// instead (plan 13 review F4). Text for an included directory this call did -// not load - an editor holding buffers for several while checking one - is -// not reported. +// instead. Text for an included directory this call did not load - an +// editor holding buffers for several while checking one - is not reported. func unusedOverrides(over map[string][]byte, used map[string]bool, mainFile string, include []string) []*Diag { known := map[string]bool{filepath.Clean(mainFile): true} for _, name := range include { diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 5f848af..a629ae5 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -72,9 +72,9 @@ func TestLoadErrors(t *testing.T) { } } -// TestLoadSyntaxErrorStopsAtOneDiag is item C: a krino.conf that fails to -// parse must report only the syntax error, not also "not in include" for -// names the caller asked for. +// TestLoadSyntaxErrorStopsAtOneDiag: a krino.conf that fails to parse must +// report only the syntax error, not also "not in include" for names the +// caller asked for. func TestLoadSyntaxErrorStopsAtOneDiag(t *testing.T) { root := t.TempDir() writeFiles(t, root, map[string]string{"krino.conf": `(include "dl"`}) @@ -146,7 +146,7 @@ func TestLoadWithOverriddenText(t *testing.T) { // TestLoadWithReportsAnUnusedOverride: an override whose path does not name // a file the load reads - a different spelling of it, or a directory not in // include - is reported, instead of the file on disk being read as though -// the unsaved text were fine (plan 13 review F4). +// the unsaved text were fine. func TestLoadWithReportsAnUnusedOverride(t *testing.T) { h := t.TempDir() t.Setenv("HOME", h) @@ -180,7 +180,7 @@ func diagText(ds []*Diag) string { // TestLoadWithOverrideForAnotherIncludedDirectory: checking one directory // while holding text for another included one is normal for an editor, and // not an error; only text for a file no configuration file names is -// reported (plan 13 review F4 follow-up). +// reported. func TestLoadWithOverrideForAnotherIncludedDirectory(t *testing.T) { h := t.TempDir() t.Setenv("HOME", h) @@ -201,8 +201,7 @@ func TestLoadWithOverrideForAnotherIncludedDirectory(t *testing.T) { } // TestLoadWithRefusesCollidingOverrides: two keys that name the same file -// would leave which text is read to map order, so they are refused (plan 13 -// review F4 follow-up). +// would leave which text is read to map order, so they are refused. func TestLoadWithRefusesCollidingOverrides(t *testing.T) { h := t.TempDir() t.Setenv("HOME", h) diff --git a/internal/config/print.go b/internal/config/print.go index a2244d0..6ae51e0 100644 --- a/internal/config/print.go +++ b/internal/config/print.go @@ -62,7 +62,7 @@ func PrintExclude(x *Exclude) string { // Pos and End - with text, and returns the new file contents. Every other // byte of src, comments and layout included, is kept exactly. Offsets that // do not belong to src - a form parsed from text that has since changed - -// are an error, not a panic (plan 13 review F6). +// are an error, not a panic. func Splice(src []byte, start, end sexp.Pos, text string) ([]byte, error) { if start.Offset < 0 || end.Offset < start.Offset || end.Offset > len(src) { return nil, fmt.Errorf("config: splice %d:%d is not inside %d bytes", start.Offset, end.Offset, len(src)) diff --git a/internal/config/print_test.go b/internal/config/print_test.go index 510fdd9..87b6878 100644 --- a/internal/config/print_test.go +++ b/internal/config/print_test.go @@ -87,8 +87,7 @@ func TestSpliceLeavesTheRestAlone(t *testing.T) { } // TestSpliceRefusesOffsetsOutsideTheSource: a stale end offset - the file -// changed since the form was parsed - is an error, not a panic (plan 13 -// review F6). +// changed since the form was parsed - is an error, not a panic. func TestSpliceRefusesOffsetsOutsideTheSource(t *testing.T) { src := []byte("(path \"/tmp\")") for _, c := range []struct{ start, end int }{{0, len(src) + 2}, {8, 4}, {-1, 3}} { diff --git a/internal/dup/dup_test.go b/internal/dup/dup_test.go index ed7c1a3..bd593de 100644 --- a/internal/dup/dup_test.go +++ b/internal/dup/dup_test.go @@ -368,13 +368,12 @@ func linked(t *testing.T, dir, name string, target scan.File) scan.File { return scan.File{Path: p, Rel: name, Name: name, Size: fi.Size(), ModTime: fi.ModTime()} } -// TestHardlinksAreNotDuplicatesOfEachOther is R10 (plan 5 Task 2, added to -// the task outside the brief): spec §5.5 groups duplicate candidates by size -// and hash, with no inode check, so two hardlinked names - one inode, byte- -// identical by construction - were judged a duplicate pair. A rule of -// (when (duplicate)) (delete) would then remove a name the user relies on -// even though nothing was ever actually copied. os.SameFile must stop a -// file being judged a duplicate of itself under another name. +// TestHardlinksAreNotDuplicatesOfEachOther: spec §5.5 groups duplicate +// candidates by size and hash, with no inode check, so two hardlinked names +// - one inode, byte-identical by construction - were judged a duplicate +// pair. A rule of (when (duplicate)) (delete) would then remove a name the +// user relies on even though nothing was ever actually copied. os.SameFile +// must stop a file being judged a duplicate of itself under another name. func TestHardlinksAreNotDuplicatesOfEachOther(t *testing.T) { d := t.TempDir() a := put(t, d, "a.pdf", []byte("same content"), 0) @@ -392,13 +391,13 @@ func TestHardlinksAreNotDuplicatesOfEachOther(t *testing.T) { } } -// TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile is R10's other -// direction, and the one a naive "same size+hash means never a duplicate" -// fix would get wrong: a.pdf and b.pdf are hardlinks of one inode, but -// c.pdf is a genuinely separate, byte-identical copy under an extra -// (duplicate "DIR") directory, so spec §5.5 prefers it as the original. -// Deleting a.pdf and b.pdf then leaves the content intact in c.pdf - they -// really are duplicates, of c.pdf, and must still be reported as such. +// TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile is the direction +// a naive "same size+hash means never a duplicate" fix would get wrong: +// a.pdf and b.pdf are hardlinks of one inode, but c.pdf is a genuinely +// separate, byte-identical copy under an extra (duplicate "DIR") directory, +// so spec §5.5 prefers it as the original. Deleting a.pdf and b.pdf then +// leaves the content intact in c.pdf - they really are duplicates, of +// c.pdf, and must still be reported as such. func TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile(t *testing.T) { scanned, filed := t.TempDir(), t.TempDir() a := put(t, scanned, "a.pdf", []byte("same content"), 0) @@ -418,15 +417,15 @@ func TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile(t *testing.T) { } } -// TestHardlinkUnderExtraDirWithNoOtherCopyIsNotADuplicate is R10 extended -// to extra-directory candidates: a candidate is never a duplicate of a -// candidate that is the same file, and extra-directory candidates are -// candidates - the rule is not scanned-vs-scanned only. This is the shape -// (duplicate "DIR") exists for: (duplicate "~/backup") means "the backup -// already holds a copy, so the local name can go", but if ~/backup/a.pdf is -// a hardlink of ~/dl/a.pdf, the backup holds no copy at all, just the same -// file under a second name - judging the scanned file a duplicate would -// delete the only copy while the user believes it is backed up. +// TestHardlinkUnderExtraDirWithNoOtherCopyIsNotADuplicate: a candidate is +// never a duplicate of a candidate that is the same file, and extra- +// directory candidates are candidates - the rule is not scanned-vs-scanned +// only. This is the shape (duplicate "DIR") exists for: (duplicate +// "~/backup") means "the backup already holds a copy, so the local name can +// go", but if ~/backup/a.pdf is a hardlink of ~/dl/a.pdf, the backup holds +// no copy at all, just the same file under a second name - judging the +// scanned file a duplicate would delete the only copy while the user +// believes it is backed up. // // Lookup's os.SameFile check compares the elected original with the file // asked about whether the original was scanned or found under an extra diff --git a/internal/engine/apply.go b/internal/engine/apply.go index 0d894c6..6c72a14 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -111,8 +111,8 @@ func (e *Engine) applyFile(ctx context.Context, dirName string, c plan.Chain, ap return FileResult{File: c.File, Steps: steps}, nil } - // Each step is logged the moment it has run (review M9), not after the - // whole chain: a run killed mid-chain must leave what it did undoable. + // Each step is logged the moment it has run, not after the whole chain: + // a run killed mid-chain must leave what it did undoable. results, err := apply.ChainLogged(ctx, c, func(i int, sr apply.StepResult) error { if err := e.logStep(j, run, dirName, rel, i+1, c.Steps[i], sr); err != nil { return unloggedStep(rel, c.Steps[i], sr, err) @@ -125,9 +125,9 @@ func (e *Engine) applyFile(ctx context.Context, dirName string, c plan.Chain, ap return FileResult{File: c.File, Steps: results}, nil } -// unloggedStep is the error for a step whose log entry could not be written -// (re-review N1). A step that ran is named with where its file is now: -// undo cannot see it, so the user must be told where to look. +// unloggedStep is the error for a step whose log entry could not be +// written. A step that ran is named with where its file is now: undo +// cannot see it, so the user must be told where to look. func unloggedStep(rel string, step plan.Step, sr apply.StepResult, err error) error { if sr.Status != "ok" { return fmt.Errorf("%s: step %s (%s) could not be logged: %w", rel, actionName(step.Kind), sr.Status, err) @@ -150,13 +150,12 @@ func unloggedStep(rel string, step plan.Step, sr apply.StepResult, err error) er // 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. +// index i count toward Failed. This is undo-mkdir's exemption: ApplyResult +// maps 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 { @@ -171,19 +170,18 @@ func tallyFile(result *ApplyResult, steps []apply.StepResult, failureCounts func 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. + // 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) 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 } @@ -220,9 +218,9 @@ func actionName(k plan.Kind) string { // 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. +// is the only place that trash entry name exists), 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. @@ -241,11 +239,11 @@ func (e *Engine) logStep(j *journal.Writer, run, dirName, rel string, stepNum in 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. + // Detail carries the trash entry name explicitly: 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 @@ -311,11 +309,10 @@ type UndoPlan struct { Files []UndoFile // Cleanup holds files with nothing left to reverse but directories the - // run made that something else occupied when the plan was built (re-review - // undo F3). They are not offered - that would repeat on every undo - but - // ApplyUndo removes any of those directories the other reversals leave - // empty, and logs it (plan 10 re-check R1). A front end that rebuilds the - // plan must carry Cleanup over. + // run made that something else occupied when the plan was built. They + // are not offered - that would repeat on every undo - but ApplyUndo + // removes any of those directories the other reversals leave empty, and + // logs it. A front end that rebuilds the plan must carry Cleanup over. Cleanup []UndoFile } @@ -331,14 +328,13 @@ type UndoFile struct { // 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. + // signature: 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 } @@ -356,12 +352,12 @@ type UndoStep struct { // 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. +// chain is intact; 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 { @@ -375,15 +371,15 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { } // Reversals an earlier undo of this same run already completed are not - // offered again (review M10): an undo that stopped part way can be - // finished by undoing the run once more. + // offered again: an undo that stopped part way can be finished by + // undoing the run once more. reversed, err := journal.ReversedSteps(e.Config.LogFile(), runID) if err != nil { return nil, fmt.Errorf("engine: plan undo: %w", err) } - // Entries are grouped by directory and file together (review M7): one run - // spans every directory, and two directories can each hold a file of the + // Entries are grouped by directory and file together: one run spans + // every directory, and two directories can each hold a file of the // same name. type fileKey struct{ dir, file string } var order []fileKey @@ -402,27 +398,27 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { up := &UndoPlan{Run: runID} for _, k := range order { uf := planUndoFile(k.dir, k.file, byFile[k], reversed) - // 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. + // 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 } if uf.Refused == "" && onlyOccupiedDirectoryRemovals(uf.Steps) { // Nothing of the file itself is left to reverse, only directories // the run made that something else still occupies: offering them - // would repeat on every undo (re-review undo F3). An empty one is - // still offered, and removed. + // would repeat on every undo. An empty one is still offered, and + // removed. up.Cleanup = append(up.Cleanup, uf) continue } @@ -455,8 +451,7 @@ func onlyOccupiedDirectoryRemovals(steps []UndoStep) bool { func isUndoRun(entries []journal.Entry) bool { any := false for _, en := range entries { - // A damaged line says nothing about which kind of run this is (plan - // 10 re-check R3). + // A damaged line says nothing about which kind of run this is. if en.Action == "run-start" || en.Action == "run-end" || en.Action == "damaged" { continue } @@ -487,30 +482,30 @@ func isFileAffecting(action string) bool { // 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. +// "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 trash.Restore uses for its own +// .trashinfo cleanup (see its comment). 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 +// 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 @@ -557,9 +552,9 @@ func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.R if k := (journal.ReversedKey{Dir: dir, File: file, Action: step.Action, Src: step.Src}); reversed[k] > 0 { // An earlier undo of this run already reversed this step: the // disk already shows it, and it is not offered again. It is not - // recorded in the projection either (re-review undo F1): what it - // put back is on disk now and is checked there, so a file changed - // since is refused rather than vouched for by the old reversal. + // recorded in the projection either: what it put back is on disk + // now and is checked there, so a file changed since is refused + // rather than vouched for by the old reversal. reversed[k]-- continue } @@ -569,7 +564,7 @@ func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.R // against the disk itself. Judged against the disk as it is now, // en.Dst is empty - a later step of the chain moved the file on - // and every rename-then-move, move-then-move or move-then-trash - // chain would be refused (plan 8, found by the property test). + // chain would be refused (found by the property test). step.Refused = "" } if step.Refused == "" { @@ -650,13 +645,12 @@ func needsOccupancyCheck(action string) bool { // "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. +// 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 { @@ -685,16 +679,16 @@ func reverseStep(en journal.Entry) UndoStep { // // 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 +// (journal.Writer.Append), 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: 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) @@ -707,10 +701,10 @@ func refuseIfChanged(en journal.Entry) string { return "" } -// refuseIfTrashChanged is the trash reversal's identity check (review M2): -// the entry must still be the file this run put there - the size and mtime -// the run logged for it - and its trashinfo must still record the path it -// came from. Emptying the Trash and trashing another file of the same name +// refuseIfTrashChanged is the trash reversal's identity check: the entry +// must still be the file this run put there - the size and mtime the run +// logged for it - and its trashinfo must still record the path it came +// from. Emptying the Trash and trashing another file of the same name // would otherwise have undo restore that file instead. func refuseIfTrashChanged(en journal.Entry) string { fi, err := os.Lstat(en.Dst) @@ -726,9 +720,9 @@ func refuseIfTrashChanged(en journal.Entry) string { return "" } -// recheck repeats, at execution time, the identity check PlanUndo made -// (review undo F8): an undo plan is shown and approved first, and a file -// changed in that window must not be moved back or trashed. +// recheck repeats, at execution time, the identity check PlanUndo made: an +// undo plan is shown and approved first, and a file changed in that window +// must not be moved back or trashed. func recheck(step UndoStep) string { switch step.Action { case "undo-move", "undo-rename", "undo-copy": @@ -741,11 +735,11 @@ func recheck(step UndoStep) string { // 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 +// whatever is there now. 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. @@ -762,9 +756,9 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { // 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. +// whose Detail records which run this undoes (journal's own convention, 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 @@ -777,9 +771,9 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { // the log exactly as the run touched them, the same as Apply's own // approved-and-declined chains do. // -// Task 1 (plan 5): after every file's reversal has been attempted, a second, -// run-wide pass retries the directory removals that were refused as -// non-empty. planUndoFile puts the undo-mkdir step for a shared destination +// After every file's reversal has been attempted, a second, run-wide pass +// retries the directory removals that were refused as non-empty. +// planUndoFile puts the undo-mkdir step for a shared destination // on whichever file's chain first created it (spec §9: only the step that // actually created a directory logs a "mkdir" entry, so only that file's // reversal carries the matching undo-mkdir); when that file reverses first, @@ -795,9 +789,9 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { // touch what the first undo-mkdir attempt already logged (that entry, ok or // failed, stands exactly as it was written), and a directory the retry does // manage to remove gets an ADDITIONAL journal entry - never a rewrite - so -// the log never disagrees with reality (my ruling on the point the brief -// left open: spec §9 logs every step, and a directory removed while the log -// still says its removal was refused would be a false record). Because +// the log never disagrees with reality (spec §9 logs every step, and a +// directory removed while the log still says its removal was refused would +// be a false record). Because // journal.ranAnyUndoStep already excludes "undo-mkdir" from what marks a run // "(undone)", this extra "ok" entry cannot change that marking either - // TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins @@ -829,16 +823,14 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, 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. + // 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. if len(f.Steps) == 0 && f.Refused == "" { continue } @@ -920,8 +912,8 @@ type dirRetry struct { log bool } -// retryDirRemovals is ApplyUndo's run-wide second pass (Task 1, plan 5): once -// every file's reversal has run, some directories an undo-mkdir step could +// retryDirRemovals is ApplyUndo's run-wide second pass: once every file's +// reversal has run, some directories an undo-mkdir step could // not remove earlier may now be empty, because a sibling file that shared // the directory has since reversed too. candidates is sorted deepest path // first (by descending path-segment count) so a nested directory is removed @@ -931,9 +923,9 @@ type dirRetry struct { // // This never rewrites or removes the original undo-mkdir entry (ok or // failed, whichever the first attempt logged): a directory the retry does -// manage to remove gets one ADDITIONAL entry instead (my ruling on the point -// the brief left open - see ApplyUndo's comment), so the log always agrees -// with what is actually on disk. The new entry's own Action is still +// manage to remove gets one ADDITIONAL entry instead (see ApplyUndo's +// comment), so the log always agrees with what is actually on disk. The +// new entry's own Action is still // "undo-mkdir", so journal.ranAnyUndoStep - which excludes that action on // principle, not by accident (see its own comment) - continues to treat this // exactly like any other undo-mkdir for the purpose of marking a run @@ -972,13 +964,13 @@ func pathDepth(path string) int { // 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. +// them", extended to undo: 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 { @@ -997,8 +989,8 @@ func (e *Engine) declineUndoFile(f UndoFile, j *journal.Writer, run string) (Fil // 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, +// 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 @@ -1059,8 +1051,7 @@ func runUndoStep(step UndoStep) apply.StepResult { case "undo-move", "undo-rename": // The directories created here are returned in Made: ApplyUndo removes // them again once empty, since an earlier file's undo-mkdir may already - // have removed the directory this file passes back through (review - // undo F6). + // have removed the directory this file passes back through. made, err := apply.MkdirAllTracked(filepath.Dir(step.Dst)) if err != nil { return apply.StepResult{Status: "failed", Detail: err.Error(), Made: made} @@ -1082,12 +1073,12 @@ func runUndoStep(step UndoStep) apply.StepResult { 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. - // The directory the file goes back into is created here, tracked, - // rather than silently by trash.Restore, so ApplyUndo removes it again - // when it ends up empty (review undo F6). + // put it explicitly - 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. The directory the + // file goes back into is created here, tracked, rather than + // silently by trash.Restore, so ApplyUndo removes it again when it + // ends up empty. made, err := apply.MkdirAllTracked(filepath.Dir(step.Dst)) if err != nil { return apply.StepResult{Status: "failed", Detail: err.Error(), Made: made} @@ -1114,8 +1105,8 @@ func runUndoStep(step UndoStep) apply.StepResult { // 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, +// The Lstat guard below is not optional: 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 @@ -1175,9 +1166,8 @@ func copyThenRemove(src, dst string) error { 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. + // 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 diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go index 142e4d2..6c724b1 100644 --- a/internal/engine/apply_test.go +++ b/internal/engine/apply_test.go @@ -20,14 +20,12 @@ import ( // 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. +// This uses writeConfig, which takes a main-file body and a dirs map keyed +// by name (see TestLoadRejectsUnsuppliedCaptures's comment in +// engine_test.go for the same shape used elsewhere in this package): the +// real config language (docs/design.md §4.2-4.3) requires `(path ...)` and +// `(rule ...)` in a directory file reached through `(include ...)`, not in +// the main file directly. func applyFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, string) { t.Helper() h := sandbox(t) @@ -205,25 +203,24 @@ func TestPlanUndoRefusesPermanentDelete(t *testing.T) { } } -// 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. +// TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete: 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. Without the guard against that, PlanUndo would append +// 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 +// The two halves live in one test, deliberately: 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. +// fails this one test immediately. func TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete(t *testing.T) { h := sandbox(t) dl := filepath.Join(h, "dl") @@ -294,10 +291,9 @@ func TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete(t *testing.T) { } } -// 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 +// TestPlanUndoAcceptsIntactRun: 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) @@ -326,8 +322,8 @@ func TestPlanUndoAcceptsIntactRun(t *testing.T) { // 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. +// window-to-EOF behaviour. A refusal here would mean PlanUndo's assumption +// and Entries' actual behaviour have drifted apart. 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 { @@ -424,7 +420,7 @@ func TestApplyChecksContextBetweenFilesNotWithinOne(t *testing.T) { } // TestApplyUndoRestoresMovedFile: the smallest possible round trip through -// ApplyUndo, since Task 9's is the only other test that exercises it. +// ApplyUndo. 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 { @@ -543,8 +539,8 @@ func TestApplyUndoSkipsRefusedFiles(t *testing.T) { } } -// 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 +// TestApplyUndoLogsDeclinedFile: 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 @@ -620,9 +616,8 @@ func TestApplyUndoLogsDeclinedFile(t *testing.T) { } } -// 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 +// TestApplyUndoDecliningEveryFileDoesNotMarkOriginalRunUndone: reproduced +// 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 - @@ -693,12 +688,10 @@ func TestRunsDelegatesToJournal(t *testing.T) { } } -// --- 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. +// TestApplyLogsTrashEntryNameInDetail: 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") @@ -773,10 +766,10 @@ func TestApplyLogsTrashEntryNameInDetail(t *testing.T) { } } -// 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. +// TestRunUndoStepTrashReadsEntryNameFromDetailNotDst: 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") @@ -794,7 +787,7 @@ func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { // Original is what the run logged for the trash step: the entry name in // Detail, and the entry's own path, size and mtime (checked again at - // execution time, review undo F8). + // execution time). entryPath := filepath.Join(trash.Dir(), "files", entry) fi, err := os.Stat(entryPath) if err != nil { @@ -815,8 +808,8 @@ func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { } } -// TestPlanUndoRefusesFileModifiedWithinSameSecond: fix round 1, item 4. The -// journal now records ModTime with sub-second precision (RFC3339Nano), so a +// TestPlanUndoRefusesFileModifiedWithinSameSecond: 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. @@ -851,8 +844,8 @@ func TestPlanUndoRefusesFileModifiedWithinSameSecond(t *testing.T) { } } -// TestUndoFileStopsAfterFailedFileAffectingStep: fix round 1, item 2. A -// failed undo-move must stop the rest of that file's reversal - continuing +// TestUndoFileStopsAfterFailedFileAffectingStep: 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) { @@ -891,9 +884,9 @@ func TestUndoFileStopsAfterFailedFileAffectingStep(t *testing.T) { } } -// 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. +// TestUndoFileContinuesPastFailedMkdir: 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") @@ -940,11 +933,9 @@ func TestUndoFileContinuesPastFailedMkdir(t *testing.T) { } } -// --- 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 +// TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution: 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 @@ -993,11 +984,10 @@ func TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution(t *testing.T) { } } -// 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. +// TestApplyUndoDoesNotCountFailedMkdirAsFailed: a file whose only failure +// is an undo-mkdir (a shared directory not yet empty) must not flip +// ApplyResult.Failed - ApplyResult maps to krino undo's exit code, and +// this specific refusal is tidiness, not a hazard. func TestApplyUndoDoesNotCountFailedMkdirAsFailed(t *testing.T) { h := sandbox(t) dir := filepath.Join(h, "Work") @@ -1044,15 +1034,13 @@ func TestApplyUndoDoesNotCountFailedMkdirAsFailed(t *testing.T) { } } -// --- 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. +// deliberately). func overwriteFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, string) { t.Helper() h := sandbox(t) @@ -1097,21 +1085,19 @@ func overwriteFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, 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. +// TestApplyUndoReversesOverwriteRoundTrip: `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: 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 { @@ -1170,10 +1156,10 @@ func TestApplyUndoReversesOverwriteRoundTrip(t *testing.T) { } } -// 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 +// TestApplyUndoStillRefusesGenuineOccupant: 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. @@ -1204,9 +1190,9 @@ func TestApplyUndoStillRefusesGenuineOccupant(t *testing.T) { } } -// 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 +// TestTallyFileCountsAnAllSkippedFileAsDeclined: 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 @@ -1240,24 +1226,22 @@ func TestTallyFileCountsMixedOutcomesOnceEach(t *testing.T) { } } -// --- Plan 5, Task 1 --- - // sharedDestUndoFixture builds a downloads directory with three pdf files // (a.pdf, b.pdf, c.pdf) and a single rule moving all of them into dest, // applies the move, and returns the sandbox home, the loaded engine, the // journal's path and the forward run's ID. // // All three files landing on one destination that this one run creates is -// the shape that exercises the run-wide directory retry (Task 1, plan 5): -// whichever file's chain first creates dest carries its undo-mkdir step(s), -// and that file's own reversal typically runs while its siblings still -// occupy dest - refusing the removal correctly, at first. dest may name a -// nested path ("Work/Sub"): apply.mkdirAllTracked then records every -// directory the move had to create, outermost first, and every one of them -// still lands on that same first file's chain. +// the shape that exercises the run-wide directory retry: whichever file's +// chain first creates dest carries its undo-mkdir step(s), and that file's +// own reversal typically runs while its siblings still occupy dest - +// refusing the removal correctly, at first. dest may name a nested path +// ("Work/Sub"): apply.mkdirAllTracked then records every directory the +// move had to create, outermost first, and every one of them still lands +// on that same first file's chain. // -// Extracted per fix round 1 (Important 2): TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses -// and TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone +// TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses and +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone // used to duplicate this setup verbatim; TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst // needed the identical shape with only dest varying, which is what named the // parameter rather than hard-coding "Filed" here. @@ -1268,10 +1252,9 @@ func sharedDestUndoFixture(t *testing.T, dest string) (h string, e *Engine, logP if err := os.MkdirAll(dl, 0o755); err != nil { t.Fatal(err) } - // Three files that all move into ONE created destination. The bug this - // task fixes is that only the file whose chain first creates it ever - // carries the undo-mkdir step, and that step is attempted while its - // siblings are still inside. + // Three files that all move into ONE created destination: only the file + // whose chain first creates it ever carries the undo-mkdir step, and + // that step is attempted while its siblings are still inside. for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} { if err := os.WriteFile(filepath.Join(dl, n), []byte(n), 0o640); err != nil { t.Fatal(err) @@ -1342,8 +1325,8 @@ func TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses(t *testing.T) { } } -// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst pins fix round 1's -// Important 1: retryDirRemovals must retry deepest path first. All three +// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst: retryDirRemovals +// must retry deepest path first. All three // files move into Work/Sub, so Apply's single mkdirAllTracked call creates // both Work and Work/Sub on the FIRST file's own chain (outermost first), // which means that one file's reversal carries two undo-mkdir steps, one for @@ -1398,16 +1381,14 @@ func TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst(t *testing.T) { } } -// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins -// the journal half of the ruling on the point Task 1's brief left open: when -// retryDirRemovals succeeds in removing a directory, it appends an +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone: +// when retryDirRemovals succeeds in removing a directory, it appends an // ADDITIONAL journal entry for it - the original "failed" undo-mkdir entry, // recorded on whichever file's chain first created the directory, is never // rewritten or removed - and, because that entry's Action is still // "undo-mkdir" like the first, journal.ranAnyUndoStep continues to exclude -// it from what marks a run "(undone)" (the brief's constraint: it "cannot -// change whether a run shows as (undone); confirm that rather than assume -// it"). +// it from what marks a run "(undone)" (this must not change whether a run +// shows as (undone); the test confirms that rather than assuming it). func TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone(t *testing.T) { _, e, logPath, run := sharedDestUndoFixture(t, "Filed") diff --git a/internal/engine/engine.go b/internal/engine/engine.go index cc776e6..cde1298 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -1,8 +1,8 @@ // SPDX-License-Identifier: GPL-3.0-or-later // Package engine is what every krino front end calls: it loads and compiles -// the configuration, matches files against rules, and (from plan 3) plans -// and applies actions. It returns data; front ends only render it. +// the configuration, matches files against rules, and plans and applies +// actions. It returns data; front ends only render it. package engine import ( @@ -20,7 +20,7 @@ import ( ) // Engine holds a loaded, compiled configuration: everything a front end -// needs to check, match and (from plan 3) act. +// needs to check, match and act. type Engine struct { Config *config.Config Dirs []*Dir @@ -98,10 +98,9 @@ func LoadWith(mainFile string, overrides map[string][]byte, names ...string) (*E reported := map[string]bool{} if len(cfg.Dirs) == 0 { // With no directory to compile them for, krino.conf's excludes are - // still checked, so a mistake is reported before one is included - // (review cli F10). + // still checked, so a mistake is reported before one is included. // Compiled with the defaults' case and fold, as a directory without - // settings of its own would compile them (triage 28g). + // settings of its own would compile them. def := cfg.Main.Defaults.Over(config.Builtin()) opt := cond.Options{IgnoreCase: def.Case == config.CaseIgnore, Fold: def.Fold} for _, x := range cfg.Main.Excludes { @@ -186,7 +185,7 @@ func checkCaptures(file string, r *config.Rule, c *cond.Cond) *config.Diag { for _, a := range r.Actions { if err := plan.CheckTemplate(a.Arg); err != nil { // A placeholder that could never expand is a config error, not a - // step skipped at plan time (plan 12). + // step skipped at plan time. return &config.Diag{File: file, Pos: a.Pos, Msg: fmt.Sprintf("rule %q: %v", r.Name, err)} } n, err := plan.MaxIndex(a.Arg) diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index b6b50fc..86cf9d9 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -140,10 +140,9 @@ func TestCheck(t *testing.T) { } // TestLoadRejectsUnsuppliedCaptures: a rule using {N} must be able to get it -// from its own name tests (spec 7.3) — adapted from the brief to this -// package's actual sandbox/writeConfig/Load helpers (writeConfig takes a -// main file body and a dirs map; there is no separate engineLoad, Load is -// called directly). +// from its own name tests (spec 7.3). This uses the package's actual +// sandbox/writeConfig/Load helpers (writeConfig takes a main file body and +// a dirs map; there is no separate engineLoad, Load is called directly). func TestLoadRejectsUnsuppliedCaptures(t *testing.T) { tests := []struct{ rule, want string }{ {`(rule "a" (when (type pdf)) (move "Work/{1}"))`, @@ -261,7 +260,7 @@ func TestLoadAcceptsDuplicateWithMove(t *testing.T) { // TestLoadRefusesBadPlaceholders: a copy or move destination or a rename // name whose placeholders could never expand is a config error at load, so // krino check reports it at the action's position instead of a run skipping -// the step (plan 12). +// the step. func TestLoadRefusesBadPlaceholders(t *testing.T) { for _, c := range []struct { action, want string diff --git a/internal/engine/exclude_test.go b/internal/engine/exclude_test.go index 7d9ce2a..36f1262 100644 --- a/internal/engine/exclude_test.go +++ b/internal/engine/exclude_test.go @@ -212,7 +212,7 @@ func TestExplainReportsExclusionAndSize(t *testing.T) { // TestExcludeFailsClosedOnUnreadableContent: an exclude meant to protect // files holds when its content test cannot read a file (over max-read), so -// no rule acts on a file krino could not check (review M11). +// no rule acts on a file krino could not check. func TestExcludeFailsClosedOnUnreadableContent(t *testing.T) { big := "confidential " + strings.Repeat("x", 2048) h, dl := excludeTree(t, map[string]string{"big.txt": big, "small.txt": "nothing to hide"}) @@ -252,8 +252,7 @@ func TestExcludeFailsClosedOnUnreadableContent(t *testing.T) { } // TestLoadChecksMainExcludesWithoutDirectories: a mistake in krino.conf's -// (exclude ...) is reported even before any directory is included (review -// cli F10). +// (exclude ...) is reported even before any directory is included. func TestLoadChecksMainExcludesWithoutDirectories(t *testing.T) { h := sandbox(t) main := writeConfig(t, h, "(include)\n(exclude (bogus 1))\n", nil) @@ -261,8 +260,7 @@ func TestLoadChecksMainExcludesWithoutDirectories(t *testing.T) { t.Error("a broken krino.conf exclude was not reported") } // Checked with the defaults' case and fold, as a directory would compile - // them (triage 28g): a keyword of a lone combining mark is empty only - // when folded. + // them: a keyword of a lone combining mark is empty only when folded. main = writeConfig(t, h, "(include)\n(defaults (fold no))\n(exclude (content \"\u0301\"))\n", nil) if _, errs := Load(main); len(errs) != 0 { t.Errorf("checked with fold on, though the defaults say no: %v", errs) @@ -272,7 +270,7 @@ func TestLoadChecksMainExcludesWithoutDirectories(t *testing.T) { // TestNoTextFormatIsNoMatch: a file whose format has no text cannot contain // a keyword, so a content exclude without a type does not set it aside, and // no "content unreadable" warning is raised - while a real read failure (over -// max-read) still fails closed (re-review N2). +// max-read) still fails closed. func TestNoTextFormatIsNoMatch(t *testing.T) { big := "confidential " + strings.Repeat("x", 2048) h, dl := excludeTree(t, map[string]string{"photo.jpg": "\xff\xd8\xff\x00\x01binary", "big.txt": big}) @@ -314,8 +312,7 @@ func TestNoTextFormatIsNoMatch(t *testing.T) { // TestTextTurningBinaryIsNoText: a file with no known extension whose first // 8 KiB read as text but which holds a NUL further on - a self-extracting // installer - counts as having no text, like an image: a content exclude -// does not set it aside and there is no warning (decided after the plan 10 -// re-check). +// does not set it aside and there is no warning. func TestTextTurningBinaryIsNoText(t *testing.T) { mixed := "confidential " + strings.Repeat("x", 9000) + "\x00tail" h, _ := excludeTree(t, map[string]string{"mixed": mixed}) @@ -388,8 +385,7 @@ func partDocx(t *testing.T, path, body string) { // TestPartlyReadableDocument: a docx with an unreadable part answers a // keyword it holds in the readable part, but a keyword not found there is -// unknown: a content exclude sets the file aside, a rule warns (plan 11, -// re-review cache F3). +// unknown: a content exclude sets the file aside, a rule warns. func TestPartlyReadableDocument(t *testing.T) { h, dl := excludeTree(t, map[string]string{}) os.MkdirAll(dl, 0o755) @@ -435,7 +431,7 @@ func TestPartlyReadableDocument(t *testing.T) { // TestExplainLeavesTheCacheAlone: explain never writes the keyword cache - // not even to remove one a directory without content tests no longer uses -// (triage 28e: explain runs without the directory's lock). +// (explain runs without the directory's lock). func TestExplainLeavesTheCacheAlone(t *testing.T) { h, dl := excludeTree(t, map[string]string{"a.txt": "x"}) main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": "(path \"~/dl\")\n(rule \"all\" (move \"Out\"))\n"}) @@ -457,7 +453,7 @@ func TestExplainLeavesTheCacheAlone(t *testing.T) { // TestPartlyReadableDocumentIsNotCached: a partial read's answers never // enter the keyword cache, so a second run with the cache still sets the -// file aside (plan 11 review M2). +// file aside. func TestPartlyReadableDocumentIsNotCached(t *testing.T) { h, dl := excludeTree(t, map[string]string{}) os.MkdirAll(dl, 0o755) @@ -485,7 +481,7 @@ func TestPartlyReadableDocumentIsNotCached(t *testing.T) { // TestExplainAgreesWithMatchOnADuplicateExclude: a (duplicate) test inside // an exclude needs the directory's other files, as one inside a rule does, -// so explain sets aside exactly the files Match does (plan 11 review M3). +// so explain sets aside exactly the files Match does. func TestExplainAgreesWithMatchOnADuplicateExclude(t *testing.T) { h, dl := excludeTree(t, map[string]string{"a.txt": "same", "b.txt": "same", "c.txt": "other"}) main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` @@ -514,7 +510,7 @@ func TestExplainAgreesWithMatchOnADuplicateExclude(t *testing.T) { // TestNotMatchedAfterAnUnknownRuleDoesNotAct: a partly read document an // earlier content rule could not decide is not caught by a later -// (not (matched)) catch-all (plan 11 review L6). +// (not (matched)) catch-all. func TestNotMatchedAfterAnUnknownRuleDoesNotAct(t *testing.T) { h, dl := excludeTree(t, map[string]string{}) os.MkdirAll(dl, 0o755) @@ -549,7 +545,7 @@ func TestNotMatchedAfterAnUnknownRuleDoesNotAct(t *testing.T) { } // TestDuplicateExcludeFailsClosed: an exclude whose duplicate lookup fails -// holds, marked as such (plan 12). +// holds, marked as such. func TestDuplicateExcludeFailsClosed(t *testing.T) { if os.Getuid() == 0 { t.Skip("root reads a chmod 000 file") @@ -579,7 +575,7 @@ func TestDuplicateExcludeFailsClosed(t *testing.T) { // TestUndecidedStopRuleStops: a (stop) rule whose condition cannot be // decided ends the search for that file, so a later rule does not act on a -// file the stop rule was written to keep (plan 12). +// file the stop rule was written to keep. func TestUndecidedStopRuleStops(t *testing.T) { big := "confidential " + strings.Repeat("x", 2048) h, dl := excludeTree(t, map[string]string{"big.txt": big, "small.txt": "nothing"}) diff --git a/internal/engine/explain_test.go b/internal/engine/explain_test.go index 83011c5..f6ade69 100644 --- a/internal/engine/explain_test.go +++ b/internal/engine/explain_test.go @@ -69,7 +69,7 @@ func TestExplainChainMatchesThePlan(t *testing.T) { // TestExplainChainOnlyWhenAsked: the command line's explain does not build // the chain (it can hash files to resolve a conflict), and a file no rule -// acts on has none at all (plan 13 review F1, F2). +// acts on has none at all. func TestExplainChainOnlyWhenAsked(t *testing.T) { h, dl := excludeTree(t, map[string]string{"a.pdf": "one", "keep-b.pdf": "two", "new.pdf": "three"}) now := time.Now() @@ -106,7 +106,7 @@ func TestExplainChainOnlyWhenAsked(t *testing.T) { // TestExplainChainIsThisFileAlone: with two files competing for one name, // the chain shows what this file alone would do; the plan is where the two -// are resolved against each other (plan 13 review F2). +// are resolved against each other. func TestExplainChainIsThisFileAlone(t *testing.T) { h, dl := excludeTree(t, map[string]string{"a.pdf": "one", "b.pdf": "two"}) main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` diff --git a/internal/engine/facts.go b/internal/engine/facts.go index ce05df4..d664b0e 100644 --- a/internal/engine/facts.go +++ b/internal/engine/facts.go @@ -66,7 +66,7 @@ func (run *matchRun) warnings() []string { } // drainDupErrors appends every duplicate index's candidate-hashing errors -// (A1: a candidate other than the file being looked up that could not be +// (a candidate other than the file being looked up that could not be // hashed) to run.warn, once matching is done and every index has seen every // Lookup it is going to see. Candidate paths are abbreviated with // xdg.Abbrev, as every other user-visible path is. @@ -80,8 +80,8 @@ func (run *matchRun) drainDupErrors() { } } -// dupWarning is the one form of a duplicate warning (triage 6): "duplicate: -// PATH: cause", the path abbreviated like every other one shown, and an OS +// dupWarning is the one form of a duplicate warning: "duplicate: PATH: +// cause", the path abbreviated like every other one shown, and an OS // error's cause without the raw path it would repeat. func dupWarning(err error) string { return "duplicate: " + dupCause(err).Error() @@ -89,8 +89,8 @@ func dupWarning(err error) string { // dupCause is a duplicate check's error as "PATH: cause", the path // shortened with xdg.Abbrev and not repeated inside the cause - a candidate -// that could not be hashed, or an OS error on the file itself (plan 11 -// review L7). It still unwraps to err. +// that could not be hashed, or an OS error on the file itself. It still +// unwraps to err. func dupCause(err error) error { path, cause := "", err var ce dup.CandidateError @@ -229,12 +229,12 @@ func (f *facts) extract(opt cond.Options, keywords []string) { switch { case errors.Is(err, extract.ErrPartial): // Partly read: the keywords found in it are answered; one not found - // is unknown (ContentContains), and nothing is cached (plan 11). + // is unknown (ContentContains), and nothing is cached. f.partialErr = err case errors.Is(err, extract.ErrUnsupported): // A format with no text cannot contain a keyword: every answer is - // no, with no warning, and the answers are cached like any other - // (re-review N2). Only a real read failure is "unreadable". + // no, with no warning, and the answers are cached like any other. + // Only a real read failure is "unreadable". text = "" case err != nil: f.contentErr = err @@ -296,7 +296,7 @@ func (f *facts) Duplicate(dirs []string) (string, bool, error) { } // The absolute path is kept for a front end that has to act on the // other copy - the window offers to keep this one instead - while the - // reason text stays as it reads best (plan 21). + // reason text stays as it reads best. f.dupOriginal = orig return displayOriginal(orig, root), true, nil } diff --git a/internal/engine/match.go b/internal/engine/match.go index 25c05e5..9943073 100644 --- a/internal/engine/match.go +++ b/internal/engine/match.go @@ -51,7 +51,7 @@ type FileMatch struct { // absolute; "" when none did. The reason text says the same thing the // way it reads best - relative to the directory when it is inside it - // which leaves a front end no way to act on the other copy, or even to - // say where it is (his report, 2026-09-17). + // say where it is. DuplicateOf string } @@ -64,7 +64,7 @@ type Result struct { Warnings []string // directory-level, sorted; e.g. "duplicate: /x/y does not exist" // Unscanned is every existing rule destination inside the root, which // the walk leaves out (spec §8.1), so -v can say its files were not - // counted (triage 4). + // counted. Unscanned []string Elapsed time.Duration } @@ -181,7 +181,7 @@ func evalFile(run *matchRun, file scan.File) FileMatch { if r.Conf.Stop { // A (stop) rule that cannot be decided ends the search too: // it may be the rule written to keep this file from the - // ones below (plan 12). + // ones below. break } } @@ -280,7 +280,7 @@ func (e *Engine) Explain(ctx context.Context, path string) (*Explanation, error) // ExplainWithChain is Explain with Explanation.Chain filled in: the steps // this file alone would get. Building them resolves conflicts against the // disk, which can read files (a copy whose target holds the same bytes), so -// the command line's explain does not ask for it (plan 13 review F1). +// the command line's explain does not ask for it. func (e *Engine) ExplainWithChain(ctx context.Context, path string) (*Explanation, error) { return e.explain(ctx, path, true) } @@ -318,8 +318,7 @@ func (e *Engine) explain(ctx context.Context, path string, withChain bool) (*Exp skip := explainSkip(d, sf, excl, now) // The directory is walked only for a duplicate test - in a rule or in an - // exclude - the one thing that needs its other files (triage 21, plan 11 - // review M3). + // exclude - the one thing that needs its other files. files := []scan.File{sf} if len(d.DupScopes) > 0 || excludesUseDuplicate(d) { files = e.filesForExplain(d, sf, excl, now) @@ -327,7 +326,7 @@ func (e *Engine) explain(ctx context.Context, path string, withChain bool) (*Exp run := newMatchRun(e, d, ctx, now, files) if len(d.ContentKeywords) > 0 { // Loaded only: openCache would remove the cache of a directory with - // no content tests, and Explain holds no lock (triage 28e). + // no content tests, and Explain holds no lock. e.openCache(run) } f := newFacts(run, sf) @@ -413,7 +412,7 @@ func (e *Engine) cacheFingerprint(d *Dir) string { // excludedBy is what x sets a file aside as, given its evaluation: its text // when it matched, its text marked "(content unreadable)" when a content // test it reached could not read the file - an exclude protects files, so -// it fails closed (review M11) - or "" when it does not hold. +// it fails closed - or "" when it does not hold. func excludedBy(x *Exclude, res cond.Result) string { switch { case res.Match: @@ -438,7 +437,7 @@ func (e *Engine) openCache(run *matchRun) []string { } if len(run.d.ContentKeywords) == 0 { // No content test left: a cache from an earlier configuration only - // holds keywords this directory no longer uses (review cache F6). + // holds keywords this directory no longer uses. if err := os.Remove(e.cacheFile(run.d)); err != nil && !os.IsNotExist(err) { return []string{"cache: " + err.Error()} } @@ -556,7 +555,7 @@ func isBusy(path string, suffixes []string) bool { // excludeDirs computes the directories Match and Explain never enter: each // rule's copy/move destination, the Trash, and the directory holding the // main config file - each kept only when it lies strictly inside d's root -// (C3: root itself is not "inside" it here - a rule cannot exclude the very +// (root itself is not "inside" it here - a rule cannot exclude the very // directory being scanned. cmd/krino/render.go's relToRoot answers a // different question, whether a destination is root or beneath it for // display purposes, and there root does count as inside; the two are each diff --git a/internal/engine/match_test.go b/internal/engine/match_test.go index 224f881..82cfd04 100644 --- a/internal/engine/match_test.go +++ b/internal/engine/match_test.go @@ -368,8 +368,7 @@ func TestMatchDrainsDupCandidateErrors(t *testing.T) { // TestDuplicateWarningsShareOneFormat: a missing extra directory and a // candidate that cannot be hashed are both reported as "duplicate: PATH: -// cause", the path abbreviated and not repeated raw inside the cause -// (triage 6). +// cause", the path abbreviated and not repeated raw inside the cause. func TestDuplicateWarningsShareOneFormat(t *testing.T) { if os.Getuid() == 0 { t.Skip("root reads a chmod 000 file") @@ -419,8 +418,7 @@ func TestDuplicateWarningsShareOneFormat(t *testing.T) { // TestRuleDuplicateWarningShortensThePath: a duplicate check that fails on // the file itself is reported on the rule with the path shortened and not -// repeated raw inside the cause, like the directory-level warnings (plan 11 -// review L7). +// repeated raw inside the cause, like the directory-level warnings. func TestRuleDuplicateWarningShortensThePath(t *testing.T) { if os.Getuid() == 0 { t.Skip("root reads a chmod 000 file") @@ -464,7 +462,7 @@ func TestRuleDuplicateWarningShortensThePath(t *testing.T) { // TestFileMatchNamesTheDuplicate: a file a (duplicate) test matched comes // back with the other copy's absolute path, so a front end can say where it // is and act on it - the reason text alone says "duplicate of NAME" for a -// copy inside the directory, which reads as no place at all (plan 21). +// copy inside the directory, which reads as no place at all. func TestFileMatchNamesTheDuplicate(t *testing.T) { e, d, _ := fixture(t) res, err := e.Match(context.Background(), d) diff --git a/internal/engine/roundtrip_test.go b/internal/engine/roundtrip_test.go index e54b0b7..218e9fe 100644 --- a/internal/engine/roundtrip_test.go +++ b/internal/engine/roundtrip_test.go @@ -181,11 +181,11 @@ func TestApplyThenUndoRestoresTheTree(t *testing.T) { } } -// TestUndoReversesChainsWithinOneFile is the property test's first finding -// (plan 8, Task 9): undo judged each move and rename against the disk as it -// is now, so the first step of a chain - a rename then a move, two moves, a -// move then trash - found its destination empty, because the later step had -// already moved the file on, and the whole file was refused. +// TestUndoReversesChainsWithinOneFile is the property test's first finding: +// undo judged each move and rename against the disk as it is now, so the +// first step of a chain - a rename then a move, two moves, a move then +// trash - found its destination empty, because the later step had already +// moved the file on, and the whole file was refused. func TestUndoReversesChainsWithinOneFile(t *testing.T) { for name, rule := range map[string]string{ "rename then move": `(rule "r" (rename "r-{name}") (move "Out"))`, diff --git a/internal/engine/session.go b/internal/engine/session.go index d79c364..c2830bd 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -105,10 +105,10 @@ func (s *Session) Apply(ctx context.Context, dp *DirPlan, approved map[string]bo // FinishDirectory ends one directory of a real run: the disk is now the // truth for the next one, so the claims start again from where this run's // files have actually ended up - every directory's, not just this one's -// (spec §7.4, plan 13 review F3). A path this directory planned but did not -// apply is free again; a path it did apply stays protected from a later -// (on-conflict overwrite) for the rest of the run, even across a directory -// that applies nothing. A dry run applies nothing and keeps every claim. +// (spec §7.4). A path this directory planned but did not apply is free +// again; a path it did apply stays protected from a later (on-conflict +// overwrite) for the rest of the run, even across a directory that applies +// nothing. A dry run applies nothing and keeps every claim. func (s *Session) FinishDirectory() { if s.dry { return diff --git a/internal/engine/session_test.go b/internal/engine/session_test.go index 98ea1ee..5cab274 100644 --- a/internal/engine/session_test.go +++ b/internal/engine/session_test.go @@ -183,8 +183,8 @@ func TestSessionLockDirsReleasesOnFailure(t *testing.T) { // TestSessionKeepsEveryAppliedDestinationClaimed: what a directory's files // ended up at stays protected for the whole run, even across a directory -// that applies nothing (plan 13 review F3): a later (on-conflict overwrite) -// takes a free name instead of trashing an earlier directory's result. +// that applies nothing: a later (on-conflict overwrite) takes a free name +// instead of trashing an earlier directory's result. func TestSessionKeepsEveryAppliedDestinationClaimed(t *testing.T) { h := sandbox(t) old := time.Now().Add(-2 * time.Hour) diff --git a/internal/engine/undo_identity_test.go b/internal/engine/undo_identity_test.go index 01f57d8..994c802 100644 --- a/internal/engine/undo_identity_test.go +++ b/internal/engine/undo_identity_test.go @@ -86,7 +86,7 @@ func undoFileNamed(t *testing.T, up *UndoPlan, dir, name string) UndoFile { // TestUndoRefusesAReusedTrashEntry: the Trash is emptied and another file of // the same name trashed after the run; undo must not restore that file in -// place of the one the run trashed (review M2). +// place of the one the run trashed. func TestUndoRefusesAReusedTrashEntry(t *testing.T) { e, run, h, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (delete))\n"}) @@ -114,7 +114,7 @@ func TestUndoRefusesAReusedTrashEntry(t *testing.T) { // TestUndoRefusesATrashEntryThatChanged: a trash entry that is no longer the // file the run put there (its size changed) is refused, like a moved file -// that changed (review M2). +// that changed. func TestUndoRefusesATrashEntryThatChanged(t *testing.T) { e, run, _, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (delete))\n"}) @@ -132,7 +132,7 @@ func TestUndoRefusesATrashEntryThatChanged(t *testing.T) { // TestUndoKeepsSameNamedFilesOfTwoDirectoriesApart: a.pdf from dl and // a.pdf from scans, moved in one run, are two files to undo, each refused or -// restored on its own (review M7). +// restored on its own. func TestUndoKeepsSameNamedFilesOfTwoDirectoriesApart(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "from dl"}, "scans": {"a.pdf": "from scans"}}, @@ -172,7 +172,7 @@ func TestUndoKeepsSameNamedFilesOfTwoDirectoriesApart(t *testing.T) { } // TestUndoRechecksAtExecution: a file edited after the undo was planned (for -// example while its review was open) is not moved back (review undo F8). +// example while its review was open) is not moved back. func TestUndoRechecksAtExecution(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) @@ -203,7 +203,7 @@ func TestUndoRechecksAtExecution(t *testing.T) { // TestUndoLeavesNoDirectoriesBehind: undo passing a file back through a // directory it had already removed for another file recreates it; that -// directory must be gone again when the undo ends (review undo F6). +// directory must be gone again when the undo ends. func TestUndoLeavesNoDirectoriesBehind(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out/{mtime:%Y}\") (move \"Out\"))\n"}) @@ -227,7 +227,7 @@ func TestUndoLeavesNoDirectoriesBehind(t *testing.T) { // TestUndoCanBeFinishedAfterAFailure: an undo whose last reversal failed // (something took the original name) leaves the file part way back; once // the obstacle is gone, undoing the same run again offers only the step -// that is left, and finishes it (review M10). +// that is left, and finishes it. func TestUndoCanBeFinishedAfterAFailure(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"}) @@ -278,7 +278,7 @@ func TestUndoCanBeFinishedAfterAFailure(t *testing.T) { // TestApplyReportsAStepThatCouldNotBeLogged: when the log stops accepting // writes mid-chain, the step that already ran is named in the error - file, // action and where the file is now - so the user can find what undo cannot -// see (re-review N1). +// see. func TestApplyReportsAStepThatCouldNotBeLogged(t *testing.T) { h := sandbox(t) p := filepath.Join(h, "dl", "a.pdf") @@ -319,8 +319,7 @@ func TestApplyReportsAStepThatCouldNotBeLogged(t *testing.T) { } // TestUndoRefusesOnlyTheFileWithADamagedLine: a crash that cuts one file's -// log line refuses that file; the other files of the run are still undone -// (re-review N1). +// log line refuses that file; the other files of the run are still undone. func TestUndoRefusesOnlyTheFileWithADamagedLine(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) @@ -362,8 +361,7 @@ func TestUndoRefusesOnlyTheFileWithADamagedLine(t *testing.T) { // TestResumedUndoStillRefusesAChangedFile: an undo that stopped after its // first reversal must not let that reversal vouch for the file later: the -// file edited in between is refused when the undo is resumed (re-review -// undo F1). +// file edited in between is refused when the undo is resumed. func TestResumedUndoStillRefusesAChangedFile(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"}) @@ -405,7 +403,7 @@ func TestResumedUndoStillRefusesAChangedFile(t *testing.T) { // TestUndoDoesNotOfferOnlyADirectoryRemoval: when a directory the run made // still holds a file of the user's, the restored file's remaining -// directory removal is not offered on every later undo (re-review undo F3). +// directory removal is not offered on every later undo. func TestUndoDoesNotOfferOnlyADirectoryRemoval(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) @@ -435,8 +433,7 @@ func TestUndoDoesNotOfferOnlyADirectoryRemoval(t *testing.T) { // TestApplyLogsEachStepAsItCompletes: the first step's log entry is written // before the second step runs - observed from the clock the log asks for -// each entry's time - so a run killed mid-chain leaves what it did undoable -// (review M9). +// each entry's time - so a run killed mid-chain leaves what it did undoable. func TestApplyLogsEachStepAsItCompletes(t *testing.T) { h := sandbox(t) p := filepath.Join(h, "dl", "a.pdf") @@ -475,7 +472,7 @@ func TestApplyLogsEachStepAsItCompletes(t *testing.T) { // TestUndoRefusesATrashEntryRecordedForAnotherPath: a trash entry with the // size and mtime the run logged, whose trashinfo now names another original -// path, belongs to another file and is refused (review M2). +// path, belongs to another file and is refused. func TestUndoRefusesATrashEntryRecordedForAnotherPath(t *testing.T) { e, run, h, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (delete))\n"}) @@ -495,8 +492,7 @@ func TestUndoRefusesATrashEntryRecordedForAnotherPath(t *testing.T) { // TestFinishedUndoRemovesADirectoryLeftEmpty: a directory made by one file's // chain and still holding another file is not offered on its own, but once -// that other file's reversal empties it, the undo removes it (plan 10 -// re-check R1). +// that other file's reversal empties it, the undo removes it. func TestFinishedUndoRemovesADirectoryLeftEmpty(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) @@ -544,8 +540,7 @@ func TestFinishedUndoRemovesADirectoryLeftEmpty(t *testing.T) { } // TestUndoRunWithADamagedLineIsStillAnUndo: a damaged line in an undo run's -// log does not make that run look like an ordinary one that can be undone -// (plan 10 re-check R3). +// log does not make that run look like an ordinary one that can be undone. func TestUndoRunWithADamagedLineIsStillAnUndo(t *testing.T) { e, run, _, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"}) @@ -575,7 +570,7 @@ func TestUndoRunWithADamagedLineIsStillAnUndo(t *testing.T) { // TestUndoOverwriteThenMove: a move that replaced an existing file and was // then moved on is undone whole - the moved file goes back, and the file it -// replaced is restored to the path the later reversal vacates (triage 34m). +// replaced is restored to the path the later reversal vacates. func TestUndoOverwriteThenMove(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "Out/a.pdf": "old"}}, map[string]string{"dl": "(path \"~/dl\")\n(ignore \"Out/\")\n(recursive yes)\n(rule \"r\" (on-conflict overwrite) (move \"Out\") (move \"Out2\"))\n"}) @@ -608,8 +603,8 @@ func TestUndoOverwriteThenMove(t *testing.T) { // TestProjectionSeesAPathAnEarlierStepWillFill: a path not on disk yet that // a queued reversal will put a file at is occupied for the steps after it, -// and free again once a later one moves that file on (triage 34m: the -// "occupied" half of the projection had no test that could fail). +// and free again once a later one moves that file on (the "occupied" half +// of the projection had no test that could fail). func TestProjectionSeesAPathAnEarlierStepWillFill(t *testing.T) { x := filepath.Join(t.TempDir(), "x.pdf") p := newUndoProjection() diff --git a/internal/extract/extract.go b/internal/extract/extract.go index c0f0262..f8f98df 100644 --- a/internal/extract/extract.go +++ b/internal/extract/extract.go @@ -29,7 +29,7 @@ var ( // ErrPartial wraps the error of a document read only in part (an // archive entry that would not open or parse). Text returns the text it // did read along with it: a keyword found there is found, but one not - // found may be in the part that could not be read (plan 11). + // found may be in the part that could not be read. ErrPartial = errors.New("part of it could not be read") // ErrTooLarge is returned when the file is larger than the configured // max-read; nothing is read in that case. @@ -70,15 +70,15 @@ var plainExt = map[string]bool{ } // zipExt is the set of Office/OpenDocument/ebook formats: a zip container -// plus XML inside it (Task 5). +// plus XML inside it. var zipExt = map[string]bool{ "docx": true, "xlsx": true, "pptx": true, "odt": true, "ods": true, "odp": true, "epub": true, } -// toolExt maps an extension to the external tool it needs (Task 6). pdf is -// handled separately, since it has its own fixed command line. +// toolExt maps an extension to the external tool it needs. pdf is handled +// separately, since it has its own fixed command line. var toolExt = map[string]string{ "doc": "antiword", // falls back to catdoc "xls": "xls2csv", @@ -108,8 +108,8 @@ func newWithPath(path string) *Extractor { for _, name := range toolNames { for _, dir := range dirs { // A relative entry would find a tool relative to the working - // directory - a bin/pdftotext an unpacked download left behind - // (review planapply F7) - so only absolute entries count. + // directory - a bin/pdftotext an unpacked download left behind - + // so only absolute entries count. if dir == "" || !filepath.IsAbs(dir) { continue } diff --git a/internal/extract/plain.go b/internal/extract/plain.go index beffae1..956c3e6 100644 --- a/internal/extract/plain.go +++ b/internal/extract/plain.go @@ -39,13 +39,12 @@ func readDecoded(path string) (string, error) { // sample that merely looks like UTF-8 must hold for the WHOLE file — no // NUL byte anywhere, and no invalid UTF-8 anywhere past the sample — or // the file is ErrUnsupported after all (a self-extracting installer has no -// text in krino's sense, decided after the plan 10 re-check); the -// Latin-1 fallback in decode -// never applies to a sniffed file, only to a file whose extension already -// names it as text. D2: when the file continues past the sample (n == -// sniffSize), the validity check is run against a trimmed copy with any -// incomplete trailing rune removed, so a multi-byte rune that happens to -// straddle byte sniffSize does not make an otherwise-valid file sniff as +// text in krino's sense); the Latin-1 fallback in decode never applies to +// a sniffed file, only to a file whose extension already names it as +// text. When the file continues past the sample (n == sniffSize), the +// validity check is run against a trimmed copy with any incomplete +// trailing rune removed, so a multi-byte rune that happens to straddle +// byte sniffSize does not make an otherwise-valid file sniff as // unsupported; sample itself, used below to build the returned text, is // left untouched — the rest of the file (read after the check) supplies // the bytes trimming set aside. @@ -89,13 +88,13 @@ func sniffText(path string) (string, error) { } // trimIncompleteTrailingRune drops an incomplete UTF-8 sequence left -// dangling at the very end of b — D2's fix for a rune cut off exactly at -// the sniff sample's boundary. It looks back at most utf8.UTFMax-1 bytes -// for the start of the trailing rune; if the bytes from there to the end -// are not a complete encoding (utf8.FullRune), that partial rune is cut, -// since more bytes to finish it may simply not have been read yet. A -// sample already ending cleanly (the common case, and every all-ASCII -// sample) is returned unchanged. +// dangling at the very end of b, so a rune cut off exactly at the sniff +// sample's boundary is not mistaken for invalid UTF-8. It looks back at +// most utf8.UTFMax-1 bytes for the start of the trailing rune; if the +// bytes from there to the end are not a complete encoding (utf8.FullRune), +// that partial rune is cut, since more bytes to finish it may simply not +// have been read yet. A sample already ending cleanly (the common case, +// and every all-ASCII sample) is returned unchanged. func trimIncompleteTrailingRune(b []byte) []byte { end := len(b) start := end - 1 @@ -143,7 +142,7 @@ func decodeUTF16(b []byte, order binary.ByteOrder) string { // decodeLatin1 decodes b as Latin-1: each byte is its own Unicode code // point. Built directly as UTF-8 (at most two bytes per input byte), not -// through a []rune of four bytes per input byte (triage 28d). +// through a []rune of four bytes per input byte. func decodeLatin1(b []byte) string { var s strings.Builder s.Grow(len(b) * 2) @@ -200,10 +199,10 @@ func stripMarkup(s string) string { gt := strings.IndexByte(s[j:], '>') if gt == -1 { - // D1: an unterminated tag (no closing '>') can no longer be - // parsed as markup, but that is no reason to discard the rest - // of the file - copy it through as literal text instead of - // simply stopping the scan. + // An unterminated tag (no closing '>') can no longer be + // parsed as markup, but that is no reason to discard the + // rest of the file - copy it through as literal text + // instead of simply stopping the scan. b.WriteString(s[i:]) break } diff --git a/internal/extract/plain_test.go b/internal/extract/plain_test.go index e53ac73..9e8b747 100644 --- a/internal/extract/plain_test.go +++ b/internal/extract/plain_test.go @@ -156,7 +156,7 @@ func TestToolsListedInOrder(t *testing.T) { // sample — sniffText must reject the whole file, not just decode what the // sample alone promised (it must not fall back to Latin-1 the way a known // text extension would). Such a file - a self-extracting installer, say - -// has no text in krino's sense (decided after the plan 10 re-check). +// has no text in krino's sense. func TestSniffWholeFileMustBeValid(t *testing.T) { e := newWithPath("") data := append([]byte(strings.Repeat("x", 8192)), 0xFF, 0x00) @@ -194,7 +194,7 @@ func TestToolLookupSkipsNonRegular(t *testing.T) { } } -// TestUnterminatedTagKeepsRemainder: D1. An unterminated ordinary tag (no +// TestUnterminatedTagKeepsRemainder: an unterminated ordinary tag (no // closing '>') must not discard the rest of the file - only the malformed // tag markup itself is unrecoverable; whatever follows it is still real // content and must still reach the extracted text. @@ -213,10 +213,10 @@ func TestUnterminatedTagKeepsRemainder(t *testing.T) { } } -// TestSniffRuneStraddlingSampleBoundary: D2. A multi-byte rune ("ż", two -// UTF-8 bytes) placed exactly so its lead byte is the sniff sample's last -// byte and its continuation byte falls just past it must not make an -// otherwise valid UTF-8 file sniff as unsupported. +// TestSniffRuneStraddlingSampleBoundary: a multi-byte rune ("ż", two UTF-8 +// bytes) placed exactly so its lead byte is the sniff sample's last byte +// and its continuation byte falls just past it must not make an otherwise +// valid UTF-8 file sniff as unsupported. func TestSniffRuneStraddlingSampleBoundary(t *testing.T) { e := newWithPath("") prefix := strings.Repeat("a", sniffSize-1) @@ -232,7 +232,7 @@ func TestSniffRuneStraddlingSampleBoundary(t *testing.T) { // TestLatin1DecodingMemory: decoding Latin-1 allocates about what the UTF-8 // result needs (at most two bytes per input byte), not a four-byte rune per -// input byte on top of it (triage 28d). +// input byte on top of it. func TestLatin1DecodingMemory(t *testing.T) { data := bytes.Repeat([]byte("Gr\xfc\xdfe "), 16000) r := testing.Benchmark(func(b *testing.B) { diff --git a/internal/extract/zipxml_test.go b/internal/extract/zipxml_test.go index 30a4cf9..5b7ba54 100644 --- a/internal/extract/zipxml_test.go +++ b/internal/extract/zipxml_test.go @@ -128,10 +128,9 @@ func TestOdfEmbeddedObjectIncluded(t *testing.T) { // The footer's mismatched end tag (</w:xyz> where </w:ftr> was open) // genuinely fails to parse under Strict=false — unlike a merely missing // end tag, which the decoder synthesises and swallows — but only after -// "broken" and the newline for </w:p> have already been written; the -// XML decoder itself confirms this token by token (see fix round 2's -// report for the trace): CharData "broken", EndElement p, then the -// error "unexpected end element </xyz>". +// "broken" and the newline for </w:p> have already been written; the XML +// decoder itself confirms this token by token: CharData "broken", +// EndElement p, then the error "unexpected end element </xyz>". func TestZipLenientOnMalformedEntry(t *testing.T) { e := newWithPath("") got, err := text(t, e, zipFile(t, "partial.docx", map[string]string{ @@ -139,7 +138,7 @@ func TestZipLenientOnMalformedEntry(t *testing.T) { "word/footer1.xml": `<w:ftr xmlns:w="w"><w:p><w:r><w:t>broken</w:t></w:r></w:p></w:xyz>`, }), 0) // Partly read: the text is kept, and ErrPartial says some of it is - // missing, so a keyword not found in it is unknown (plan 11). + // missing, so a keyword not found in it is unknown. if !errors.Is(err, ErrPartial) { t.Fatalf("good entry alongside a malformed one: err %v, want ErrPartial", err) } @@ -193,9 +192,9 @@ func TestZipNoTextAndFailingSiblingErrors(t *testing.T) { } } -// TestMaxReadCapsZipOutput: B1. A directory's max-read, when smaller than -// the fixed zipBudget default, caps a single archive's extracted text on -// its own — zipBudget is left at its default, so only budget(zipBudget, +// TestMaxReadCapsZipOutput: a directory's max-read, when smaller than the +// fixed zipBudget default, caps a single archive's extracted text on its +// own — zipBudget is left at its default, so only budget(zipBudget, // maxRead) picking the smaller maxRead explains the result. func TestMaxReadCapsZipOutput(t *testing.T) { e := newWithPath("") diff --git a/internal/journal/journal.go b/internal/journal/journal.go index 085c88a..60dc37f 100644 --- a/internal/journal/journal.go +++ b/internal/journal/journal.go @@ -92,12 +92,12 @@ func endWithNewline(f *os.File) error { // 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. +// identically under both). 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), diff --git a/internal/journal/journal_test.go b/internal/journal/journal_test.go index e8b18ae..1d53941 100644 --- a/internal/journal/journal_test.go +++ b/internal/journal/journal_test.go @@ -88,16 +88,15 @@ func TestRoundTripsAwkwardNames(t *testing.T) { } } -// 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. +// TestAppendWritesRFC3339WithOffsetAndKeepsNanoseconds pins the journal's +// on-disk time format: nothing else does - 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) @@ -151,7 +150,7 @@ func TestAppendIsAppendOnly(t *testing.T) { // TestOpenRepairsAMissingFinalNewline: a crash can leave the log's last // line without its newline; the next run's first line must not be glued -// onto it, or that run could never be undone (review M8). +// onto it, or that run could never be undone. func TestOpenRepairsAMissingFinalNewline(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") if err := os.WriteFile(path, []byte("2026-09-11T10:02:03+02:00\tR0\tdl\ta.pdf\t1\tmo"), 0o644); err != nil { diff --git a/internal/journal/read.go b/internal/journal/read.go index 5a1de04..aa79118 100644 --- a/internal/journal/read.go +++ b/internal/journal/read.go @@ -25,12 +25,12 @@ const wantFields = 13 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. +// record which run it reverses (see undoOfPrefix). 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 } @@ -44,8 +44,8 @@ type Run struct { Counts map[string]int // action -> count of status "ok" Undone bool // a later run reversed this one // PartlyUndone is set with Undone while fewer of the run's reversible - // steps have been reversed, over all its undo runs, than it took - // (triage 34l): some were declined, refused or failed. + // steps have been reversed, over all its undo runs, than it took: some + // were declined, refused or failed. PartlyUndone bool UndoOf string // for an undo run, the run it reverses; "" otherwise } @@ -67,10 +67,10 @@ type ReversedKey struct { // ReversedSteps counts, for runID, every reversal that earlier undo runs of // it completed ("ok" undo- entries of runs whose run-start says they undo -// runID), so a later undo of the same run can offer only what is left -// (review M10). An undo run's own unparsable lines are skipped; a missing -// reversal is then offered again, where its own checks refuse it if it had -// in fact happened. +// runID), so a later undo of the same run can offer only what is left. An +// undo run's own unparsable lines are skipped; a missing reversal is then +// offered again, where its own checks refuse it if it had in fact +// happened. func ReversedSteps(path, runID string) (map[ReversedKey]int, error) { lines, err := readLines(path) if err != nil { @@ -93,19 +93,20 @@ func ReversedSteps(path, runID string) (map[ReversedKey]int, error) { return out, nil } -// Entries returns every entry belonging to runID, in file order. Since plan -// 10 (re-review N1), an unparsable line whose run column names another run -// is ignored, and one of this run whose directory and file columns are still -// readable is returned as a "damaged" entry for that file, so undo refuses -// that file alone. Otherwise 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. +// Entries returns every entry belonging to runID, in file order. An +// unparsable line whose run column names another run is ignored, and one +// of this run whose directory and file columns are still readable is +// returned as a "damaged" entry for that file, so undo refuses that file +// alone. Otherwise 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 @@ -154,7 +155,7 @@ func Entries(path, runID string) ([]Entry, error) { run, runFound := runFieldOf(line) if runFound && run != runID { // Another run's damaged line: runs of different directories can - // interleave, and it says nothing about this one (re-review N1). + // interleave, and it says nothing about this one. continue } ours := inWindow || (runFound && run == runID) @@ -165,7 +166,7 @@ func Entries(path, runID string) ([]Entry, error) { // A line of this run cut or damaged where its file is still // readable: that file's chain may be missing a step, so it is // returned as damaged and PlanUndo refuses just that file; the - // rest of the run stays undoable (re-review N1). + // rest of the run stays undoable. out = append(out, Entry{Run: runID, Dir: dir, File: file, Action: "damaged", Status: "damaged", Detail: fmt.Sprintf("line %d", i+1)}) continue } @@ -185,8 +186,8 @@ func Entries(path, runID string) ([]Entry, error) { // 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. The column counts only when a tab -// ends it: a line cut inside it holds a prefix of some run's ID, which names -// no run (plan 10 re-check R2). +// ends it: a line cut inside it holds a prefix of some run's ID, which +// names no run. func runFieldOf(line string) (string, bool) { f := strings.SplitN(line, "\t", 3) if len(f) < 3 { @@ -214,13 +215,13 @@ func fileFieldsOf(line string) (dir, file string, ok bool) { // // 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. +// reversed something: a fully declined undo - every file declined rather +// than reversed - 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. Without this check, `krino +// undo` with every file declined would leave `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 { @@ -230,9 +231,8 @@ func Runs(path string, n int) ([]Run, error) { order := make([]string, 0) byID := make(map[string]*Run) pendingUndo := make(map[string]string) // undo run ID -> the run ID it claims to undo - // A file whose chain ended in a permanent delete is never undone, so its - // reversible steps do not count toward what a run took (plan 11 review - // L8). + // A file whose chain ended in a permanent delete is never undone, so + // its reversible steps do not count toward what a run took. type fileOf struct{ run, dir, file string } reversibleOf := map[fileOf]int{} deletedFile := map[fileOf]bool{} @@ -312,22 +312,22 @@ func Runs(path string, n int) ([]Run, error) { 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. +// 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, or having failed to restore anything +// while a wholly unrelated undo-mkdir still happened to succeed: +// "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 the same +// 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-") { diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go index 4f1c902..d2bbd3a 100644 --- a/internal/journal/read_test.go +++ b/internal/journal/read_test.go @@ -104,17 +104,17 @@ func TestRunsMarksAnUndoneRun(t *testing.T) { } } -// 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. +// TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined: an undo run's +// run-start Detail alone is not 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 - otherwise `krino log` +// would tell 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) @@ -146,19 +146,19 @@ func TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined(t *testing.T) { } } -// 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. +// TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir: "at least one ok +// undo-* entry" alone is too loose a check, by the same shape as the bug +// it fixes above. 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 the same bug again, by a narrower +// route. func TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -196,12 +196,12 @@ func TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir(t *testing.T) { } } -// TestEntriesCrashedRunReturnsNilError is item 1: a run-start present, -// run-end absent, and otherwise clean is exactly the crashed-run shape -// Entries' own doc comment says it must accept - "to end of file when there -// is no run-end (a crashed run, which is precisely when corruption is -// likely)". Pinning it as its own test, rather than leaving it implicit in -// tests about something else, is the point of the item. +// TestEntriesCrashedRunReturnsNilError: a run-start present, run-end +// absent, and otherwise clean is exactly the crashed-run shape Entries' +// own doc comment says it must accept - "to end of file when there is no +// run-end (a crashed run, which is precisely when corruption is likely)". +// Pinning it as its own test, rather than leaving it implicit in tests +// about something else, is deliberate. func TestEntriesCrashedRunReturnsNilError(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -227,10 +227,10 @@ func TestEntriesCrashedRunReturnsNilError(t *testing.T) { } } -// TestEntriesIntactRunReturnsNilError is item 2: a complete, clean run - -// run-start, a step, run-end, nothing corrupt - must read back with a nil -// error. Every other test in this file needs this to be true along the way, -// but none of them state it as their own point; this one does. +// TestEntriesIntactRunReturnsNilError: a complete, clean run - run-start, +// a step, run-end, nothing corrupt - must read back with a nil error. +// Every other test in this file needs this to be true along the way, but +// none of them state it as their own point; this one does. func TestEntriesIntactRunReturnsNilError(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -258,9 +258,9 @@ func TestEntriesIntactRunReturnsNilError(t *testing.T) { } } -// TestEntriesBothFailureModesReportsBadLineFirst is item 3: a run that both -// has an unparsable line inside its window AND lacks a readable run-start -// must surface as the unparsable-line error, not the missing-run-start one - +// TestEntriesBothFailureModesReportsBadLineFirst: a run that both has an +// unparsable line inside its window AND lacks a readable run-start must +// surface as the unparsable-line error, not the missing-run-start one - // Entries checks badLine before sawRunStart. The run-start line here is // destroyed unattributably (as in TestEntriesFailsClosedOnMissingRunStart), // and a second, still-attributable line is separately corrupted so badLine @@ -303,9 +303,9 @@ func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) { t.Fatal(err) } - // Since plan 10 a damaged line whose file is readable refuses only that - // file (a "damaged" entry); the run as a whole still fails closed here, - // on its missing run-start. + // A damaged line whose file is readable refuses only that file (a + // "damaged" entry); the run as a whole still fails closed here, on its + // missing run-start. got, err := Entries(path, "A") if err == nil { t.Fatal("Entries returned no error with a missing run-start") @@ -318,11 +318,10 @@ func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) { } } -// TestEntriesAdjacentRunStartsOneCorrupted is item 4. Ruling R2: this pins -// what journal.Entries does TODAY for two runs whose run-start lines are -// adjacent, one of them corrupted - it does not assert an invented "correct" -// result, and internal/journal is not touched by this task. The log here is -// exactly: +// TestEntriesAdjacentRunStartsOneCorrupted pins what journal.Entries does +// for two runs whose run-start lines are adjacent, one of them corrupted - +// it asserts the observed behaviour, not an invented "correct" result. +// The log here is exactly: // // 1 run-start A (good) // 2 run-start B (corrupted: no tabs, unattributable) @@ -342,11 +341,10 @@ func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) { // line), so it reaches the end of the file with no badLine, and instead // fails on B's missing run-start. // -// Concern (not fixed here, per R2 - flagged for judgement, not code -// change): the SAME corrupted line produces two different error shapes -// depending only on which run asks, which is a surprising inconsistency in -// the message a caller sees, even though both directions correctly fail -// closed. +// This is a known inconsistency, left as is rather than fixed: the SAME +// corrupted line produces two different error shapes depending only on +// which run asks, which is a surprising inconsistency in the message a +// caller sees, even though both directions correctly fail closed. func TestEntriesAdjacentRunStartsOneCorrupted(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -415,10 +413,10 @@ func TestEntriesAdjacentRunStartsOneCorrupted(t *testing.T) { // 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. Since plan 10 (re-review -// N1) a line whose directory and file columns are readable is returned as a -// "damaged" entry for that file, so only that file is refused and the rest -// of the run can still be undone. +// - PlanUndo needs to know a step went missing. A line whose directory and +// file columns are readable is returned as a "damaged" entry for that +// file, so only that file is refused and the rest of the run can still be +// undone. func TestEntriesReportsAMangledLine(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -464,9 +462,9 @@ func TestEntriesReportsAMangledLine(t *testing.T) { } } -// TestEntriesIgnoresAnotherRunsDamagedLine: a damaged line whose run column -// names another run - two directories' runs can interleave in one log - does -// not refuse this run, even inside its window (re-review N1). +// TestEntriesIgnoresAnotherRunsDamagedLine: a damaged line whose run +// column names another run - two directories' runs can interleave in one +// log - does not refuse this run, even inside its window. func TestEntriesIgnoresAnotherRunsDamagedLine(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -657,8 +655,8 @@ func TestEntriesFailsClosedOnMissingRunStart(t *testing.T) { // TestReversedStepsCountsEveryUndoOfARun: the steps earlier undo runs of a // run already reversed - only "ok" undo entries of runs that undo it - so -// a later undo of the same run can offer just what is left (review M10). -// Runs also names the run an undo run reversed. +// a later undo of the same run can offer just what is left. Runs also +// names the run an undo run reversed. func TestReversedStepsCountsEveryUndoOfARun(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, err := Open(path) @@ -705,8 +703,8 @@ func TestReversedStepsCountsEveryUndoOfARun(t *testing.T) { // TestEntriesRefusesALineCutInsideItsRunColumn: a crash that cuts the last // line inside its run column leaves a prefix of some run's ID - it cannot -// be called another run's line, so inside this run's window it refuses the -// run (plan 10 re-check R2). +// be called another run's line, so inside this run's window it refuses +// the run. func TestEntriesRefusesALineCutInsideItsRunColumn(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -733,9 +731,9 @@ func TestEntriesRefusesALineCutInsideItsRunColumn(t *testing.T) { } // TestRunsMarksAPartlyUndoneRun: a run whose undo reversed some of its -// reversible steps but not all is partly undone; once a later undo reverses -// the rest, it is undone in full. A permanent delete counts toward neither -// (triage 34l). +// reversible steps but not all is partly undone; once a later undo +// reverses the rest, it is undone in full. A permanent delete counts +// toward neither. func TestRunsMarksAPartlyUndoneRun(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -780,9 +778,9 @@ func TestRunsMarksAPartlyUndoneRun(t *testing.T) { } } -// TestRunsIgnoresAPermanentlyDeletedFilesSteps: a file whose chain ended in -// a permanent delete can never be undone, so its earlier steps do not keep -// the run partly undone forever (plan 11 review L8). +// TestRunsIgnoresAPermanentlyDeletedFilesSteps: a file whose chain ended +// in a permanent delete can never be undone, so its earlier steps do not +// keep the run partly undone forever. func TestRunsIgnoresAPermanentlyDeletedFilesSteps(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) diff --git a/internal/lock/lock.go b/internal/lock/lock.go index 2b98227..76a2b71 100644 --- a/internal/lock/lock.go +++ b/internal/lock/lock.go @@ -48,11 +48,10 @@ type Lock struct { // 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. +// waiting Acquire return ctx.Err() promptly instead of ignoring it - 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 diff --git a/internal/lock/lock_test.go b/internal/lock/lock_test.go index 53ce5ca..04023aa 100644 --- a/internal/lock/lock_test.go +++ b/internal/lock/lock_test.go @@ -69,12 +69,12 @@ func TestAcquireWaitsUntilReleased(t *testing.T) { } } -// 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. +// TestAcquireRespectsContextCancellation: a waiting Acquire must not +// ignore an interrupt - a cancelled ctx must return promptly with +// ctx.Err(), not poll forever. A regression here would HANG rather than +// fail, 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) diff --git a/internal/norm/norm.go b/internal/norm/norm.go index 1c2f61d..9a22e62 100644 --- a/internal/norm/norm.go +++ b/internal/norm/norm.go @@ -20,7 +20,7 @@ const Version = 1 // Fingerprint names everything Text and Name's output depends on: Version, // and the Unicode tables of the standard library and of x/text's -// normalisation, which a Go or x/text upgrade can change (review cache F4). +// normalisation, which a Go or x/text upgrade can change. func Fingerprint() string { return fmt.Sprintf("norm%d unicode%s nfd%s", Version, unicode.Version, unorm.Version) } @@ -125,7 +125,7 @@ func FoldMapped(s string) Folded { // Source returns the original text that Text[a:b] was folded from, widened // to whole characters and to the characters right after them that fold to // nothing - the combining marks of a decomposed name, which belong to the -// letter before them (plan 11 review L5); without a map, Text[a:b] itself. +// letter before them; without a map, Text[a:b] itself. func (f Folded) Source(a, b int) string { switch { case f.same || f.start == nil: diff --git a/internal/norm/norm_test.go b/internal/norm/norm_test.go index dfae915..9d4e214 100644 --- a/internal/norm/norm_test.go +++ b/internal/norm/norm_test.go @@ -76,8 +76,8 @@ func TestFoldASCIINoAlloc(t *testing.T) { // TestFoldMappedSource: a part of the folded text leads back to the original // characters it came from, widened to whole characters and to the combining -// marks that follow them (a decomposed name, plan 11 review L5) - so a -// capture can be written with its diacritics. +// marks that follow them (a decomposed name) - so a capture can be written +// with its diacritics. func TestFoldMappedSource(t *testing.T) { cases := []struct { in, folded string diff --git a/internal/plan/chain_test.go b/internal/plan/chain_test.go index 28c208f..0c89987 100644 --- a/internal/plan/chain_test.go +++ b/internal/plan/chain_test.go @@ -118,9 +118,9 @@ func TestBuildRenameWithSlash(t *testing.T) { } } -// TestBuildKeepsSteplessChains is D6: Build returns one Chain per Input even -// when a file's rules contribute no actions, so a caller can tell "matched a -// rule that does nothing" (an exclusion) from "not matched at all". Plan 3's +// TestBuildKeepsSteplessChains: Build returns one Chain per Input even +// when a file's rules contribute no actions, so a caller can tell "matched +// a rule that does nothing" (an exclusion) from "not matched at all". The // "to act on" count and the JSON document's empty steps array both rest on // this. func TestBuildKeepsSteplessChains(t *testing.T) { diff --git a/internal/plan/conflict.go b/internal/plan/conflict.go index c660a8b..7cebc23 100644 --- a/internal/plan/conflict.go +++ b/internal/plan/conflict.go @@ -62,13 +62,13 @@ type claimed map[string]bool // the step must not run) and Displaces (non-empty only for overwrite of a // file that exists on disk). func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk, c claimed) (resolved, skip, displaces string) { - // A1/A2: the file is already where this step would put it, so its own - // existence must not read as a conflict with itself. Without this guard a - // move or rename plans a rename to stem_1 and every later run adds - // another generation; under (on-conflict overwrite) the step records the - // file as its own Displaces, which plan 4 would trash before moving from - // a path that no longer exists. Checked before the policy switch, so - // overwrite never reaches its own branch. + // The file is already where this step would put it, so its own + // existence must not read as a conflict with itself. Without this + // guard a move or rename plans a rename to stem_1 and every later run + // adds another generation; under (on-conflict overwrite) the step + // records the file as its own Displaces, which would then be trashed + // before moving from a path that no longer exists. Checked before the + // policy switch, so overwrite never reaches its own branch. if dst == src { return dst, "already there", "" } @@ -81,9 +81,9 @@ func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk, // through to the ordinary conflict policy below instead of failing // outright. A wrong "different" verdict costs at worst an // unnecessary suffixed copy, never data loss, so resolving the - // conflict anyway is an acceptable trade-off here (D8) - a caller - // that wants the failure itself visible would need it surfaced as - // a chain warning instead. + // conflict anyway is an acceptable trade-off here - a caller that + // wants the failure itself visible would need it surfaced as a + // chain warning instead. if same, err := d.SameContent(src, dst); err == nil && same { return dst, "already there", "" } @@ -98,17 +98,17 @@ func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk, return dst, "target exists", "" case config.ConflictOverwrite: if onDisk && !c[dst] { - // Only a regular file is ever trashed to make room (review M4): - // a directory or link of the same name stays, and so does the + // Only a regular file is ever trashed to make room: a + // directory or link of the same name stays, and so does the // step - skipped, saying why. if !d.Regular(dst) { return dst, "target is not a regular file", "" } - // The existing file is trashed first (plan 4). Only the first - // step to reach this path may displace it: once another step - // in this same plan has already claimed dst, that path will - // hold that step's own output by the time this one runs, so - // displacing it again would destroy it. + // The existing file is trashed first. Only the first step to + // reach this path may displace it: once another step in this + // same plan has already claimed dst, that path will hold that + // step's own output by the time this one runs, so displacing + // it again would destroy it. return dst, "", dst } // Either claimed in-plan only (nothing on disk to displace — an @@ -124,21 +124,20 @@ func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk, resolved, skip := suffixed(dst, d, c) return resolved, skip, "" } - // Every policy has its own branch (review cli F13): a new one must not - // quietly plan as another. + // Every policy has its own branch: a new one must not quietly plan as + // another. panic(fmt.Sprintf("plan: unknown config.Conflict %d", int(policy))) } // maxSuffixAttempts bounds suffixed(): it is unbounded by design and -// terminates on a real filesystem, but C2 - without a cap, a Disk that -// always reports existence (or a directory A1 had been filling before its -// fix) turns planning quadratic instead of failing fast. +// terminates on a real filesystem, but without a cap, a Disk that always +// reports existence (or a directory being filled without the "already +// there" guard above) turns planning quadratic instead of failing fast. const maxSuffixAttempts = 10000 // suffixed finds the first stem_N.ext (N starting at 1) that is free: // neither on disk nor already claimed by an earlier step in this plan. It -// gives up after maxSuffixAttempts, returning a Skip reason and no path -// (C2). +// gives up after maxSuffixAttempts, returning a Skip reason and no path. func suffixed(dst string, d Disk, c claimed) (resolved, skip string) { dir, base := filepath.Split(dst) stem, ext := splitExt(base) diff --git a/internal/plan/placeholder_test.go b/internal/plan/placeholder_test.go index e155430..531a51f 100644 --- a/internal/plan/placeholder_test.go +++ b/internal/plan/placeholder_test.go @@ -30,7 +30,7 @@ func TestExpand(t *testing.T) { {"{mtime:%j}", "227"}, {"{now:%Y-%m-%d}", "2026-09-12"}, {"{1}", "2026"}, - // D2: an expanded value that itself contains "}}" must reach the + // An expanded value that itself contains "}}" must reach the // output unchanged. Expand's single-pass scanner jumps past a // placeholder's closing brace, so written bytes are never re-scanned; // a refactor to scan-then-replace would silently re-collapse them. @@ -77,11 +77,11 @@ func TestExpandErrors(t *testing.T) { {"{name", "unclosed placeholder"}, {"{mtime:%Q}", "unknown time format %Q in {mtime:...}"}, {"{mtime}", "unknown placeholder {mtime}"}, - // B3: the error must name the placeholder actually written ("now"), + // The error must name the placeholder actually written ("now"), // not hardcode "mtime" - the {mtime:%Q} case above passes either // way, which is why that defect survived. {"{now:%Q}", "unknown time format %Q in {now:...}"}, - // D3: {1}...{9} is the syntax (spec §7.3); {10} and up must be + // {1}...{9} is the syntax (spec §7.3); {10} and up must be // rejected the same way an unknown placeholder is. {"{10}", "unknown placeholder {10}"}, } diff --git a/internal/plan/step.go b/internal/plan/step.go index e4f280f..b3e34c1 100644 --- a/internal/plan/step.go +++ b/internal/plan/step.go @@ -20,7 +20,7 @@ const ( DeletePermanent // (delete permanent) ) -// String is for display only; Task 5's JSON representation defines its own +// String is for display only; the JSON representation defines its own // action names. func (k Kind) String() string { switch k { diff --git a/internal/trash/trash_test.go b/internal/trash/trash_test.go index 9c3eed2..38552bb 100644 --- a/internal/trash/trash_test.go +++ b/internal/trash/trash_test.go @@ -174,8 +174,8 @@ func TestPutRefusesOtherFilesystem(t *testing.T) { } // TestPutSkipsOrphanedFiles: a name already taken in files/ - left there -// without its trashinfo - is not overwritten; Put takes the next free name -// (review planapply F5). +// without its trashinfo - is not overwritten; Put takes the next free +// name. func TestPutSkipsOrphanedFiles(t *testing.T) { h := sandbox(t) orphan := filepath.Join(Dir(), "files", "a.pdf") @@ -195,8 +195,8 @@ func TestPutSkipsOrphanedFiles(t *testing.T) { } // TestPutTrashesLongNames: a name near the 255-byte limit still goes to the -// Trash - its entry name is shortened so ".trashinfo" and a suffix fit - and -// comes back under its full original name (review planapply F6). +// Trash - its entry name is shortened so ".trashinfo" and a suffix fit - +// and comes back under its full original name. func TestPutTrashesLongNames(t *testing.T) { h := sandbox(t) name := strings.Repeat("ż", 123) + ".pdf" // 246 + 4 = 250 bytes |