feat(orchestrator): milestone rotation and thrash detection (S11)

Closes the last two S11 triggers. internal/herdr/activity.go normalizes tool/function calls per harness (ClaudeActivity verified against the existing transcript format, CodexActivity best-effort/unverified, OpenCodeActivity refuses — no confirmed per-tool-call source exists) and implements the three thrash rules plus a narrow milestone check (successful git commit as the last call). CLIAdapter.RequestHandoffReason asks the agent to write a handoff with meta.reason set, same "ask, don't invent" pattern as the existing handoff/ report requests. rotate() and TurnDecision generalize the manual-bypass shortcut to manual/milestone/thrash and request (never directly release) on a detected trigger. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1rkJ2hBMybnJctPbcy4tT
2026-07-28 00:09:57 +04:00
parent d678959d65
commit c85fb81663
7 changed files with 978 additions and 16 deletions
@@ -187,6 +187,9 @@ type Coordinator struct {
 	// defaultSoft), so existing callers that never set this field keep
 	// working unchanged.
 	Soft float64
+	// Thrash tunes DetectThrash's three circuit breakers (§5.3). Zero-value
+	// fields fall back to herdr's own defaults, so leaving this unset works.
+	Thrash herdr.ThrashConfig
 }

 // defaultSoft is used whenever Coordinator.Soft is unset (zero value).
@@ -199,6 +202,59 @@ func (c *Coordinator) soft() float64 {
 	return defaultSoft
 }

