From bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Thu, 17 Sep 2026 12:11:42 +0200 Subject: 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. --- cmd/krino/commands_test.go | 15 +++-- cmd/krino/exclude_test.go | 4 +- cmd/krino/history_test.go | 87 +++++++++++++++-------------- cmd/krino/log.go | 10 ++-- cmd/krino/main.go | 14 ++--- cmd/krino/main_test.go | 12 ++-- cmd/krino/matching_test.go | 47 ++++++++-------- cmd/krino/render.go | 52 ++++++++--------- cmd/krino/render_test.go | 10 ++-- cmd/krino/review.go | 17 +++--- cmd/krino/review_test.go | 12 ++-- cmd/krino/sort.go | 135 ++++++++++++++++++++++----------------------- cmd/krino/sort_test.go | 48 ++++++++-------- cmd/krino/undo.go | 67 +++++++++++----------- 14 files changed, 260 insertions(+), 270 deletions(-) (limited to 'cmd') 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: " on stderr, but is not one +// TestDirectoryWarningNotCountedInWarningsField: a directory-level warning +// is printed as "krino: NAME: " 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 ("