Close B19-B21 and S12-S13, and fix the flaky router test

All five defects filed while implementing B18, plus the router flake that predated them. None of this has run on the deployed instance: the service is stopped and /usr/local/bin/orchestra predates every change here. B20 is the one that could silently defeat approvals. The capture revision was UnixNano, so it changed on every read and said nothing about whether the pane had changed; it is now an FNV-1a hash of the pane text, changing iff the text does. The worse half was precedence: capture() preferred the coordinator over a published worker capture, handing Queue a timestamp the owning worker's staleness check could never match, so every federated approval resolved "stale" and the keystroke never happened. Worker captures now win — their existence means a registered worker owns that pane — and capturePane follows the same precedence via Capture.Source rather than guessing. B19 was filed as "federated approvals emit no event", which overstated it: the resolution half already existed, and correctly fires only on an acknowledged worker report. The missing half was the request. Server.action now appends ApprovalRequested at queue time, subject_ref set to the command ID the later resolution carries. If that append fails the queued command is resolved "rejected" — a keystroke that left no audit trail must not run. B21 bounds the command list: resolved commands prune after 30 minutes on both Queue and Commands, pending ones never at any age, since dropping one would discard an operator decision. The persistence half stays open and is recorded as such — captures and commands are still in-memory only. S12 splits ORCHESTRA_NTFY_TOKEN, which was both the secret handed to the ntfy server and a valid inbound credential for the ntfy surface; the latter is now ORCHESTRA_NTFY_SURFACE_TOKEN. Breaking: a deployment relying on the old dual use has no inbound gate until it sets the new variable. S13 deletes the dead auth() copy of the authorization policy. The router flake was in the test, not in assignment. Store.Tasks() ranges a map, and the assertion indexed two separate Tasks() calls, failing whenever the orderings disagreed; instrumenting it showed a valid TaskLeased and a genuinely leased task on every "failing" run. It now snapshots once and asserts that exactly one task is leased, and passes at -count=60. AUDIT.md records what is still not done: the deployed env and binary, the live re-verification B13-B17 has always lacked, and two operational faults found in the journal that block it — all six herdrs are refusing connections, and ntfy delivery is failing 403 on every send. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01535A3Y8RtkAi8wYuWhtkEd
2026-07-29 01:33:59 +04:00
parent 0b4b52ac45
commit 95454afa72
8 changed files with 349 additions and 89 deletions
@@ -5,6 +5,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"hash/fnv"
 	"net/http"
 	"orchestra/internal/authz"
 	"orchestra/internal/domain"
