Address third round of Gadfly findings on #76

- hexCenters now derives its exact point count BEFORE building anything and
  returns it alongside the points, refusing over the cap without allocating.
  Previously it materialised the whole lattice and fillLoaded checked len()
  afterwards — so the "too large" path paid for the thing it was rejecting.
  This also makes the preallocation exact, which subsumes the earlier
  over-allocation finding I'd declined: staggered rows hold cols-1, so
  rows*cols over-reserved by ~12%.

- Region.empty() names the invariant that clampTo expresses "no overlap" by
  INVERTING the region rather than zeroing it. A bare `MaxX < MinX` at each
  call site was spreading a non-obvious convention across three functions.

The count is now load-bearing (it gates the cap), so the test asserts it
matches what actually gets built.

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

CLAUDE.md

@@ -129,6 +129,21 @@ fix what's real → merge when the pipeline is green. Do not grade Gadfly findin
 A push to `main` builds the image and deploys to Komodo; the live instance at
 `pansy.orgrimmar.dudenhoeffer.casa` updates a few minutes later.
 
+**Gadfly reviews the PR as opened, not as merged.** The workflow triggers on
+`opened`/`reopened`/`ready_for_review` — deliberately *not* `synchronize` — so
+every commit you push afterwards, including the ones you push in response to
+Gadfly itself, is unreviewed unless you ask. Once you've stopped pushing and
+before you merge, comment **`@gadfly review`** on the PR to re-trigger it. The
+phrase is required, and this is not hypothetical: on #76 the follow-up commit
+was the one that contained a real bug.
+
+**A skipped Gadfly run reports success.** A comment without the trigger phrase
+still starts the workflow, which logs `comment does not contain trigger phrase`
+and exits green in ~2 seconds. So "the pipeline is green" does NOT mean "this
+was reviewed". Confirm a re-review actually ran by checking that it posted a new
+consensus comment — or that the run took minutes rather than seconds — not by
+its status.
+
 Workflow- and config-only changes (CI, this file, docs) go straight to `main`
 without the PR dance.
 

internal/service/ops.go

@@ -29,10 +29,8 @@ type Region struct {
 }
 
 // clampTo intersects the region with an object's local bounds (±halfW, ±halfH),
