krino: acting — trash, journal, apply, lock, review, undo

1 files changed, 317 insertions, 39 deletions

diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go
index 0ba82bc..68f9eda 100644
--- a/cmd/krino/sort.go
+++ b/cmd/krino/sort.go
@@ -3,34 +3,59 @@
 package main
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
+	"os/signal"
 	"sort"
 	"strings"
+	"syscall"
 	"unicode/utf8"
 
+	"golang.org/x/term"
+
 	"krino/internal/engine"
+	"krino/internal/journal"
+	"krino/internal/lock"
 	"krino/internal/plan"
 	"krino/internal/scan"
+	"krino/internal/tui"
 	"krino/internal/xdg"
 )
 
-// cmdSort plans and applies the included directories. Only -n (dry run) is
-// implemented; applying arrives in plan 4.
+// 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.
+var stdin = os.Stdin
+
+// zeroOutcome is the per-directory outcome line for a directory that had
+// nothing applied to it - either because nothing was actionable, or
+// because the user chose [s] or [q] - so the same wording is not retyped
+// (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 journal.Writer, run id and
+// plan.Claims cover 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")
 	}
 	if g.json && !g.dry {
-		fmt.Fprintln(stderr, "krino: --json is not implemented yet")
-		return 2
-	}
-	if !g.dry {
-		fmt.Fprintln(stderr, "krino: applying files is not implemented yet; use -n to see what would happen")
-		return 2
+		return usageError(stderr, "--json is only valid with -n")
 	}
 
 	e, errs := engine.Load(mainFile(g), names...)
@@ -39,6 +64,50 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
 		return 2
 	}
 
+	// Spec §8.4: with neither -y nor -n, krino asks; asking a non-terminal
+	// stdin would just hang (or read garbage), so it refuses instead.
+	if !g.yes && !g.dry && !term.IsTerminal(int(stdin.Fd())) {
+		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
+	// 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
+	// SIGTERM, which - unlike a keyboard Ctrl-C during tui.ReadKey's raw
+	// read (ISIG is off, so that never even reaches us as a signal) - can
+	// land mid-read with no defer left to run.
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	stopSignals := installSignalHandler(cancel)
+	defer stopSignals()
+
+	// Ruling 2: journal.Open creates $XDG_STATE_HOME/krino/ and an empty
+	// krino.log as a side effect of merely being called, so a dry run must
+	// never call it at all - not open it and clean up afterwards. -n is
+	// known from the flags before the loop starts, so the gate is exactly
+	// that, nothing per-directory. One Writer and one run id cover every
+	// directory in the run (Task 5's undo depends on a single run id
+	// spanning all of them).
+	var j *journal.Writer
+	var run string
+	if !g.dry {
+		var err error
+		j, err = journal.Open(e.Config.LogFile())
+		if err != nil {
+			fmt.Fprintf(stderr, "krino: %v\n", err)
+			return 1
+		}
+		defer func() {
+			if cerr := j.Close(); cerr != nil {
+				fmt.Fprintf(stderr, "krino: %v\n", cerr)
+			}
+		}()
+		run = journal.NewRunID(e.Now())
+	}
+
 	exit := 0
 	printed := false
 	jsonDirs := []plan.JSONDir{} // never nil: the document's "dirs" must marshal as [], not null
@@ -47,36 +116,154 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
 	// same destination resolve the collision at planning time instead of
 	// each independently believing it owns that path.
 	claims := plan.NewClaims()
+
 	for _, d := range e.Dirs {
-		if fi, err := os.Stat(d.Root); err != nil || !fi.IsDir() {
-			fmt.Fprintf(stderr, "krino: skipping %s: %s is not a directory\n", d.Name, xdg.Abbrev(d.Root))
-			exit = 1
-			continue
-		}
-		dp, err := e.Plan(context.Background(), d, claims)
+		// 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.
+		l, err := lock.Acquire(ctx, e.Config.LockFile(d.Name), !g.yes)
 		if err != nil {
-			fmt.Fprintf(stderr, "krino: skipping %s: %v\n", d.Name, err)
+			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+				// Interrupted while waiting for the lock: an interrupt, not
+				// a failure - the ctx.Err() check at the end of this
+				// function already turns this into exit 130, and nothing
+				// further should even be attempted.
+				break
+			}
+			fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, err)
 			exit = 1
 			continue
 		}
