Efficiency, robustness and Studio: fewer model calls, retries instead of dead ends, a live and linked Studio

cmd/slmcode/cmd_review.go

@@ -30,23 +30,50 @@ type pendingPatch struct {
 	Path    string `json:"path"`
 	Kind    string `json:"kind"`
 	Content string `json:"content"`
+	// From is the source of a ws_mv proposal (stamped by pkg/workspace).
+	From string `json:"from,omitempty"`
+	// TaskID / Agent / QueryID attribute the proposal when known.
+	TaskID  string `json:"task_id,omitempty"`
+	Agent   string `json:"agent,omitempty"`
+	QueryID string `json:"query_id,omitempty"`
 }
 
+// Queue kinds that are not "write Content to Path". See writePatch.
+const (
+	pendingKindDelete = "delete"
+	pendingKindMove   = "mv"
+	pendingKindShell  = "shell"
+)
+
 // abs resolves the target file inside the project root.
 func (p pendingPatch) abs(root string) string { return filepath.Join(root, p.Path) }
 
-// before reads the on-disk content the patch would replace.
+// before reads the on-disk content the patch would replace. A move is diffed
+// against its SOURCE: the destination does not exist yet, and what the
+// reviewer needs to see is that the content is unchanged in transit.
 func (p pendingPatch) before(root string) string {
-	data, err := os.ReadFile(p.abs(root))
+	path := p.abs(root)
+	if p.Kind == pendingKindMove && p.From != "" {
+		path = filepath.Join(root, p.From)
+	}
+	data, err := os.ReadFile(path) //nolint:gosec // project path under root
 	if err != nil {
 		return ""
 	}
 	return string(data)
 }
 
+// after is the content the applier will leave at Path ("" for a delete).
+func (p pendingPatch) after() string {
+	if p.Kind == pendingKindDelete {
+		return ""
+	}
+	return p.Content
+}
+
 // diff computes the unified diff for this patch.
 func (p pendingPatch) diff(root string) cli.FileDiff {
-	fd := cli.Diff(p.Path, p.before(root), p.Content, 3)
+	fd := cli.Diff(p.Path, p.before(root), p.after(), 3)
 	if mode, ok := fileMode(p.abs(root)); ok && mode&0o111 != 0 {
 		fd.ModeNote = fmt.Sprintf("mode %04o", mode)
 	}
@@ -87,17 +114,76 @@ func loadPending(slmDir string) ([]pendingPatch, error) {
 		if json.Unmarshal(data, &p) != nil || strings.TrimSpace(p.Path) == "" {
 			continue
 		}
+		if p.Kind == pendingKindShell {
+			// An older workspace mirrored every shell ask into the queue;
+			// a command is not a file change and must never be "applied".
+			continue
+		}
 		p.File = e.Name()
 		out = append(out, p)
 	}
 	sort.Slice(out, func(i, j int) bool { return out[i].File < out[j].File })
 	return out, nil
 }
 
-// writePatch applies one patch, preserving the existing file mode. A brand new
-// file gets 0o644; an existing executable keeps its +x bits.
+// writePatch applies one patch according to its KIND, preserving the existing
+// file mode for writes. A brand new file gets 0o644; an existing executable
+// keeps its +x bits.
+//
+// Every kind used to be applied as "write Content to Path", which truncated a
+// "deleted" file to zero bytes, copied a moved file while leaving its source
+// behind, and wrote a literal shell.sh for a shell approval.
 func writePatch(root string, p pendingPatch) error {
 	abs := p.abs(root)
+	if !underRoot(root, abs) {
+		return fmt.Errorf("refusing %s: the path resolves outside the project", p.Path)
+	}
+	if p.Kind == pendingKindMove && p.From != "" && !underRoot(root, filepath.Join(root, p.From)) {
+		return fmt.Errorf("refusing %s: the source resolves outside the project", p.From)
+	}
+	switch p.Kind {
+	case pendingKindShell:
+		return errors.New("shell approvals are not file changes; reject the entry instead")
+	case pendingKindDelete:
+		if err := os.Remove(abs); err != nil && !os.IsNotExist(err) {
+			return err
+		}
+		return nil
+	case pendingKindMove:
+		if strings.TrimSpace(p.From) == "" {
+			return errors.New("move entry has no source path")
+		}
+		src := filepath.Join(root, p.From)
+		if err := os.MkdirAll(filepath.Dir(abs), 0o755); err != nil { //nolint:gosec // directory in the user's source tree — conventional 0755, not harness state
+			return err
+		}
+		if _, err := os.Stat(abs); err == nil {
+			return fmt.Errorf("destination %s already exists", p.Path)
+		}
+		if _, err := os.Lstat(src); err != nil {
+			if !os.IsNotExist(err) {
+				return err
+			}
+			// Source gone in the meantime: the intent is the content at the
+			// destination, which was recorded.
+			return os.WriteFile(abs, []byte(p.Content), 0o644) //nolint:gosec // project source file, conventional perms
+		}
+		if err := os.Rename(src, abs); err == nil {
+			return nil
+		}
+		data, err := os.ReadFile(src) //nolint:gosec // project path under root
+		if err != nil {
+			return err
+		}
+		mode := os.FileMode(0o644)
+		if m, ok := fileMode(src); ok {
+			mode = m
+		}
+		if err := os.WriteFile(abs, data, mode); err != nil { //nolint:gosec // abs is checked against root above; a project source file
+			return err
+		}
+		return os.Remove(src)
+	}
 	if err := os.MkdirAll(filepath.Dir(abs), 0o755); err != nil { //nolint:gosec // directory in the user's source tree — conventional 0755, not harness state
 		return err
 	}
@@ -788,3 +874,15 @@ func matchesAnyPrefix(rel string, prefixes []string) bool {
 	}
 	return false
 }