-// so a fill can't plant outside the object it was aimed at.
-//
-// Note this INVERTS (Max ends up below Min) rather than empties a region that
-// misses the object altogether — hexCenters treats that as "nothing to plant".
+// so a fill can't plant outside the object it was aimed at. A region that misses
+// the object entirely comes back empty — see empty().
 func (r Region) clampTo(halfW, halfH float64) Region {
 	return Region{
 		MinX: math.Max(r.MinX, -halfW), MinY: math.Max(r.MinY, -halfH),
@@ -40,6 +38,16 @@ func (r Region) clampTo(halfW, halfH float64) Region {
 	}
 }
 
+// empty reports whether the region encloses nothing.
+//
+// This exists because clampTo expresses "no overlap" by INVERTING the region —
+// Max clamps below Min — rather than by zeroing it, which is not something a
+// reader guesses. Naming it once here beats a bare `MaxX < MinX` at each place
+// that has to care.
+func (r Region) empty() bool {
+	return r.MaxX < r.MinX || r.MaxY < r.MinY
+}
+
 // rect builds a rectangular region.
 func rect(minX, minY, maxX, maxY float64) Region {
 	return Region{MinX: minX, MinY: minY, MaxX: maxX, MaxY: maxY}
@@ -142,8 +150,8 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde
 	}
 
 	region = region.clampTo(o.WidthCM/2, o.HeightCM/2)
-	centers := hexCenters(region, radius, spacing)
-	if len(centers) > maxFillPlops {
+	centers, total := hexCenters(region, radius, spacing, maxFillPlops)
+	if total > maxFillPlops {
 		return nil, domain.ErrInvalidInput // region too large for this spacing; ask for less
 	}
 
@@ -206,16 +214,19 @@ type localPoint struct{ x, y float64 }
 // Do not "simplify" this back to anchoring at the region's min corner. That is
 // what #75 was: staggered rows start a full pitch in, and the leftover all lands
 // on the far edge, where clumps hang outside a bed that nothing clips them to.
-func hexCenters(r Region, radius, spacing float64) []localPoint {
+// It reports the total alongside the points, and works that total out BEFORE
+// building anything: a fill large enough to be refused shouldn't allocate its
+// whole lattice first just to be counted and thrown away. Over `limit` it
+// returns (nil, total) so the caller can refuse with the real number.
+func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int) {
 	if radius <= 0 {
-		return nil
+		return nil, 0
 	}
-	// clampTo INVERTS a region that lies wholly outside the object (Max clamps
-	// below Min), and an inverted region has no inside to plant. The old
-	// loop-until-past-MaxX form got this for free by never entering the loop;
-	// counting positions up front does not, and would site a plop off the bed.
-	if r.MaxX < r.MinX || r.MaxY < r.MinY {
-		return nil
+	// An empty region has no inside to plant. The old loop-until-past-MaxX form
+	// got this for free by never entering the loop; counting positions up front
+	// does not, and would site a plop off the bed.
+	if r.empty() {
+		return nil, 0
 	}
 	pitch := 2 * radius
 	rowH := pitch * math.Sqrt(3) / 2
@@ -228,7 +239,19 @@ func hexCenters(r Region, radius, spacing float64) []localPoint {
 	rows, y0 := fitAxis(r.MaxY-r.MinY, rowH, inset)
 	cols, x0 := fitAxis(r.MaxX-r.MinX, pitch, inset)
 
-	pts := make([]localPoint, 0, rows*cols)
+	// Exact, not an upper bound: staggered rows hold one fewer, so rows*cols would
+	// over-reserve by ~12% — and, more to the point, allocating it is the thing we
+	// are trying to avoid when the answer is "too many".
+	staggered := cols
+	if cols > 1 {
+		staggered = cols - 1
+	}
+	total := (rows+1)/2*cols + rows/2*staggered
+	if total > limit {
+		return nil, total
+	}
+
+	pts := make([]localPoint, 0, total)
 	for row := 0; row < rows; row++ {
 		y := r.MinY + y0 + float64(row)*rowH
 		n, x := cols, r.MinX+x0
@@ -236,13 +259,13 @@ func hexCenters(r Region, radius, spacing float64) []localPoint {
 		// and centering THAT run puts it exactly half a pitch off its neighbours.
 		// A single-column region has nothing to stagger against.
 		if row%2 == 1 && cols > 1 {
-			n, x = cols-1, r.MinX+x0+radius
+			n, x = staggered, r.MinX+x0+radius
 		}
 		for i := 0; i < n; i++ {
 			pts = append(pts, localPoint{x + float64(i)*pitch, y})
 		}
 	}
-	return pts
+	return pts, total
 }
 
 // fitAxis returns how many lattice positions fit along a span at `step`, keeping

internal/service/ops_test.go

@@ -96,10 +96,15 @@ func TestHexCentersEdgeInset(t *testing.T) {
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			r := rect(-tc.w/2, -tc.h/2, tc.w/2, tc.h/2)
-			pts := hexCenters(r, tc.radius, tc.spacing)
+			pts, total := hexCenters(r, tc.radius, tc.spacing, maxFillPlops)
 			if len(pts) == 0 {
 				t.Fatal("no centers")
 			}
+			// The count is derived up front so an oversized fill is refused without
+			// building its lattice — which only works if it matches what gets built.
+			if total != len(pts) {
+				t.Errorf("reported total %d, built %d", total, len(pts))
+			}
 
 			// A clump may cross the edge, but only by the half-spacing the rule
 			// allows — never enough to be mostly out in the path.
@@ -168,7 +173,7 @@ func TestHexCentersTinyRegion(t *testing.T) {
 		{"off in a corner", rect(20, -40, 30, -30), 25, -35},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			pts := hexCenters(tc.r, 15, 10)
+			pts, _ := hexCenters(tc.r, 15, 10, maxFillPlops)
 			if len(pts) != 1 || pts[0].x != tc.wantX || pts[0].y != tc.wantY {
 				t.Errorf("got %+v, want one plop at (%v,%v)", pts, tc.wantX, tc.wantY)
 			}