-		if !g.json {
-			if printed {
-				fmt.Fprintln(stdout)
+
+		quit := func() bool {
+			defer func() {
+				if rerr := l.Release(); rerr != nil {
+					fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, rerr)
+				}
+			}()
+
+			if fi, err := os.Stat(d.Root); err != nil || !fi.IsDir() {
+				fmt.Fprintf(stderr, "krino: skipping %s: %s is not a directory\n", d.Name, xdg.Abbrev(d.Root))
+				exit = 1
+				return false
 			}
-			printed = true
-			fmt.Fprintf(stdout, "krino: %s  %s\n", d.Name, xdg.Abbrev(d.Root))
-		}
-		// C3: directory-level warnings go to stderr after the header
-		// line above, not before it, so on a terminal they read as
-		// describing the directory just named instead of floating above it.
-		for _, w := range dp.Result.Warnings {
-			fmt.Fprintf(stderr, "krino: %s: %s\n", d.Name, w)
-		}
-		if g.json {
-			jsonDirs = append(jsonDirs, plan.NewJSONDir(d.Name, d.Root, dp.Chains, dp.Result.Warnings))
-			continue
+			dp, err := e.Plan(ctx, d, claims)
+			if err != nil {
+				fmt.Fprintf(stderr, "krino: skipping %s: %v\n", d.Name, err)
+				exit = 1
+				return false
+			}
+
+			if !g.json {
+				if printed {
+					fmt.Fprintln(stdout)
+				}
+				printed = true
+				fmt.Fprintf(stdout, "krino: %s  %s\n", d.Name, xdg.Abbrev(d.Root))
+			}
+			// C3: directory-level warnings go to stderr after the header
+			// line above, not before it, so on a terminal they read as
+			// describing the directory just named instead of floating above it.
+			for _, w := range dp.Result.Warnings {
+				fmt.Fprintf(stderr, "krino: %s: %s\n", d.Name, w)
+			}
+			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.
+				jsonDirs = append(jsonDirs, plan.NewJSONDir(d.Name, d.Root, dp.Chains, 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
+			// 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
+			// re-rendered here.
+			var buf bytes.Buffer
+			printPlan(&buf, dp, g.verbose)
+			text := colourDeletePermanently(buf.String(), tui.Colour(stdout))
+			if err := tui.Page(stdout, text); err != nil {
+				fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, err)
+				exit = 1
+				return false
+			}
+
+			if g.dry {
+				return false
+			}
+
+			actionable := actionableChains(dp.Chains)
+			if len(actionable) == 0 {
+				// Nothing to decide: neither -y nor the interactive menu
+				// has anything useful to do here, so neither is asked.
+				fmt.Fprintln(stdout, zeroOutcome)
+				return false
+			}
+
+			var approved map[string]bool
+			var action rune
+			if g.yes {
+				approved, action = approveAll(actionable), 'a'
+			} else {
+				var rerr error
+				approved, action, rerr = reviewDir(stdout, actionable, d.Root)
+				if rerr != nil {
+					fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, rerr)
+					exit = 1
+					return false
+				}
+			}
+
+			switch action {
+			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).
+				fmt.Fprintln(stdout, zeroOutcome)
+				return false
+			case 'q':
+				// Spec §8.2: [q] stops krino; directories already applied
+				// this run stay applied. Nothing is applied here either,
+				// and no further directory is even planned.
+				fmt.Fprintln(stdout, zeroOutcome)
+				return true
+			}
+
+			res, aerr := e.Apply(ctx, dp, approved, j, run)
+			if aerr != nil {
+				if errors.Is(aerr, context.Canceled) || errors.Is(aerr, context.DeadlineExceeded) {
+					// 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),
+					// and no further directory is even attempted. The
+					// ctx.Err() check at the end of this function already
+					// turns this into exit 130.
+					return true
+				}
+				fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, aerr)
+				exit = 1
+				return false
+			}
+			fmt.Fprintf(stdout, "%d applied · %d failed · %d declined\n", res.Applied, res.Failed, res.Declined)
+			// Ruling 1: 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
+			}
+			return false
+		}()
+
+		if quit {
+			break
 		}
-		printPlan(stdout, dp, g.verbose)
 	}
 
 	if g.json {
@@ -90,9 +277,107 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
 		stdout.Write(b)
 		fmt.Fprintln(stdout)
 	}
+
+	// Spec §11: 130 interrupted takes priority over whatever exit already
+	// accumulated - ctx is only ever cancelled by installSignalHandler, and
+	// package main's own cancel() (deferred above) has not run yet here.
+	if ctx.Err() != nil {
+		return 130
+	}
 	return exit
 }
 
+// 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
+// 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 -
+// unchanged - logs it as declined exactly like any other file this review
+// never approved (tallyFile then counts it there, not as a fall-through).
+func actionableChains(chains []plan.Chain) []plan.Chain {
+	var out []plan.Chain
+	for _, c := range chains {
+		if chainActing(c) {
+			out = append(out, c)
+		}
+	}
+	return out
+}
+
+// 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.
+//
+// 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
+// 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
+// lock.Acquire's own fix, that made a run waiting on a held lock ignore
+// every Ctrl-C and every SIGTERM forever, with no escape but another
+// shell's kill -9. Looping fixes the common case (ctx cancellation reaches
+// something that is actually checking it, e.g. a waiting lock.Acquire or
+// Apply between files) and the second signal is also the user's guarantee
+// of an exit even when it does not: by then a clean shutdown has already
+// been asked for once and not delivered, so it restores the terminal once
+// more (harmless if already restored) and exits immediately with the same
+// 130 spec §11 already uses for "interrupted".
+func installSignalHandler(cancel context.CancelFunc) func() {
+	fd := int(stdin.Fd())
+	var saved *term.State
+	if term.IsTerminal(fd) {
+		saved, _ = term.GetState(fd) // best-effort: nothing to restore if this fails
+	}
+
+	sig := make(chan os.Signal, 1)
+	signal.Notify(sig, os.Interrupt, syscall.SIGTERM)
+	done := make(chan struct{})
+	go func() {
+		signals := 0
+		for {
+			select {
+			case <-sig:
+				cancel()
+				if saved != nil {
+					term.Restore(fd, saved)
+				}
+				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).
+					os.Exit(130)
+				}
+			case <-done:
+				return
+			}
+		}
+	}()
+	return func() {
+		signal.Stop(sig)
+		close(done)
+	}
+}
+
 // warnLine is one file's warning, for the warnings section.
 type warnLine struct {
 	rel  string
@@ -235,14 +520,7 @@ func chainOutcomes(chains []plan.Chain) (excluded, allSkipped int) {
 			excluded++
 			continue
 		}
-		acting := false
-		for _, s := range c.Steps {
-			if s.Skip == "" {
-				acting = true
-				break
-			}
-		}
-		if !acting {
+		if !chainActing(c) {
 			allSkipped++
 		}
 	}