Use objectPlantingsPath helper instead of inline URL (#88)

Gadfly: I was wrong that no plantings-path helper existed — objectPlantingsPath
is defined in plantings_test.go in the same package. Use it for the two
planting POSTs in the remaining test.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

internal/api/agent.go

@@ -108,21 +108,6 @@ func (h *handlers) agentChat(c *gin.Context) {
 	send(chatEvent{Done: turn})
 }
 
-// sseWriteTimeout bounds ONE write to the stream, not the stream itself.
-//
-// It is refreshed per frame, which is the only shape that satisfies both ends:
-// the server's absolute WriteTimeout would cut a long turn (#78), while removing
-// the deadline entirely would let a client that stops reading block a write
-// forever once the socket buffer fills — pinning the run goroutine and this
-// stream's mutex with it, and taking the keep-alive down too since it needs the
-// same lock. Generous, because it is a backstop against a stuck peer and not a
-// pacing mechanism.
-//
-// A var, not a const, ONLY so the test can shrink it to prove the deadline is
-// refreshed per frame rather than set once — a set-once 30s deadline would pass
-// a test whose whole run is under a second. Production never reassigns it.
-var sseWriteTimeout = 30 * time.Second
-
 // eventStream serializes writes to one SSE response.
 //
 // The mutex is load-bearing, not decoration: step events are sent from the
@@ -131,7 +116,6 @@ var sseWriteTimeout = 30 * time.Second
 // frames long before it crashes anything.
 type eventStream struct {
 	c  *gin.Context
-	rc *http.ResponseController
 	mu sync.Mutex
 }
 
@@ -140,34 +124,12 @@ type eventStream struct {
 // Headers go out before the first write and the stream is flushed immediately,
 // so a proxy holding the response until it looks complete can't reintroduce
 // exactly the silence streaming exists to remove.
-//
-// Taking the write deadline off the server's absolute WriteTimeout and onto a
-// per-write one is what makes a turn longer than 30s possible at all (#78).
-// WriteTimeout is an ABSOLUTE deadline from when the request header was read,
-// not an idle timeout, so a streaming response is cut mid-turn however recently
-// it wrote. Without this the 4-minute runTimeout is unreachable and the
-// keep-alive below tops out at one tick — pacing a connection that is destroyed
-// underneath it.
-//
-// That failure is INVISIBLE from in here: writes past the deadline return
-// err == nil and their bytes are dropped, so there is nothing to detect on the
-// write path. Only the client sees it, as a truncated stream it reports as a
-// dropped connection. Hence a deadline set up front and refreshed per frame,
-// rather than anything checked after the fact.
 func openEventStream(c *gin.Context) *eventStream {
 	c.Header("Content-Type", "text/event-stream")
 	c.Header("Cache-Control", "no-cache")
 	c.Header("X-Accel-Buffering", "no")
-	s := &eventStream{c: c, rc: http.NewResponseController(c.Writer)}
-	// Probe once here rather than reporting per frame: a writer that can't take
-	// deadlines will fail identically on every write, and the operator needs to
-	// hear it once. If this fails the stream still works — it is just back to
-	// being cut at WriteTimeout, which is worth saying out loud.
-	if err := s.rc.SetWriteDeadline(time.Now().Add(sseWriteTimeout)); err != nil {
-		slog.Error("api: SSE write deadlines unavailable; long turns will be truncated at the server WriteTimeout", "error", err)
-	}
 	c.Writer.Flush()
-	return s
+	return &eventStream{c: c}
 }
 
 func (s *eventStream) send(ev chatEvent) {
@@ -182,11 +144,6 @@ func (s *eventStream) send(ev chatEvent) {
 func (s *eventStream) write(frame string) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	// Refresh for THIS write, so the stream as a whole is unbounded but no single
-	// write is. Error deliberately unchecked: openEventStream already reported
-	// whether deadlines work at all, and this call can only fail the same way, so
-	// checking here would log once per frame to say the same thing.
-	_ = s.rc.SetWriteDeadline(time.Now().Add(sseWriteTimeout))
 	_, _ = io.WriteString(s.c.Writer, frame)
 	s.c.Writer.Flush()
 }

internal/api/seed_lots_test.go

@@ -0,0 +1,264 @@
+package api
+
+import (
+	"encoding/json"
+	"net/http"
+	"strconv"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+)
+
+func seedLotPath(id int64) string {
+	return "/api/v1/seed-lots/" + strconv.FormatInt(id, 10)
+}
+
+// decodeList decodes a bare JSON array body. Seed lot listing returns the array
+// directly rather than wrapping it (unlike /journal's {"entries": …}), so a
+// helper that assumed an object would quietly read nothing.
+func decodeList(t *testing.T, body []byte) []any {
+	t.Helper()
+	var out []any
+	if err := json.Unmarshal(body, &out); err != nil {
+		t.Fatalf("decode list: %v (%s)", err, body)
+	}
+	return out
+}
+
+// createPlantAPI makes a custom plant and returns its id.
+func createPlantAPI(t *testing.T, r *gin.Engine, cookie *http.Cookie, name string, spacing float64) int64 {
+	t.Helper()
+	w := doJSON(t, r, http.MethodPost, "/api/v1/plants", map[string]any{
+		"name": name, "category": "vegetable", "spacingCm": spacing, "color": "#4a7c3f", "icon": "🌱",
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create plant %q: status %d, body %s", name, w.Code, w.Body.String())
+	}
+	return int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+}
+
+// TestSeedLotCrudAPI walks the whole seed lot lifecycle over HTTP.
+//
+// It exists for the reason CLAUDE.md gives: service tests cannot see a route
+// that was never registered, or one registered with the wrong :param name. Every
+// other handler file had a sibling API test; this group did not, which is the
+// state PATCH/DELETE /journal/:id shipped in — implemented, unit-tested, and
+// completely unreachable.
+func TestSeedLotCrudAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	cookie := registerAndCookie(t, r, "[email protected]")
+	plantID := createPlantAPI(t, r, cookie, "Music Garlic", 15)
+
+	// Create.
+	w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{
+		"plantId": plantID, "vendor": "Johnny's", "sku": "2761",
+		"quantity": 100, "unit": "seeds", "packedForYear": 2026, "costCents": 495,
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create: status %d, body %s", w.Code, w.Body.String())
+	}
+	lot := decodeMap(t, w.Body.Bytes())
+	id := int64(lot["id"].(float64))
+	if lot["vendor"] != "Johnny's" || lot["unit"] != "seeds" {
+		t.Errorf("unexpected lot: %+v", lot)
+	}
+
+	// GET by id — the route most likely to be missing or mis-registered.
+	w = doJSON(t, r, http.MethodGet, seedLotPath(id), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("get: status %d, body %s", w.Code, w.Body.String())
+	}
+	if got := decodeMap(t, w.Body.Bytes()); int64(got["id"].(float64)) != id {
+		t.Errorf("get returned id %v, want %d", got["id"], id)
+	}
+
+	// List, and the ?plantId= filter.
+	w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots", nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("list: status %d, body %s", w.Code, w.Body.String())
+	}
+	if n := len(decodeList(t, w.Body.Bytes())); n != 1 {
+		t.Fatalf("list returned %d lots, want 1: %s", n, w.Body.String())
+	}
+
+	other := createPlantAPI(t, r, cookie, "Cherokee Purple", 45)
+	w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots?plantId="+strconv.FormatInt(other, 10), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("filtered list: status %d, body %s", w.Code, w.Body.String())
+	}
+	if n := len(decodeList(t, w.Body.Bytes())); n != 0 {
+		t.Errorf("filter by a plant with no lots returned %d, want 0", n)
+	}
+	// A bad plantId filter is 400, whether non-numeric or out of range — the
+	// handler rejects id < 1, not just unparseable strings.
+	for _, bad := range []string{"nope", "0", "-1"} {
+		if w := doJSON(t, r, http.MethodGet, "/api/v1/seed-lots?plantId="+bad, nil, cookie); w.Code != http.StatusBadRequest {
+			t.Errorf("plantId=%q filter = %d, want 400", bad, w.Code)
+		}
+	}
+
+	// PATCH with the current version.
+	w = doJSON(t, r, http.MethodPatch, seedLotPath(id), map[string]any{
+		"vendor": "Fedco", "version": lot["version"],
+	}, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("patch: status %d, body %s", w.Code, w.Body.String())
+	}
+	updated := decodeMap(t, w.Body.Bytes())
+	if updated["vendor"] != "Fedco" {
+		t.Errorf("vendor = %v, want Fedco", updated["vendor"])
+	}
+	if updated["version"].(float64) != lot["version"].(float64)+1 {
+		t.Errorf("patch didn't bump version: %v -> %v", lot["version"], updated["version"])
+	}
+
+	// A stale version conflicts and carries the current row back, so the client
+	// can rebase without a second request.
+	w = doJSON(t, r, http.MethodPatch, seedLotPath(id), map[string]any{
+		"vendor": "stale", "version": lot["version"],
+	}, cookie)
+	if w.Code != http.StatusConflict {
+		t.Fatalf("stale patch: status %d, want 409", w.Code)
+	}
+	if cur, ok := decodeMap(t, w.Body.Bytes())["current"].(map[string]any); !ok || cur["vendor"] != "Fedco" {
+		t.Errorf("409 body missing the current row: %s", w.Body.String())
+	}
+
+	// DELETE.
+	if w := doJSON(t, r, http.MethodDelete, seedLotPath(id), nil, cookie); w.Code != http.StatusNoContent {
+		t.Fatalf("delete: status %d, body %s", w.Code, w.Body.String())
+	}
+	if w := doJSON(t, r, http.MethodGet, seedLotPath(id), nil, cookie); w.Code != http.StatusNotFound {
+		t.Errorf("get after delete = %d, want 404", w.Code)
+	}
+}
+
+// TestSeedLotRemainingIsDerivedAPI checks that `remaining` reflects plantings
+// through the HTTP surface, not just in the service.
+//
+// DESIGN.md makes derivation load-bearing — "a decremented column drifts the
+// moment a planting is edited behind its back" — so a route that returned a
+// stored or stale figure would break the invariant silently, and the number is
+// the whole reason anyone opens the seed shelf.
+func TestSeedLotRemainingIsDerivedAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	cookie := registerAndCookie(t, r, "[email protected]")
+	plantID := createPlantAPI(t, r, cookie, "Beans", 10)
+
+	w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{
+		"plantId": plantID, "quantity": 50, "unit": "seeds",
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create lot: status %d, body %s", w.Code, w.Body.String())
+	}
+	lotID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+
+	gid := createGardenAPI(t, r, cookie, "G")
+	w = doJSON(t, r, http.MethodPost, objectsPath(gid), map[string]any{
+		"kind": "bed", "widthCm": 100, "heightCm": 100, "plantable": true,
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create object: status %d, body %s", w.Code, w.Body.String())
+	}
+	oid := int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+
+	// Plant 12 of them against the lot.
+	w = doJSON(t, r, http.MethodPost, objectPlantingsPath(oid), map[string]any{
+		"plantId": plantID, "xCm": 0, "yCm": 0, "radiusCm": 20, "count": 12, "seedLotId": lotID,
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create planting: status %d, body %s", w.Code, w.Body.String())
+	}
+
+	w = doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, cookie)
+	if w.Code != http.StatusOK {
+		t.Fatalf("get lot: status %d, body %s", w.Code, w.Body.String())
+	}
+	got := decodeMap(t, w.Body.Bytes())
+	if rem, ok := got["remaining"].(float64); !ok || rem != 38 {
+		t.Errorf("remaining = %v, want 38 (50 bought - 12 planted): %s", got["remaining"], w.Body.String())
+	}
+
+	// Over-plant it: 45 more (57 total against 50 bought) drives remaining
+	// NEGATIVE. That's deliberate — the number is a derived truth about what
+	// you've committed, not a floor clamped at zero, and "you've planted more than
+	// you bought" is exactly the signal a gardener wants rather than a hidden -7.
+	w = doJSON(t, r, http.MethodPost, objectPlantingsPath(oid), map[string]any{
+		"plantId": plantID, "xCm": 40, "yCm": 40, "radiusCm": 20, "count": 45, "seedLotId": lotID,
+	}, cookie)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("over-plant: status %d, body %s", w.Code, w.Body.String())
+	}
+	w = doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, cookie)
+	if rem, ok := decodeMap(t, w.Body.Bytes())["remaining"].(float64); !ok || rem != -7 {
+		t.Errorf("remaining after over-planting = %v, want -7 (50 - 57)", rem)
+	}
+}
+
+// TestSeedLotsArePrivateAPI checks the ACL through the router.
+//
+// Lots are private to the buyer and deliberately never travel with a shared
+// garden, so another user must not be able to read or edit one. Per the
+// project's convention, no-access is ErrNotFound rather than ErrForbidden —
+// existence is masked — so every one of these is a 404, not a 403.
+func TestSeedLotsArePrivateAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	alice := registerAndCookie(t, r, "[email protected]")
+	bob := registerAndCookie(t, r, "[email protected]")
+
+	plantID := createPlantAPI(t, r, alice, "Alice's garlic", 15)
+	w := doJSON(t, r, http.MethodPost, "/api/v1/seed-lots", map[string]any{
+		"plantId": plantID, "vendor": "Secret Vendor", "quantity": 10, "unit": "bulbs",
+	}, alice)
+	if w.Code != http.StatusCreated {
+		t.Fatalf("alice create: status %d, body %s", w.Code, w.Body.String())
+	}
+	lotID := int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
+
+	for _, tc := range []struct {
+		name   string
+		method string
+		body   any
+	}{
+		{"get", http.MethodGet, nil},
+		{"patch", http.MethodPatch, map[string]any{"vendor": "hijacked", "version": 1}},
+		{"delete", http.MethodDelete, nil},
+	} {
+		if w := doJSON(t, r, tc.method, seedLotPath(lotID), tc.body, bob); w.Code != http.StatusNotFound {
+			t.Errorf("bob %s = %d, want 404 (existence is masked)", tc.name, w.Code)
+		}
+	}
+
+	// Bob's own listing must not include it either.
+	w = doJSON(t, r, http.MethodGet, "/api/v1/seed-lots", nil, bob)
+	if w.Code != http.StatusOK {
+		t.Fatalf("bob list: status %d", w.Code)
+	}
+	if n := len(decodeList(t, w.Body.Bytes())); n != 0 {
+		t.Errorf("bob sees %d of alice's lots, want 0: %s", n, w.Body.String())
+	}
+
+	// And it's still intact for alice.
+	if w := doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, alice); w.Code != http.StatusOK {
+		t.Errorf("alice lost access to her own lot: %d", w.Code)
+	}
+}
+
+// TestSeedLotsRequireAuthAPI: the group is behind requireAuth, and an
+// unauthenticated caller gets 401 rather than an empty list.
+func TestSeedLotsRequireAuthAPI(t *testing.T) {
+	r := authEngine(t, localCfg())
+	for _, tc := range []struct {
+		method, path string
+	}{
+		{http.MethodGet, "/api/v1/seed-lots"},
+		{http.MethodPost, "/api/v1/seed-lots"},
+		{http.MethodGet, seedLotPath(1)},
+		{http.MethodPatch, seedLotPath(1)},
+		{http.MethodDelete, seedLotPath(1)},
+	} {
+		if w := doJSON(t, r, tc.method, tc.path, nil, nil); w.Code != http.StatusUnauthorized {
+			t.Errorf("%s %s = %d, want 401", tc.method, tc.path, w.Code)
+		}
+	}
+}