+
+// underRoot reports whether abs lies inside root (lexically: no ".." escape).
+// The pending entries are written by the tool layer, which already jails
+// paths; this is the applier's own guarantee that a hand-edited entry cannot
+// point it elsewhere.
+func underRoot(root, abs string) bool {
+	rel, err := filepath.Rel(root, abs)
+	if err != nil {
+		return false
+	}
+	return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))
+}

cmd/slmcode/cmd_review_kinds_test.go

@@ -0,0 +1,108 @@
+package main
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func writeKindFixture(t *testing.T, slmDir, name string, obj map[string]string) {
+	t.Helper()
+	dir := filepath.Join(slmDir, "pending")
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	body, err := json.Marshal(obj)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(dir, name), body, 0o644); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// `slmcode apply --all` used to write every entry's content to its path:
+// a delete became a 0-byte file, a move left its source, a shell ask became
+// a literal shell.sh. Each kind now does what it says.
+func TestWritePatchHonorsKind(t *testing.T) {
+	root := t.TempDir()
+	slm := filepath.Join(root, ".slmcode")
+
+	// delete
+	if err := os.WriteFile(filepath.Join(root, "gone.go"), []byte("package a\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := writePatch(root, pendingPatch{Path: "gone.go", Kind: "delete"}); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := os.Stat(filepath.Join(root, "gone.go")); !os.IsNotExist(err) {
+		t.Fatalf("delete left the file (err=%v)", err)
+	}
+	// deleting an already-missing file is not an error
+	if err := writePatch(root, pendingPatch{Path: "gone.go", Kind: "delete"}); err != nil {
+		t.Fatalf("second delete: %v", err)
+	}
+
+	// mv
+	if err := os.MkdirAll(filepath.Join(root, "old"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(root, "old", "a.sh"), []byte("#!/bin/sh\n"), 0o755); err != nil { //nolint:gosec // executable fixture
+		t.Fatal(err)
+	}
+	if err := writePatch(root, pendingPatch{Path: "new/a.sh", Kind: "mv", From: "old/a.sh", Content: "#!/bin/sh\n"}); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := os.Stat(filepath.Join(root, "old", "a.sh")); !os.IsNotExist(err) {
+		t.Fatalf("mv left the source (err=%v)", err)
+	}
+	if got, err := os.ReadFile(filepath.Join(root, "new", "a.sh")); err != nil || string(got) != "#!/bin/sh\n" {
+		t.Fatalf("mv destination: %q err=%v", got, err)
+	}
+	// mv whose source vanished falls back to the recorded content
+	if err := writePatch(root, pendingPatch{Path: "new/b.go", Kind: "mv", From: "old/b.go", Content: "package b\n"}); err != nil {
+		t.Fatal(err)
+	}
+	if got, _ := os.ReadFile(filepath.Join(root, "new", "b.go")); string(got) != "package b\n" {
+		t.Fatalf("mv fallback: %q", got)
+	}
+	// mv onto an existing destination is refused
+	if err := writePatch(root, pendingPatch{Path: "new/b.go", Kind: "mv", From: "new/a.sh", Content: "x"}); err == nil {
+		t.Fatal("mv over an existing destination was allowed")
+	}
+
+	// shell: never a file
+	err := writePatch(root, pendingPatch{Path: "shell.sh", Kind: "shell", Content: "rm -rf /"})
+	if err == nil || !strings.Contains(err.Error(), "shell") {
+		t.Fatalf("shell entry applied: err=%v", err)
+	}
+	if _, serr := os.Stat(filepath.Join(root, "shell.sh")); serr == nil {
+		t.Fatal("shell.sh written to the project root")
+	}
+
+	// loadPending hides stale shell mirrors entirely, and surfaces from/stamps.
+	writeKindFixture(t, slm, "1000_shell_shell.sh.patch.json", map[string]string{"path": "shell.sh", "kind": "shell", "content": "ls"})
+	writeKindFixture(t, slm, "2000_mv_x.patch.json", map[string]string{"path": "n.go", "kind": "mv", "from": "o.go", "content": "p", "task_id": "T1", "agent": "worker"})
+	got, err := loadPending(slm)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(got) != 1 || got[0].Kind != "mv" || got[0].From != "o.go" || got[0].TaskID != "T1" || got[0].Agent != "worker" {
+		t.Fatalf("loadPending = %+v", got)
+	}
+
+	// The diff of a delete is everything removed; of a move, source vs content.
+	if err := os.WriteFile(filepath.Join(root, "d.go"), []byte("a\nb\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	fd := pendingPatch{Path: "d.go", Kind: "delete"}.diff(root)
+	if fd.Removed != 2 || fd.Added != 0 {
+		t.Fatalf("delete diff: +%d -%d", fd.Added, fd.Removed)
+	}
+	fd = pendingPatch{Path: "moved.go", Kind: "mv", From: "d.go", Content: "a\nb\n"}.diff(root)
+	if fd.Removed != 0 || fd.Added != 0 {
+		t.Fatalf("mv diff should be empty for unchanged content: +%d -%d", fd.Added, fd.Removed)
+	}
+}

docs/agents.md

@@ -34,7 +34,7 @@ see <a href="providers.md">Providers</a>. Budget diplomacy is a feature.
 | `escalate` | — | action JSON | HITL timeout arbitrator (retry/re-scope/…) ⚖️ |
 | `memory` | — | bullets | Learn 💾 |
 | `composer` | — | pipeline JSON | Assemble a task-specific pipeline (dynamic_pipeline) 🎯 |
-| `reviewer-strict` | — | approve JSON | Second opinion in the speculative review race (`max_parallel >= 3`), temperature 0 🔍🔍 |
+| `reviewer-strict` | — | approve JSON | Sequential second opinion, asked only when `reviewer` returns no readable verdict; temperature 0 🔍🔍 |
 | `describer` | — | prose | Architect half of the describer→editor pair (`architect_editor`) 🗣️ |
 | `editor` | ✅ + `find_models` / `mcp_call` | status | Editor half: applies a described change, minimal reasoning, strict format ✍️ |
 

docs/calibration.md

@@ -170,6 +170,27 @@ That last one is the delicate one, so it has three guardrails:
   The seed can only ever tighten a budget for a model fast enough that a role
   provably does not need the whole ceiling.
 
+Where the two numbers are enforced:
+
+* **`max_parallel` is one run-wide limit.** Every model request the harness
+  makes — the phase roles, the execute loop's workers, reviewers and
+  correctors, self-critique, triage, the speculative review race, the planner's
+  multipass cycle — takes a slot of a single gate sized `max_parallel`. It used
+  to bound only the execute wave, while context ran beside explore and a review
+  race added slots of its own, so a `max_parallel: 1` endpoint still saw two or
+  three requests queue on each other and every measured role timeout was
+  inflated by queueing the harness itself created. At `max_parallel: 1` the
+  phase pairs also run sequentially instead of racing for the one slot.
+* **Role latency memory covers the execute loop too.** The measured budget
+  (`p95 × 1.5`, floored per role class, capped at `task_timeout`) used to apply
+  only to the phase roles; workers, reviewers and correctors ran on the flat
+  `task_timeout` and recorded no samples, so `reviewer` had none after a
+  hundred runs. Each loop request is now dispatched on
+  `min(task_timeout clamped to the runway, measured budget)` and records a
+  sample under its base role (an escalation rung shares its base role's
+  series). A reviewer measured at 20s is given its 60s floor, not the whole
+  ceiling.
+
 ## Commands
 
 ```