diff options
| author | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-17 12:11:42 +0200 |
|---|---|---|
| committer | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-17 12:11:42 +0200 |
| commit | bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd (patch) | |
| tree | 187e6a1fb722d9ab97d2076f69f997f5d371e943 | |
| parent | cd7425b81f963a948f0abe7df3f9e58e190c2b78 (diff) | |
| download | krino-bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd.tar.gz krino-bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd.zip | |
comments that explain the code, not how it was written
About 340 comments cited the development process: task and plan
numbers, fix waves, rulings, reviewers, and the author in the third
person with a date. None of that exists outside the work itself, so to
a reader it pointed at nothing. Each one now states the engineering
reason it was standing in front of; where a comment was provenance and
nothing else, it is gone.
References to docs/design.md and docs/gui-design.md by section stay:
both ship with the repository. The design documents lose their
amendment diaries - CHANGELOG.md is that record - and the GUI's says
plainly that the window has gone further than the document.
Only comments changed. Every .go file was parsed and its code printed
with comments stripped, before and after: the two hashes are identical
across all 175 files.
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 |