diff options
78 files changed, 974 insertions, 1064 deletions
@@ -4,3 +4,5 @@ /local/ /.superpowers/ gui/krino-gui +*.test +coverage.out diff --git a/cmd/krino/commands_test.go b/cmd/krino/commands_test.go index b8364e6..f681a0f 100644 --- a/cmd/krino/commands_test.go +++ b/cmd/krino/commands_test.go @@ -10,12 +10,11 @@ import ( ) // home gives each test its own HOME with no XDG overrides, and points the -// package's stdin seam at something guaranteed non-terminal (fix round -// 2026-09-12/item 4): every test that reaches cmdSort's terminal check or -// the interactive review must not depend on what the ambient test binary's -// stdin happens to be - if that were ever a real terminal, such a test -// would silently fall through to the interactive prompt and block on a -// keypress instead of failing. +// package's stdin seam at something guaranteed non-terminal: every test +// that reaches cmdSort's terminal check or the interactive review must not +// depend on what the ambient test binary's stdin happens to be - if that +// were ever a real terminal, such a test would silently fall through to +// the interactive prompt and block on a keypress instead of failing. func home(t *testing.T) string { t.Helper() h := t.TempDir() @@ -78,8 +77,8 @@ func TestInitNewCheck(t *testing.T) { } } -// TestConfigFlagExpandsTilde is item B: -c=~/... is not expanded by the -// shell (the ~ comes after =), so krino must expand it itself. +// TestConfigFlagExpandsTilde: -c=~/... is not expanded by the shell (the ~ +// comes after =), so krino must expand it itself. func TestConfigFlagExpandsTilde(t *testing.T) { h := home(t) cwd := t.TempDir() diff --git a/cmd/krino/exclude_test.go b/cmd/krino/exclude_test.go index f76be0c..2c77438 100644 --- a/cmd/krino/exclude_test.go +++ b/cmd/krino/exclude_test.go @@ -137,7 +137,7 @@ func TestSkipSummaryCountsTooBig(t *testing.T) { // TestVerboseListsDirectoriesLeftOutOfTheWalk: a rule's destination inside // the directory is not walked, so its files never show in any count; -v -// says so, and only for directories that exist (triage 4). +// says so, and only for directories that exist. func TestVerboseListsDirectoriesLeftOutOfTheWalk(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") @@ -167,7 +167,7 @@ func TestVerboseListsDirectoriesLeftOutOfTheWalk(t *testing.T) { } // TestExplainShowsAnUndecidedRule: explain names a rule it could not decide -// "undecided", not "no" (plan 12). +// "undecided", not "no". func TestExplainShowsAnUndecidedRule(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") diff --git a/cmd/krino/history_test.go b/cmd/krino/history_test.go index 9c409bc..d85766f 100644 --- a/cmd/krino/history_test.go +++ b/cmd/krino/history_test.go @@ -47,8 +47,8 @@ func TestLogListsRunsAndUndoReverses(t *testing.T) { if _, out, _ = runCLI(t, "log"); !strings.Contains(out, "undone") { t.Errorf("log does not mark the run undone:\n%s", out) } - // Plain undo after an undo continues the run it undid (review M10): - // everything came back, so nothing is left and nothing moves. + // Plain undo after an undo continues the run it undid: everything came + // back, so nothing is left and nothing moves. if code, out, errOut := runCLI(t, "undo", "-y"); code != 0 || !strings.Contains(out, "0 applied") { t.Errorf("undo after a complete undo: %d\n%s\n%s", code, out, errOut) } @@ -75,21 +75,21 @@ func TestUndoDryRunChangesNothing(t *testing.T) { } } -// TestUndoFailsImmediatelyWithHeldLock is fix round 2026-09-12, item 1: an -// undo moves files just as an apply does, so it needs the same per-directory -// guard sort's TestSecondRunFailsImmediatelyWithYes already pins for the -// forward path (spec §11: "a second krino on the same directory ... fails -// immediately with -y"). The lock file is held under the config NAME "dl", -// not any filesystem path - UndoFile.Dir is the journal's `dir` column, -// which is the directory's name from krino.conf, not its root. +// TestUndoFailsImmediatelyWithHeldLock: an undo moves files just as an +// apply does, so it needs the same per-directory guard sort's +// TestSecondRunFailsImmediatelyWithYes already pins for the forward path +// (spec §11: "a second krino on the same directory ... fails immediately +// with -y"). The lock file is held under the config NAME "dl", not any +// filesystem path - UndoFile.Dir is the journal's `dir` column, which is +// the directory's name from krino.conf, not its root. // -// Ruling (fix round 2026-09-12, follow-up): the lock is acquired before the -// plan is even shown, matching cmdSort's own window (acquired before -// Plan/review, held across both) rather than only around ApplyUndo - so -// this also asserts the refusal is noticed before any plan output reaches -// stdout. A version of this test that only checked the exit code and -// stderr would pass equally whether the lock were taken early or late, and -// so would not be pinning the thing this ruling is actually about. +// The lock is acquired before the plan is even shown, matching cmdSort's +// own window (acquired before Plan/review, held across both) rather than +// only around ApplyUndo - so this also asserts the refusal is noticed +// before any plan output reaches stdout. A version of this test that only +// checked the exit code and stderr would pass equally whether the lock +// were taken early or late, and so would not be pinning the thing this +// test is actually meant to catch. func TestUndoFailsImmediatelyWithHeldLock(t *testing.T) { h := matchingFixture(t) if code, _, errOut := runCLI(t, "-y"); code != 0 { @@ -131,12 +131,12 @@ func undoFiles(rels ...string) []engine.UndoFile { return out } -// TestFinalizeUndoPlanMarksUnapprovedAsDeclined is the wiring point for fix -// round 2026-09-12, item 2: a refused file rides through untouched (its own -// Refused reason is what ApplyUndo checks first), an approved file rides -// through untouched too, and anything else - explicitly declined, or never -// reached because [d]/[q] cut a per-file review short - comes out with -// Declined set rather than being dropped from the plan. +// TestFinalizeUndoPlanMarksUnapprovedAsDeclined: a refused file rides +// through untouched (its own Refused reason is what ApplyUndo checks +// first), an approved file rides through untouched too, and anything else +// - explicitly declined, or never reached because [d]/[q] cut a per-file +// review short - comes out with Declined set rather than being dropped +// from the plan. func TestFinalizeUndoPlanMarksUnapprovedAsDeclined(t *testing.T) { up := &engine.UndoPlan{Run: "r1", Files: []engine.UndoFile{ {File: "a"}, // index 0: approved @@ -239,19 +239,18 @@ func TestReviewUndoRefusedFileNotPrompted(t *testing.T) { } } -// TestPrintUndoPlan is fix wave item 3 (Important), rebuilding fix round -// 2026-09-12's own golden test: that version hand-built its UndoSteps, -// including a Dst on the undo-copy step the real code never sets (Dst is -// deliberately left "" - trash.Put only chooses the entry name at execution -// time), so it was structurally incapable of catching the bug it was meant -// to guard against - an undo-copy row rendering as a bare -// "undo-copy → " with nothing said about what it would do to the +// TestPrintUndoPlan rebuilds an earlier golden test that hand-built its +// UndoSteps, including a Dst on the undo-copy step the real code never +// sets (Dst is deliberately left "" - trash.Put only chooses the entry +// name at execution time), so it was structurally incapable of catching +// the bug it was meant to guard against - an undo-copy row rendering as a +// bare "undo-copy → " with nothing said about what it would do to the // user's backup copy, the single most destructive step an undo plan takes. -// The same lesson as Task 9's review: a hand-assembled fixture hides a test -// that cannot detect a broken copy-undo. This version runs a REAL forward -// apply (copy then move, so mkdir, copy and move all appear in one file's -// own chain) and a REAL PlanUndo, editing one file's result afterward so -// the plan also carries a genuinely refused row, then renders that. +// A hand-assembled fixture hides a test that cannot detect a broken +// copy-undo. This version runs a REAL forward apply (copy then move, so +// mkdir, copy and move all appear in one file's own chain) and a REAL +// PlanUndo, editing one file's result afterward so the plan also carries a +// genuinely refused row, then renders that. func TestPrintUndoPlan(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") @@ -315,9 +314,9 @@ func TestPrintUndoPlan(t *testing.T) { "undo-move → ~/dl/inv1.pdf\n", "undo-copy ~/backup/inv1.pdf → trash\n", "undo-mkdir ~/backup\n", - // Minor 4 / fix wave item 5: the refusal reason must be abbreviated - // against $HOME exactly like every step cell above it, not printed - // as a raw absolute path. + // The refusal reason must be abbreviated against $HOME exactly + // like every step cell above it, not printed as a raw absolute + // path. "refused: ~/dl/Work/notes.pdf changed since the run\n", } { if !strings.Contains(out, want) { @@ -412,8 +411,8 @@ func TestGlobalYesBeforeUndo(t *testing.T) { } // TestUndoWithoutRunContinuesTheLastUndo: when the most recent run is an -// undo that could not finish, plain `krino undo` offers what that undo left -// instead of refusing because the last run is an undo (review M10). +// undo that could not finish, plain `krino undo` offers what that undo +// left instead of refusing because the last run is an undo. func TestUndoWithoutRunContinuesTheLastUndo(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") @@ -471,9 +470,9 @@ func TestUndoWithoutRunContinuesTheLastUndo(t *testing.T) { } // TestReviewUndoMatchesReview: undo's per-file review behaves as review's -// does (review cli F2): n is recorded, each choice is echoed in red, and w -// leaves the files it never reached out of the plan, counted as not -// reviewed rather than logged as declined. +// does: n is recorded, each choice is echoed in red, and w leaves the +// files it never reached out of the plan, counted as not reviewed rather +// than logged as declined. func TestReviewUndoMatchesReview(t *testing.T) { out := new(strings.Builder) files := undoFiles("a", "b", "c") @@ -501,7 +500,7 @@ func TestReviewUndoMatchesReview(t *testing.T) { // TestMinAgeRejectedOutsideSortAndExplain: --min-age only changes sorting // and explain; any other command refuses it rather than silently ignoring -// a mistyped value, and an empty value is an error (review cli F4). +// a mistyped value, and an empty value is an error. func TestMinAgeRejectedOutsideSortAndExplain(t *testing.T) { matchingFixture(t) for _, args := range [][]string{ @@ -524,7 +523,7 @@ func TestMinAgeRejectedOutsideSortAndExplain(t *testing.T) { // TestIgnoredGlobalFlagsAreRefused: a global flag a command does not use is // refused instead of silently ignored, so "krino -n new ..." or "krino -n -// init" - meant as a preview - cannot write config (re-review cli F4). +// init" - meant as a preview - cannot write config. func TestIgnoredGlobalFlagsAreRefused(t *testing.T) { h := home(t) if code, _, errOut := runCLI(t, "-n", "init"); code != 2 || !strings.Contains(errOut, "-n") { diff --git a/cmd/krino/log.go b/cmd/krino/log.go index bfe61a2..e3e9dd1 100644 --- a/cmd/krino/log.go +++ b/cmd/krino/log.go @@ -74,8 +74,8 @@ func cmdLog(g *globals, args []string, stdout, stderr io.Writer) int { if err != nil { // journal.Runs (via Engine.Runs) fails closed on a genuine read // error - permissions, a corrupt file - and that must stay - // distinguishable from the ordinary "no log yet" case below rather - // than collapsing into the same message (dispatch notes). + // distinguishable from the ordinary "no log yet" case below + // rather than collapsing into the same message. if errors.Is(err, os.ErrNotExist) { fmt.Fprintln(stdout, "nothing logged yet") return 0 @@ -84,9 +84,9 @@ func cmdLog(g *globals, args []string, stdout, stderr io.Writer) int { return 1 } // A log file can exist and still hold no runs (a real run with nothing - // actionable still opens the journal - dispatch notes). That is just as - // ordinary as no log file at all: same message, same exit 0, and no - // table header is printed over zero rows. + // actionable still opens the journal). That is just as ordinary as no + // log file at all: same message, same exit 0, and no table header is + // printed over zero rows. if len(runs) == 0 { fmt.Fprintln(stdout, "nothing logged yet") return 0 diff --git a/cmd/krino/main.go b/cmd/krino/main.go index 973711e..5c504c8 100644 --- a/cmd/krino/main.go +++ b/cmd/krino/main.go @@ -101,13 +101,13 @@ func run(args []string, stdout, stderr io.Writer) int { return cmd(g, rest[1:], stdout, stderr) } } - // Ruling 2026-09-12/4: Go's flag package stops parsing at the first - // non-flag argument, so "krino dl -n" leaves "-n" in rest as a second - // directory name instead of a flag - the dry run is silently never - // honoured. A leftover argument that still looks like a flag is a - // usage error rather than a guess; there is deliberately no second - // pass that re-parses trailing flags, which would make "krino -- - // -weird-dir" ambiguous between the two syntaxes. + // Go's flag package stops parsing at the first non-flag argument, so + // "krino dl -n" leaves "-n" in rest as a second directory name + // instead of a flag - the dry run is silently never honoured. A + // leftover argument that still looks like a flag is a usage error + // rather than a guess; there is deliberately no second pass that + // re-parses trailing flags, which would make "krino -- -weird-dir" + // ambiguous between the two syntaxes. for _, a := range rest { if strings.HasPrefix(a, "-") { fmt.Fprintf(stderr, "krino: %s: flags must come before directory names\n", a) diff --git a/cmd/krino/main_test.go b/cmd/krino/main_test.go index 6e4df44..6d1d30e 100644 --- a/cmd/krino/main_test.go +++ b/cmd/krino/main_test.go @@ -41,8 +41,8 @@ func TestBadFlag(t *testing.T) { } } -// TestCommandsAreReserved is item E: every subcommand name must also be a -// reserved directory name, so a directory can never shadow a command. +// TestCommandsAreReserved: every subcommand name must also be a reserved +// directory name, so a directory can never shadow a command. func TestCommandsAreReserved(t *testing.T) { for name := range commands { if !config.Reserved[name] { @@ -51,10 +51,10 @@ func TestCommandsAreReserved(t *testing.T) { } } -// TestSortNoConfig: apply is implemented as of Task 7, so running with no -// config at all now fails the same way every other command does — "not -// found" from engine.Load, not the old "not implemented yet" hard stop this -// test used to pin (removed as part of Task 7; see cmd/krino/sort.go). +// TestSortNoConfig: running with no config at all fails the same way every +// other command does — "not found" from engine.Load, not the old "not +// implemented yet" hard stop this test used to pin before apply was +// implemented (see cmd/krino/sort.go). func TestSortNoConfig(t *testing.T) { home(t) code, _, errOut := runCLI(t) diff --git a/cmd/krino/matching_test.go b/cmd/krino/matching_test.go index 754e577..4ce2205 100644 --- a/cmd/krino/matching_test.go +++ b/cmd/krino/matching_test.go @@ -180,10 +180,10 @@ func TestCheckListsExtractors(t *testing.T) { } } -// TestDryRunWarningsSortedByRel is controller ruling 2026-09-12: the -// warnings section is one Rel-sorted list across matched and unmatched -// files, not matched files followed by unmatched files - a reader scans it -// by name and has no way to see which group a file fell into. "cover.pdf" +// TestDryRunWarningsSortedByRel: the warnings section is one Rel-sorted +// list across matched and unmatched files, not matched files followed by +// unmatched files - a reader scans it by name and has no way to see which +// group a file fell into. "cover.pdf" // matches "pdfs" but still carries the warning "acme" recorded before it // gave up; "brochure.doc" never matches at all. Their Rel order // ("brochure.doc" < "cover.pdf") is the reverse of matched-then-unmatched @@ -230,7 +230,7 @@ func TestDryRunWarningsSortedByRel(t *testing.T) { } } -// TestDirectoryWarningAfterHeader: C3. A directory-level warning must be +// TestDirectoryWarningAfterHeader: a directory-level warning must be // emitted after its own header line, not before it, so on a terminal (both // streams sharing one tty, hence stdout and stderr driven into the same // buffer here to observe their relative order) it reads as describing the @@ -272,8 +272,8 @@ func TestDirectoryWarningAfterHeader(t *testing.T) { } } -// TestSortSkipsMissingRootAndContinues: C5, the exit-1 skip path. A -// directory whose root has vanished since it was configured is skipped +// TestSortSkipsMissingRootAndContinues: the exit-1 skip path. A directory +// whose root has vanished since it was configured is skipped // with one line on stderr naming it, but every other directory is still // processed, with a blank line still separating their two outputs, and // the run as a whole exits 1. @@ -311,8 +311,8 @@ func TestSortSkipsMissingRootAndContinues(t *testing.T) { } } -// TestDirectoryWarningNotCountedInWarningsField: C5. A directory-level -// warning is printed as "krino: NAME: <warning>" on stderr, but is not one +// TestDirectoryWarningNotCountedInWarningsField: a directory-level warning +// is printed as "krino: NAME: <warning>" on stderr, but is not one // of the per-file warnings the "N warnings" field in the summary line // counts. func TestDirectoryWarningNotCountedInWarningsField(t *testing.T) { @@ -352,14 +352,14 @@ func TestDirectoryWarningNotCountedInWarningsField(t *testing.T) { } } -// TestLongNameNotPaddedLayoutIntact: C5, the 40-character cap. A file name +// TestLongNameNotPaddedLayoutIntact: the 40-character cap. A file name // longer than the 40-character column cap is left unpadded (not truncated, // not stretched further), while a short name alongside it is still padded // out to the full 40-column cap — the layout stays a clean two-column grid // even though one row's first cell overruns it. -// TestApplyWithYesMovesFiles is brief 7's basic apply-path test: -y applies -// the plan with no prompt, the file actually moves, the outcome line says -// so, and the run is logged with both boundaries. +// TestApplyWithYesMovesFiles is the basic apply-path test: -y applies the +// plan with no prompt, the file actually moves, the outcome line says so, +// and the run is logged with both boundaries. func TestApplyWithYesMovesFiles(t *testing.T) { h := matchingFixture(t) code, out, errOut := runCLI(t, "-y") @@ -382,9 +382,9 @@ func TestApplyWithYesMovesFiles(t *testing.T) { } } -// TestDryRunLogsNothing is Ruling 2: journal.Open must never be called at -// all in dry-run mode, since it materialises both the state directory and -// an empty log file as a side effect of merely opening it. +// TestDryRunLogsNothing: journal.Open must never be called at all in +// dry-run mode, since it materialises both the state directory and an +// empty log file as a side effect of merely opening it. func TestDryRunLogsNothing(t *testing.T) { h := matchingFixture(t) if code, _, errOut := runCLI(t, "-n"); code != 0 { @@ -425,11 +425,10 @@ func TestSecondRunFailsImmediatelyWithYes(t *testing.T) { } } -// TestFlagsMustPrecedeDirectoryNames is Ruling 4: Go's flag package stops -// parsing at the first non-flag argument, so "krino dl -n" would otherwise -// silently take "-n" as a second directory name and never honour the dry -// run. A leftover argument starting with "-" is a usage error instead of a -// guess. +// TestFlagsMustPrecedeDirectoryNames: Go's flag package stops parsing at +// the first non-flag argument, so "krino dl -n" would otherwise silently +// take "-n" as a second directory name and never honour the dry run. A +// leftover argument starting with "-" is a usage error instead of a guess. func TestFlagsMustPrecedeDirectoryNames(t *testing.T) { home(t) code, _, errOut := runCLI(t, "dl", "-n") @@ -438,8 +437,8 @@ func TestFlagsMustPrecedeDirectoryNames(t *testing.T) { } } -// TestNoColourEscapeToNonTerminal is Ruling 7's one pinned guarantee: a plan -// piped to a file or read by another tool must be plain text. tui.Colour(w) +// TestNoColourEscapeToNonTerminal pins one guarantee: a plan piped to a +// file or read by another tool must be plain text. tui.Colour(w) // already returns false for anything that is not a terminal *os.File, and // runCLI's stdout is a bytes.Buffer, so this holds end to end through the // real command path, not just at the helper that decides it. @@ -502,7 +501,7 @@ func TestLongNameGetsItsOwnLine(t *testing.T) { // TestDryRunJSONCarriesExclusionsAndWarnings: the JSON plan says which // exclude set a file aside and carries the warnings matching raised, for -// matched and unmatched files alike, as the text plan does (triage 28h). +// matched and unmatched files alike, as the text plan does. func TestDryRunJSONCarriesExclusionsAndWarnings(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") diff --git a/cmd/krino/render.go b/cmd/krino/render.go index 5b75df3..ca7dad7 100644 --- a/cmd/krino/render.go +++ b/cmd/krino/render.go @@ -35,17 +35,17 @@ const minWrap = 10 // never wraps, which keeps a plan piped to a file one field per line. func printPlan(w io.Writer, dp *engine.DirPlan, verbose bool, p palette, width int) { r := dp.Result - // C1 (plan 2): scanned counts matched, unmatched and skipped alike, not - // just matched plus unmatched - spec §8.2's worked example is "266 - // scanned" against "41 to act on" and "not acted on: 3 busy · 12 - // ignored · 210 unmatched", and 41+3+12+210 = 266. + // scanned counts matched, unmatched and skipped alike, not just matched + // plus unmatched - spec §8.2's worked example is "266 scanned" against + // "41 to act on" and "not acted on: 3 busy · 12 ignored · 210 + // unmatched", and 41+3+12+210 = 266. scanned := len(r.Matched) + len(r.Unmatched) + len(r.Skipped) - // B2: warning lines come from both the match itself (fm.Warnings) and - // the chains plan.Build produced (Chain.Warnings, e.g. "moved more than + // warning lines come from both the match itself (fm.Warnings) and the + // chains plan.Build produced (Chain.Warnings, e.g. "moved more than // once") - both computed once here so the count and the section below // agree on the exact same list. lines := collectWarnings(r, dp.Chains) - // D12: dp.Elapsed spans Match plus Build, unlike r.Elapsed, which stops + // dp.Elapsed spans Match plus Build, unlike r.Elapsed, which stops // before Build ever runs - the label says "planning", so the number // must cover all of it. counts := fmt.Sprintf("%d scanned · %d to act on · %d warnings · %.2fs", scanned, countActing(dp.Chains), warnedCount(lines), dp.Elapsed.Seconds()) @@ -131,14 +131,14 @@ func excludedLines(r *engine.Result, chains []plan.Chain) []string { // chainActing reports whether c has at least one step that will actually // run - the single definition of "actionable" that countActing, -// actionableChains (sort.go) and chainOutcomes (sort.go) all share (fix -// wave item 4 / Minor 5). Before this fix, countActing and actionableChains -// each kept their own copy of this question and disagreed: countActing -// excluded an all-skipped chain (len(Steps) > 0, but every step's Skip is -// set) while actionableChains's own len(Steps) > 0 check included it, so a -// directory could print "N scanned · 0 to act on" and then still ask the -// user to approve a file it had just said there were none of - and on -// approval, log a run-start/run-end pair holding only "skipped" entries. +// actionableChains (sort.go) and chainOutcomes (sort.go) all share. +// countActing and actionableChains used to each keep their own copy of +// this question and disagree: countActing excluded an all-skipped chain +// (len(Steps) > 0, but every step's Skip is set) while actionableChains's +// own len(Steps) > 0 check included it, so a directory could print "N +// scanned · 0 to act on" and then still ask the user to approve a file it +// had just said there were none of - and on approval, log a +// run-start/run-end pair holding only "skipped" entries. func chainActing(c plan.Chain) bool { for _, s := range c.Steps { if s.Skip == "" { @@ -149,9 +149,9 @@ func chainActing(c plan.Chain) bool { } // countActing reports how many chains have at least one step that will -// actually run. C1/ruling 2026-09-12: a rule with no actions is an -// exclusion, and a chain every one of whose steps is skipped is not about -// to do anything either - neither must inflate "to act on". +// actually run. A rule with no actions is an exclusion, and a chain every +// one of whose steps is skipped is not about to do anything either - +// neither must inflate "to act on". func countActing(chains []plan.Chain) int { n := 0 for _, c := range chains { @@ -186,7 +186,7 @@ func printBlocks(w io.Writer, chains []plan.Chain, root string, p palette, width fmt.Fprintln(w) head := " " + padLeft(strconv.Itoa(i), numW) + " " // A long name continues at the value column, never at the label - // column, so its text cannot pass for a step line (triage 28l). + // column, so its text cannot pass for a step line. for _, l := range wrapped(head, display(c.File.Rel), indent+labelWidth+1, width, plainText) { fmt.Fprintln(w, l) } @@ -328,8 +328,8 @@ func wrapText(s string, max int) []string { // character (CJK), none for a combining mark, one for the rest - format // characters included: a terminal may draw one (a soft hyphen), and // counting a column too many only wraps early, while one too few runs past -// the edge (triage 28j, plan 11 review L4). A terminal can still draw some -// characters wider (emoji, ambiguous-width letters); see KNOWN LIMITATIONS. +// the edge. A terminal can still draw some characters wider (emoji, +// ambiguous-width letters); see KNOWN LIMITATIONS. func cols(s string) int { n := 0 for _, r := range s { @@ -362,9 +362,9 @@ func destText(s plan.Step, root string) string { dir := filepath.Dir(s.Dst) if rel, ok := relToRoot(root, dir); ok { if rel == "" { - // D10: rel is "" exactly when dir is root itself (relToRoot's - // own case below); rendering that as bare rel+"/" would print - // "/", which reads as the filesystem root rather than "this + // rel is "" exactly when dir is root itself (relToRoot's own + // case below); rendering that as bare rel+"/" would print "/", + // which reads as the filesystem root rather than "this // directory". return "./" } @@ -376,7 +376,7 @@ func destText(s plan.Step, root string) string { // relToRoot returns dir relative to root (slash-separated) when dir is // root itself or lies inside it; ok is false when dir lies outside root, // including when the two cannot be related at all (e.g. one relative, one -// absolute). C3: root itself counts as "inside" here (rel is "", ok true) - +// absolute). root itself counts as "inside" here (rel is "", ok true) - // unlike internal/engine/match.go's excludeDirs, which asks a different // question (what may a rule exclude from the walk) and treats root as // outside it; do not "unify" the two. @@ -405,7 +405,7 @@ func padLeft(s string, w int) string { // colWidth returns the widest string in ss, in runes, capped at max when // max is positive; 0 leaves it uncapped. Shares relWidth's rune-counting -// rule (C4): a name carrying diacritics must not misalign its column. +// rule: a name carrying diacritics must not misalign its column. func colWidth(ss []string, max int) int { w := 0 for _, s := range ss { diff --git a/cmd/krino/render_test.go b/cmd/krino/render_test.go index 1b2f8cc..7c2072e 100644 --- a/cmd/krino/render_test.go +++ b/cmd/krino/render_test.go @@ -31,7 +31,7 @@ import ( // // "excluded.txt" matched a rule with no actions (an exclusion, spec §4.5): // it has zero steps, so it must not appear in the table and must not -// inflate "to act on" (ruling 2026-09-12). +// inflate "to act on". func TestPrintPlan(t *testing.T) { h := home(t) root := filepath.Join(h, "downloads") @@ -78,7 +78,7 @@ func TestPrintPlan(t *testing.T) { dp := &engine.DirPlan{ Dir: &engine.Dir{Name: "downloads", Root: root}, Chains: []plan.Chain{scan001, fv123, setup, excluded}, - Elapsed: 420 * time.Millisecond, // D12: the counts line renders DirPlan.Elapsed (Match plus Build), not Result.Elapsed alone + Elapsed: 420 * time.Millisecond, // the counts line renders DirPlan.Elapsed (Match plus Build), not Result.Elapsed alone Result: &engine.Result{ Matched: []engine.FileMatch{ {File: scan.File{Rel: "scan001.pdf"}, Warnings: []string{"acme: content unreadable: needs pdftotext, not installed"}}, @@ -380,7 +380,7 @@ func TestSkipSummaryLineAccountsForEveryFile(t *testing.T) { // TestWrappedNameCannotFakeAStepLine: a long name's continuation lines start // at the value column, not at the column step labels use, so a name holding -// "rename → x" cannot pass for a step of its own block (triage 28l). +// "rename → x" cannot pass for a step of its own block. func TestWrappedNameCannotFakeAStepLine(t *testing.T) { home(t) name := strings.Repeat("a", 30) + " rename → evil.pdf" @@ -401,7 +401,7 @@ func TestWrappedNameCannotFakeAStepLine(t *testing.T) { // TestColumnsCountWideAndCombiningCharacters: widths and wrapping count // terminal columns - two for a CJK character, none for a combining mark - // so a name in a wide script neither overruns the terminal nor misaligns -// its column (triage 28j). +// its column. func TestColumnsCountWideAndCombiningCharacters(t *testing.T) { if n := cols("漢字"); n != 4 { t.Errorf("cols(漢字) = %d, want 4", n) @@ -411,7 +411,7 @@ func TestColumnsCountWideAndCombiningCharacters(t *testing.T) { } // A format character is counted as one column: a terminal may draw it // (a soft hyphen), and counting one too many only wraps a line early, - // while one too few lets it run past the edge (plan 11 review L4). + // while one too few lets it run past the edge. if n := cols("a\u00adb"); n != 3 { t.Errorf("cols(a + soft hyphen + b) = %d, want 3", n) } diff --git a/cmd/krino/review.go b/cmd/krino/review.go index 8253405..671cbab 100644 --- a/cmd/krino/review.go +++ b/cmd/krino/review.go @@ -58,9 +58,9 @@ func (k keyReader) Read(p []byte) (int, error) { // decided and stops. approved holds every file decided, true for yes and // false for no; a file [w] left unreviewed is absent. replaced holds the // files [t] or [d] chose to trash or delete instead of what the rules -// planned; replaceChains applies it. Enter is ignored at both prompts. root is the directory being reviewed - passed only to -// reviewPerFile's destination rendering (review finding 1, fix round -// 2026-09-12); nothing here uses it directly. +// planned; replaceChains applies it. Enter is ignored at both prompts. root +// is the directory being reviewed - passed only to reviewPerFile's +// destination rendering; nothing here uses it directly. func reviewChains(in io.Reader, out io.Writer, chains []plan.Chain, root string, p palette) (map[string]bool, map[string]plan.Kind, rune, error) { fmt.Fprintln(out) for _, l := range wrapped("", "[a] apply all [c] choose per file [s] skip this directory [q] quit", 0, widthPolicy(out), p.keys) { @@ -108,10 +108,9 @@ func reviewChains(in io.Reader, out io.Writer, chains []plan.Chain, root string, // other key asks about the same file again); every choice is echoed in red // on its own line; [w] stops asking and applies whatever was already chosen, // leaving the rest unreviewed - reported back as end 'w'; [q] aborts the -// review entirely, discarding even files already marked yes - end 'q'. root is -// passed down so a destination inside root renders root-relative and one -// outside it renders ~-abbreviated, exactly as in the plan (review finding -// 1, fix round 2026-09-12). +// review entirely, discarding even files already marked yes - end 'q'. root +// is passed down so a destination inside root renders root-relative and one +// outside it renders ~-abbreviated, exactly as in the plan. func reviewPerFile(in io.Reader, out io.Writer, chains []plan.Chain, root string, p palette) (approved map[string]bool, replaced map[string]plan.Kind, end rune, err error) { approved = map[string]bool{} replaced = map[string]plan.Kind{} @@ -122,8 +121,8 @@ func reviewPerFile(in io.Reader, out io.Writer, chains []plan.Chain, root string continue } - // The heading wraps with its continuation past the label column, so - // a long name cannot pass for a step line (plan 11 review L4). + // The heading wraps with its continuation past the label column, + // so a long name cannot pass for a step line. fmt.Fprintln(out) for _, l := range wrapped("", fmt.Sprintf("[%d/%d] %s", i+1, len(chains), display(c.File.Rel)), 7+labelWidth+1, widthPolicy(out), plainText) { fmt.Fprintln(out, l) diff --git a/cmd/krino/review_test.go b/cmd/krino/review_test.go index fc175cb..0731ba9 100644 --- a/cmd/krino/review_test.go +++ b/cmd/krino/review_test.go @@ -111,11 +111,11 @@ func TestInvalidKeyReprompts(t *testing.T) { } } -// TestPerFileDestinationIsRootRelative is review finding 1 (fix round -// 2026-09-12): reviewPerFile must render a destination the same way the -// directory-level table does (render.go's destText) - root-relative for a -// destination inside root, ~-abbreviated for one outside it - not always -// abbreviated because root was never passed through to actionCell at all. +// TestPerFileDestinationIsRootRelative: reviewPerFile must render a +// destination the same way the directory-level table does (render.go's +// destText) - root-relative for a destination inside root, ~-abbreviated +// for one outside it - not always abbreviated because root was never +// passed through to actionCell at all. // Both halves are pinned: getting only the "inside" half right would still // let an outside-root destination silently regress to some other form. func TestPerFileDestinationIsRootRelative(t *testing.T) { @@ -330,7 +330,7 @@ func TestStopAfterApply(t *testing.T) { // TestReviewHeadingsCannotFakeAStepLine: a long name in the per-file // heading of review and of undo wraps with its continuation past the column -// step labels use, so the name cannot pass for a step (plan 11 review L4). +// step labels use, so the name cannot pass for a step. func TestReviewHeadingsCannotFakeAStepLine(t *testing.T) { old := widthPolicy t.Cleanup(func() { widthPolicy = old }) diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go index c2038fd..12812c7 100644 --- a/cmd/krino/sort.go +++ b/cmd/krino/sort.go @@ -26,13 +26,12 @@ import ( // stdin is os.Stdin, threaded through this seam rather than referenced // directly: cmdSort's terminal check, installSignalHandler and reviewDir // all read it, and a test must never depend on what the ambient test -// binary's stdin happens to be (fix round 2026-09-12/item 4). If it were -// ever a real terminal, code that only worked by assuming otherwise would -// fall through to the interactive prompt and block the test suite on a -// keypress - the same kind of hang the lock-cancellation test was built to -// never risk. Tests point this at something guaranteed non-terminal -// (commands_test.go's home helper) instead of relying on a claim about -// what go test does with stdin. +// binary's stdin happens to be. If it were ever a real terminal, code that +// only worked by assuming otherwise would fall through to the interactive +// prompt and block the test suite on a keypress - the same kind of hang +// the lock-cancellation test was built to never risk. Tests point this at +// something guaranteed non-terminal (commands_test.go's home helper) +// instead of relying on a claim about what go test does with stdin. var stdin = os.Stdin // stdinIsTerminal is the check that review can prompt at all, a seam so a @@ -46,11 +45,11 @@ var stdinIsTerminal = func() bool { return term.IsTerminal(int(stdin.Fd())) } // (and cannot drift) across the three places it applies. const zeroOutcome = "0 applied · 0 failed · 0 declined" -// cmdSort plans and, from Task 7, applies the included directories: flags -// are checked before any config is read, a bad config stops the whole run -// before scanning (spec §11), and one engine.Session - its log, run id and -// claims - covers every directory in the run. See docs/design.md -// §8.2-§8.4 and §11 for the flow this follows. +// cmdSort plans and applies the included directories: flags are checked +// before any config is read, a bad config stops the whole run before +// scanning (spec §11), and one engine.Session - its log, run id and claims +// - covers every directory in the run. See docs/design.md §8.2-§8.4 and +// §11 for the flow this follows. func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { if g.yes && g.dry { return usageError(stderr, "-y and -n cannot be used together") @@ -80,9 +79,9 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { return usageError(stderr, "refusing to prompt: stdin is not a terminal (use -y or -n)") } - // Ruling 5: internal/tui deliberately does not trap signals - a package - // that installs process-wide handlers as a side effect of reading one - // key would surprise every caller. It lands here because cmdSort must + // internal/tui deliberately does not trap signals - a package that + // installs process-wide handlers as a side effect of reading one key + // would surprise every caller. It lands here because cmdSort must // install one anyway: spec §11 says Ctrl-C finishes the current step, // logs it, and stops, which means the ctx passed to Apply below must be // cancelled on SIGINT. The same handler also restores the terminal on @@ -96,8 +95,8 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { // The run itself - the log, its run id and the claims every directory // shares - belongs to the engine, so the GUI runs a directory exactly - // as this does (GUI design §1.3). Ruling 2: a dry session opens no log, - // since journal.Open creates the state directory and an empty krino.log + // as this does (GUI design §1.3). A dry session opens no log, since + // journal.Open creates the state directory and an empty krino.log // merely by being called. sess, err := e.NewSession(g.dry) if err != nil { @@ -125,9 +124,8 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { // Spec §3/§11: a second krino on the same directory waits for the // lock, or fails immediately with -y, so a cron job never piles up // behind a stuck run. lock.Acquire takes ctx precisely so that wait - // is not unbounded in practice (fix round 2026-09-12/item 1): a - // signal cancels it and Acquire returns ctx.Err() promptly instead - // of polling forever. + // is not unbounded in practice: a signal cancels it and Acquire + // returns ctx.Err() promptly instead of polling forever. l, err := sess.Lock(ctx, d, !g.yes) if err != nil { if interrupted(err) { @@ -176,15 +174,15 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { } if g.json { // --json is only ever reached with -n (checked above), and - // Ruling 7 is explicit that JSON must never be paged, so - // this returns before any of the paging/review code below. + // JSON must never be paged, so this returns before any of + // the paging/review code below. jsonDirs = append(jsonDirs, plan.NewJSONDir(d.Name, d.Root, dp.Chains, fileNotes(dp.Result), dp.Result.Warnings)) return false } - // Ruling 6: the plan goes through tui.Page - taller than the - // terminal, it is shown through $PAGER and the prompt follows - // once the pager exits (spec §8.2) - for -n as much as for the + // The plan goes through tui.Page - taller than the terminal, it + // is shown through $PAGER and the prompt follows once the + // pager exits (spec §8.2) - for -n as much as for the // interactive and -y paths; a dry run that scrolls 200 files // off the top of the terminal is exactly the case the pager // exists for. printPlan is reused as-is (render.go), never @@ -228,8 +226,8 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { case 's': // Spec §8.2: [s] applies nothing in this directory and // moves on - no Apply call at all, so nothing is logged - // for it either (Ruling 1: this is a chosen outcome, not a - // failure, and must not set exit 1). + // for it either: this is a chosen outcome, not a failure, + // and must not set exit 1. fmt.Fprintln(stdout, zeroOutcome) return false case 'q': @@ -254,11 +252,11 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { } res, aerr := sess.Apply(ctx, toApply, approved) if aerr != nil { - // Interrupted mid-apply (fix round 2026-09-12/item 2): - // treated exactly like the cancelled lock wait above - not - // a failure ("context canceled" is a Go-ism, not something - // to show a user who just pressed Ctrl-C). The ctx.Err() - // check at the end of this function turns it into exit 130. + // Interrupted mid-apply: treated exactly like the + // cancelled lock wait above - not a failure ("context + // canceled" is a Go-ism, not something to show a user who + // just pressed Ctrl-C). The ctx.Err() check at the end of + // this function turns it into exit 130. if !interrupted(aerr) { fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, aerr) exit = 1 @@ -266,8 +264,8 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { return stopAfterApply(action, aerr) } fmt.Fprintln(stdout, withNotReviewed(outcome(p, res.Applied, res.Failed, res.Declined), notReviewed)) - // Ruling 1: only an actual step failure makes the run exit 1 - // here - a directory the user declined or skipped must not. + // Only an actual step failure makes the run exit 1 here - a + // directory the user declined or skipped must not. if res.Failed > 0 { exit = 1 } @@ -305,12 +303,12 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { } // actionableChains returns the chains of dp.Chains that have at least one -// step that will actually run (chainActing, render.go) - fix wave item 4 / -// Minor 5: this used to be a separate len(c.Steps) > 0 check, which -// disagreed with render.go's countActing over a chain every one of whose -// steps is skipped, so a directory could report "0 to act on" and then -// still offer such a chain for approval. Converged on chainActing, this is -// now also stricter than the filter engine.Apply's own forward-path loop +// step that will actually run (chainActing, render.go). This used to be a +// separate len(c.Steps) > 0 check, which disagreed with render.go's +// countActing over a chain every one of whose steps is skipped, so a +// directory could report "0 to act on" and then still offer such a chain +// for approval. Converged on chainActing, this is now also stricter than +// the filter engine.Apply's own forward-path loop // applies (internal/engine/apply.go's Apply, still len(c.Steps) > 0): an // all-skipped chain is simply never a candidate for approval here, so it // can never reach Apply with approved == true, and Apply's own loop - @@ -328,12 +326,12 @@ func actionableChains(chains []plan.Chain) []plan.Chain { // installSignalHandler arranges for SIGINT and SIGTERM to cancel cancel // and, if stdin is a terminal, restore it to the state it was in when this -// was called (Ruling 5). The returned func stops the handler and must be -// called once the run is over, or its goroutine and signal registration -// outlive cmdSort. +// was called. The returned func stops the handler and must be called once +// the run is over, or its goroutine and signal registration outlive +// cmdSort. // -// Fix round 2026-09-12/item 2: the handler loops rather than servicing one -// signal and exiting. A single-shot select left signal.Notify's +// The handler loops rather than servicing one signal and exiting. A +// single-shot select left signal.Notify's // registration in place (which suppresses Go's default terminate) with no // goroutine left reading the channel, so a second Ctrl-C landed in the // buffered channel unread and a third was dropped outright - together with @@ -356,8 +354,8 @@ func installSignalHandler(cancel context.CancelFunc) func() { sig := make(chan os.Signal, 1) signal.Notify(sig, os.Interrupt, syscall.SIGTERM) // A hangup stops krino like SIGTERM - unless it was started ignoring - // hangups (nohup), which must go on meaning "keep running" (re-review - // pa F3): asking for SIGHUP would re-enable it. + // hangups (nohup), which must go on meaning "keep running": asking for + // SIGHUP would re-enable it. if !signal.Ignored(syscall.SIGHUP) { signal.Notify(sig, syscall.SIGHUP) } @@ -374,20 +372,19 @@ func installSignalHandler(cancel context.CancelFunc) func() { signals++ if signals >= 2 { // Deliberate exception to "the lock is released on - // every path" (fix round 2026-09-12/item 3, by - // design, documented on review): every deferred - // cleanup in cmdSort - including the held directory's - // lock.Release - is skipped here. That is intentional: - // this is the user's escape hatch when a clean - // shutdown was already asked for once (the first - // signal) and not delivered, so trying to unwind - // cleanly a second time is exactly what would make the - // hatch unreliable. It is safe to skip that unwind: - // journal.Append flushes each line as it writes, so - // nothing buffered is lost by exiting immediately, and - // an abandoned lock file is reclaimed automatically by - // Task 4's stale-pid takeover the next time anything - // tries to acquire it (lock.go's tryAcquire). + // every path": every deferred cleanup in cmdSort - + // including the held directory's lock.Release - is + // skipped here. That is intentional: this is the + // user's escape hatch when a clean shutdown was + // already asked for once (the first signal) and not + // delivered, so trying to unwind cleanly a second time + // is exactly what would make the hatch unreliable. It + // is safe to skip that unwind: journal.Append flushes + // each line as it writes, so nothing buffered is lost + // by exiting immediately, and an abandoned lock file is + // reclaimed automatically by the stale-pid takeover + // the next time anything tries to acquire it (lock.go's + // tryAcquire). os.Exit(130) } case <-done: @@ -497,17 +494,17 @@ func printSkipped(w io.Writer, skipped []scan.Skipped) { } } -// skipReasonOrder is plan 2's reviewed order for the skip reasons the last -// line reports, before "unmatched". +// skipReasonOrder is the order the skip reasons the last line reports, +// before "unmatched". var skipReasonOrder = []scan.Reason{scan.Ignored, scan.Busy, scan.TooNew, scan.TooBig, scan.Symlink, scan.NotRegular, scan.Unreadable} // skipSummaryLine builds the "not acted on: N ignored · N busy · ... · N // unmatched" line per spec §8.2's item format ("<count> <label>", not // "<label>: <count>"), only the non-zero counts, or "" when every count is -// zero. Ordering is plan 2's reviewed skipReasonOrder, with "unmatched" -// last: the spec's own worked example shows only three of the seven -// categories and states no ordering rule, so its incidental order is not -// adopted, only its item format and unmatched's trailing position. +// zero. Ordering follows skipReasonOrder, with "unmatched" last: the +// spec's own worked example shows only three of the seven categories and +// states no ordering rule, so its incidental order is not adopted, only +// its item format and unmatched's trailing position. func skipSummaryLine(r *engine.Result, chains []plan.Chain, verbose bool) string { counts := map[scan.Reason]int{} for _, s := range r.Skipped { @@ -600,8 +597,8 @@ func interrupted(err error) bool { } // stopAfterApply reports whether krino stops once a directory has been -// applied: an interrupt stops it, and so does [w], whether or not its apply -// succeeded (review cli F3) - no later directory is planned or asked about. +// applied: an interrupt stops it, and so does [w], whether or not its +// apply succeeded - no later directory is planned or asked about. func stopAfterApply(action rune, err error) bool { return interrupted(err) || action == 'w' } diff --git a/cmd/krino/sort_test.go b/cmd/krino/sort_test.go index 4f3e29f..4e68cb5 100644 --- a/cmd/krino/sort_test.go +++ b/cmd/krino/sort_test.go @@ -31,9 +31,9 @@ func TestRelWidthAndPadCellCountRunes(t *testing.T) { } } -// TestActionableChainsAgreesWithCountActing is fix wave item 4 / Minor 5: -// countActing (render.go) and actionableChains used to disagree over a -// chain every one of whose steps is skipped (len(Steps) > 0, but every +// TestActionableChainsAgreesWithCountActing: countActing (render.go) and +// actionableChains used to disagree over a chain every one of whose steps +// is skipped (len(Steps) > 0, but every // step's own Skip is set) - countActing already excluded it from "to act // on", while actionableChains's own len(Steps) > 0 check still offered it // for approval, so a directory could print "N scanned · 0 to act on" and @@ -53,17 +53,17 @@ func TestActionableChainsAgreesWithCountActing(t *testing.T) { } } -// TestAllSkippedDirectoryReportsZeroAndLogsNothing is fix wave item 4 / -// Minor 5 and 6, end to end. Before the fix: a directory whose one file -// matches a rule under (on-conflict skip) - so its single step's own Skip -// is set ("target exists") - printed "0 to act on" (countActing) and then, -// with -y, still ran that chain through Apply anyway (actionableChains' -// own len(Steps) > 0 check approved it regardless), logging a -// run-start/run-end pair holding only a "skipped" entry while the outcome -// line read "0 applied · 0 failed · 0 declined" for a file that had just -// been silently processed. After the fix, the chain is never offered for -// approval, Apply is never even called for this directory, and the journal -// gains nothing at all. +// TestAllSkippedDirectoryReportsZeroAndLogsNothing, end to end: a +// directory whose one file matches a rule under (on-conflict skip) - so +// its single step's own Skip is set ("target exists") - must report "0 to +// act on" (countActing) and, with -y, must not run that chain through +// Apply anyway: an all-skipped chain offered for approval regardless +// (as actionableChains' own len(Steps) > 0 check alone would allow) would +// log a run-start/run-end pair holding only a "skipped" entry while the +// outcome line read "0 applied · 0 failed · 0 declined" for a file that +// had just been silently processed. Instead the chain is never offered +// for approval, Apply is never even called for this directory, and the +// journal gains nothing at all. func TestAllSkippedDirectoryReportsZeroAndLogsNothing(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") @@ -225,7 +225,7 @@ func TestDryRunShowsNeverDeletedSkip(t *testing.T) { if !strings.Contains(out, "skipped: a duplicate is never deleted") { t.Errorf("plan lacks the skipped delete:\n%s", out) } - // explain shows it too, for the copy that is the duplicate (triage 30a). + // explain shows it too, for the copy that is the duplicate. _, outA, _ := runCLI(t, "explain", filepath.Join(dl, "a.pdf")) _, outB, _ := runCLI(t, "explain", filepath.Join(dl, "b.pdf")) if strings.Count(outA+outB, "a duplicate is never deleted") != 1 { @@ -235,8 +235,8 @@ func TestDryRunShowsNeverDeletedSkip(t *testing.T) { // TestLaterDirectoryIsNotBlockedByAnEarlierOnesClaims: in a real run each // directory is applied before the next is planned, so the disk is the -// truth; a path an earlier directory moved a file away from, or planned and -// did not apply, is not "taken" for a later one (triage 28i). +// truth; a path an earlier directory moved a file away from, or planned +// and did not apply, is not "taken" for a later one. func TestLaterDirectoryIsNotBlockedByAnEarlierOnesClaims(t *testing.T) { h := home(t) old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -300,8 +300,8 @@ func devFullFixture(t *testing.T) { } // TestApplyErrorExitsOne: an error applying a directory - here the log -// cannot be written - makes krino exit 1 (triage 34m: removing that exit -// code left every test passing). +// cannot be written - makes krino exit 1; removing that exit code left +// every other test passing. func TestApplyErrorExitsOne(t *testing.T) { devFullFixture(t) code, _, errOut := runCLI(t, "-y", "d1") @@ -311,9 +311,9 @@ func TestApplyErrorExitsOne(t *testing.T) { } // TestWriteStopsKrinoWhenApplyFails: [w] in review stops krino after its -// directory even when applying it fails - the next directory is not planned -// or asked about (review cli F3), driven through the real command with a -// pipe standing in for the terminal (triage 28m). +// directory even when applying it fails - the next directory is not +// planned or asked about, driven through the real command with a pipe +// standing in for the terminal. func TestWriteStopsKrinoWhenApplyFails(t *testing.T) { devFullFixture(t) r, w, err := os.Pipe() @@ -337,7 +337,7 @@ func TestWriteStopsKrinoWhenApplyFails(t *testing.T) { // TestLaterDirectoryNeverOverwritesAnEarlierOnesResult: what an earlier // directory of the run put somewhere stays claimed, so a later directory's // (on-conflict overwrite) takes a free name instead of trashing it - as a -// dry run of the same two directories shows (plan 11 review M1). +// dry run of the same two directories shows. func TestLaterDirectoryNeverOverwritesAnEarlierOnesResult(t *testing.T) { h := home(t) old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) @@ -369,7 +369,7 @@ func TestLaterDirectoryNeverOverwritesAnEarlierOnesResult(t *testing.T) { // TestOnlyWhereFilesEndedUpStaysClaimed: a path an earlier directory's file // passed through and left - renamed, then moved on - is free for a later -// directory; only where files ended up stays claimed (plan 11 re-check). +// directory; only where files ended up stays claimed. func TestOnlyWhereFilesEndedUpStaysClaimed(t *testing.T) { h := home(t) old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) diff --git a/cmd/krino/undo.go b/cmd/krino/undo.go index 5a7db4b..85b74b5 100644 --- a/cmd/krino/undo.go +++ b/cmd/krino/undo.go @@ -26,9 +26,9 @@ func init() { commands["undo"] = cmdUndo } // the most recent one otherwise. Since the newest run in the log can never // itself be marked Undone - that would require a still-later run to have // reversed it - "the most recent run" and "the most recent run that has not -// been undone" are the same run in every case, including the one this -// task's own test exercises: undoing an undo run a second time with no RUN -// argument targets that very undo run, which PlanUndo then refuses by name. +// been undone" are the same run in every case, including the case tested +// below: undoing an undo run a second time with no RUN argument targets +// that very undo run, which PlanUndo then refuses by name. // // Undo builds a plan like any other, shown and approved the same way (spec // §10) - reviewUndoDir/-Files/-PerFile below are undo's own counterpart to @@ -74,10 +74,9 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int { } // Installed here, before any paging or review, not just around - // ApplyUndo: Ruling 5 (Task 7) is that SIGTERM landing between - // keystrokes or during the pager needs the terminal restored, and that - // window starts as soon as this command might show something on a - // terminal. + // ApplyUndo: SIGTERM landing between keystrokes or during the pager + // needs the terminal restored, and that window starts as soon as this + // command might show something on a terminal. ctx, cancel := context.WithCancel(context.Background()) defer cancel() stopSignals := installSignalHandler(cancel) @@ -101,9 +100,9 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int { return 0 } runID = runs[0].ID - // The most recent run is itself an undo: continue it, by planning the - // run it undid again - reversals it completed are not offered twice - // (review M10). Naming an undo run explicitly is still refused. + // The most recent run is itself an undo: continue it, by planning + // the run it undid again - reversals it completed are not offered + // twice. Naming an undo run explicitly is still refused. if runs[0].UndoOf != "" { runID = runs[0].UndoOf } @@ -128,9 +127,8 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int { return 1 } - // Fix round 2026-09-12 (widened per the coordinator's follow-up - // ruling): an undo moves files just as an apply does, so it needs - // cmdSort's same per-directory guard (spec §11: a second krino on the + // An undo moves files just as an apply does, so it needs cmdSort's + // same per-directory guard (spec §11: a second krino on the // same directory waits for the lock, or fails immediately with -y), // held across the SAME window cmdSort holds its own lock across - the // plan display and the review, not just the apply. Failing before the @@ -165,8 +163,8 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int { var buf bytes.Buffer printUndoPlan(&buf, up) text := colourRefused(buf.String(), p) - // Ruling 6 (Task 7), carried over: the plan goes through tui.Page for - // -n as much as for -y and the interactive path. + // The plan goes through tui.Page for -n as much as for -y and the + // interactive path. if err := show(g, stdout, text); err != nil { fmt.Fprintf(stderr, "krino: %v\n", err) return 1 @@ -204,9 +202,9 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int { toApply, notReviewed := finalizeUndoPlan(up, approved, action) - // Ruling 2 (Task 7), carried over: journal.Open creates the state - // directory and the log file as a side effect of merely being called, - // so the session opens it only now, when something will actually be + // journal.Open creates the state directory and the log file as a side + // effect of merely being called, so the session opens it only now, + // when something will actually be // applied - never for -n (returned above), and not merely because -y // or a review session ran, unlike cmdSort, which opens before it knows // whether anything is actionable (a difference forced by cmdSort not @@ -281,18 +279,17 @@ func releaseUndoLocks(locks []*lock.Lock) []error { // finalizeUndoPlan builds the *engine.UndoPlan ApplyUndo actually runs, // preserving up.Files' own order: every refused file rides along unchanged // (ApplyUndo declines these itself, silently, exactly as it already does -// when handed the unfiltered plan - spec §10's refusal is not this task's -// to make noisier); every actionable file approved marks true rides along +// when handed the unfiltered plan - spec §10's refusal is not made any +// noisier here); every actionable file approved marks true rides along // unchanged too. A file the user said no to, or left unmarked when [d] or -// [q] cut a per-file review short, is not dropped - fix round 2026-09-12, -// item 2 of Task 8's review: spec §9 says a declined file is logged even -// though nothing happens to it, the same as the forward path already does, -// so it is kept with Declined set, which tells ApplyUndo to log its steps -// as declined rather than reverse them. +// [q] cut a per-file review short, is not dropped: spec §9 says a +// declined file is logged even though nothing happens to it, the same as +// the forward path already does, so it is kept with Declined set, which +// tells ApplyUndo to log its steps as declined rather than reverse them. // // After [w] (action 'w'), a reversible file the review never reached is // left out of the plan entirely, as review's [w] leaves a forward file -// unlogged, and counted in notReviewed (review cli F2). +// unlogged, and counted in notReviewed. func finalizeUndoPlan(up *engine.UndoPlan, approved map[int]bool, action rune) (plan *engine.UndoPlan, notReviewed int) { out := &engine.UndoPlan{Run: up.Run, Cleanup: up.Cleanup} for i, f := range up.Files { @@ -374,15 +371,15 @@ func reviewUndoFiles(in io.Reader, out io.Writer, files []engine.UndoFile, p pal // its reason and reverses nothing of it regardless of anything chosen here // - but it still gets its own [i/N] line, so the numbering accounts for // every file in the plan, not just the reversible ones. It behaves as -// review.go's reviewPerFile does (review cli F2): a no is recorded as false, -// each choice is echoed in red, [w] ends with 'w' leaving unreached files -// out of approved, and [q] ends with 'q'. +// review.go's reviewPerFile does: a no is recorded as false, each choice is +// echoed in red, [w] ends with 'w' leaving unreached files out of +// approved, and [q] ends with 'q'. func reviewUndoPerFile(in io.Reader, out io.Writer, files []engine.UndoFile, p palette) (approved map[int]bool, end rune, err error) { approved = map[int]bool{} yesRest := false for i, f := range files { // Heading and steps wrap past the action column, so a long name or - // path cannot pass for a step line (plan 11 review L4). + // path cannot pass for a step line. width := widthPolicy(out) fmt.Fprintln(out) for _, l := range wrapped("", fmt.Sprintf("[%d/%d] %s/%s", i+1, len(files), display(f.Dir), display(f.File)), 7+undoStepWidth+2, width, plainText) { @@ -468,11 +465,11 @@ const undoStepWidth = len("undo-displace") // one (a sibling directory not yet empty for undo-mkdir - spec §10's one // case where a step's own failure does not refuse its whole file), the // directory removed for undo-mkdir (no destination to show), the file being -// trashed for undo-copy (fix wave item 3: its Dst is deliberately empty - -// trash.Put only chooses the entry name at execution time - so this is the -// one action with no path to point an arrow at; before this fix the cell -// rendered as a bare "undo-copy → ", the plan's one row that said -// nothing about what it would do to the user's file), or an arrow to where +// trashed for undo-copy (its Dst is deliberately empty - trash.Put only +// chooses the entry name at execution time - so this is the one action +// with no path to point an arrow at; rendering it as a bare "undo-copy +// → " would be the plan's one row that says nothing about what it would do +// to the user's file), or an arrow to where // the step puts the file back, ~-abbreviated - undo has no single root the // way a sort plan does (one run can span several directories), so there is // no root-relative form to render here the way actionCell has. diff --git a/docs/design.md b/docs/design.md index a7584e6..a3356c7 100644 --- a/docs/design.md +++ b/docs/design.md @@ -1,20 +1,9 @@ # krino design -Status: describes krino 0.0.1, 2026-09-13; amended 2026-09-14 for 0.0.2: -duplicates are never deleted (§5.5), and coloured output with `--no-color` -(§8.2, §11); amended again 2026-09-14 for 0.0.3: the plan -shown as one block per file, wrapped to the terminal, and `-P` (§8.2, §8.3, §11); -amended again 2026-09-14 for 0.0.4: `max-size` (§4.4), `(exclude ...)` -(§4.2, §4.3, §4.6) and `--min-age` (§11); amended again 2026-09-14 for -0.0.5: the keyword cache (§3, §6.1, §13), and `t`, `d` and `w` in review -(§8.3, §10); amended again 2026-09-14 for 0.0.6: `w` applies and quits, and -each choice is echoed in red (§8.2, §8.3); amended again 2026-09-14 for -0.0.7: the threat model and its tests (§15.1); amended 2026-09-15 for -0.0.8: three-valued content tests (§4.6, §5.4), captures from the original -name (§7.3), claims across directories (§7.4), `-v` destinations not scanned -(§8.1), `(partly undone)` (§10), build checks (§14) and real document -fixtures (§15); amended 2026-09-15 for 0.0.9: failed duplicate lookups and -undecided `(stop)` rules (§5.4), placeholders refused at load (§7.3). +The design of krino's engine and command line, up to date for 0.0.9. The +window has a document of its own, `gui-design.md`; what changed in each +release is in `CHANGELOG.md`. Source comments cite the sections here by +number, so the numbering is stable. krino (from Greek κρίνω, "to separate, to judge, to decide") sorts files in chosen directories by rules. A rule tests a file's type, name, path, size, @@ -515,8 +504,8 @@ name. that exists, under "not scanned". Known limitation: a `DEST` whose *first* path component is itself a placeholder (`{ext}`, `{mtime:%Y}`) has no static prefix, so nothing can be - excluded before the walk, and krino would re-examine its own output; plan 3 - closes this by treating a file already at its computed destination as a + excluded before the walk, and krino would re-examine its own output. What + closes it is treating a file already at its computed destination as a no-op. - Skipped: files newer than `min-age` ("too new"), larger than `max-size` ("too big"), or with a `busy` sibling ("busy"). @@ -990,7 +979,7 @@ keyword. Its defects shaped these decisions: | `pdftotext` rather than a Go PDF library | best text quality of the open tools, fast, packaged on all three systems | | Delete to Trash by default | recoverable, and undo can restore it | | Duplicates are found, never deleted | duplicate conditions with different scopes elect different originals, so deleting duplicates could remove every copy; moving them aside is always recoverable, and choosing which copy to delete belongs to the user or a tool built for it (jdupes). Decided 2026-09-14 after 0.0.1 shipped with the hazard documented | -| A new repository rather than the prototype's | the prototype's history holds personal data; the prototype keeps working until krino reaches parity | +| A new repository rather than the prototype's | a history that can be published as it stands; the prototype keeps working until krino reaches parity | | Personal configuration never enters the repository | rules hold private data; the leak check enforces it on every commit and in `make ci` | ## 18. Settled before implementation diff --git a/docs/gui-checklist.md b/docs/gui-checklist.md index d07ca17..d81e57c 100644 --- a/docs/gui-checklist.md +++ b/docs/gui-checklist.md @@ -3,7 +3,7 @@ The GTK layer has no automated tests (gui-design.md §6): package `gui/internal/model` is tested, `gui/internal/ui` is not. This list is run by hand before every release that changes the GUI, and what it found goes -in the release's plan record. +in that release's entry in `CHANGELOG.md`. Run it against a sandbox, never a real directory: diff --git a/docs/gui-design.md b/docs/gui-design.md index a7c5089..602368f 100644 --- a/docs/gui-design.md +++ b/docs/gui-design.md @@ -1,10 +1,12 @@ # krino-gui design -Status: draft, 2026-09-15. The window layout (B), the rules editor layout -(A) and sections 1–4 were agreed from mockups; sections 5–6 are the -author's defaults, to be corrected before implementation. Companion to -`docs/design.md` (the engine spec, "the spec" below), whose §12 anticipated -this GUI. +The shape of the window, written before it was built and still the +description of what it is for. Where the window has since gone further — +the plan's columns, the filter and the sort, previews, settings — +`CHANGELOG.md` is the record. Companion to `docs/design.md` (the engine +spec, "the spec" below), whose §12 anticipated this GUI. Source comments +and `gui-checklist.md` cite the sections here by number, so the numbering +is stable. `krino-gui` is a GTK4 window for what `krino` does in a terminal: review a directory's plan and apply the chosen files, look back over runs and undo diff --git a/gui/internal/model/filter.go b/gui/internal/model/filter.go index af617c3..b9844cf 100644 --- a/gui/internal/model/filter.go +++ b/gui/internal/model/filter.go @@ -11,8 +11,7 @@ import ( // FuzzyMatch reports whether every character of pattern appears in text in // order - the way fzf matches - and how good the match is. Capitals and // accents are ignored, using krino's own folding, so "zazolc" finds -// "zażółć" exactly as a rule with (fold yes) would (his request, -// 2026-09-17). +// "zażółć" exactly as a rule with (fold yes) would. // // The score rewards characters that follow one another and those at the // start of a word, so "lec" ranks "lectio-2026.pdf" above diff --git a/gui/internal/model/highlight.go b/gui/internal/model/highlight.go index e34d9d9..55742ef 100644 --- a/gui/internal/model/highlight.go +++ b/gui/internal/model/highlight.go @@ -29,7 +29,7 @@ var actionHeads = map[string]bool{ // string comments out the rest of its line; a string runs to its closing // quote, a backslash escaping the next character; the first symbol after // "(" is the form's head. Nothing here knows about widgets, so the rules of -// the little language stay testable (his request, 2026-09-16). +// the little language stay testable. func Spans(text string) []Span { var out []Span runes := []rune(text) diff --git a/gui/internal/model/newdir.go b/gui/internal/model/newdir.go index d0df6db..f65763c 100644 --- a/gui/internal/model/newdir.go +++ b/gui/internal/model/newdir.go @@ -10,8 +10,7 @@ import ( // AddDirectory writes dirs/NAME.conf from the template with path filled in // and adds NAME to krino.conf's include - exactly what `krino new NAME // PATH` does, through the same code, so a directory made in the window is -// indistinguishable from one made on the command line (his report that the -// window had no way to add one, 2026-09-17). +// indistinguishable from one made on the command line. // // It returns the new file's path. The caller reloads the engine: until it // does, the window knows nothing of the new directory. diff --git a/gui/internal/model/plan.go b/gui/internal/model/plan.go index 7c24991..12b0d13 100644 --- a/gui/internal/model/plan.go +++ b/gui/internal/model/plan.go @@ -240,8 +240,7 @@ func (t *PlanTab) Replace(i int, kind plan.Kind) error { // ReplaceSelected swaps the steps of every checked file for the one action // chosen - "Trash the checked files", "Delete them permanently" - and // reports how many were changed. Nothing happens on disk: like every other -// review decision, it changes the plan, and Apply carries it out (his -// request, 2026-09-17). +// review decision, it changes the plan, and Apply carries it out. func (t *PlanTab) ReplaceSelected(kind plan.Kind) (int, error) { n := 0 for i, r := range t.Rows { @@ -261,7 +260,7 @@ func (t *PlanTab) ReplaceSelected(kind plan.Kind) (int, error) { // the Trash, where krino undo can still reach it. It is a review decision, // like trashing a file by hand, so the rule that a duplicate is never // deleted - which binds rules, not the person reading the plan - does not -// stand in its way (his request, 2026-09-17). +// stand in its way. func (t *PlanTab) KeepThisCopy(i int) error { if i < 0 || i >= len(t.Rows) { return fmt.Errorf("model: no row %d", i) @@ -339,7 +338,7 @@ func (t *PlanTab) record(res *engine.ApplyResult) { // AgeText is how long ago a file was last written, in the units krino's own // (age ...) test uses: minutes, hours, days and weeks, and years past that, -// so a plan can be read at a glance (his request, 2026-09-17). +// so a plan can be read at a glance. func AgeText(mod time.Time, now time.Time) string { if mod.IsZero() { return "" diff --git a/gui/internal/model/plan_test.go b/gui/internal/model/plan_test.go index 0aa0361..8873e55 100644 --- a/gui/internal/model/plan_test.go +++ b/gui/internal/model/plan_test.go @@ -315,8 +315,7 @@ func equal(a, b []string) bool { } // TestReplaceSelected: one choice for every checked file at once - trash -// them, or delete them - changing the plan and nothing on disk until Apply -// (his request, 2026-09-17). +// them, or delete them - changing the plan and nothing on disk until Apply. func TestReplaceSelected(t *testing.T) { conf := "(path \"~/dl\")\n(rule \"all\" (move \"Out\"))\n" e, h := sandboxDir(t, conf, map[string]string{"a.pdf": "one", "b.pdf": "two", "c.pdf": "three"}) @@ -411,7 +410,7 @@ func TestAgeText(t *testing.T) { // TestKeepThisCopy: choosing the downloaded copy over the filed one puts it // in the other's place and sends the other to the Trash, where undo can -// still reach it (his request, 2026-09-17). +// still reach it. func TestKeepThisCopy(t *testing.T) { conf := "(path \"~/dl\")\n(rule \"dupes\" (when (duplicate \"~/docs\")) (move \"Dupes\"))\n" e, h := sandboxDir(t, conf, map[string]string{"report.pdf": "the same bytes"}) diff --git a/gui/internal/model/prefs.go b/gui/internal/model/prefs.go index 2d2dc04..4b70de3 100644 --- a/gui/internal/model/prefs.go +++ b/gui/internal/model/prefs.go @@ -12,7 +12,7 @@ import ( // Prefs is how the window behaves - nothing about what krino does to // files, which belongs in the configuration. It lives beside krino.conf as -// gui.json, a file krino itself never reads (his request, 2026-09-16). +// gui.json, a file krino itself never reads. type Prefs struct { // Colours paints the configuration in the Text tab. Colours bool `json:"colours"` @@ -28,7 +28,7 @@ type Prefs struct { PreviewWidth int `json:"preview_width"` // ListWidth and ListHeight are where the divider between the file list // and the explanation was left, in each layout. Every divider a hand - // moves is kept (his request, 2026-09-17). + // moves is kept. ListWidth int `json:"list_width"` ListHeight int `json:"list_height"` // ShowSize, ShowAge and ShowRule are the columns that can be turned off diff --git a/gui/internal/model/preview.go b/gui/internal/model/preview.go index a180dff..f8bf75d 100644 --- a/gui/internal/model/preview.go +++ b/gui/internal/model/preview.go @@ -25,7 +25,7 @@ const ( ) // Preview is what to show of a file beside its explanation: a picture, some -// of its text, or nothing with the reason (his request, 2026-09-16). +// of its text, or nothing with the reason. type Preview struct { Kind PreviewKind Image string // a file to show: the file itself, or a rendered page @@ -82,9 +82,8 @@ func MakePreview(ctx context.Context, path, tmp string, px int) Preview { // // Each render goes in a directory of its own. pdftoppm names its output // after the page number, so a shared directory would hold several files -// called page-1.png and the wrong one could be picked up - which is what -// happened: a preview showed the page of a PDF looked at earlier (his -// report, 2026-09-17). +// called page-1.png and the wrong one could be picked up - a preview could +// show the page of a PDF looked at earlier. func pdfPreview(ctx context.Context, path, tmp string, sz int64, px int) Preview { if _, err := exec.LookPath("pdftoppm"); err == nil { dir, err := os.MkdirTemp(tmp, "page-") diff --git a/gui/internal/model/preview_test.go b/gui/internal/model/preview_test.go index 388ff7b..f310386 100644 --- a/gui/internal/model/preview_test.go +++ b/gui/internal/model/preview_test.go @@ -100,8 +100,8 @@ func TestPreviewShowsOnlyTheHead(t *testing.T) { // TestPdfPreviewsDoNotMixUp: two PDFs looked at one after the other each // get their own rendered page. pdftoppm names its file after the page -// number, so previews sharing a directory would collide - one of his ended -// up showing another document's cover. +// number, so previews sharing a directory would collide, one showing +// another document's cover. func TestPdfPreviewsDoNotMixUp(t *testing.T) { if _, err := exec.LookPath("pdftoppm"); err != nil { t.Skip("pdftoppm is not installed") @@ -168,8 +168,7 @@ func pngSize(t *testing.T, path string) (w, h int) { } // TestPreviewRendersToTheSizeAsked: a taller preview gets a larger page, so -// dragging the pane open does not just magnify a small render (his request, -// 2026-09-17). +// dragging the pane open does not just magnify a small render. func TestPreviewRendersToTheSizeAsked(t *testing.T) { if _, err := exec.LookPath("pdftoppm"); err != nil { t.Skip("pdftoppm is not installed") diff --git a/gui/internal/model/rules_test.go b/gui/internal/model/rules_test.go index c7fa384..c8ee212 100644 --- a/gui/internal/model/rules_test.go +++ b/gui/internal/model/rules_test.go @@ -34,8 +34,8 @@ func TestOpenAndCheck(t *testing.T) { t.Error("an untouched file counts as modified") } - // An unknown placeholder is refused at check time (plan 12 task 3), and - // the position is the action's, inside this file. + // An unknown placeholder is refused at check time, and the position is + // the action's, inside this file. r.SetText("(path \"~/dl\")\n(rule \"all\" (move \"Out/{nope}\"))\n") if !r.Modified() { t.Error("edited text does not count as modified") diff --git a/gui/internal/model/sort.go b/gui/internal/model/sort.go index 9968a08..1918576 100644 --- a/gui/internal/model/sort.go +++ b/gui/internal/model/sort.go @@ -9,7 +9,7 @@ import ( // The orders a plan can be read in. "default" is the order krino planned // the directory in, which is the order the files were scanned; the rest are -// what a column is worth sorting by (his request, 2026-09-17). +// what a column is worth sorting by. const ( SortDefault = "default" SortName = "name" diff --git a/gui/internal/model/testrule_test.go b/gui/internal/model/testrule_test.go index 99989f4..8e64a32 100644 --- a/gui/internal/model/testrule_test.go +++ b/gui/internal/model/testrule_test.go @@ -10,7 +10,7 @@ import ( // TestTestRule: testing one rule says which of the directory's files it // would act on, and what would happen to each - answered from the unsaved -// text, and changing nothing (GUI design §5.3, his request 2026-09-16). +// text, and changing nothing (GUI design §5.3). func TestTestRule(t *testing.T) { conf := "(path \"~/dl\")\n" + "(rule \"pdfs\" (when (type pdf)) (move \"Docs\") (stop))\n" + diff --git a/gui/internal/ui/forms.go b/gui/internal/ui/forms.go index fd97546..a7211cb 100644 --- a/gui/internal/ui/forms.go +++ b/gui/internal/ui/forms.go @@ -18,15 +18,14 @@ import ( // condKinds are the tests a condition row offers, plus the three operators // that hold other conditions. A row's entry holds that form's arguments // exactly as they are written, so no test is out of reach of the form -// editor and none is silently rewritten (see docs/gui-design.md §5.1 and -// the deviation noted in plan 17). +// editor and none is silently rewritten (see docs/gui-design.md §5.1). var condKinds = []string{"type", "name", "path", "content", "size", "age", "duplicate", "matched", "and", "or", "not"} // condLabels is how the picker names them. The rows are already joined by // "all of these must hold", so the three operators say what they are for - // grouping conditions inside one row - rather than looking like the way to -// join two rows (his report, 2026-09-16). +// join two rows. var condLabels = map[string]string{ "and": "and (all of these)", "or": "or (any of these)", @@ -299,7 +298,7 @@ var settingChoices = map[string][]string{ } // settingHelp is what each setting does, shown when the pointer rests on -// its row (his request, 2026-09-17). The wording follows krino.conf(5). +// its row. The wording follows krino.conf(5). var settingHelp = map[string]string{ "path": "the directory krino sorts; a directory's file must set it", "recursive": "look in subdirectories too, not only the directory itself", @@ -502,7 +501,7 @@ func (f *formsView) refreshLabels() { f.forms = forms for i, form := range forms { // Row 0 is the directory itself, so form i is row i+1: without the - // offset every label moved up a row after an edit (his report). + // offset every label moved up a row after an edit. if row := f.list.RowAtIndex(i + 1); row != nil { if label, ok := row.Child().(*gtk.Label); ok { label.SetText(escape(formLabel(form))) @@ -598,7 +597,7 @@ func (f *formsView) move(delta int) { // onTestRule scans the directory with the unsaved text and lists the files // the selected rule would take, in the pane on the right. It reads only - // no lock, nothing moved - but takes as long as a scan, so it runs off the -// main loop (his request, 2026-09-16). +// main loop. func (f *formsView) onTestRule() { if f.sel < 0 || f.sel >= len(f.forms) { f.owner.showTestOutput("Select a rule in the list first.") @@ -694,8 +693,7 @@ func newFormEditor(form model.Form, changed func()) *formEditor { // An exclude has no rule behind it and a rule may have no (when ...) at // all, so neither pointer is followed without asking first: reading - // form.Rule for an exclude crashed the window (found by the release - // checklist, 2026-09-17). + // form.Rule for an exclude crashed the window. var when []*sexp.Node switch { case form.Kind == model.ExcludeForm: @@ -784,7 +782,7 @@ func (fe *formEditor) text() (string, error) { // drawConds rebuilds the condition tree: one row per condition, indented by // how deep it sits, with the operators holding the conditions under them -// (GUI design §5.1, his choice 2026-09-17). +// (GUI design §5.1). func (fe *formEditor) drawConds() { for child := fe.conds.FirstChild(); child != nil; child = fe.conds.FirstChild() { fe.conds.Remove(child) @@ -918,7 +916,7 @@ var compareOps = []string{">", ">=", "<", "<=", "="} // condRow is one condition: its kind, and its arguments. A size or an age // is a comparison, so it gets an operator of its own and a value to type -// rather than one field holding both (his request, 2026-09-16). +// rather than one field holding both. type condRow struct { root *gtk.Box kind *gtk.DropDown diff --git a/gui/internal/ui/highlight.go b/gui/internal/ui/highlight.go index 8a4acd4..786acee 100644 --- a/gui/internal/ui/highlight.go +++ b/gui/internal/ui/highlight.go @@ -10,7 +10,7 @@ import ( // The colours a configuration is painted in. They are chosen to read on a // light and a dark theme alike, since the window follows whatever GTK theme -// is in use (his request, 2026-09-16). +// is in use. var spanColours = map[model.SpanKind]string{ model.SpanComment: "#8b8b8b", model.SpanString: "#2e8b57", diff --git a/gui/internal/ui/plan.go b/gui/internal/ui/plan.go index 650668c..bcbb8a2 100644 --- a/gui/internal/ui/plan.go +++ b/gui/internal/ui/plan.go @@ -97,7 +97,7 @@ func newPlanView(w *Window) *planView { p.dirs.Connect("notify::selected", p.showPath) p.scan = gtk.NewButtonWithLabel("Scan") // The one button that starts everything, so it carries the theme's - // accent like Apply does (his request, 2026-09-17). + // accent like Apply does. p.scan.AddCSSClass("suggested-action") p.scan.SetTooltipText("read the directory and work out what would happen to each file; nothing is touched until Apply") p.selAll = gtk.NewButtonWithLabel("Select all") @@ -120,13 +120,13 @@ func newPlanView(w *Window) *planView { p.filter.SetSizeRequest(200, -1) // The order the plan is read in. It starts as the settings say and can - // be changed for this window alone (his request, 2026-09-17). + // be changed for this window alone. p.sort = gtk.NewDropDownFromStrings(sortItems()) p.sort.SetTooltipText("the order the plan is listed in; Settings has the one a new window starts with") // The bar reads as the order of operations: which directory, how it // will be listed, what of it, where it is on disk - then Scan, and only - // then what to do with what comes back (his request, 2026-09-17). + // then what to do with what comes back. bar.Append(gtk.NewLabel("Directory")) bar.Append(p.dirs) bar.Append(gtk.NewLabel("sort")) @@ -160,7 +160,7 @@ func newPlanView(w *Window) *planView { // The explanation is laid out rather than printed: the file's name, then // a line per step with the action in its own colour, centred in the - // pane so the eye lands on it (his request, 2026-09-17). + // pane so the eye lands on it. p.details = gtk.NewBox(gtk.OrientationVertical, 6) p.details.SetHAlign(gtk.AlignCenter) p.details.SetVAlign(gtk.AlignStart) @@ -176,7 +176,7 @@ func newPlanView(w *Window) *planView { // Under the explanation, a look at the file itself: a picture for an // image, the first page for a PDF, the first lines for anything that is - // text (his request, 2026-09-16). + // text. p.previewNote = gtk.NewLabel("") p.previewNote.SetXAlign(0) p.previewNote.SetMarginStart(8) @@ -186,7 +186,7 @@ func newPlanView(w *Window) *planView { p.previewNote.AddCSSClass("dim-label") // The picture fills whatever the divider leaves it. Inside a scrolled // window it would be given its smallest size instead, which is what - // made the page a stamp (his report, 2026-09-17). + // made the page a stamp. p.picture = gtk.NewPicture() p.picture.SetCanShrink(true) p.picture.SetContentFit(gtk.ContentFitContain) @@ -252,7 +252,7 @@ func newPlanView(w *Window) *planView { } // checkedMenu is "With checked": the same two overrides the row menu has, -// for every file that is checked at once (his request, 2026-09-17). +// for every file that is checked at once. func (p *planView) checkedMenu() *gtk.MenuButton { box := gtk.NewBox(gtk.OrientationVertical, 0) trash := gtk.NewButtonWithLabel("Trash them instead") @@ -603,7 +603,7 @@ func (p *planView) setBusy(busy bool) { // selectAll checks or unchecks every file that can be applied - and, while // a filter is on, only the files it leaves on screen, so what Apply acts on -// is what was in front of him. +// is what is visible. func (p *planView) selectAll(on bool) { if p.tab == nil { return @@ -670,8 +670,7 @@ func (p *planView) fillList() { p.selNone.SetSensitive(!p.tab.Applied) p.sayWhatIsShown() // A fresh list starts at its left edge: without this the view can open - // scrolled sideways, with the file names out of sight (his report, - // 2026-09-17). + // scrolled sideways, with the file names out of sight. if adj := p.listScroll.HAdjustment(); adj != nil { adj.SetValue(0) } @@ -680,7 +679,7 @@ func (p *planView) fillList() { // widths is how wide each column has to be for this plan: enough for the // longest value it holds, within limits, so a rule name or an outcome is // shown whole rather than cut to an ellipsis. The list scrolls sideways -// when the total does not fit (his report, 2026-09-16). +// when the total does not fit. func (p *planView) widths() [7]int { w := [7]int{16, 5, 4, 6, 16, 8, 6} root := p.dirRoot() @@ -701,7 +700,7 @@ func (p *planView) widths() [7]int { w[i] = min(w[i], cap) } // A column is never narrower than its own heading, or the heading is - // the thing that ends in an ellipsis (his report, 2026-09-17). + // the thing that ends in an ellipsis. for i, title := range columnTitles { w[i] = max(w[i], len([]rune(title))) } @@ -830,8 +829,7 @@ func (p *planView) rowWidget(i int, r model.Row, w [7]int) *gtk.ListBoxRow { // ever the column cut to an ellipsis. action, colour := rowAction(r) // Every column can shrink: a pane narrower than their natural widths - // used to push the whole row out of view to the left (his report, - // 2026-09-17). + // used to push the whole row out of view to the left. cells := [7]gtk.Widgetter{ colFile: columnMin(escape(r.Rel), w[colFile], 12, true), colSize: columnMin(model.SizeText(r.Size), w[colSize], 4, false), @@ -949,8 +947,7 @@ func (p *planView) showDetails(i int) { p.details.Append(detailLine(what, "dim-label")) } // Where the other copy is, in full: the reason names it relative to the - // directory when it is inside it, which reads as no place at all (his - // report, 2026-09-17). + // directory when it is inside it, which reads as no place at all. if r.DuplicateOf != "" { p.details.Append(detailLine("the same bytes as "+escape(xdg.Abbrev(r.DuplicateOf)), "dim-label")) } @@ -1058,9 +1055,8 @@ func (p *planView) showPreview(rel string) { } // setLayout arranges the tab the way the settings ask: the file list beside -// the explanation, or above it with the preview to its side (his sketch, -// 2026-09-17). The widgets are the same either way; only the panes holding -// them change. +// the explanation, or above it with the preview to its side. The widgets +// are the same either way; only the panes holding them change. func (p *planView) setLayout(which string) { if p.layout == which && p.arrangement != nil { return diff --git a/gui/internal/ui/rules.go b/gui/internal/ui/rules.go index 5cce276..62cf074 100644 --- a/gui/internal/ui/rules.go +++ b/gui/internal/ui/rules.go @@ -102,7 +102,7 @@ func newRulesView(w *Window) *rulesView { // Line numbers: their own view beside the editor, in the same scrolled // window so the two always line up. The editor does not wrap, so one - // line of text is one line on screen (his request, 2026-09-16). + // line of text is one line on screen. r.nums = gtk.NewTextView() r.nums.SetMonospace(true) r.nums.SetEditable(false) @@ -185,7 +185,7 @@ func newRulesView(w *Window) *rulesView { r.root.Append(gtk.NewSeparator(gtk.OrientationHorizontal)) r.root.Append(panes) // The problems sit under both sub-tabs: a form's mistake is reported - // where the form is, not only in the text (his report, 2026-09-16). + // where the form is, not only in the text. r.root.Append(gtk.NewSeparator(gtk.OrientationHorizontal)) r.root.Append(diagScroll) diff --git a/gui/internal/ui/settings.go b/gui/internal/ui/settings.go index 73c4072..79dba6a 100644 --- a/gui/internal/ui/settings.go +++ b/gui/internal/ui/settings.go @@ -13,7 +13,7 @@ import ( // inherits, and how this window behaves. The first are written to // krino.conf with the care a rules file is written with - checked first, // the previous text kept as krino.conf.bak - and the second to gui.json, -// which krino itself never reads (his request, 2026-09-16). +// which krino itself never reads. type settingsWindow struct { w *Window win *gtk.Window @@ -175,7 +175,7 @@ func (w *Window) showSettings() { closeBtn := gtk.NewButtonWithLabel("Close") closeBtn.ConnectClicked(func() { s.win.Close() }) // The buttons sit outside the scrolled area: on a short screen they - // would otherwise be below the fold, which is where he found them. + // would otherwise be below the fold, where they are easy to miss. buttons := gtk.NewBox(gtk.OrientationHorizontal, 6) buttons.SetHAlign(gtk.AlignEnd) buttons.SetMarginStart(12) diff --git a/gui/internal/ui/window.go b/gui/internal/ui/window.go index 5132c12..c52234b 100644 --- a/gui/internal/ui/window.go +++ b/gui/internal/ui/window.go @@ -67,7 +67,7 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window { }) // Settings sits at the end of the tab strip - the window's top right - - // with a gear beside the word (his request, 2026-09-17). + // with a gear beside the word. settings := gtk.NewButton() settingsBox := gtk.NewBox(gtk.OrientationHorizontal, 6) settingsBox.Append(gtk.NewImageFromIconName("emblem-system-symbolic")) @@ -100,7 +100,7 @@ func NewWindow(app *gtk.Application, e *engine.Engine) *Window { // holds, rather than leaving a lock file for the next run to find. w.win.ConnectCloseRequest(func() bool { // A plan is only a plan until Apply: leaving with one open throws - // it away, which is worth saying out loud (his report, 2026-09-17). + // it away, which is worth saying out loud. if w.plan.hasUnapplied() && !w.leaving { w.confirmLeaving() return true @@ -154,7 +154,7 @@ func (w *Window) confirmLeaving() { } // addDirectory asks for a name and a path and makes krino sort that -// directory too - the window had no way to do it (his report, 2026-09-17). +// directory too - the window had no way to do it. func (w *Window) addDirectory() { d := gtk.NewWindow() d.SetTitle("Add a directory") @@ -311,7 +311,7 @@ func runInBackground(work func(context.Context) error, done func(error)) context // deletions without reading: they are the ones that cannot be undone from // the window. They are the fallbacks; themeColours replaces them with the // running theme's own, so the window looks like the rest of the desktop -// rather than like GNOME's palette (his request, 2026-09-17). +// rather than like GNOME's palette. var actionColours = map[plan.Kind]string{ plan.Copy: "#2a9d8f", plan.Move: "#3584e4", @@ -323,8 +323,7 @@ var actionColours = map[plan.Kind]string{ // actionClasses name the CSS class each action's cell carries. The colour // is applied by a style sheet rather than by painting the text, so that a // selected row - which draws its own background - can take the colour back -// and stay readable: his green accent on his green selection was not (his -// report, 2026-09-17). +// and stay readable: a green accent on a green selection is not. var actionClasses = map[plan.Kind]string{ plan.Copy: "krino-copy", plan.Move: "krino-move", 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 |