root 6c93a38093 scrum multi_coord_phase3: 4 fixes from cross-lineage review
Cross-lineage scrum on bundle 87cbd10..f971e64 (3,652 lines)
produced 4 actionable findings, all defensive hardening.

1. (Opus WARN) internal/langfuse/client.go:queue
   Synchronous Flush at maxBatch threshold blocked the calling
   goroutine for the full 5s HTTP timeout when Langfuse hiccupped,
   defeating the "best-effort, never blocks calling path" contract
   in the package doc. Now fire-and-forget via goroutine.

2. (Opus + Kimi convergent) cmd/observerd/main.go:handleInbox
   - Free-form priority string was accepted; "nonsense" passed
     through unchecked. Now closed enum: urgent|high|medium|low (+
     empty defaults to medium). Tested: TestInbox_RejectsBadPriority.
   - No size cap on body, only emptiness check; multi-MB payloads
     would bloat observer's ring + JSONL. Now 8 KiB cap returns 413.
     Tested: TestInbox_RejectsOversizedBody.
   - Subject/sender/tag concatenated into InputSummary without
     newline stripping; embedded \n could corrupt JSONL line-based
     parsers. New sanitizeInboxField strips \r\n + caps at 256 chars
     before interpolation.

3. (Opus INFO) scripts/multi_coord_stress/main.go
   Removed dead `must[T]` generic — tracedSearch took over the
   fail-fast role for matrix searches, so the helper became unused.

4. (Opus INFO) scripts/multi_coord_stress/main.go:Event
   `JudgeRating int` collapsed "judge errored" and "judge said
   unrated" both to 0. Changed to *int — nil = errored, 1-5 =
   verdict. judgeInboxResult still returns 0 on error; caller
   gates on > 0 before assigning.

Dismissed (with rationale):
- Opus WARN ExcludeIDs ordering: verified by code read — filter
  applies after sort + before top-K truncation as documented;
  no slot waste possible.
- Opus INFO 10 prior-run reports contradict #011: those are
  point-in-time snapshots; intentional history.
- Kimi INFO Langfuse error suppression: design intent (best-effort
  per package doc).
- Kimi INFO contract schema validation: defer until contract count
  grows enough to make hand-edit drift a real risk.
- Kimi INFO paraphrase prompt duplicated across lift + multi_coord:
  defer (lift to internal/paraphrase/ when a third consumer appears).
- Qwen HOLD: single-line, no actionable finding.

go test ./cmd/observerd ./internal/langfuse all green; multi_coord
driver builds clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-30 17:42:07 -05:00

259 lines
9.1 KiB
Go

