diff --git a/lab/internal/molecule/amend.go b/lab/internal/molecule/amend.go index f47dc24..bcec33e 100644 --- a/lab/internal/molecule/amend.go +++ b/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 diff --git a/lab/internal/molecule/amend_test.go b/lab/internal/molecule/amend_test.go new file mode 100644 index 0000000..ffa6601 --- /dev/null +++ b/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) + } +} diff --git a/lab/internal/molecule/diffstat.go b/lab/internal/molecule/diffstat.go new file mode 100644 index 0000000..33622ce --- /dev/null +++ b/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 /workspaces/ 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 +} diff --git a/lab/internal/molecule/diffstat_test.go b/lab/internal/molecule/diffstat_test.go new file mode 100644 index 0000000..c9b441d --- /dev/null +++ b/lab/internal/molecule/diffstat_test.go @@ -0,0 +1,150 @@ +package molecule + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// numstatRepo is a Lab with one workspace repository: a base commit and the tree +// of a second commit that edits one file, adds a text and a binary file and +// deletes one. +func numstatRepo(t *testing.T) (lab, workspace, base, tree string) { + t.Helper() + lab = resolvedTempDir(t) + workspace = filepath.Join(lab, "workspaces", "issue-1") + if err := os.MkdirAll(workspace, 0700); err != nil { + t.Fatal(err) + } + git := func(args ...string) string { + return testGit(t, workspace, append([]string{"-c", "user.name=Test", "-c", "user.email=test@example.test", "-c", "commit.gpgsign=false"}, args...)...) + } + write := func(name, body string) { + if err := os.WriteFile(filepath.Join(workspace, name), []byte(body), 0600); err != nil { + t.Fatal(err) + } + } + git("init", "-q") + write("edit.txt", "one\ntwo\nthree\n") + write("gone.txt", "a\nb\n") + git("add", ".") + git("commit", "-qm", "base") + base = git("rev-parse", "HEAD") + write("edit.txt", "one\n2\nthree\nfour\n") + write("new.txt", "x\ny\nz\n") + write("blob.bin", "\x00\x01\x02binary") + if err := os.Remove(filepath.Join(workspace, "gone.txt")); err != nil { + t.Fatal(err) + } + git("add", "-A") + git("commit", "-qm", "change") + tree = git("rev-parse", "HEAD^{tree}") + return lab, workspace, base, tree +} + +func TestDiffNumstatCountsTheCapturedTreeAgainstTheBase(t *testing.T) { + lab, workspace, base, tree := numstatRepo(t) + got, err := DiffNumstat(context.Background(), lab, workspace, base, tree) + if err != nil { + t.Fatal(err) + } + // edit.txt +2 -1, new.txt +3, gone.txt -2, blob.bin binary. + if want := (DiffStat{Files: 4, Added: 5, Removed: 3, Binary: 1}); got != want { + t.Fatalf("numstat %+v, want %+v", got, want) + } + // The same tree against itself is no change at all. + if same, err := DiffNumstat(context.Background(), lab, workspace, base, testGit(t, workspace, "rev-parse", base+"^{tree}")); err != nil || same != (DiffStat{}) { + t.Fatalf("unchanged tree: %+v %v", same, err) + } +} + +func TestDiffNumstatRefusesInvalidIdsAndWorkspaces(t *testing.T) { + lab, workspace, base, tree := numstatRepo(t) + for name, c := range map[string][2]string{ + "short": {base[:12], tree}, + "upper": {strings.ToUpper(base), tree}, + "option": {"--output=/tmp/x", tree}, + "expression": {base, "HEAD~1"}, + "range": {base, base + ".." + tree}, + "empty": {"", tree}, + } { + if _, err := DiffNumstat(context.Background(), lab, workspace, c[0], c[1]); !errors.Is(err, ErrDiffStatInvalid) { + t.Errorf("%s: %v", name, err) + } + } + outside := resolvedTempDir(t) + link := filepath.Join(lab, "workspaces", "link") + if err := os.Symlink(workspace, link); err != nil { + t.Fatal(err) + } + for name, dir := range map[string]string{ + "relative": "workspaces/issue-1", + "outside": outside, + "lab root": lab, + "nested": filepath.Join(workspace, "sub"), + "traversal": filepath.Join(lab, "workspaces", "..", "workspaces", "..", "x"), + "symlink": link, + "missing": filepath.Join(lab, "workspaces", "absent"), + "dot entries": filepath.Join(lab, "workspaces", ".hidden"), + } { + if _, err := DiffNumstat(context.Background(), lab, dir, base, tree); !errors.Is(err, ErrDiffStatInvalid) { + t.Errorf("%s: %v", name, err) + } + } + // An object that does not exist fails safe without a size. + if got, err := DiffNumstat(context.Background(), lab, workspace, base, strings.Repeat("0", 40)); !errors.Is(err, ErrDiffStatFailed) || got != (DiffStat{}) { + t.Fatalf("missing object: %+v %v", got, err) + } +} + +func TestDiffNumstatParseBoundsFailSafe(t *testing.T) { + if got := parseNumstat("1\t2\ta.go\x00-\t-\tb.png\x00", true); got != (DiffStat{Files: 2, Added: 1, Removed: 2, Binary: 1}) { + t.Fatalf("plain: %+v", got) + } + // Output cut by the byte bound counts only complete records and is truncated. + if got := parseNumstat("1\t2\ta.go\x003\t4\tb.g", false); got != (DiffStat{Files: 1, Added: 1, Removed: 2, Truncated: true}) { + t.Fatalf("cut: %+v", got) + } + // Malformed and hostile records are never counted as sizes. + for _, raw := range []string{"x\ty\ta.go\x00", "1\ta.go\x00", "-1\t2\ta.go\x00", "1\t2\t\x00", "99999999999999999999\t1\ta.go\x00"} { + if got := parseNumstat(raw, true); !got.Truncated || got.Files != 0 || got.Added != 0 { + t.Errorf("%q: %+v", raw, got) + } + } + // More files than the bound stop at the bound, truncated. + var b strings.Builder + for i := 0; i <= MaxDiffStatFiles; i++ { + b.WriteString("1\t0\tf\x00") + } + if got := parseNumstat(b.String(), true); !got.Truncated || got.Files != MaxDiffStatFiles || got.Added != MaxDiffStatFiles { + t.Fatalf("file bound: %+v", got) + } +} + +// A change whose numstat output exceeds the byte bound is reported truncated with +// the complete records read, never as the whole change. +func TestDiffNumstatOutputBoundIsTruncated(t *testing.T) { + lab, workspace, base, _ := numstatRepo(t) + // Long names make the output exceed MaxDiffStatBytes with few files. + name := strings.Repeat("n", 200) + files := MaxDiffStatBytes/len(name) + 64 + for i := 0; i < files; i++ { + if err := os.WriteFile(filepath.Join(workspace, fmt.Sprintf("%s-%06d", name, i)), []byte("x\n"), 0600); err != nil { + t.Fatal(err) + } + } + testGit(t, workspace, "add", "-A") + testGit(t, workspace, "-c", "user.name=Test", "-c", "user.email=test@example.test", "-c", "commit.gpgsign=false", "commit", "-qm", "many") + tree := testGit(t, workspace, "rev-parse", "HEAD^{tree}") + got, err := DiffNumstat(context.Background(), lab, workspace, base, tree) + if err != nil { + t.Fatal(err) + } + if !got.Truncated || got.Files == 0 || got.Files >= files { + t.Fatalf("over the byte bound: %+v of %d files", got, files) + } +} diff --git a/lab/internal/ui/amend.go b/lab/internal/ui/amend.go new file mode 100644 index 0000000..24d1e49 --- /dev/null +++ b/lab/internal/ui/amend.go @@ -0,0 +1,212 @@ +package ui + +import ( + "context" + "errors" + "net/http" + "strings" + "time" + + "github.com/DanielNetzer/forgecell/lab/internal/molecule" + "github.com/DanielNetzer/forgecell/lab/internal/readiness" +) + +// POST /api/molecules/{id}/amend {"digest","addAcceptance","addScope"} is +// Request changes: it records a successor revision of the plan waiting for +// approval through molecule.AmendFromLatest, the engine path of forgecell amend +// --add-acceptance and --add-scope PATH --reason TEXT, without --run. It never +// approves: the new revision waits for its own exact approval through the scope +// gate. It has the gate write protection (cookie, tab header, exact Origin, +// Fetch Metadata and one strict JSON object of at most 4 KiB), refuses a digest +// that is not the one waiting the way the scope gate does, and serves only fixed +// refusal categories. Committed comes only from the ledger read-back recording +// a successor of exactly the submitted digest. + +// AmendAction is the action name /api/lab lists for the Request changes route. +const AmendAction = "request plan changes" + +const ( + // maxUIAmendAdditions bounds the acceptance notes and the scope paths of one + // request; the engine allows more, the form sends one of each at most. + maxUIAmendAdditions = 8 + // maxUIAmendText bounds one acceptance note or scope reason in bytes. + maxUIAmendText = 1000 + // amendTimeout keeps an amendment inside the server's write timeout. + amendTimeout = 45 * time.Second +) + +type amendScope struct { + Path string `json:"path"` + Reason string `json:"reason"` +} + +type amendRequest struct { + Digest string `json:"digest"` + AddAcceptance []string `json:"addAcceptance"` + AddScope []amendScope `json:"addScope"` +} + +// engineAmend is the CLI's amend request for the same additions: --add-scope +// PATH --reason TEXT pairs and --add-acceptance TEXT, in order, and nothing else +// (no note, continuation, accepted tree or --run). +func (q amendRequest) engineAmend() molecule.AmendRequest { + var req molecule.AmendRequest + for _, s := range q.AddScope { + req.AddScope = append(req.AddScope, molecule.ScopeAddition{Path: s.Path, Reason: s.Reason}) + } + req.AddAcceptance = append(req.AddAcceptance, q.AddAcceptance...) + return req +} + +// valid checks what the engine would refuse anyway, so a malformed request never +// reaches it: at least one addition, bounded counts and text, exact repository +// paths and no path twice. +func (q amendRequest) valid() bool { + if len(q.AddAcceptance)+len(q.AddScope) == 0 || len(q.AddAcceptance) > maxUIAmendAdditions || len(q.AddScope) > maxUIAmendAdditions { + return false + } + for _, a := range q.AddAcceptance { + if readiness.ValidateOperatorText(a, maxUIAmendText) != nil { + return false + } + } + seen := map[string]bool{} + for _, s := range q.AddScope { + if readiness.ValidatePath(s.Path) != nil || readiness.ValidateOperatorText(s.Reason, maxUIAmendText) != nil || seen[s.Path] { + return false + } + seen[s.Path] = true + } + return true +} + +// AmendView is every accepted Request changes response. Committed is true only +// when the ledger read back holds a revision whose parent is the submitted digest +// and waits for approval; Digest and Revision are that revision's. +type AmendView struct { + MoleculeID string `json:"moleculeId"` + Committed bool `json:"committed"` + Revision int `json:"revision,omitempty"` + Digest string `json:"digest,omitempty"` + Canonical MoleculeState `json:"canonical"` +} + +// amendRefusal is one fixed refusal for engine error text that names its cause; +// its reason is categoryReasons[category]. +type amendRefusal struct { + parts []string + code int + category, message string +} + +var amendRefusals = []amendRefusal{ + {[]string{"already in scope", "acceptance item already present"}, http.StatusConflict, "duplicate", "the plan already lists that path or acceptance note; nothing was recorded"}, + {[]string{"unsafe repository path", "needs evidence for its parent component", readiness.InScopeDefinitionMessage}, http.StatusUnprocessableEntity, "scope-refused", "the engine refused that scope path: it is unsafe, a frozen check definition, or no frozen manifest covers it; nothing was recorded"}, + {[]string{"coding checkout unavailable"}, http.StatusConflict, "checkout", "the coding checkout is unavailable; nothing was recorded"}, + {[]string{"is locked", "lock could not be taken"}, http.StatusConflict, "locked", "the Molecule is locked by another command or a loop; nothing was recorded"}, +} + +// amendStaleMessage is the stale refusal of Request changes, before or inside the +// engine lock: the plan shown is no longer the latest. +const amendStaleMessage = "a newer plan exists: this plan digest is not the one waiting for approval; reload the Molecule and review the current plan" + +// refuseRetiredAmend refuses Request changes on a retired Molecule before the +// engine, which refuses it too, and reports whether it did. +func (h *handler) refuseRetiredAmend(w http.ResponseWriter, r molecule.Record) bool { + if r.Status != molecule.StatusRetired && r.Retired == nil { + return false + } + h.refuseWith(w, http.StatusConflict, "retired", "this Molecule is retired; nothing was recorded", "Start a new Molecule for the Issue; a retired Molecule keeps every record but is never resumed") + return true +} + +func (h *handler) amendPlan(w http.ResponseWriter, r molecule.Record, req amendRequest) { + if !sha256Hex.MatchString(req.Digest) || !req.valid() { + h.refuseGate(w, http.StatusBadRequest, "invalid", "give the exact plan digest shown and an acceptance note or a scope path with a one-line reason") + return + } + if h.refuseRetiredAmend(w, r) { + return + } + // One amendment at a time from this UI, so the waiting digest checked here is + // the parent the engine amends. + h.amendMu.Lock() + defer h.amendMu.Unlock() + record, err := h.records(h.opts.LabDir, r.ID) + if err != nil { + h.refuseGate(w, http.StatusUnprocessableEntity, "refused", "the ledger was refused: it is invalid, unsafe, oversized or not this Lab's") + return + } + if h.refuseRetiredAmend(w, record) { + return + } + if digest, pending := pendingDigest(record); !pending || req.Digest != digest { + h.refuseGate(w, http.StatusConflict, "stale", amendStaleMessage) + return + } + if h.jobs.busy(record.ID) { + h.refuseGate(w, http.StatusConflict, "busy", "a gate job is still running for this Molecule; nothing was recorded") + return + } + ctx, cancel := context.WithTimeout(h.jobs.ctx, amendTimeout) + defer cancel() + // The engine rechecks the shown digest under its lock, so a revision another + // command recorded after the check above is refused as stale too. + engineReq := req.engineAmend() + engineReq.ExpectParent = req.Digest + if _, err := guardedAmend(ctx, h.amend, h.opts.LabDir, record.ID, engineReq); err != nil { + h.amendRefused(w, err) + return + } + h.json(w, http.StatusOK, h.amendView(record.ID, req.Digest)) +} + +// guardedAmend runs the engine call; a panic becomes a refusal. +func guardedAmend(ctx context.Context, amend func(context.Context, string, string, molecule.AmendRequest) (molecule.Record, error), lab, id string, req molecule.AmendRequest) (r molecule.Record, err error) { + defer func() { + if recover() != nil { + err = errGatePanic + } + }() + return amend(ctx, lab, id, req) +} + +// amendRefused maps an engine error to a fixed category; its text is never served. +func (h *handler) amendRefused(w http.ResponseWriter, err error) { + switch { + case errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded): + h.refuseGate(w, http.StatusServiceUnavailable, "interrupted", "the request was interrupted before the engine finished; read the Molecule back before trying again") + return + case errors.Is(err, molecule.ErrMoleculeRetired): + h.refuseWith(w, http.StatusConflict, "retired", "this Molecule is retired; nothing was recorded", "Start a new Molecule for the Issue; a retired Molecule keeps every record but is never resumed") + return + case errors.Is(err, molecule.ErrStalePlan): + h.refuseGate(w, http.StatusConflict, "stale", amendStaleMessage) + return + } + text := err.Error() + for _, a := range amendRefusals { + for _, p := range a.parts { + if strings.Contains(text, p) { + h.refuseGate(w, a.code, a.category, a.message) + return + } + } + } + h.refuseGate(w, http.StatusUnprocessableEntity, "refused", "the engine refused the request; nothing else was attempted") +} + +// amendView reads the ledger back: committed once the latest plan is a successor +// of parent and waits for approval. +func (h *handler) amendView(id, parent string) AmendView { + v := AmendView{MoleculeID: id, Canonical: h.moleculeState(id)} + record, err := h.records(h.opts.LabDir, id) + if err != nil || record.Readiness == nil || len(record.Readiness.Plans) == 0 { + return v + } + latest := record.Readiness.Plans[len(record.Readiness.Plans)-1] + if digest, pending := pendingDigest(record); pending && digest == latest.Digest && latest.Plan.ParentDigest == parent && sha256Hex.MatchString(latest.Digest) { + v.Committed, v.Revision, v.Digest = true, latest.Plan.Revision, latest.Digest + } + return v +} diff --git a/lab/internal/ui/amend_test.go b/lab/internal/ui/amend_test.go new file mode 100644 index 0000000..27654d2 --- /dev/null +++ b/lab/internal/ui/amend_test.go @@ -0,0 +1,296 @@ +package ui + +import ( + "context" + "errors" + "fmt" + "net/http" + "reflect" + "strings" + "sync" + "testing" + + "github.com/DanielNetzer/forgecell/lab/internal/molecule" + "github.com/DanielNetzer/forgecell/lab/internal/readiness" +) + +// fakeAmend replaces the amend seam: it records every call and, unless err is +// set, appends the successor revision to the ledger as the engine would, so the +// read-back can see it. No git or harness process starts. +type fakeAmend struct { + mu sync.Mutex + calls []molecule.AmendRequest + err error + write bool +} + +func (f *fakeAmend) count() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.calls) +} + +var successorDigest = strings.Repeat("5e", 32) + +func installAmend(t *testing.T, h http.Handler, f *fakeAmend) *fakeAmend { + t.Helper() + hh := h.(*handler) + hh.amend = func(_ context.Context, lab, id string, req molecule.AmendRequest) (molecule.Record, error) { + f.mu.Lock() + f.calls = append(f.calls, req) + err, write := f.err, f.write + f.mu.Unlock() + if err != nil { + return molecule.Record{}, err + } + r, rerr := molecule.ReadRecord(lab, id) + if rerr != nil { + return r, rerr + } + if write { + latest := r.Readiness.Plans[len(r.Readiness.Plans)-1] + next := latest.Plan + next.Revision, next.ParentDigest = latest.Plan.Revision+1, latest.Digest + r.Readiness.Plans = append(r.Readiness.Plans, readiness.Proposal{Plan: next, Digest: successorDigest}) + if err := saveRecord(lab, r); err != nil { + return r, err + } + } + return r, nil + } + return f +} + +func amendBody(digest string) string { + return `{"digest":"` + digest + `","addAcceptance":["The help text names the flag"],"addScope":[{"path":"lab/x/new.go","reason":"New helper"}]}` +} + +// Request changes calls the engine path of forgecell amend with exactly the +// request the CLI builds for --add-scope lab/x/new.go --reason "New helper" +// --add-acceptance "The help text names the flag", and nothing else: no note, +// continuation, accepted tree or --run. It never approves: no scope job runs and +// the new revision waits for its own approval. Committed comes only from the +// ledger read-back of a successor of exactly the submitted digest. +func TestUIAmendRecordsTheCLIRevisionWithoutApproving(t *testing.T) { + lab, r, digest := gateFixture(t) + h := testHandler(t, Options{LabDir: lab}) + if reflect.ValueOf(h.(*handler).amend).Pointer() != reflect.ValueOf(molecule.AmendFromLatest).Pointer() { + t.Fatal("Request changes is not wired to molecule.AmendFromLatest") + } + engine := refuseEngine(t, h) + f := installAmend(t, h, &fakeAmend{write: true}) + v := viewOf[AmendView](t, do(t, h, gatePost("/api/molecules/"+r.ID+"/amend", amendBody(digest))), http.StatusOK) + cli := molecule.AmendRequest{AddScope: []molecule.ScopeAddition{{Path: "lab/x/new.go", Reason: "New helper"}}, AddAcceptance: []string{"The help text names the flag"}} + if f.count() != 1 || f.calls[0].ExpectParent != digest { + t.Fatalf("engine request %+v does not expect the shown digest %s", f.calls, digest) + } + // Apart from the digest the user saw, it is exactly the CLI's request. + sent := f.calls[0] + sent.ExpectParent = "" + if !reflect.DeepEqual(sent, cli) { + t.Fatalf("engine request %+v, want the CLI's %+v", sent, cli) + } + if f.calls[0].Run || f.calls[0].Note != "" || f.calls[0].Continuation != "" || f.calls[0].AcceptTree != "" || f.calls[0].AcceptProtectedFailures { + t.Fatalf("Request changes asked for more than the additions: %+v", f.calls[0]) + } + if !v.Committed || v.Revision != 2 || v.Digest != successorDigest || v.MoleculeID != r.ID || v.Canonical.Phase != "scope-waiting" { + t.Fatalf("read-back %+v", v) + } + if scope, _, _ := engine.counts(); scope != 0 || h.(*handler).jobs.busy(r.ID) { + t.Fatal("Request changes approved or started a job") + } + // The detail now waits for approval of the new digest, offered as a new primary. + if d := detailOf(t, h, r.ID); d.View.Decision.Primary == nil || d.View.Decision.Primary.Digest != successorDigest { + t.Fatalf("after the amendment the decision is %+v", d.View.Decision) + } + // The old digest is stale now, for Request changes and approval alike. + refusalOf(t, do(t, h, gatePost("/api/molecules/"+r.ID+"/amend", amendBody(digest))), http.StatusConflict, "stale") + refusalOf(t, do(t, h, gatePost("/api/molecules/"+r.ID+"/scope", `{"digest":"`+digest+`"}`)), http.StatusConflict, "stale") + if f.count() != 1 { + t.Fatalf("a stale request reached the engine: %d calls", f.count()) + } + + // An engine that records nothing is never shown as committed. + lab2, r2, digest2 := gateFixture(t) + h2 := testHandler(t, Options{LabDir: lab2}) + installAmend(t, h2, &fakeAmend{}) + if v := viewOf[AmendView](t, do(t, h2, gatePost("/api/molecules/"+r2.ID+"/amend", amendBody(digest2))), http.StatusOK); v.Committed || v.Digest != "" { + t.Fatalf("no successor read back, yet %+v", v) + } +} + +// Request changes has the gate write protection and a strict body: forged and +// malformed requests, stale digests, retired Molecules, busy Molecules and +// sample data never reach the engine, and nothing in the Lab changes. +func TestUIAmendRefusalsNeverReachTheEngine(t *testing.T) { + lab, r, digest := gateFixture(t) + h := testHandler(t, Options{LabDir: lab}) + f := installAmend(t, h, &fakeAmend{write: true}) + engine := refuseEngine(t, h) + before := snapshot(t, lab) + path := "/api/molecules/" + r.ID + "/amend" + for name, forged := range forgedWrites(path, amendBody(digest)) { + if w := do(t, h, forged); w.Code != http.StatusForbidden { + t.Errorf("%s: %d %s", name, w.Code, w.Body) + } + } + many := make([]string, maxUIAmendAdditions+1) + for i := range many { + many[i] = fmt.Sprintf(`"note %d"`, i) + } + for name, body := range map[string]string{ + "nothing to add": `{"digest":"` + digest + `","addAcceptance":[],"addScope":[]}`, + "approve field": `{"digest":"` + digest + `","addAcceptance":["x"],"approve":true}`, + "run field": `{"digest":"` + digest + `","addAcceptance":["x"],"run":true}`, + "note field": `{"digest":"` + digest + `","addAcceptance":["x"],"note":"n"}`, + "short digest": `{"digest":"abc","addAcceptance":["x"]}`, + "no digest": `{"addAcceptance":["x"]}`, + "blank note": `{"digest":"` + digest + `","addAcceptance":[" "]}`, + "control note": `{"digest":"` + digest + `","addAcceptance":["a\u0007"]}`, + "long note": `{"digest":"` + digest + `","addAcceptance":["` + strings.Repeat("n", maxUIAmendText+1) + `"]}`, + "too many": `{"digest":"` + digest + `","addAcceptance":[` + strings.Join(many, ",") + `]}`, + "traversal": `{"digest":"` + digest + `","addScope":[{"path":"../x.go","reason":"r"}]}`, + "absolute": `{"digest":"` + digest + `","addScope":[{"path":"/etc/x","reason":"r"}]}`, + "git dir": `{"digest":"` + digest + `","addScope":[{"path":"a/.git/config","reason":"r"}]}`, + "option path": `{"digest":"` + digest + `","addScope":[{"path":"-x","reason":"r"}]}`, + "no reason": `{"digest":"` + digest + `","addScope":[{"path":"lab/x.go","reason":""}]}`, + "duplicate path": `{"digest":"` + digest + `","addScope":[{"path":"lab/x.go","reason":"r"},{"path":"lab/x.go","reason":"r"}]}`, + "scope extra": `{"digest":"` + digest + `","addScope":[{"path":"lab/x.go","reason":"r","evidence":["issue"]}]}`, + "trailing data": amendBody(digest) + `{}`, + "oversized": `{"digest":"` + digest + `"` + strings.Repeat(" ", maxGateBody) + `,"addAcceptance":["x"]}`, + "form body": "digest=" + digest, + } { + q := gatePost(path, body) + if name == "form body" { + q.header["Content-Type"] = "application/x-www-form-urlencoded" + } + if w := do(t, h, q); w.Code != http.StatusBadRequest { + t.Errorf("%s: %d %s", name, w.Code, w.Body) + } + } + refusalOf(t, do(t, h, gatePost(path, amendBody(strings.Repeat("0", 64)))), http.StatusConflict, "stale") + if w := do(t, h, gatePost("/api/molecules/absent/amend", amendBody(digest))); w.Code != http.StatusNotFound { + t.Errorf("absent Molecule: %d", w.Code) + } + if after := snapshot(t, lab); after != before { + t.Fatalf("refused requests changed the Lab:\n%s\n---\n%s", before, after) + } + if f.count() != 0 || engine.calls() != 0 { + t.Fatalf("refused requests reached the engine: %d amend, %d gate calls", f.count(), engine.calls()) + } + + // A running gate job for the Molecule keeps Request changes out. + block := make(chan struct{}) + engine.scope = func(ctx context.Context, _ molecule.Options) error { + select { + case <-block: + case <-ctx.Done(): + } + return nil + } + jobOf(t, do(t, h, gatePost("/api/molecules/"+r.ID+"/scope", `{"digest":"`+digest+`"}`)), http.StatusAccepted) + refusalOf(t, do(t, h, gatePost(path, amendBody(digest))), http.StatusConflict, "busy") + close(block) + waitJob(t, h, r.ID) + + // A retired Molecule is refused before the engine. + retired := r + retired.Status = molecule.StatusRetired + retired.Retired = &molecule.Retirement{At: "2026-10-26T20:00:00Z", Reason: "stale", PriorStatus: "waiting"} + writeRecord(t, lab, retired) + if w := do(t, h, gatePost(path, amendBody(digest))); w.Code != http.StatusConflict || !strings.Contains(w.Body.String(), `"category":"retired"`) { + t.Fatalf("retired: %d %s", w.Code, w.Body) + } + if f.count() != 0 { + t.Fatalf("a busy or retired request reached the engine %d times", f.count()) + } + + demo := testHandler(t, Options{Demo: true}) + installAmend(t, demo, f) + refusalOf(t, do(t, demo, gatePost("/api/molecules/demo-101/amend", amendBody(digest))), http.StatusNotFound, "sample") + if f.count() != 0 { + t.Fatal("sample data reached the engine") + } +} + +// Engine refusals are served as fixed categories with actionable reasons; the +// engine's text, which can name local paths, is never served. +func TestUIAmendEngineRefusalsShowFixedCategories(t *testing.T) { + for text, want := range map[string]struct { + code int + category string + }{ + "lab/x/new.go is already in scope": {http.StatusConflict, "duplicate"}, + "acceptance item already present: x": {http.StatusConflict, "duplicate"}, + `unsafe repository path: "../x"`: {http.StatusUnprocessableEntity, "scope-refused"}, + "new path x needs evidence for its parent component: no frozen ...": {http.StatusUnprocessableEntity, "scope-refused"}, + readiness.InScopeDefinitionMessage: {http.StatusUnprocessableEntity, "scope-refused"}, + "coding checkout unavailable; refresh intake": {http.StatusConflict, "checkout"}, + "Molecule is locked: another approval, amend or recover holds it": {http.StatusConflict, "locked"}, + "something else at /Users/someone/lab/ledgers/x.json": {http.StatusUnprocessableEntity, "refused"}, + } { + lab, r, digest := gateFixture(t) + h := testHandler(t, Options{LabDir: lab}) + f := installAmend(t, h, &fakeAmend{err: errors.New(text)}) + w := do(t, h, gatePost("/api/molecules/"+r.ID+"/amend", amendBody(digest))) + refusalOf(t, w, want.code, want.category) + if strings.Contains(w.Body.String(), text) || f.count() != 1 { + t.Errorf("%q: served %s after %d calls", text, w.Body, f.count()) + } + } + lab, r, digest := gateFixture(t) + h := testHandler(t, Options{LabDir: lab}) + installAmend(t, h, &fakeAmend{err: fmt.Errorf("wrapped: %w", molecule.ErrMoleculeRetired)}) + if w := do(t, h, gatePost("/api/molecules/"+r.ID+"/amend", amendBody(digest))); w.Code != http.StatusConflict || !strings.Contains(w.Body.String(), `"category":"retired"`) { + t.Fatalf("engine retired refusal: %d %s", w.Code, w.Body) + } + installAmend(t, h, &fakeAmend{err: context.DeadlineExceeded}) + refusalOf(t, do(t, h, gatePost("/api/molecules/"+r.ID+"/amend", amendBody(digest))), http.StatusServiceUnavailable, "interrupted") +} + +// A plan revision recorded after the UI checked the shown digest, but before the +// engine call, is refused by molecule.AmendFromLatest under its lock and served as +// stale, saying a newer plan exists; nothing more is recorded. +func TestUIAmendRefusesALedgerAdvancedBeforeTheEngine(t *testing.T) { + lab, r, digest := gateFixture(t) + h := testHandler(t, Options{LabDir: lab}) + var sent molecule.AmendRequest + h.(*handler).amend = func(ctx context.Context, lab, id string, req molecule.AmendRequest) (molecule.Record, error) { + sent = req + // Another command records a successor between the UI check and this call. + advanced, err := molecule.ReadRecord(lab, id) + if err != nil { + return advanced, err + } + latest := advanced.Readiness.Plans[len(advanced.Readiness.Plans)-1] + next := latest.Plan + next.Revision, next.ParentDigest = latest.Plan.Revision+1, latest.Digest + advanced.Readiness.Plans = append(advanced.Readiness.Plans, readiness.Proposal{Plan: next, Digest: successorDigest}) + if err := saveRecord(lab, advanced); err != nil { + return advanced, err + } + return molecule.AmendFromLatest(ctx, lab, id, req) + } + w := do(t, h, gatePost("/api/molecules/"+r.ID+"/amend", amendBody(digest))) + refusalOf(t, w, http.StatusConflict, "stale") + if !strings.Contains(w.Body.String(), "a newer plan exists") || sent.ExpectParent != digest { + t.Fatalf("stale race served %s for expected parent %q", w.Body, sent.ExpectParent) + } + after, err := molecule.ReadRecord(lab, r.ID) + if err != nil { + t.Fatal(err) + } + if n := len(after.Readiness.Plans); n != 2 || after.Readiness.Plans[1].Digest != successorDigest { + t.Fatalf("the stale amend recorded a revision: %d plans", n) + } +} + +// /api/lab lists Request changes, with or without a source checkout. +func TestUIAmendAction(t *testing.T) { + for _, source := range []string{"", "/src/checkout"} { + lab := viewOf[LabView](t, do(t, testHandler(t, Options{LabDir: t.TempDir(), SourceRoot: source}), authed("/api/lab")), http.StatusOK) + if !reflect.DeepEqual(lab.Actions[len(lab.Actions)-1:], []string{AmendAction}) || len(lab.Gates) != 2 { + t.Errorf("source %q: actions %v gates %v", source, lab.Actions, lab.Gates) + } + } +} diff --git a/lab/internal/ui/diffsize.go b/lab/internal/ui/diffsize.go new file mode 100644 index 0000000..8e0c8f8 --- /dev/null +++ b/lab/internal/ui/diffsize.go @@ -0,0 +1,102 @@ +package ui + +import ( + "context" + "sync" + "time" + + "github.com/DanielNetzer/forgecell/lab/internal/molecule" +) + +// Diff size states: measured from the captured tree, nothing captured yet, or +// not measurable now (no workspace, an invalid id or a git failure). +const ( + DiffMeasured = "measured" + DiffNone = "none" + DiffUnavailable = "unavailable" +) + +// DiffSizeView is the "+412 −96 in 4 files" line of the review and decision +// cards: git diff --numstat of the Molecule's captured tree against its base +// commit, computed on demand by the engine and never taken from ledger text. +// Truncated means a bound stopped the count, so the numbers are a lower bound. +type DiffSizeView struct { + State string `json:"state"` + Files int `json:"files,omitempty"` + Added int64 `json:"added,omitempty"` + Removed int64 `json:"removed,omitempty"` + Binary int `json:"binary,omitempty"` + Truncated bool `json:"truncated,omitempty"` +} + +const ( + // maxDiffCache bounds the sizes kept; the oldest is dropped first. + maxDiffCache = 128 + // diffSizeTimeout bounds one measurement inside the server's write timeout. + diffSizeTimeout = 25 * time.Second +) + +// diffCache keeps each measured size by base commit and tree: those object ids +// name the exact content, so a size never goes stale. Failures are not kept. +type diffCache struct { + mu sync.Mutex + entries map[string]molecule.DiffStat + order []string +} + +func newDiffCache() *diffCache { return &diffCache{entries: map[string]molecule.DiffStat{}} } + +func (c *diffCache) get(key string) (molecule.DiffStat, bool) { + c.mu.Lock() + defer c.mu.Unlock() + s, ok := c.entries[key] + return s, ok +} + +func (c *diffCache) put(key string, s molecule.DiffStat) { + c.mu.Lock() + defer c.mu.Unlock() + if _, ok := c.entries[key]; ok { + return + } + for len(c.order) >= maxDiffCache { + delete(c.entries, c.order[0]) + c.order = c.order[1:] + } + c.entries[key] = s + c.order = append(c.order, key) +} + +// diffBase is the base commit the captured tree is measured against: the latest +// plan's frozen base, else the workspace's. +func diffBase(r molecule.Record) string { + if r.Readiness != nil && len(r.Readiness.Plans) > 0 { + if b := r.Readiness.Plans[len(r.Readiness.Plans)-1].Plan.Inputs.BaseCommit; b != "" { + return b + } + } + return r.Workspace.BaseCommit +} + +// diffSize measures the record's captured tree, from the cache when this exact +// base and tree were measured before. Nothing captured, or a capture with no +// change, is DiffNone without running git. +func (h *handler) diffSize(ctx context.Context, r molecule.Record) DiffSizeView { + if r.Capture == nil || r.Capture.Tree == "" || len(r.Capture.Changes) == 0 { + return DiffSizeView{State: DiffNone} + } + base, tree := diffBase(r), r.Capture.Tree + key := base + ":" + tree + s, ok := h.diffs.get(key) + if !ok { + ctx, cancel := context.WithTimeout(ctx, diffSizeTimeout) + defer cancel() + var err error + if s, err = h.numstat(ctx, h.opts.LabDir, r.Workspace.Path, base, tree); err != nil { + // The error is a fixed engine category; it is not served. + return DiffSizeView{State: DiffUnavailable} + } + h.diffs.put(key, s) + } + return DiffSizeView{State: DiffMeasured, Files: s.Files, Added: s.Added, Removed: s.Removed, Binary: s.Binary, Truncated: s.Truncated} +} diff --git a/lab/internal/ui/drive.go b/lab/internal/ui/drive.go index 6716535..49e57bf 100644 --- a/lab/internal/ui/drive.go +++ b/lab/internal/ui/drive.go @@ -332,16 +332,7 @@ func (h *handler) stopMolecule(w http.ResponseWriter, id string) { h.refuseGate(w, http.StatusConflict, "not-running", "the ledger does not show this Molecule running; nothing was signalled") return } - stopped := h.jobs.cancelRunning(id, []string{"scope"}, func(time.Time) bool { return true }) - if !stopped && record.Issue.Number >= 1 && record.Issue.Number <= maxIssueNumber { - key := runKey(record.Issue.Number) - if j, ok := h.jobs.get(key); ok && j.state == "running" { - if owned, found, _ := h.runMolecule(record.Issue.Number, j.started); found && owned == id { - stopped = h.jobs.cancelRunning(key, []string{"run"}, j.started.Equal) - } - } - } - if !stopped { + if runner := h.runner(record, true); runner != RunnerScope && runner != RunnerRun { h.refuseGate(w, http.StatusConflict, "not-owned", "this Lab UI does not run this Molecule's engine call (a loop, another process or the CLI does); nothing was signalled") return } @@ -350,6 +341,126 @@ func (h *handler) stopMolecule(w http.ResponseWriter, id string) { h.json(w, http.StatusAccepted, v) } +// Runners of a running Molecule's engine call, as the Molecule view names them. +const ( + // RunnerScope is a scope approval job of this Lab UI; Stop cancels it. + RunnerScope = "scope" + // RunnerRun is a run job of this Lab UI whose ledger this is; Stop cancels it. + RunnerRun = "run" + // RunnerLoop is this Lab UI's loop, whose current pass works on the Molecule + // (driveLoop.works). One Molecule of a loop is not stopped on its own: the + // loop is. + RunnerLoop = "loop" + // RunnerOther is the command line, another Lab UI or another process. + RunnerOther = "other" +) + +// runner names who runs a running Molecule's engine call. It is the ownership +// check of stopMolecule: with cancel it also cancels the job this Lab UI owns, +// under the same lock as the check; without it, it only reads. +func (h *handler) runner(record molecule.Record, cancel bool) string { + owned := func(key string, kinds []string, owns func(time.Time) bool) bool { + if cancel { + return h.jobs.cancelRunning(key, kinds, owns) + } + return h.jobs.ownsRunning(key, kinds, owns) + } + if owned(record.ID, []string{"scope"}, func(time.Time) bool { return true }) { + return RunnerScope + } + if record.Issue.Number >= 1 && record.Issue.Number <= maxIssueNumber { + key := runKey(record.Issue.Number) + if j, ok := h.jobs.get(key); ok && j.state == "running" { + if id, found, _ := h.runMolecule(record.Issue.Number, j.started); found && id == record.ID && owned(key, []string{"run"}, j.started.Equal) { + return RunnerRun + } + } + } + if h.loop.works(record) { + return RunnerLoop + } + return RunnerOther +} + +// loopPass is what this UI's loop pass works on: for each Issue the pass +// dispatches, keyed by issueKey, the Molecules it leaves alone. The engine starts +// a Molecule only for an Issue with none and advances only one waiting at the +// scope gate; every other Molecule of a dispatched Issue it only reports. +type loopPass map[string]map[string]bool + +func issueKey(repo string, number int64) string { + return strings.ToLower(repo) + "#" + strconv.FormatInt(number, 10) +} + +// observePass records the pass about to dispatch eligible. Ledgers that existed +// when it listed and were not at the scope gate are left alone. An incomplete +// ledger inventory records no pass, so no Molecule is named the loop's. +func (h *handler) observePass(eligible []intake.OpenIssue) { + list := h.list(ledgerETag(h.opts.LabDir)) + var pass loopPass + if list.Complete && !list.Truncated { + pass = loopPass{} + for _, issue := range eligible { + pass[issueKey(issue.Repository, issue.Number)] = map[string]bool{} + } + for _, m := range list.Molecules { + if alone, ok := pass[issueKey(m.Repository, m.Number)]; ok && m.Phase != "scope-waiting" { + alone[m.ID] = true + } + } + } + h.loop.mu.Lock() + h.loop.pass = pass + h.loop.mu.Unlock() +} + +// works reports whether this UI's loop is running and its current pass works on +// record: a Molecule it started for a dispatched Issue, or one it found waiting +// at the scope gate. A running loop alone never makes a Molecule its own. +func (l *driveLoop) works(record molecule.Record) bool { + l.mu.Lock() + defer l.mu.Unlock() + if !l.running { + return false + } + alone, ok := l.pass[issueKey(record.Issue.Repo, record.Issue.Number)] + return ok && !alone[record.ID] +} + +// ControlView is what the Molecule view may do with a running Molecule. +// Stoppable is true only when this Lab UI runs its engine call; otherwise Text +// names the runner in plain words and the view offers no Stop. +type ControlView struct { + Stoppable bool `json:"stoppable"` + Runner string `json:"runner"` + Text string `json:"text,omitempty"` +} + +// runningWork is the plain words of what a running Molecule is doing at step. +var runningWork = map[int]string{StepPlan: "Its plan is being written", StepCoding: "Its coding is running", StepChecks: "Its checks are running", StepReview: "Its review is running", StepDraft: "Its draft PR is being prepared"} + +// control is the stop control of a running Molecule at step, or nil when the +// ledger does not show it running. It only reads. +func (h *handler) control(record molecule.Record, step int) *ControlView { + if !active(record.Status) { + return nil + } + c := &ControlView{Runner: h.runner(record, false)} + work, ok := runningWork[step] + if !ok { + work = "It is running" + } + switch c.Runner { + case RunnerScope, RunnerRun: + c.Stoppable = true + case RunnerLoop: + c.Text = "Run by this Lab's loop. " + work + "; stop the loop to stop it. Nothing needs you." + default: + c.Text = "Started from the command line or another Lab UI. " + work + "; nothing needs you." + } + return c +} + var ( errLoopBusy = errors.New("a loop is running in this Lab UI") errRunsBusy = errors.New("a run is running in this Lab UI") @@ -386,6 +497,8 @@ type driveLoop struct { // freed, a test seam, runs once a loop's slot reads as free, before done // closes; the Lab's marker must already be released by then. freed func() + // pass is what the running loop's current pass works on (observePass). + pass loopPass } // start refuses while runsBusy reports a UI run job running; the check and the @@ -417,7 +530,7 @@ func (l *driveLoop) start(o molecule.LoopOptions, runsBusy func() bool, run func // The stop request closes Drain only; Work is cancelled only with the server. o.Drain, o.Work, o.Marker = drained, ctx, marker l.running, l.drain, l.drained, l.cancel, l.done = true, false, drained, cancel, done - l.selector, l.max, l.report, l.token = o.Select.String(), o.MaxConcurrent, nil, marker.Token() + l.selector, l.max, l.report, l.token, l.pass = o.Select.String(), o.MaxConcurrent, nil, marker.Token(), nil l.state, l.category, l.message, l.reason = "running", "", "", "" freed := l.freed go func() { @@ -433,7 +546,7 @@ func (l *driveLoop) start(o molecule.LoopOptions, runsBusy func() bool, run func // never meets this loop's own marker. marker.Release() l.mu.Lock() - l.running, l.report = false, &report + l.running, l.report, l.pass = false, &report, nil l.state, l.category, l.message, l.reason = state, category, message, reason l.mu.Unlock() if freed != nil { @@ -508,6 +621,17 @@ func (h *handler) startLoop(w http.ResponseWriter, req loopRequest) { return } o := molecule.LoopOptions{Template: molecule.Options{LabDir: h.opts.LabDir, SourceRoot: h.opts.SourceRoot, Resources: h.opts.Resources}, Select: selector, MaxConcurrent: req.MaxConcurrent} + // Each pass lists first, so the listing marks which Molecules it works on. A + // failed or refused listing dispatches nothing, and records an empty pass. + o.ListIssues = func(ctx context.Context, repository string) (intake.OpenIssues, error) { + listed, err := h.listIssues(ctx, repository, selector) + var eligible []intake.OpenIssue + if err == nil && (!listed.Truncated || selector.Kind == "" || selector.Kind == intake.SelectOldest) { + eligible = selector.Apply(listed.Issues) + } + h.observePass(eligible) + return listed, err + } switch err := h.loop.start(o, func() bool { return h.jobs.running("run") }, h.runLoop); { case errors.Is(err, errLoopBusy): h.refuseGate(w, http.StatusConflict, "busy", "a loop is already running in this Lab UI; this submission was not started") diff --git a/lab/internal/ui/drive_test.go b/lab/internal/ui/drive_test.go index c4647c5..2adf674 100644 --- a/lab/internal/ui/drive_test.go +++ b/lab/internal/ui/drive_test.go @@ -28,6 +28,8 @@ type fakeDrive struct { loops []molecule.LoopOptions run func(context.Context, molecule.Options) error loop func(context.Context, molecule.LoopOptions) (molecule.LoopReport, error) + // issues is what the loop's open Issue listing returns. + issues []intake.OpenIssue } func (d *fakeDrive) calls() int { @@ -38,6 +40,11 @@ func (d *fakeDrive) calls() int { func installDrive(h http.Handler, d *fakeDrive) *fakeDrive { hh := h.(*handler) + hh.listIssues = func(context.Context, string, intake.Selector) (intake.OpenIssues, error) { + d.mu.Lock() + defer d.mu.Unlock() + return intake.OpenIssues{Issues: append([]intake.OpenIssue(nil), d.issues...)}, nil + } hh.runIssue = func(ctx context.Context, o molecule.Options) (molecule.Record, error) { d.mu.Lock() d.runs = append(d.runs, o) @@ -399,6 +406,11 @@ func TestUIDriveStopsOnlyAMoleculeThisUIRuns(t *testing.T) { finished <- saveRecord(lab, r) return nil }}) + // The CLI's running Molecule offers no Stop: the view names its runner. + wantControl(t, h, "mol-9-cli", false, RunnerOther, "Started from the command line or another Lab UI. ") + if d := detailOf(t, h, base.ID); d.View.Control != nil { + t.Fatalf("a Molecule that is not running has a stop control %+v", d.View.Control) + } // The crash leftover and the CLI's ledger are refused before any job runs. refusalOf(t, do(t, h, gatePost("/api/molecules/mol-9-cli/stop", `{}`)), http.StatusConflict, "not-owned") refusalOf(t, do(t, h, gatePost("/api/molecules/mol-1-none/stop", `{}`)), http.StatusNotFound, "missing") @@ -412,6 +424,10 @@ func TestUIDriveStopsOnlyAMoleculeThisUIRuns(t *testing.T) { writeRecord(t, lab, intakeRecord(base, "mol-42-old", 42, "2000-01-01T00:00:00Z", "running", "")) refusalOf(t, do(t, h, gatePost("/api/molecules/mol-42-old/stop", `{}`)), http.StatusConflict, "not-owned") refusalOf(t, do(t, h, gatePost("/api/molecules/mol-9-cli/stop", `{}`)), http.StatusConflict, "not-owned") + // The view offers Stop for this UI's run only, and reading it signals nothing. + wantControl(t, h, "mol-42-live", true, RunnerRun, "") + wantControl(t, h, "mol-42-old", false, RunnerOther, "Started from the command line or another Lab UI. ") + wantControl(t, h, "mol-9-cli", false, RunnerOther, "Started from the command line or another Lab UI. ") select { case <-cancelled: t.Fatal("a refused stop signalled the run") @@ -435,6 +451,101 @@ func TestUIDriveStopsOnlyAMoleculeThisUIRuns(t *testing.T) { } } +// wantControl checks the Molecule view's stop control for id. +func wantControl(t *testing.T, h http.Handler, id string, stoppable bool, runner, textPrefix string) { + t.Helper() + c := detailOf(t, h, id).View.Control + if c == nil || c.Stoppable != stoppable || c.Runner != runner || !strings.HasPrefix(c.Text, textPrefix) || (stoppable && c.Text != "") || (!stoppable && !strings.HasSuffix(c.Text, "nothing needs you.") && !strings.HasSuffix(c.Text, "Nothing needs you.")) { + t.Fatalf("%s: control %+v, want stoppable=%t runner %q", id, c, stoppable, runner) + } +} + +// A Molecule the UI loop's pass works on offers no Stop of its own: the view says +// the loop runs it and that the loop is what to stop. A running loop alone never +// makes a Molecule its own: one the command line runs, for an Issue the loop +// listed or not, is named as run from the command line. +func TestUIDriveLoopMoleculeOffersNoStop(t *testing.T) { + lab, base := labFixture(t) + running, release := make(chan struct{}), make(chan struct{}) + gate := intakeRecord(base, "mol-6-gate", 6, now(), "waiting", "scope-waiting") + cli := intakeRecord(base, "mol-8-cli", 8, now(), "running", "coding") + other := intakeRecord(base, "mol-9-cli", 9, now(), "running", "coding") + issues := []intake.OpenIssue{{Repository: "O/R", Number: 5}, {Repository: "o/r", Number: 6}, {Repository: "o/r", Number: 8}} + h, _ := driveHandler(t, lab, guardWith(64<<30), &fakeDrive{issues: issues, loop: func(ctx context.Context, o molecule.LoopOptions) (molecule.LoopReport, error) { + _, err := o.ListIssues(ctx, "o/r") + // The pass starts a Molecule for Issue 5 and approves the one at the gate. + approved := gate + approved.Status, approved.Readiness = "running", &readiness.State{SchemaVersion: "v1", Phase: "coding"} + if err == nil { + err = saveRecord(lab, intakeRecord(base, "mol-5-loop", 5, now(), "running", "coding")) + } + if err == nil { + err = saveRecord(lab, approved) + } + close(running) + select { + case <-release: + case <-ctx.Done(): + } + return molecule.LoopReport{}, err + }}) + for _, r := range []molecule.Record{gate, cli, other} { + writeRecord(t, lab, r) + } + wantControl(t, h, "mol-8-cli", false, RunnerOther, "Started from the command line or another Lab UI. ") + viewOf[LoopView](t, do(t, h, gatePost("/api/loop/start", `{"select":"oldest","maxConcurrent":1}`)), http.StatusAccepted) + <-running + for _, id := range []string{"mol-5-loop", "mol-6-gate"} { + wantControl(t, h, id, false, RunnerLoop, "Run by this Lab's loop. ") + refusalOf(t, do(t, h, gatePost("/api/molecules/"+id+"/stop", `{}`)), http.StatusConflict, "not-owned") + } + // The loop runs, on other Molecules; the CLI's are still the command line's. + wantControl(t, h, "mol-8-cli", false, RunnerOther, "Started from the command line or another Lab UI. ") + wantControl(t, h, "mol-9-cli", false, RunnerOther, "Started from the command line or another Lab UI. ") + close(release) + waitLoop(t, h) + wantControl(t, h, "mol-5-loop", false, RunnerOther, "Started from the command line or another Lab UI. ") +} + +// A pass whose ledger inventory or listing cannot say what it works on names no +// Molecule the loop's, and a new pass replaces the last one. +func TestUIDriveLoopPassFailsSafe(t *testing.T) { + lab, base := labFixture(t) + h, _ := driveHandler(t, lab, nil, &fakeDrive{}) + hh := h.(*handler) + r := intakeRecord(base, "mol-5-loop", 5, now(), "running", "coding") + five := []intake.OpenIssue{{Repository: "o/r", Number: 5}} + hh.loop.running = true + hh.observePass(five) + writeRecord(t, lab, r) + if !hh.loop.works(r) { + t.Fatal("a Molecule new to a dispatched Issue is not the loop's") + } + hh.loop.running = false + if hh.loop.works(r) { + t.Fatal("a stopped loop still works on a Molecule") + } + hh.loop.running = true + hh.observePass(nil) + if hh.loop.works(r) { + t.Fatal("a Molecule of an earlier pass is still the loop's") + } + // A later pass finds it running and only reports it. + hh.observePass(five) + if hh.loop.works(r) { + t.Fatal("a running Molecule a pass found is the loop's") + } + // An incomplete inventory records no pass. + if err := os.WriteFile(filepath.Join(lab, "ledgers", "mol-4-bad.json"), []byte("{"), 0o600); err != nil { + t.Fatal(err) + } + hh.observePass(five) + if hh.loop.pass != nil || hh.loop.works(r) { + t.Fatalf("an incomplete inventory recorded pass %+v", hh.loop.pass) + } + hh.loop.running = false +} + func TestUIDriveStopsAScopeApprovalJob(t *testing.T) { lab, r, digest := gateFixture(t) h, _ := driveHandler(t, lab, nil, &fakeDrive{}) @@ -449,6 +560,10 @@ func TestUIDriveStopsAScopeApprovalJob(t *testing.T) { // The approved Molecule is coding. r.Status = "running" writeRecord(t, lab, r) + wantControl(t, h, r.ID, true, RunnerScope, "") + if s, _ := h.(*handler).jobs.get(r.ID); s.state != "running" { + t.Fatalf("reading the stop control changed the scope job %+v", s) + } if v := viewOf[StopView](t, do(t, h, gatePost("/api/molecules/"+r.ID+"/stop", `{}`)), http.StatusAccepted); !v.Requested || v.Committed || v.Canonical.Status != "running" { t.Fatalf("stop %+v", v) } diff --git a/lab/internal/ui/estimate.go b/lab/internal/ui/estimate.go new file mode 100644 index 0000000..997d098 --- /dev/null +++ b/lab/internal/ui/estimate.go @@ -0,0 +1,159 @@ +package ui + +import ( + "fmt" + "math" + "sort" + + "github.com/DanielNetzer/forgecell/lab/internal/molecule" +) + +// Estimates ("usually 30–45 min", "~$4") come only from this Lab's own +// history: the median and interquartile range of the active Atom time (the +// analytics activeMS, not wall time including time waiting for you) and the +// reported cost of the most recent comparable finished Molecules. Comparable +// means the same repository and Formula id, in the Finished lane (a published +// draft receipt) and with its active time recorded; the Decision, Running, +// Blocked, Other and Retired lanes never count. An estimate is hidden below +// minEstimateBasis Molecules, always names its basis, and is never zero-filled: +// cost uses only attempts that reported one and says when coverage is partial. +const ( + // estimateWindow is how many of the most recent comparable Molecules count. + estimateWindow = 20 + // minEstimateBasis is the fewest comparable Molecules an estimate needs. + minEstimateBasis = 5 +) + +// EstimateView is the estimate shown beside a decision. Shown is false, with +// Time and Cost absent, when the basis is too small. +type EstimateView struct { + Shown bool `json:"shown"` + Molecules int `json:"molecules"` + Basis string `json:"basis"` + Time *TimeRangeView `json:"time,omitempty"` + Cost *CostRangeView `json:"cost,omitempty"` + Note string `json:"note"` +} + +// TimeRangeView is the active-time quartiles in milliseconds: Low is the first +// quartile, High the third. +type TimeRangeView struct { + LowMS int64 `json:"lowMs"` + MedianMS int64 `json:"medianMs"` + HighMS int64 `json:"highMs"` +} + +// CostRangeView is the reported-cost quartiles in USD over the Molecules that +// reported a cost. Partial means some attempts of the basis reported none, so +// the cost undercounts them; Coverage says how many. +type CostRangeView struct { + LowUSD float64 `json:"lowUsd"` + MedianUSD float64 `json:"medianUsd"` + HighUSD float64 `json:"highUsd"` + Molecules int `json:"molecules"` + Partial bool `json:"partial"` + Coverage string `json:"coverage"` +} + +// estimateNote is said with every shown estimate: it is history, not a promise. +const estimateNote = "From this Lab's own history; not a promise." + +// comparableTo reports a Summary that counts toward an estimate for a Molecule +// of repo and formulaID, other than self. +func comparableTo(s Summary, f *moleculeFacts, repo, formulaID, self string) bool { + return s.ID != self && s.Repository == repo && s.FormulaID == formulaID && repo != "" && formulaID != "" && f != nil && f.activeOK && f.activeMS >= 0 && s.Lane == LaneFinished +} + +// estimateBasis is the basis label of an estimate over n Molecules. +func estimateBasis(n int) string { + return fmt.Sprintf("based on your last %d finished Molecules · active time, not time waiting for you", n) +} + +// estimate derives the estimate for self from one inventory. It only reads inv. +func estimate(inv inventory, repo, formulaID, self string) EstimateView { + type sample struct { + id string + end int64 + facts *moleculeFacts + } + var all []sample + for _, s := range inv.list.Molecules { + f := inv.facts[s.ID] + if comparableTo(s, f, repo, formulaID, self) { + // Recency is the finish time; one without a recorded wall time sorts oldest. + end := int64(math.MinInt64) + if f.wallOK { + end = f.wallEnd + } + all = append(all, sample{id: s.ID, end: end, facts: f}) + } + } + sort.Slice(all, func(i, j int) bool { + if all[i].end != all[j].end { + return all[i].end > all[j].end + } + return all[i].id < all[j].id + }) + if len(all) > estimateWindow { + all = all[:estimateWindow] + } + n := len(all) + if n < minEstimateBasis { + return EstimateView{Molecules: n, Note: fmt.Sprintf("Estimates appear once this Lab has published drafts for %d comparable Molecules.", minEstimateBasis)} + } + active := make([]float64, 0, n) + var costs []float64 + attempts, reported := 0, 0 + for _, s := range all { + active = append(active, float64(s.facts.activeMS)) + sum, seen := 0.0, false + for _, a := range s.facts.attempts { + attempts++ + if a.cost == nil || math.IsNaN(*a.cost) || math.IsInf(*a.cost, 0) || *a.cost < 0 { + continue + } + reported++ + sum, seen = sum+*a.cost, true + } + if seen && !math.IsInf(sum, 0) { + costs = append(costs, sum) + } + } + v := EstimateView{Shown: true, Molecules: n, Basis: estimateBasis(n), Note: estimateNote} + low, mid, high := quartiles(active) + v.Time = &TimeRangeView{LowMS: int64(math.Round(low)), MedianMS: int64(math.Round(mid)), HighMS: int64(math.Round(high))} + if len(costs) >= minEstimateBasis { + low, mid, high := quartiles(costs) + v.Cost = &CostRangeView{LowUSD: low, MedianUSD: mid, HighUSD: high, Molecules: len(costs), Partial: reported < attempts || len(costs) < n, + Coverage: fmt.Sprintf("cost reported for %d of %d harness attempts", reported, attempts)} + } + return v +} + +// quartiles is the first quartile, median and third quartile of values by linear +// interpolation between closest ranks. values is not empty. +func quartiles(values []float64) (float64, float64, float64) { + sorted := append([]float64(nil), values...) + sort.Float64s(sorted) + at := func(q float64) float64 { + pos := q * float64(len(sorted)-1) + lo := int(math.Floor(pos)) + hi := int(math.Ceil(pos)) + return sorted[lo] + (sorted[hi]-sorted[lo])*(pos-float64(lo)) + } + return at(0.25), at(0.5), at(0.75) +} + +// StatsView is GET /api/molecules/{id}/stats: the diff size and the estimate a +// decision card shows. Both are read-only; the diff size runs git diff +// --numstat on demand through the engine, and the estimate reads the same +// bounded inventory as /api/molecules. +type StatsView struct { + Diff DiffSizeView `json:"diff"` + Estimate EstimateView `json:"estimate"` +} + +func (h *handler) stats(r molecule.Record) StatsView { + inv := h.inventory(ledgerETag(h.opts.LabDir)) + return StatsView{Diff: h.diffSize(h.jobs.ctx, r), Estimate: estimate(inv, r.Issue.Repo, r.FormulaID, r.ID)} +} diff --git a/lab/internal/ui/estimate_test.go b/lab/internal/ui/estimate_test.go new file mode 100644 index 0000000..a980643 --- /dev/null +++ b/lab/internal/ui/estimate_test.go @@ -0,0 +1,255 @@ +package ui + +import ( + "context" + "errors" + "fmt" + "math" + "net/http" + "strings" + "sync" + "testing" + + "github.com/DanielNetzer/forgecell/lab/internal/molecule" + "github.com/DanielNetzer/forgecell/lab/internal/verification" +) + +// estimateEpoch is a fixed time, so no estimate test reads the clock. +const estimateEpoch = int64(1_790_000_000_000) + +// historyEntry is one Molecule of a fixture history: minutes of active Atom +// time; its wall time also holds a long wait for the human at a gate. +type historyEntry struct { + id, repo, formula, lane string + minutes int64 + costs []*float64 + noActive bool +} + +func usdPtr(v float64) *float64 { return &v } + +// waitMinutes is the human wait each fixture's wall time adds to its active time. +const waitMinutes = 600 + +// historyInventory builds an inventory whose i-th entry finished i hours after +// the epoch, so later entries are more recent. +func historyInventory(entries []historyEntry) inventory { + inv := inventory{facts: map[string]*moleculeFacts{}} + for i, e := range entries { + end := estimateEpoch + int64(i+1)*3_600_000 + s := Summary{ID: e.id, Repository: e.repo, FormulaID: e.formula, Lane: e.lane, StartedAt: "set", FinishedAt: "set"} + f := &moleculeFacts{wallOK: true, wallStart: end - (e.minutes+waitMinutes)*60_000, wallEnd: end, activeOK: !e.noActive, activeMS: e.minutes * 60_000} + for _, c := range e.costs { + f.attempts = append(f.attempts, attemptFact{cost: c}) + } + inv.list.Molecules = append(inv.list.Molecules, s) + inv.facts[e.id] = f + } + return inv +} + +func comparableEntries(n int, minutes func(int) int64, costs func(int) []*float64) []historyEntry { + var out []historyEntry + for i := 1; i <= n; i++ { + out = append(out, historyEntry{id: fmt.Sprintf("m%02d", i), repo: "o/r", formula: "f", lane: LaneFinished, minutes: minutes(i), costs: costs(i)}) + } + return out +} + +func near(a, b float64) bool { return math.Abs(a-b) < 1e-9 } + +// With enough history the estimate is the interquartile range and median of the +// comparable Molecules' active Atom time (never their wall time, which holds the +// wait for you) and reported cost, labelled with its basis and never a promise. +func TestEstimateWithEnoughHistory(t *testing.T) { + entries := comparableEntries(12, func(i int) int64 { return int64(i) * 10 }, func(i int) []*float64 { return []*float64{usdPtr(float64(i))} }) + v := estimate(historyInventory(entries), "o/r", "f", "self") + if !v.Shown || v.Molecules != 12 || v.Basis != "based on your last 12 finished Molecules · active time, not time waiting for you" || v.Note != estimateNote { + t.Fatalf("estimate %+v", v) + } + // Active minutes 10..120: quartiles 37.5, 65 and 92.5 minutes. + if v.Time == nil || v.Time.LowMS != 2_250_000 || v.Time.MedianMS != 3_900_000 || v.Time.HighMS != 5_550_000 { + t.Fatalf("time %+v", v.Time) + } + if c := v.Cost; c == nil || !near(c.LowUSD, 3.75) || !near(c.MedianUSD, 6.5) || !near(c.HighUSD, 9.25) || c.Partial || c.Molecules != 12 || c.Coverage != "cost reported for 12 of 12 harness attempts" { + t.Fatalf("cost %+v", v.Cost) + } + // More recent Decision-lane and Other-lane Molecules, far slower, change nothing. + mixed := append([]historyEntry(nil), entries...) + for i := range 6 { + mixed = append(mixed, historyEntry{id: fmt.Sprintf("d%02d", i), repo: "o/r", formula: "f", lane: LaneDecision, minutes: 5000, costs: []*float64{usdPtr(99)}}, + historyEntry{id: fmt.Sprintf("o%02d", i), repo: "o/r", formula: "f", lane: LaneOther, minutes: 5000, costs: []*float64{usdPtr(99)}}) + } + if m := estimate(historyInventory(mixed), "o/r", "f", "self"); m.Molecules != 12 || *m.Time != *v.Time || *m.Cost != *v.Cost { + t.Fatalf("Decision or Other lane counted: %+v %+v", m, m.Time) + } + // Only the most recent estimateWindow comparable Molecules count: the older + // ones here are far slower and would move the median. + window := comparableEntries(estimateWindow+5, func(i int) int64 { + if i <= 5 { + return 1000 + } + return 10 + }, func(int) []*float64 { return []*float64{usdPtr(1)} }) + if v := estimate(historyInventory(window), "o/r", "f", "self"); v.Molecules != estimateWindow || v.Time.HighMS != 600_000 || v.Basis != estimateBasis(estimateWindow) { + t.Fatalf("window: %+v %+v", v, v.Time) + } +} + +// Fewer than minEstimateBasis comparable Molecules hide the estimate entirely: +// only the Finished lane (a published draft receipt) counts, so Molecules of +// another repository or Formula, in the Decision, Running, Blocked, Other or +// Retired lane, without recorded active time, and the Molecule itself never do. +func TestEstimateHiddenWithTooLittleHistory(t *testing.T) { + entries := comparableEntries(minEstimateBasis-1, func(int) int64 { return 30 }, func(int) []*float64 { return []*float64{usdPtr(2)} }) + for _, e := range []historyEntry{ + {id: "other-repo", repo: "o/x", formula: "f", lane: LaneFinished, minutes: 30}, + {id: "other-formula", repo: "o/r", formula: "g", lane: LaneFinished, minutes: 30}, + {id: "decision", repo: "o/r", formula: "f", lane: LaneDecision, minutes: 30}, + {id: "blocked", repo: "o/r", formula: "f", lane: LaneBlocked, minutes: 30}, + {id: "running", repo: "o/r", formula: "f", lane: LaneRunning, minutes: 30}, + {id: "retired", repo: "o/r", formula: "f", lane: LaneRetired, minutes: 30}, + {id: "other", repo: "o/r", formula: "f", lane: LaneOther, minutes: 30}, + {id: "no-active-time", repo: "o/r", formula: "f", lane: LaneFinished, minutes: 30, noActive: true}, + {id: "self", repo: "o/r", formula: "f", lane: LaneFinished, minutes: 30}, + } { + entries = append(entries, e) + } + v := estimate(historyInventory(entries), "o/r", "f", "self") + if v.Shown || v.Molecules != minEstimateBasis-1 || v.Time != nil || v.Cost != nil || v.Basis != "" { + t.Fatalf("estimate shown on too little history: %+v", v) + } + if v := estimate(inventory{facts: map[string]*moleculeFacts{}}, "o/r", "f", "self"); v.Shown || v.Molecules != 0 { + t.Fatalf("empty history: %+v", v) + } + // An unknown repository or Formula id matches nothing. + if v := estimate(historyInventory(comparableEntries(8, func(int) int64 { return 5 }, func(int) []*float64 { return nil })), "", "f", "self"); v.Shown { + t.Fatalf("no repository: %+v", v) + } +} + +// Cost uses only attempts that reported one and says when coverage is partial; +// with fewer than minEstimateBasis Molecules reporting a cost, only the time is +// shown. A missing cost is never zero-filled. +func TestEstimatePartialCostCoverageIsLabelled(t *testing.T) { + entries := comparableEntries(6, func(int) int64 { return 20 }, func(i int) []*float64 { return []*float64{usdPtr(float64(i)), nil} }) + v := estimate(historyInventory(entries), "o/r", "f", "self") + if c := v.Cost; !v.Shown || c == nil || !c.Partial || c.Molecules != 6 || c.Coverage != "cost reported for 6 of 12 harness attempts" || !near(c.MedianUSD, 3.5) { + t.Fatalf("partial cost %+v", v.Cost) + } + few := comparableEntries(6, func(int) int64 { return 20 }, func(i int) []*float64 { + if i <= 3 { + return []*float64{usdPtr(9)} + } + return []*float64{nil, usdPtr(math.NaN()), usdPtr(-1)} + }) + v = estimate(historyInventory(few), "o/r", "f", "self") + if !v.Shown || v.Time == nil || v.Cost != nil { + t.Fatalf("cost shown from too few reports: %+v", v) + } + if lo, mid, hi := quartiles([]float64{4}); lo != 4 || mid != 4 || hi != 4 { + t.Fatalf("one value: %v %v %v", lo, mid, hi) + } +} + +// numstatSeam counts diff measurements and returns stat or err. +type numstatSeam struct { + mu sync.Mutex + calls [][4]string + stat molecule.DiffStat + err error +} + +func (s *numstatSeam) install(h http.Handler) { + h.(*handler).numstat = func(_ context.Context, lab, workspace, base, tree string) (molecule.DiffStat, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.calls = append(s.calls, [4]string{lab, workspace, base, tree}) + return s.stat, s.err + } +} + +func (s *numstatSeam) count() int { + s.mu.Lock() + defer s.mu.Unlock() + return len(s.calls) +} + +// GET /api/molecules/{id}/stats measures the captured tree against the plan's +// base through the engine once per tree, serves only the counts, and reports a +// failed measurement as unavailable without caching it. It needs the tab +// session and serves nothing for sample data. +func TestUIStatsMeasuresTheCapturedTreeOncePerTree(t *testing.T) { + lab, r, _ := viewFixture(t) + tree := strings.Repeat("7", 40) + r.Capture = &verification.CaptureResult{Tree: tree, Changes: []string{"lab/internal/ui/web/app.js"}} + writeRecord(t, lab, r) + h := testHandler(t, Options{LabDir: lab}) + seam := &numstatSeam{stat: molecule.DiffStat{Files: 4, Added: 412, Removed: 96, Binary: 1}} + seam.install(h) + path := "/api/molecules/" + r.ID + "/stats" + for range 2 { + v := viewOf[StatsView](t, do(t, h, authed(path)), http.StatusOK) + if v.Diff != (DiffSizeView{State: DiffMeasured, Files: 4, Added: 412, Removed: 96, Binary: 1}) || v.Estimate.Shown { + t.Fatalf("stats %+v", v) + } + } + if seam.count() != 1 || seam.calls[0] != [4]string{lab, r.Workspace.Path, r.Workspace.BaseCommit, tree} { + t.Fatalf("numstat calls %q", seam.calls) + } + // A truncated measurement says so. + h2 := testHandler(t, Options{LabDir: lab}) + (&numstatSeam{stat: molecule.DiffStat{Files: 10000, Added: 1, Truncated: true}}).install(h2) + if v := viewOf[StatsView](t, do(t, h2, authed(path)), http.StatusOK); !v.Diff.Truncated || v.Diff.State != DiffMeasured { + t.Fatalf("truncated %+v", v.Diff) + } + // A failure is unavailable and measured again next time. + h3 := testHandler(t, Options{LabDir: lab}) + failing := &numstatSeam{err: molecule.ErrDiffStatFailed} + failing.install(h3) + for range 2 { + w := do(t, h3, authed(path)) + if v := viewOf[StatsView](t, w, http.StatusOK); v.Diff != (DiffSizeView{State: DiffUnavailable}) { + t.Fatalf("failed measurement %+v", v.Diff) + } + noLeak(t, w.Body.String()) + } + if failing.count() != 2 { + t.Fatalf("a failed measurement was cached: %d calls", failing.count()) + } + // Nothing captured, or no change, runs no git at all. + r.Capture = &verification.CaptureResult{Tree: tree} + writeRecord(t, lab, r) + h4 := testHandler(t, Options{LabDir: lab}) + none := &numstatSeam{err: errors.New("must not run")} + none.install(h4) + if v := viewOf[StatsView](t, do(t, h4, authed(path)), http.StatusOK); v.Diff.State != DiffNone || none.count() != 0 { + t.Fatalf("no change measured: %+v after %d calls", v.Diff, none.count()) + } + // The tab session is required, and sample data has no stats. + noTab := authed(path) + noTab.header = map[string]string{sessionHeader: ""} + if w := do(t, h4, noTab); w.Code != http.StatusForbidden { + t.Fatalf("stats without the tab session: %d", w.Code) + } + if w := do(t, testHandler(t, Options{Demo: true}), authed("/api/molecules/demo-101/stats")); w.Code != http.StatusNotFound { + t.Fatalf("sample stats: %d", w.Code) + } +} + +// The diff cache is bounded: the oldest size is dropped first. +func TestUIDiffCacheIsBounded(t *testing.T) { + c := newDiffCache() + for i := 0; i < maxDiffCache+10; i++ { + c.put(fmt.Sprint(i), molecule.DiffStat{Files: i}) + } + if len(c.entries) != maxDiffCache || len(c.order) != maxDiffCache { + t.Fatalf("cache holds %d", len(c.entries)) + } + if _, ok := c.get("0"); ok { + t.Fatal("the oldest size was kept") + } + if s, ok := c.get(fmt.Sprint(maxDiffCache + 9)); !ok || s.Files != maxDiffCache+9 { + t.Fatalf("the newest size is %+v %v", s, ok) + } +} diff --git a/lab/internal/ui/gates.go b/lab/internal/ui/gates.go index a2b9d1d..d218032 100644 --- a/lab/internal/ui/gates.go +++ b/lab/internal/ui/gates.go @@ -29,6 +29,11 @@ import ( // → delivery.Publish (draft PR) as a job // GET /api/molecules/{id}/job → the job and its canonical read-back // +// Request changes (amend.go) is the same kind of POST and never approves: +// +// POST /api/molecules/{id}/amend {"digest","addAcceptance","addScope"} +// → molecule.AmendFromLatest (no --run); the new revision is approved separately +// // The engine validates every digest, lock and receipt; the UI only refuses // early what the engine would refuse anyway. There is no merge, deployment or // permission route. A job's own outcome never shows a decision as committed: @@ -60,7 +65,7 @@ func gateRoute(path string) (id, action string, ok bool) { } id, action, _ = strings.Cut(rest, "/") switch action { - case "scope", "delivery/preview", "delivery/publish": + case "scope", "amend", "delivery/preview", "delivery/publish": return id, action, true } return "", "", false @@ -149,11 +154,14 @@ func (h *handler) gate(w http.ResponseWriter, r *http.Request, id, action string return } var scope scopeRequest + var amend amendRequest var publish publishRequest var body any = &previewRequest{} switch action { case "scope": body = &scope + case "amend": + body = &amend case "delivery/publish": body = &publish } @@ -174,6 +182,8 @@ func (h *handler) gate(w http.ResponseWriter, r *http.Request, id, action string switch action { case "scope": h.approveScope(w, record, scope) + case "amend": + h.amendPlan(w, record, amend) case "delivery/preview": h.previewDelivery(w, record) default: @@ -386,6 +396,9 @@ var categoryReasons = map[string]string{ "reconciliation": "Check the pull request on GitHub, then reconcile the receipt with forgecell deliver; this page never retries a publication", "demo": "This demo Lab never contacts GitHub; run forgecell init in a Lab of your own repository to publish", "interrupted": "Read the state back here; if the Molecule stays locked, inspect it with forgecell ledger and recover it with forgecell recover", + "duplicate": "Add a path or an acceptance note the plan does not list yet, or approve the plan as it is", + "scope-refused": "Choose an exact path inside a component the plan's evidence covers, or run forgecell amend to see the engine's own reason", + "checkout": "Refresh intake with forgecell run so the Molecule has a coding checkout, then review the new plan", } // engineReasons are exact next steps for engine errors the UI recognises, @@ -657,6 +670,15 @@ func (g *gateJobs) cancelRunning(id string, kinds []string, owns func(time.Time) return true } +// ownsRunning is cancelRunning's check alone: it reports a running job under id +// whose kind is one of kinds and for which owns(started) holds, and cancels nothing. +func (g *gateJobs) ownsRunning(id string, kinds []string, owns func(time.Time) bool) bool { + g.mu.Lock() + defer g.mu.Unlock() + j := g.jobs[id] + return j != nil && j.state == "running" && slices.Contains(kinds, j.kind) && owns(j.started) +} + func (g *gateJobs) busy(id string) bool { g.mu.Lock() defer g.mu.Unlock() diff --git a/lab/internal/ui/gates_test.go b/lab/internal/ui/gates_test.go index e75bccd..95b60f1 100644 --- a/lab/internal/ui/gates_test.go +++ b/lab/internal/ui/gates_test.go @@ -191,7 +191,7 @@ func TestUIGateWriteProtectionRefusesForgedAndMalformedWrites(t *testing.T) { engine := refuseEngine(t, h) before := snapshot(t, lab) scope := `{"digest":"` + digest + `"}` - for _, action := range []string{"scope", "delivery/preview", "delivery/publish"} { + for _, action := range []string{"scope", "amend", "delivery/preview", "delivery/publish"} { for name, forged := range forgedWrites("/api/molecules/"+r.ID+"/"+action, scope) { if w := do(t, h, forged); w.Code != http.StatusForbidden { t.Errorf("%s %s: %d %s", name, action, w.Code, w.Body) @@ -673,11 +673,16 @@ func TestUIGateServedJSSubmitsOnceAndReadsBack(t *testing.T) { "method: 'POST'", "'Content-Type': 'application/json'", "'/scope'", + "'/amend'", "'/delivery/preview'", "'/delivery/publish'", "'/job'", "if (state.gate.pending) return null;", - "disabled: state.gate.pending", + "const busy = gate.pending || running;", + "c.approve.disabled = busy;", + "c.preview.disabled = busy;", + "c.publish.disabled = busy;", + "c.amendSubmit.disabled = busy;", "JSON.stringify({ digest: digest })", "JSON.stringify({ digest: preview.digest, title: title, acceptUnverified: accept })", "readBack(", @@ -687,6 +692,16 @@ func TestUIGateServedJSSubmitsOnceAndReadsBack(t *testing.T) { t.Errorf("app.js lacks %q", want) } } + // An ambiguous response is read back and never resubmitted. + submit := jsFunction(t, js, "submitGate") + for _, want := range []string{"if (!res || res.status >= 500) {", "readBack(gate);", "return null;"} { + if !strings.Contains(submit, want) { + t.Errorf("submitGate lacks %q", want) + } + } + if strings.Contains(submit, "gatePost(gate.id, action, payload);\n res = await gatePost") { + t.Error("submitGate posts twice") + } for _, gone := range []string{"approvals are not available in this UI", "/merge", "/deploy"} { if strings.Contains(js, gone) { t.Errorf("app.js still has %q", gone) diff --git a/lab/internal/ui/inbox.go b/lab/internal/ui/inbox.go index 89edc07..43ad0ba 100644 --- a/lab/internal/ui/inbox.go +++ b/lab/internal/ui/inbox.go @@ -48,7 +48,7 @@ const maxInboxCount = 9999 // Summary stays comparable. Each group of counts is served only with its flag: // Planned for Files and Checks, Verified for Passed of Ran, Captured for // Changed; without the flag it was not recorded, which is never zero. -// StepSince is the current step's start as RFC 3339 UTC, for the browser's +// StepSince is the current attempt's start as RFC 3339 UTC, for the browser's // elapsed time only; it is data, never shown. type InboxView struct { Kind string `json:"kind"` @@ -68,8 +68,11 @@ type InboxView struct { Step int `json:"step"` StepLabel string `json:"stepLabel"` StepSince string `json:"stepSince,omitempty"` - Queued bool `json:"queued,omitempty"` - Merged bool `json:"merged,omitempty"` + // StepNote is a fixed rerun note of the current attempt, "rerun" or "rerun + // after a plan revision", or empty. + StepNote string `json:"stepNote,omitempty"` + Queued bool `json:"queued,omitempty"` + Merged bool `json:"merged,omitempty"` } // inboxView maps one Molecule's lane, phase, Atoms and receipt to its Inbox @@ -80,8 +83,8 @@ func inboxView(x redactor, r molecule.Record, lane, activity string, d *Delivery if lane == LaneRunning && d.Published() { lane = LaneFinished } - step, label, since := progress(r, lane, activity, d) - v := InboxView{Step: step, StepLabel: label, StepSince: since} + step, label, since, note := stepProgress(r, lane, activity, d) + v := InboxView{Step: step, StepLabel: label, StepSince: since, StepNote: note} switch lane { case LaneDecision: if phase(r) == "scope-waiting" { @@ -217,19 +220,33 @@ func atomStep(typ string, coded bool) int { // phaseSteps is the fallback step of a readiness phase when no Atom is current. var phaseSteps = map[string]int{"scope-waiting": StepApproval, "approved": StepCoding, "coding": StepCoding, "check-pending": StepChecks, "review-waiting": StepReview} -// progress is a Molecule's current step, its plain label and when it started. -// A published receipt is the Draft PR step done (Merged when the receipt observed -// a merge); otherwise the first active or waiting Atom decides, then a failed one, -// then the readiness phase. +// Rerun notes of the current attempt. +const ( + rerunNote = "rerun" + rerunRevisionNote = "rerun after a plan revision" +) + +// progress is a Molecule's current step, its plain label and when the current +// attempt of that step started; stepProgress also gives the rerun note. func progress(r molecule.Record, lane, activity string, d *DeliveryState) (int, string, string) { + step, label, since, _ := stepProgress(r, lane, activity, d) + return step, label, since +} + +// stepProgress is a Molecule's current step, its plain label, when the current +// attempt of that step started, and a rerun note. A published receipt is the +// Draft PR step done (Merged when the receipt observed a merge); otherwise the +// first active or waiting Atom decides, then a failed one, then the readiness +// phase. +func stepProgress(r molecule.Record, lane, activity string, d *DeliveryState) (int, string, string, string) { if d.Published() { if d.Lifecycle == "MERGED" { - return StepMerged, "Merged", "" + return StepMerged, "Merged", "", "" } - return StepMerged, "Draft published", "" + return StepMerged, "Draft published", "", "" } if activity == ActivityQueued { - return StepChecks, "Queued for checks", "" + return StepChecks, "Queued for checks", "", "" } coded, failedAt := false, -1 for _, a := range r.Atoms { @@ -240,7 +257,8 @@ func progress(r molecule.Record, lane, activity string, d *DeliveryState) (int, if s < 0 { s = StepDraft } - return s, StepLabels[s], stepSince(a.StartedAt) + since, note := attemptSince(r, a) + return s, StepLabels[s], since, note case "failed", "blocked": if failedAt < 0 && s >= 0 { failedAt = s @@ -248,12 +266,57 @@ func progress(r molecule.Record, lane, activity string, d *DeliveryState) (int, } } if failedAt >= 0 { - return failedAt, StepLabels[failedAt], "" + return failedAt, StepLabels[failedAt], "", "" } if s, ok := phaseSteps[phase(r)]; ok { - return s, StepLabels[s], "" + return s, StepLabels[s], "", "" + } + return StepPlan, StepLabels[StepPlan], "", "" +} + +// attemptSince is when the current attempt of a current Atom started, and its +// rerun note. Coding counts from the latest harness attempt's start recorded on +// the Atom, Checks from the latest verification attempt and Review from the +// latest review pass, never from the Atom's first start, which an earlier +// attempt set. A step with attempts but no recorded start of the latest one +// shows no duration; a step with no attempt recorded yet, and other Atoms, +// which run once, count from the Atom's own start. +func attemptSince(r molecule.Record, a molecule.Atom) (string, string) { + var starts, digests []string + switch a.Type { + case "harness": + recorded := map[string]string{} + if a.Provenance != nil { + for _, act := range a.Provenance.Actions { + recorded[act.AttemptID] = act.StartedAt + } + } + for _, h := range r.HarnessAttempts { + starts, digests = append(starts, recorded[h.ID]), append(digests, h.PlanDigest) + } + case "check": + for _, v := range r.VerificationAttempts { + starts, digests = append(starts, v.StartedAt), append(digests, v.PlanDigest) + } + case "review": + for _, v := range r.ReviewAttempts { + starts, digests = append(starts, v.StartedAt), append(digests, v.PlanDigest) + } + default: + return stepSince(a.StartedAt), "" + } + n := len(starts) + if n == 0 { + return stepSince(a.StartedAt), "" + } + note := "" + if n > 1 { + note = rerunNote + if digests[n-1] != digests[n-2] { + note = rerunRevisionNote + } } - return StepPlan, StepLabels[StepPlan], "" + return stepSince(starts[n-1]), note } // stepSince re-formats a recorded start time, or returns "" for anything that diff --git a/lab/internal/ui/inbox_test.go b/lab/internal/ui/inbox_test.go index 99bcbde..b970d07 100644 --- a/lab/internal/ui/inbox_test.go +++ b/lab/internal/ui/inbox_test.go @@ -68,7 +68,10 @@ func inboxLab(t *testing.T) string { writeRecord(t, lab, blocked) running := inboxRecord(base, "mol-4-running", 4, "running", "coding", inboxAtoms("done", "done", "active")) - running.Atoms[2].StartedAt = "2026-10-26T10:00:00.123+02:00" + // The Atom started earlier; the duration counts from the coding attempt. + running.Atoms[2].StartedAt = "2026-10-26T05:00:00Z" + running.Atoms[2].Provenance = &molecule.AtomProvenance{Actions: []molecule.AtomAction{{AttemptID: "attempt-1", StartedAt: "2026-10-26T10:00:00.123+02:00"}}} + running.HarnessAttempts = []molecule.HarnessAttempt{{ID: "attempt-1", PlanDigest: digest64}} writeRecord(t, lab, running) queued := inboxRecord(base, "mol-5-queued", 5, "running", "check-pending", inboxAtoms("done", "done", "done", "active")) @@ -204,10 +207,14 @@ func TestUIInboxProgressStepsFromAtoms(t *testing.T) { published := &DeliveryState{State: "published", Lifecycle: "OPEN"} mergedReceipt := &DeliveryState{State: "published", Lifecycle: "MERGED"} at := func(statuses ...string) []molecule.Atom { return inboxAtoms(statuses...) } + // With no attempt recorded yet the Atom's own start is the duration; with + // attempts it never is (TestUIProgressCountsFromTheCurrentAttempt). since := at("done", "done", "active") since[2].StartedAt = "2026-10-26T10:00:00Z" bad := at("done", "done", "active") bad[2].StartedAt = "