@@ -74,15 +75,21 @@ func (s Server) capture(ctx context.Context, t domain.Task) (*Capture, error) {
 	if t.State != domain.StateLeased || t.Lease == nil {
 		return nil, nil
 	}
+	// B20: precedence is deliberate, not incidental. A published worker
+	// capture means a registered worker owns that pane, and its revision is
+	// the counter the worker's own staleness check compares against — so it
+	// must win. Preferring the coordinator here (as this once did) handed
+	// Queue a revision the worker could never match, and every such approval
+	// resolved "stale" and silently never happened.
+	if s.Workers != nil {
+		if c, ok := s.Workers.Capture(t.Lease.HarnessID, t.ID); ok {
+			return &Capture{TaskID: t.ID, Source: "worker", Text: c.Text, Revision: c.Revision, At: c.At}, nil
+		}
+	}
 	if s.Coordinator != nil {
 		text, err := s.Coordinator.Capture(ctx, t.ID, "recent")
 		if err == nil {
-			return &Capture{TaskID: t.ID, Source: "recent", Text: text, Revision: uint64(time.Now().UnixNano()), At: time.Now().UTC()}, nil
-		}
-	}
-	if s.Workers != nil {
-		if c, ok := s.Workers.Capture(t.Lease.HarnessID, t.ID); ok {
-			return &Capture{TaskID: t.ID, Source: "recent", Text: c.Text, Revision: c.Revision, At: c.At}, nil
+			return &Capture{TaskID: t.ID, Source: "recent", Text: text, Revision: captureRevision(t.Lease.HarnessID, text), At: time.Now().UTC()}, nil
 		}
 	}
 	return nil, fmt.Errorf("capture unavailable from owning worker")
@@ -152,7 +159,7 @@ func (s Server) detail(ctx context.Context, id string) (TaskDetail, error) {
 		session := &Session{HarnessID: t.Lease.HarnessID, LeaseUntil: &t.Lease.Until}
 		if c, err := s.capture(ctx, t); err == nil && c != nil {
 			session.Capture = c
-			session.PaneID = capturePane(s, t.ID, c)
+			session.PaneID = capturePane(s, t, c)
 			session.Approval = ParsePendingApproval(c.Text, session.PaneID, c.Revision, c.At)
 		} else if err != nil {
 			session.Blocker = "capture unavailable: " + err.Error()
@@ -161,17 +168,35 @@ func (s Server) detail(ctx context.Context, id string) (TaskDetail, error) {
 	}
 	return d, nil
 }
-func capturePane(s Server, taskID string, c *Capture) string {
-	if s.Coordinator != nil {
-		if session, ok := s.Coordinator.Session(taskID); ok {
-			return session.PaneID
-		}
+// captureRevision identifies *what the operator saw*, not when they saw it.
+// B20: this was UnixNano, so it changed on every read and said nothing about
+// whether the pane had changed. A content hash changes if and only if the
+// text does, which is the only property any consumer of a revision wants.
+func captureRevision(harness, text string) uint64 {
+	h := fnv.New64a()
+	_, _ = h.Write([]byte(harness))
+	_, _ = h.Write([]byte{0})
+	_, _ = h.Write([]byte(text))
+	// 0 means "no revision" to federation.Queue; never collide with it.
+	if v := h.Sum64(); v != 0 {
+		return v
 	}
-	if s.Workers != nil {
-		for _, w := range s.Workers.Snapshot() {
-			if remote, ok := s.Workers.Capture(w.ID, taskID); ok && remote.Revision == c.Revision {
-				return remote.PaneID
-			}
+	return 1
+}
+
+// capturePane resolves the pane the capture came from, following the same
+// source precedence capture() used — asking the coordinator about a pane a
+// worker owns (or vice versa) can return a pane the text never came from.
+func capturePane(s Server, t domain.Task, c *Capture) string {
+	if c.Source == "worker" && s.Workers != nil && t.Lease != nil {
+		if remote, ok := s.Workers.Capture(t.Lease.HarnessID, t.ID); ok && remote.Revision == c.Revision {
+			return remote.PaneID
+		}
+		return ""
+	}
+	if s.Coordinator != nil {
+		if session, ok := s.Coordinator.Session(t.ID); ok {
+			return session.PaneID
 		}
 	}
 	return ""
@@ -304,22 +329,40 @@ func (s Server) action(w http.ResponseWriter, r *http.Request, id, action string
 			http.Error(w, "current capture unavailable", 503)
 			return
 		}
-		pane := capturePane(s, id, c)
+		pane := capturePane(s, t, c)
 		approval := ParsePendingApproval(c.Text, pane, c.Revision, c.At)
 		if approval == nil || approval.Kind == "unknown" || pane == "" {
 			http.Error(w, "current prompt is not safely actionable", 409)
 			return
 		}
-		if s.Workers != nil {
-			if _, remote := s.Workers.Capture(t.Lease.HarnessID, t.ID); remote {
-				command, err := s.Workers.Queue(t.Lease.HarnessID, federation.Command{TaskID: id, Kind: action, PaneID: pane, CaptureRevision: c.Revision})
-				if err != nil {
-					http.Error(w, err.Error(), 409)
-					return
-				}
-				writeJSON(w, map[string]any{"command": command, "task": t})
+		if c.Source == "worker" {
+			command, err := s.Workers.Queue(t.Lease.HarnessID, federation.Command{TaskID: id, Kind: action, PaneID: pane, CaptureRevision: c.Revision})
+			if err != nil {
+				http.Error(w, err.Error(), 409)
 				return
 			}
+			// B19: the federated path used to return here, leaving the most
+			// safety-critical operation in the system invisible in the event
+			// log exactly when it crossed a machine boundary. The honest
+			// event at this point is a *request* — the keystroke has only
+			// been queued. The matching ApprovalGranted/ApprovalDenied is
+			// appended when the worker reports the input was acknowledged
+			// (see /v1/federation/commands/ in cmd/orchestra).
+			payload, _ := json.Marshal(map[string]any{
+				"subject_ref": command.ID, "options": []string{"grant_approval", "deny_approval"},
+				"decision": action, "task_id": id, "worker": t.Lease.HarnessID,
+				"pane_id": pane, "capture_revision": c.Revision,
+			})
+			e := domain.Event{ID: domain.NewID(), Type: "ApprovalRequested", TaskID: id, Version: t.Version + 1, Payload: payload, Surface: string(authz.Web)}
+			if err := s.Store.Append(e); err != nil {
+				// No audit trail, no approval. Resolve the queued command so
+				// the worker never executes an unrecorded keystroke.
+				_ = s.Workers.CompleteCommand(t.Lease.HarnessID, command.ID, "rejected", "audit append failed: "+err.Error())
+				http.Error(w, "approval not recorded: "+err.Error(), 409)
+				return
+			}
+			writeJSON(w, map[string]any{"command": command, "task": t})
+			return
 		}
 		if s.Coordinator == nil {
 			http.Error(w, "approval executor unavailable", 503)
@@ -2,8 +2,11 @@ package ui

 import (
 	"bytes"
+	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"orchestra/internal/domain"
+	"orchestra/internal/federation"
 	"orchestra/internal/store"
 	"strings"
 	"testing"
@@ -43,3 +46,87 @@ func TestParsePendingApprovalDoesNotInventActionablePrompt(t *testing.T) {
 		t.Fatalf("got %#v", p)
 	}
 }
+
+// TestFederatedApprovalIsQueuedAtTheWorkerRevisionAndAudited guards B19 and
+// B20 together: the queued command must carry the worker's own capture
+// counter (not a coordinator timestamp the worker can never match, which
+// resolved every such approval "stale"), and queueing must leave an
+// ApprovalRequested event behind — the grant/deny is appended later, when
+// the worker reports the keystroke was acknowledged.
+func TestFederatedApprovalIsQueuedAtTheWorkerRevisionAndAudited(t *testing.T) {
+	s, err := store.Open(t.TempDir())
+	if err != nil {
+		t.Fatal(err)
+	}
+	workers := &federation.Registry{}
+	if err := workers.Register(federation.Worker{ID: "wpc-claude", Token: "tok"}, ""); err != nil {
+		t.Fatal(err)
+	}
+	h := Server{Store: s, Workers: workers}.Handler()
+	req := httptest.NewRequest(http.MethodPost, "/v1/ui/tasks", bytes.NewBufferString(`{"source":"web","external_id":"fed","project":"demo","title":"Federated"}`))
+	r := httptest.NewRecorder()
+	h.ServeHTTP(r, req)
+	if r.Code != http.StatusOK {
+		t.Fatalf("create status=%d body=%s", r.Code, r.Body.String())
+	}
+	id := s.Tasks()[0].ID
+	if _, err := s.Lease(id, "wpc-claude", time.Hour); err != nil {
+		t.Fatal(err)
+	}
+	capture, err := workers.PutCapture("wpc-claude", federation.Capture{TaskID: id, PaneID: "wA:p1", Text: "Permission required\n$ rm -rf build"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	req = httptest.NewRequest(http.MethodPost, "/v1/ui/tasks/"+id+"/actions/grant_approval", bytes.NewBufferString(`{}`))
+	r = httptest.NewRecorder()
+	h.ServeHTTP(r, req)
+	if r.Code != http.StatusOK {
+		t.Fatalf("grant status=%d body=%s", r.Code, r.Body.String())
+	}
+	pending, err := workers.Commands("wpc-claude")
+	if err != nil || len(pending) != 1 {
+		t.Fatalf("commands=%#v err=%v", pending, err)
+	}
+	if pending[0].CaptureRevision != capture.Revision || pending[0].PaneID != "wA:p1" {
+		t.Fatalf("queued at revision %d pane %q, worker published %d wA:p1", pending[0].CaptureRevision, pending[0].PaneID, capture.Revision)
+	}
+	var requested *domain.Event
+	for _, e := range s.Events(0) {
+		if e.Type == "ApprovalRequested" && e.TaskID == id {
+			x := e
+			requested = &x
+		}
+		if e.Type == "ApprovalGranted" {
+			t.Fatal("queueing a command must not claim an outcome the worker has not reported")
+		}
+	}
+	if requested == nil {
+		t.Fatal("federated approval left no audit trail")
+	}
+	var p struct {
+		SubjectRef string `json:"subject_ref"`
+		Decision   string `json:"decision"`
+	}
+	if err := json.Unmarshal(requested.Payload, &p); err != nil {
+		t.Fatal(err)
+	}
+	if p.SubjectRef != pending[0].ID || p.Decision != "grant_approval" {
+		t.Fatalf("audit event does not identify the command: %s", requested.Payload)
+	}
+}
+
+// TestCoordinatorCaptureRevisionTracksContentNotTime guards B20: the local
+// revision used to be UnixNano, so it changed on every read and told a
+// caller nothing about whether the pane had changed.
+func TestCoordinatorCaptureRevisionTracksContentNotTime(t *testing.T) {
+	a := captureRevision("claude", "Permission required\n$ ls")
+	if a != captureRevision("claude", "Permission required\n$ ls") {
+		t.Fatal("revision changed without the pane text changing")
+	}
+	if a == captureRevision("claude", "Permission required\n$ rm -rf /") {
+		t.Fatal("revision did not change when the pane text changed")
+	}
+	if a == 0 {
+		t.Fatal("revision 0 means \"no revision\" to federation.Queue")
+	}
+}