internal/api/sse_deadline_test.go

@@ -1,101 +0,0 @@
-package api
-
-import (
-	"bufio"
-	"net/http/httptest"
-	"strings"
-	"testing"
-	"time"
-
-	"github.com/gin-gonic/gin"
-)
-
-// streamFrames spins up a real http.Server with the given WriteTimeout and an
-// SSE handler that emits `frames` data frames, one every `tick`, then returns.
-// It reports how many frames the client actually received and any read error —
-// the only vantage point from which the deadline failures in #78/#87 are
-// visible, since the writes themselves return nil when the bytes are dropped.
-func streamFrames(t *testing.T, serverWriteTimeout, tick time.Duration, frames int) (int, error) {
-	t.Helper()
-	gin.SetMode(gin.TestMode)
-	r := gin.New()
-	r.GET("/stream", func(c *gin.Context) {
-		s := openEventStream(c)
-		for i := 0; i < frames; i++ {
-			time.Sleep(tick)
-			s.send(chatEvent{Error: "frame"})
-		}
-	})
-
-	srv := httptest.NewUnstartedServer(r)
-	srv.Config.WriteTimeout = serverWriteTimeout
-	srv.Start()
-	defer srv.Close()
-
-	resp, err := srv.Client().Get(srv.URL + "/stream")
-	if err != nil {
-		t.Fatalf("get: %v", err)
-	}
-	defer resp.Body.Close()
-
-	got := 0
-	sc := bufio.NewScanner(resp.Body)
-	for sc.Scan() {
-		if strings.HasPrefix(sc.Text(), "data: ") {
-			got++
-		}
-	}
-	return got, sc.Err()
-}
-
-// TestEventStreamOutlivesServerWriteTimeout is the regression test for #78.
-//
-// http.Server.WriteTimeout is an ABSOLUTE deadline measured from when the
-// request header was read — not an idle timeout — so a streaming response is cut
-// once it passes, however recently the handler wrote. pansy sets it to 30s while
-// an agent turn may run for minutes. openEventStream must override it.
-//
-// This has to be asserted from the CLIENT side because the failure cannot be
-// observed from the handler: writes made after the deadline return err == nil
-// and their bytes are silently discarded. A test that checked the return of
-// io.WriteString would pass against the bug.
-func TestEventStreamOutlivesServerWriteTimeout(t *testing.T) {
-	// sseWriteTimeout stays at its 30s default here, so the per-frame refresh
-	// keeps the stream alive with a huge margin — CI slowness only ever makes
-	// this pass more surely. The server's 300ms WriteTimeout is the thing being
-	// overridden; frames straddle it (300ms/600ms/900ms).
-	got, err := streamFrames(t, 300*time.Millisecond, 300*time.Millisecond, 3)
-	if err != nil {
-		t.Errorf("client read error after %d/3 frames: %v", got, err)
-	}
-	if got != 3 {
-		t.Errorf("client received %d frames, want 3 — the stream was cut at the server WriteTimeout", got)
-	}
-}
-
-// TestEventStreamRefreshesDeadlinePerFrame guards the #87 fix specifically: the
-// write deadline is refreshed on EVERY frame, not set once.
-//
-// A set-once deadline is a plausible "simplification" and it reintroduces the
-// unbounded-block risk the per-frame refresh exists to prevent — yet it would
-// sail through the test above, whose whole run is far under sseWriteTimeout. So
-// shrink sseWriteTimeout below the stream's total duration and send frames whose
-// gap stays comfortably under it: per-frame refresh delivers them all, while a
-// deadline set once at open would expire mid-stream and cut it short.
-func TestEventStreamRefreshesDeadlinePerFrame(t *testing.T) {
-	orig := sseWriteTimeout
-	sseWriteTimeout = 400 * time.Millisecond
-	t.Cleanup(func() { sseWriteTimeout = orig })
-
-	// The server WriteTimeout is generous (5s), so it isn't the limiter — the
-	// per-frame sseWriteTimeout is. 8 frames at a 100ms tick span 800ms, well past
-	// the 400ms deadline, but each 100ms gap is a 4× margin under it.
-	got, err := streamFrames(t, 5*time.Second, 100*time.Millisecond, 8)
-	if err != nil {
-		t.Errorf("client read error after %d/8 frames: %v", got, err)
-	}
-	if got != 8 {
-		t.Errorf("client received %d frames, want 8 — a set-once deadline would cut the stream at ~%v; the refresh must be per-frame",
-			got, sseWriteTimeout)
-	}
-}