feat(ui): redesign 3/5 — Molecule view: plan in plain words, one primary Approve, Request changes, Retire behind a confirmation (#251)

lab/internal/molecule/amend.go

@@ -253,10 +253,17 @@ type AmendRequest struct {
 	AcceptProtectedFailures bool
 	// Run reports that the caller approves and runs the new revision right after
 	// the amend (amend --run).
-	Run             bool
+	Run bool
+	// ExpectParent, when set, is the plan digest the caller reviewed: the amend is
+	// refused with ErrStalePlan unless it is still the latest plan under the lock.
+	ExpectParent    string
 	protectedDeltas []readiness.ProtectedDelta
 }
 
+// ErrStalePlan is the refusal of an amend whose ExpectParent is no longer the
+// latest plan: a newer revision was recorded after the caller read it.
+var ErrStalePlan = errors.New("the latest plan is not the one reviewed; a newer plan exists")
+
 // Bounds of one flag-driven amendment. Plan.Validate bounds the totals.
 const (
 	maxAmendAdditions  = 64
@@ -350,8 +357,9 @@ func refuseSelectionInputChange(previous, next readiness.Plan) error {
 // AmendFromLatest records the successor of the Molecule's latest plan built from
 // req. The parent is the latest plan digest, read under the Molecule flock and the
 // approval lock directory, so a concurrent amend either fails as locked or has
-// already been recorded and becomes the parent: the history never forks. Every
-// check of Amend applies; a refusal writes nothing.
+// already been recorded and becomes the parent: the history never forks. A set
+// req.ExpectParent that is not that latest digest is refused with ErrStalePlan.
+// Every check of Amend applies; a refusal writes nothing.
 func AmendFromLatest(ctx context.Context, lab, id string, req AmendRequest) (Record, error) {
 	var empty Record
 	if err := req.validate(); err != nil {
@@ -386,10 +394,13 @@ func AmendFromLatest(ctx context.Context, lab, id string, req AmendRequest) (Rec
 	if r.Readiness == nil || len(r.Readiness.Plans) == 0 {
 		return r, fmt.Errorf("Molecule has no readiness plan to amend")
 	}
+	latest := r.Readiness.Plans[len(r.Readiness.Plans)-1]
+	if req.ExpectParent != "" && req.ExpectParent != latest.Digest {
+		return r, ErrStalePlan
+	}
 	if r.Workspace.Path == "" || (r.CheckoutStatus != "" && r.CheckoutStatus != "available") {
 		return r, fmt.Errorf("coding checkout unavailable; refresh intake")
 	}
-	latest := r.Readiness.Plans[len(r.Readiness.Plans)-1]
 	if req.AcceptProtectedFailures {
 		if req.protectedDeltas, err = protectedFailureDeltas(ctx, r, latest); err != nil {
 			return r, err

lab/internal/molecule/amend_test.go

@@ -0,0 +1,38 @@
+package molecule
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"testing"
+)
+
+// A caller that reviewed one plan, while another amend recorded a newer one
+// before its engine call, is refused as stale under the lock and records nothing.
+func TestAmendFromLatestRefusesStaleExpectedParent(t *testing.T) {
+	t.Parallel()
+	o, r := amendFixture(t)
+	seen := latestPlan(r).Digest
+	// The ledger moves forward between the caller's check and its engine call.
+	advanced, err := AmendFromLatest(context.Background(), o.LabDir, r.ID, AmendRequest{AddAcceptance: []string{"Someone else's note"}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	before := ledgerBytes(t, advanced)
+	_, err = AmendFromLatest(context.Background(), o.LabDir, r.ID, AmendRequest{ExpectParent: seen, AddAcceptance: []string{"My note"}})
+	if !errors.Is(err, ErrStalePlan) {
+		t.Fatalf("stale expected parent was not refused as stale: %v", err)
+	}
+	if !bytes.Equal(before, ledgerBytes(t, advanced)) {
+		t.Fatal("a stale amend changed the ledger")
+	}
+	// The digest the caller now sees is accepted and becomes the parent.
+	current := latestPlan(advanced).Digest
+	next, err := AmendFromLatest(context.Background(), o.LabDir, r.ID, AmendRequest{ExpectParent: current, AddAcceptance: []string{"My note"}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := latestPlan(next).Plan.ParentDigest; got != current {
+		t.Fatalf("parent = %s, want %s", got, current)
+	}
+}

lab/internal/molecule/diffstat.go

@@ -0,0 +1,167 @@
+package molecule
+
+import (
+	"context"
+	"errors"
+	"os"
+	"path/filepath"
+	"regexp"
+	"strconv"
+	"strings"
+	"syscall"
+	"time"
+
+	"github.com/DanielNetzer/forgecell/lab/internal/process"
+)
+
+// DiffStat is the size of a captured tree against its base commit, as git diff
+// --numstat counts it. Binary files count as changed files with no lines.
+// Truncated means a bound was hit: the counts cover only the files read before
+// it, so they are a lower bound and never the whole change.
+type DiffStat struct {
+	Files     int   `json:"files"`
+	Added     int64 `json:"added"`
+	Removed   int64 `json:"removed"`
+	Binary    int   `json:"binary"`
+	Truncated bool  `json:"truncated"`
+}
+
+// Bounds of one numstat: the files counted, the bytes read and the time taken.
+const (
+	MaxDiffStatFiles = 10000
+	MaxDiffStatBytes = 1 << 20
+	diffStatTimeout  = 20 * time.Second
+	// maxDiffStatLines bounds one file's line count, so no sum can overflow.
+	maxDiffStatLines = 1 << 40
+)
+
+var (
+	// ErrDiffStatInvalid refuses an object id, Lab or workspace the numstat never runs on.
+	ErrDiffStatInvalid = errors.New("diff size refused: invalid object id or workspace")
+	// ErrDiffStatFailed is a git failure, timeout or unreadable output; no size is known.
+	ErrDiffStatFailed = errors.New("diff size unavailable: git diff --numstat failed")
+)
+
+// objectID is a full SHA-1 or SHA-256 Git object id, lower-case hex only.
+var objectID = regexp.MustCompile(`^(?:[0-9a-f]{40}|[0-9a-f]{64})$`)
+
+// DiffNumstat measures tree, a captured tree object, against the base commit with
+// git diff --numstat, run directly (no shell) in workspace, which must be a
+// directory of the Lab's own workspaces/ owned by this user and not a symlink.
+// Both object ids must be full hex ids, so neither can be read as an option or a
+// revision expression. Output, file count and time are bounded; a bound hit is
+// reported as Truncated, a failure as ErrDiffStatFailed. Error values never carry
+// local paths or git output.
+func DiffNumstat(ctx context.Context, lab, workspace, base, tree string) (DiffStat, error) {
+	var out DiffStat
+	if !objectID.MatchString(base) || !objectID.MatchString(tree) {
+		return out, ErrDiffStatInvalid
+	}
+	dir, err := labWorkspace(lab, workspace)
+	if err != nil {
+		return out, err
+	}
+	argv := []string{"git", "-c", "core.fsmonitor=false", "-c", "core.hooksPath=/dev/null", "-c", "core.quotePath=false",
+		"diff", "--numstat", "-z", "--no-renames", "--no-ext-diff", "--no-textconv", "--no-color", base, tree, "--"}
+	r := process.Run(ctx, process.Options{Argv: argv, Dir: dir, Env: numstatEnv(), Stdin: []byte{}, Timeout: diffStatTimeout, MaxOutputBytes: MaxDiffStatBytes})
+	switch {
+	case r.Overflow && r.Reconciled && !r.TimedOut && !r.Interrupted && !r.ParentDeadline:
+		// The output was cut at the byte bound: count the complete records read.
+		out = parseNumstat(r.Stdout, false)
+		out.Truncated = true
+		return out, nil
+	case !r.OK:
+		return DiffStat{}, ErrDiffStatFailed
+	}
+	return parseNumstat(r.Stdout, true), nil
+}
+
+// labWorkspace is workspace when it is exactly <lab>/workspaces/<name> with both
+// directories real (not symlinks) and owned by this user.
+func labWorkspace(lab, workspace string) (string, error) {
+	if lab == "" || workspace == "" || !filepath.IsAbs(workspace) {
+		return "", ErrDiffStatInvalid
+	}
+	lab, err := filepath.Abs(lab)
+	if err != nil {
+		return "", ErrDiffStatInvalid
+	}
+	root := filepath.Join(lab, "workspaces")
+	clean := filepath.Clean(workspace)
+	name := filepath.Base(clean)
+	if filepath.Dir(clean) != root || name == "." || name == ".." || strings.HasPrefix(name, ".") {
+		return "", ErrDiffStatInvalid
+	}
+	for _, dir := range []string{root, clean} {
+		info, err := os.Lstat(dir)
+		if err != nil || !info.IsDir() || info.Mode()&os.ModeSymlink != 0 {
+			return "", ErrDiffStatInvalid
+		}
+		if st, ok := info.Sys().(*syscall.Stat_t); !ok || int(st.Uid) != os.Getuid() {
+			return "", ErrDiffStatInvalid
+		}
+	}
+	return clean, nil
+}
+
+// numstatEnv is this process's environment without GIT_* variables, which could
+// point git at another repository, index or config, and with optional locks off,
+// so a read never writes the index.
+func numstatEnv() []string {
+	env := []string{}
+	for _, s := range os.Environ() {
+		name, _, _ := strings.Cut(s, "=")
+		if !strings.HasPrefix(name, "GIT_") {
+			env = append(env, s)
+		}
+	}
+	return append(env, "GIT_OPTIONAL_LOCKS=0", "GIT_CONFIG_NOSYSTEM=1")
+}
+
+// parseNumstat counts NUL-terminated "added\tremoved\tpath" records. A record
+// that is not a complete, well-formed numstat line marks the result truncated
+// instead of being guessed at; complete says the output ended on a record
+// boundary, so a trailing fragment is only expected when it did not.
+func parseNumstat(raw string, complete bool) DiffStat {
+	var out DiffStat
+	records := strings.Split(raw, "\x00")
+	last := records[len(records)-1]
+	records = records[:len(records)-1]
+	if last != "" {
+		// A trailing fragment is a record cut by the byte bound.
+		out.Truncated = true
+	}
+	for _, rec := range records {
+		if rec == "" {
+			continue
+		}
+		if out.Files == MaxDiffStatFiles {
+			out.Truncated = true
+			break
+		}
+		added, rest, ok1 := strings.Cut(rec, "\t")
+		removed, file, ok2 := strings.Cut(rest, "\t")
+		if !ok1 || !ok2 || file == "" {
+			out.Truncated = true
+			continue
+		}
+		if added == "-" && removed == "-" {
+			out.Files++
+			out.Binary++
+			continue
+		}
+		a, errA := strconv.ParseInt(added, 10, 64)
+		d, errD := strconv.ParseInt(removed, 10, 64)
+		if errA != nil || errD != nil || a < 0 || d < 0 || a > maxDiffStatLines || d > maxDiffStatLines {
+			out.Truncated = true
+			continue
+		}
+		out.Files++
+		out.Added += a
+		out.Removed += d
+	}
+	if !complete {
+		out.Truncated = true
+	}
+	return out
+}