+// checkActivityTriggers is S11's milestone/thrash pair: given an adapter that
+// implements herdr.ActivityReader, read its tool-call history and evaluate
+// both detectors. thrash takes priority (a circuit breaker overrides a
+// coherent-looking commit), same as the caller would want either way since
+// only one handoff request happens per tick. Returns the reason to request
+// ("thrash"/"milestone") and its dead ends, or "" if neither fired or the
+// adapter has no activity source at all — the latter is not degraded-and-
+// recorded the way TurnBoundary's absence is, since these two triggers are
+// additive on top of threshold/manual rotation, not a required safety gate.
+func checkActivityTriggers(ctx context.Context, a herdr.Adapter, session herdr.Session, cfg herdr.ThrashConfig) (reason string, deadEnds []continuity.DeadEnd) {
+	reader, ok := a.(herdr.ActivityReader)
+	if !ok {
+		return "", nil
+	}
+	calls, err := reader.Activity(ctx, session)
+	if err != nil {
+		return "", nil
+	}
+	if thrash, de := herdr.DetectThrash(calls, cfg); thrash {
+		return "thrash", de
+	}
+	if herdr.DetectMilestone(calls) {
+		return "milestone", nil
+	}
+	return "", nil
+}
+
+// requestReasonedHandoff is the shared "ask once, remember we asked" wiring
+// checkActivityTriggers' two callers (rotate, TurnDecision) both need — same
+// HandoffRequested guard the occupancy-driven HandoffRequester path already
+// uses, so a repeated thrash/milestone detection on later ticks doesn't
+// reprompt every time before the agent has finished writing the file.
+func (c *Coordinator) requestReasonedHandoff(ctx context.Context, taskID string, session herdr.Session, a herdr.Adapter, reason string, deadEnds []continuity.DeadEnd) {
+	if session.HandoffRequested {
+		return
+	}
+	if _, statErr := os.Stat(filepath.Join(session.Worktree, herdr.HandoffFile)); statErr == nil {
+		return
+	}
+	requester, ok := a.(herdr.ReasonedHandoffRequester)
+	if !ok {
+		return
+	}
+	if err := requester.RequestHandoffReason(ctx, session, reason, deadEnds); err != nil {
+		return
+	}
+	session.HandoffRequested = true
+	c.mu.Lock()
+	c.sessions[taskID] = session
+	_ = c.saveSessionsLocked()
+	c.mu.Unlock()
+}
+
 type MonitorHealth struct {
 	Running   bool      `json:"running"`
 	LastRun   time.Time `json:"last_run"`
@@ -570,14 +626,20 @@ func (c *Coordinator) rotate(ctx context.Context, hard float64) {
 			continue
 		}
 		reason := "threshold"
-		// Agent-initiated ROTATE (§5.3): the agent itself wrote a handoff
-		// with reason=manual — "a coherent unit finished and the next is
-		// independent". That is the boundary signal in its own right; skip
-		// occupancy and the turn-boundary probe and go straight to release.
-		manual := handoffReason(session.Worktree) == "manual"
-		if manual {
-			reason = "manual"
+		// Agent-initiated ROTATE, and the two orchestrator-detected triggers
+		// (§5.3: manual / milestone / thrash) all short-circuit the same way
+		// once a handoff carrying that reason already exists: the boundary
+		// question has already been answered, so skip occupancy and the
+		// turn-boundary probe and go straight to release.
+		existingReason := handoffReason(session.Worktree)
+		bypass := existingReason == "manual" || existingReason == "milestone" || existingReason == "thrash"
+		if bypass {
+			reason = existingReason
 		} else {
+			if trigger, deadEnds := checkActivityTriggers(ctx, a, session, c.Thrash); trigger != "" {
+				c.requestReasonedHandoff(ctx, taskID, session, a, trigger, deadEnds)
+				continue
+			}
 			occupancy, err := a.Occupancy(session)
 			if err != nil || occupancy < c.soft() {
 				continue
@@ -694,11 +756,16 @@ func (c *Coordinator) TurnDecision(ctx context.Context, taskID string) (string,
 	if err != nil {
 		return "", fmt.Errorf("orchestrator: adapter: %w", err)
 	}
-	// Agent-initiated ROTATE (§5.3): a handoff already written with
-	// reason=manual is itself the boundary signal — skip occupancy and the
+	// Agent-initiated ROTATE, and the two orchestrator-detected triggers
+	// (§5.3: manual / milestone / thrash): a handoff already written with one
+	// of these reasons is itself the boundary signal — skip occupancy and the
 	// turn-boundary probe and release immediately.
-	if handoffReason(session.Worktree) == "manual" {
-		return c.finishRelease(ctx, taskID, task, session, a, "manual")
+	if existingReason := handoffReason(session.Worktree); existingReason == "manual" || existingReason == "milestone" || existingReason == "thrash" {
+		return c.finishRelease(ctx, taskID, task, session, a, existingReason)
+	}
+	if trigger, deadEnds := checkActivityTriggers(ctx, a, session, c.Thrash); trigger != "" {
+		c.requestReasonedHandoff(ctx, taskID, session, a, trigger, deadEnds)
+		return TurnPrepareHandoff, nil
 	}
 	occupancy, err := a.Occupancy(session)
 	if err != nil {
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"orchestra/internal/authz"
+	"orchestra/internal/continuity"
 	"orchestra/internal/domain"
 	"orchestra/internal/herdr"
 	"orchestra/internal/orchestrator"
@@ -578,6 +579,189 @@ func (a *handoffRequestingAdapter) RequestHandoff(context.Context, herdr.Session
 	return nil
 }

+// activityAdapter is fakeAdapter plus S11's milestone/thrash pair: it reports
+// a fixed tool-call history and records reasoned handoff requests, so tests
+// can drive checkActivityTriggers without a real herdr transcript.
+type activityAdapter struct {
+	fakeAdapter
+	calls        []herdr.ToolCall
+	activityErr  error
+	reasonAsked  []string
+	deadEndsSeen []continuity.DeadEnd
+}
+
+func (a *activityAdapter) Activity(context.Context, herdr.Session) ([]herdr.ToolCall, error) {
+	return a.calls, a.activityErr
+}
+
+func (a *activityAdapter) RequestHandoffReason(_ context.Context, _ herdr.Session, reason string, deadEnds []continuity.DeadEnd) error {
+	a.reasonAsked = append(a.reasonAsked, reason)
+	a.deadEndsSeen = deadEnds
+	return nil
+}
+
+// TestActivityTriggersRequestReasonedHandoffWithoutReleasing guards S11's two
+// orchestrator-detected rotation triggers (milestone, thrash): both rotate()
+// and TurnDecision must ask for a reasoned handoff — never release — the
+// first time the trigger fires, entirely independent of occupancy (both
+// cases here use occupancy=0, far below even the soft threshold).
+func TestActivityTriggersRequestReasonedHandoffWithoutReleasing(t *testing.T) {
+	repo := t.TempDir()
+	run(t, repo, "init")
+	run(t, repo, "config", "user.email", "t@t")
+	run(t, repo, "config", "user.name", "t")
+	run(t, repo, "commit", "--allow-empty", "-m", "init")
+
+	newCoordinator := func(a *activityAdapter) (*orchestrator.Coordinator, *store.Store, domain.Task) {
+		s, err := store.Open(t.TempDir())
+		if err != nil {
+			t.Fatal(err)
+		}
+		if err := s.Append(domain.Event{ID: domain.NewID(), Type: "TaskCreated", TaskID: "t1", Surface: string(authz.System), Payload: mustJSON(map[string]any{
+			"source": "jsonl", "external_id": "1", "project": "p",
+		})}); err != nil {
+			t.Fatal(err)
+		}
+		task := s.Tasks()[0]
+		c := &orchestrator.Coordinator{Store: s, Worktrees: worktrees{path: repo}, Adapters: adapters{a}, StatePath: t.TempDir() + "/sessions.json", Hard: .8}
+		leaseEvt, err := s.Lease(task.ID, "h1", time.Minute)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if err := c.Start(context.Background(), leaseEvt); err != nil {
+			t.Fatal(err)
+		}
+		return c, s, task
+	}
+
+	thrashCalls := []herdr.ToolCall{
+		{Name: "Bash", Kind: "command", Key: "go test ./...", Success: false, IsTest: true},
+		{Name: "Bash", Kind: "command", Key: "go test ./...", Success: false, IsTest: true},
+		{Name: "Bash", Kind: "command", Key: "go test ./...", Success: false, IsTest: true},
+	}
+	milestoneCalls := []herdr.ToolCall{
+		{Name: "Bash", Kind: "command", Key: "git commit -m done", Success: true},
+	}
+
+	t.Run("thrash requests a reasoned handoff and does not release, via TurnDecision", func(t *testing.T) {
+		a := &activityAdapter{fakeAdapter: fakeAdapter{occupancy: 0, boundary: true}, calls: thrashCalls}
+		c, st, task := newCoordinator(a)
+		decision, err := c.TurnDecision(context.Background(), task.ID)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if decision != orchestrator.TurnPrepareHandoff {
+			t.Fatalf("decision=%q want %q", decision, orchestrator.TurnPrepareHandoff)
+		}
+		if len(a.reasonAsked) != 1 || a.reasonAsked[0] != "thrash" {
+			t.Fatalf("reasonAsked=%v want [thrash]", a.reasonAsked)
+		}
+		if len(a.deadEndsSeen) == 0 {
+			t.Fatal("want populated dead ends for the thrash trigger")
+		}
+		if a.releases != 0 {
+			t.Fatal("Release must not be invoked on a bare thrash detection")
+		}
+		got, ok := st.Task(task.ID)
+		if !ok || got.State != domain.StateLeased {
+			t.Fatalf("task state=%v ok=%v, want still leased", got.State, ok)
+		}
+	})
+
+	t.Run("milestone requests a reasoned handoff and does not release, via TurnDecision", func(t *testing.T) {
+		a := &activityAdapter{fakeAdapter: fakeAdapter{occupancy: 0, boundary: true}, calls: milestoneCalls}
+		c, st, task := newCoordinator(a)
+		decision, err := c.TurnDecision(context.Background(), task.ID)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if decision != orchestrator.TurnPrepareHandoff {
+			t.Fatalf("decision=%q want %q", decision, orchestrator.TurnPrepareHandoff)
+		}
+		if len(a.reasonAsked) != 1 || a.reasonAsked[0] != "milestone" {
+			t.Fatalf("reasonAsked=%v want [milestone]", a.reasonAsked)
+		}
+		if a.releases != 0 {
+			t.Fatal("Release must not be invoked on a bare milestone detection")
+		}
+		got, ok := st.Task(task.ID)
+		if !ok || got.State != domain.StateLeased {
+			t.Fatalf("task state=%v ok=%v, want still leased", got.State, ok)
+		}
+	})
+
+	// Once the agent has actually written a thrash/milestone-reasoned
+	// handoff, that reason is itself the boundary signal (same as manual) —
+	// TurnDecision must release immediately, bypassing occupancy/boundary.
+	t.Run("a written thrash handoff bypasses occupancy and releases", func(t *testing.T) {
+		a := &activityAdapter{fakeAdapter: fakeAdapter{occupancy: 0, boundary: false}, calls: thrashCalls}
+		c, st, task := newCoordinator(a)
+		artifactRef, err := st.PutArtifact([]byte("handoff"))
+		if err != nil {
+			t.Fatal(err)
+		}
+		a.ref = artifactRef
+		head, err := herdr.HeadSHA(repo)
+		if err != nil {
+			t.Fatal(err)
+		}
+		handoff := map[string]any{
+			"meta":   map[string]any{"id": "h3", "reason": "thrash", "rotation_index": 0},
+			"anchor": map[string]any{"git_sha": head, "branch": "orchestra/t1"},
+			"goal":   "g", "done_when": []string{"x"}, "action": "prompt", "command": "go test ./...",
+			"dead_ends": []map[string]any{{"tried": "go test ./...", "why_failed": "failed 3 times"}},
+		}
+		b, err := json.Marshal(handoff)
+		if err != nil {
+			t.Fatal(err)
+		}
+		handoffPath := repo + "/" + herdr.HandoffFile
+		if err := os.WriteFile(handoffPath, b, 0o644); err != nil {
+			t.Fatal(err)
+		}
+		t.Cleanup(func() { os.Remove(handoffPath) })
+
+		decision, err := c.TurnDecision(context.Background(), task.ID)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if decision != orchestrator.TurnRotateNow {
+			t.Fatalf("decision=%q want %q", decision, orchestrator.TurnRotateNow)
+		}
+		if a.releases == 0 {
+			t.Fatal("Release was never invoked for a written thrash handoff")
+		}
+		got, ok := st.Task(task.ID)
+		if !ok || got.State != domain.StateQueued {
+			t.Fatalf("task state=%v ok=%v, want queued", got.State, ok)
+		}
+	})
+
+	t.Run("rotate() also requests a reasoned handoff on thrash, without releasing", func(t *testing.T) {
+		a := &activityAdapter{fakeAdapter: fakeAdapter{occupancy: 0, boundary: true}, calls: thrashCalls}
+		c, st, task := newCoordinator(a)
+
+		ctx, cancel := context.WithCancel(context.Background())
+		defer cancel()
+		go c.Monitor(ctx, .8, time.Millisecond)
+
+		deadline := time.Now().Add(300 * time.Millisecond)
+		for time.Now().Before(deadline) && len(a.reasonAsked) == 0 {
+			time.Sleep(time.Millisecond)
+		}
+		if len(a.reasonAsked) == 0 || a.reasonAsked[0] != "thrash" {
+			t.Fatalf("reasonAsked=%v want a thrash request from rotate()", a.reasonAsked)
+		}
+		if a.releases != 0 {
+			t.Fatal("rotate() must not release on a bare thrash detection")
+		}
+		got, ok := st.Task(task.ID)
+		if !ok || got.State != domain.StateLeased {
+			t.Fatalf("task state=%v ok=%v, want still leased", got.State, ok)
+		}
+	})
+}
+
 // TestRotationRequestsHandoffBeforeReleasing guards Phase 4 item 2 (AUDIT.md):
 // rotate() must not call Release until the agent has been told to write its
 // §6.1 handoff and the file actually exists — never invent or skip the ask.