package main
import (
"bytes"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/go-chi/chi/v5"
"git.agentview.dev/profit/golangLAKEHOUSE/internal/observer"
"git.agentview.dev/profit/golangLAKEHOUSE/internal/workflow"
)
// newTestRouter builds the observerd router with an in-memory store
// and a workflow runner with no modes registered. Closes R-005 for
// observerd.
//
// Returns chi.Router (not http.Handler) so chi.Walk works without a
// type assertion that would panic if a future refactor wraps the
// router in plain net/http middleware.
func newTestRouter(t *testing.T) chi.Router {
t.Helper()
h := &handlers{
store: observer.NewStore(nil),
runner: workflow.NewRunner(),
}
r := chi.NewRouter()
h.register(r)
return r
}
func TestRoutesMounted(t *testing.T) {
r := newTestRouter(t)
want := map[string]bool{
"GET /observer/stats": false,
"POST /observer/event": false,
"POST /observer/workflow/run": false,
"GET /observer/workflow/modes": false,
"POST /observer/inbox": false,
}
_ = chi.Walk(r, func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error {
key := method + " " + route
if _, ok := want[key]; ok {
want[key] = true
}
return nil
})
for k, mounted := range want {
if !mounted {
t.Errorf("route not mounted: %s", k)
}
}
}
func TestStats_GET(t *testing.T) {
r := newTestRouter(t)
req := httptest.NewRequest("GET", "/observer/stats", nil)
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Errorf("expected 200, got %d", w.Code)
}
}
func TestWorkflowModes_GET(t *testing.T) {
r := newTestRouter(t)
req := httptest.NewRequest("GET", "/observer/workflow/modes", nil)
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Errorf("expected 200, got %d", w.Code)
}
}
// TestEvent_InvalidOp locks the validation path: an ObservedOp with
// missing required fields must 400, not 500. Without this assertion,
// observer.ErrInvalidOp could silently slip into the 500 branch on a
// future refactor and clients would see "internal" instead of the
// actual validation error.
func TestEvent_InvalidOp(t *testing.T) {
r := newTestRouter(t)
// Empty body — no endpoint, no source — fails ObservedOp validation.
body := []byte(`{}`)
req := httptest.NewRequest("POST", "/observer/event", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusBadRequest {
t.Errorf("expected 400 on invalid op, got %d (body=%s)", w.Code, w.Body.String())
}
}
// TestWorkflowRun_AllProvenanceRecordedPostRun proves the gap ratified
// in ADR-005 Decision 5.3: handleWorkflowRun calls runner.Run
// synchronously and only records ObservedOps from the returned
// RunResult AFTER Run completes. A crash mid-Run would lose ALL
// provenance for that workflow.
//
// The test pauses inside a node, samples observer state (must be 0),
// unblocks, then samples again (must be N). If a future commit adds
// per-node streaming (e.g. runner.NodeHook firing before Run returns),
// the first assertion fires — that's the intentional test-as-spec
// lock so the behavior change is visible in `go test` instead of
// surfacing under load.
func TestWorkflowRun_AllProvenanceRecordedPostRun(t *testing.T) {
pauseCh := make(chan struct{})
runner := workflow.NewRunner()
runner.RegisterMode("test.pause", func(_ workflow.Context, _ map[string]any) (map[string]any, error) {
<-pauseCh
return map[string]any{"unpaused": true}, nil
})
h := &handlers{
store: observer.NewStore(nil),
runner: runner,
}
r := chi.NewRouter()
h.register(r)
// Two-node serial workflow so we have something to record post-run.
body := []byte(`{"workflow":{"name":"adr_005_5_3","nodes":[
{"id":"n1","mode":"test.pause"},
{"id":"n2","mode":"test.pause","depends_on":["n1"]}
]}}`)
// Send the request in a goroutine — it'll block until pauseCh closes.
done := make(chan int)
go func() {
req := httptest.NewRequest("POST", "/observer/workflow/run", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
done <- w.Code
}()
// Wait briefly for the runner to enter n1 and block on pauseCh.
// 50ms is conservative; the goroutine + chi routing + topo sort
// take well under that on this hardware.
time.Sleep(50 * time.Millisecond)
// LOCK: store MUST be empty while runner.Run is paused.
// If a future change adds streaming-record-as-each-node-finishes,
// n1's record would land here as soon as n1 returns — but n1
// hasn't returned yet (we're paused before it does), so the
// only way this assertion passes is if recording is post-run-only.
if got := h.store.Stats().Total; got != 0 {
t.Errorf("expected 0 observer ops during paused run, got %d "+
"(if non-zero, ADR-005 Decision 5.3 must be updated — recording "+
"is no longer post-run-only)", got)
}
// Unblock all paused nodes (channel close broadcasts to all receivers).
close(pauseCh)
// Wait for the handler to return + record post-run.
if code := <-done; code != http.StatusOK {
t.Errorf("workflow run failed: HTTP %d", code)
}
// LOCK: store MUST have 2 ops after run completes.
if got := h.store.Stats().Total; got != 2 {
t.Errorf("expected 2 observer ops after run, got %d", got)
}
}
// TestInbox_AcceptsValidEmail locks the happy-path contract for the
// /observer/inbox route — accepts an email message with required
// fields, records as ObservedOp, returns 200 with ring-size.
func TestInbox_AcceptsValidEmail(t *testing.T) {
r := newTestRouter(t)
body := []byte(`{"type":"email","sender":"client@northstar.com","subject":"URGENT: 50 forklift ops","body":"Need 50 forklift operators in Cleveland OH for next week. Day shift.","priority":"urgent","tag":"alpha-surge"}`)
req := httptest.NewRequest("POST", "/observer/inbox", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Fatalf("expected 200, got %d (body=%s)", w.Code, w.Body.String())
}
if !strings.Contains(w.Body.String(), `"accepted":true`) {
t.Errorf("expected accepted=true, got %s", w.Body.String())
}
}
// TestInbox_RejectsBadType locks the validation: type must be
// "email" or "sms", anything else is 400.
func TestInbox_RejectsBadType(t *testing.T) {
r := newTestRouter(t)
body := []byte(`{"type":"smoke-signal","sender":"x","body":"y","priority":"high"}`)
req := httptest.NewRequest("POST", "/observer/inbox", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusBadRequest {
t.Errorf("expected 400 on bad type, got %d", w.Code)
}
}
// TestInbox_RejectsEmptyBody locks the body-required invariant.
func TestInbox_RejectsEmptyBody(t *testing.T) {
r := newTestRouter(t)
body := []byte(`{"type":"email","sender":"x","body":"","priority":"high"}`)
req := httptest.NewRequest("POST", "/observer/inbox", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusBadRequest {
t.Errorf("expected 400 on empty body, got %d", w.Code)
}
}
// TestInbox_RejectsBadPriority locks the priority enum (per
// scrum Kimi WARN — free-form priority strings were accepted).
func TestInbox_RejectsBadPriority(t *testing.T) {
r := newTestRouter(t)
body := []byte(`{"type":"email","sender":"x","body":"y","priority":"nonsense"}`)
req := httptest.NewRequest("POST", "/observer/inbox", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusBadRequest {
t.Errorf("expected 400 on bad priority, got %d", w.Code)
}
}
// TestInbox_RejectsOversizedBody locks the body-size cap (per scrum
// Opus WARN — only emptiness was checked before).
func TestInbox_RejectsOversizedBody(t *testing.T) {
r := newTestRouter(t)
bigBody := strings.Repeat("x", inboxMaxBodyChars+1)
body := []byte(`{"type":"email","sender":"x","body":"` + bigBody + `","priority":"high"}`)
req := httptest.NewRequest("POST", "/observer/inbox", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusRequestEntityTooLarge {
t.Errorf("expected 413 on oversized body, got %d", w.Code)
}
}
// TestWorkflowRun_UnknownMode locks the 400 path on workflow definitions
// that reference modes not registered with the runner. The harness's
// reality test runs depend on this so an unknown-mode misconfiguration
// surfaces as a definition error, not a server error.
func TestWorkflowRun_UnknownMode(t *testing.T) {
r := newTestRouter(t)
body := []byte(`{"workflow":{"name":"t","nodes":[{"id":"n1","mode":"does.not.exist"}]}}`)
req := httptest.NewRequest("POST", "/observer/workflow/run", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusBadRequest {
t.Errorf("expected 400 on unknown mode, got %d (body=%s)", w.Code, w.Body.String())
}
}