From 70ff9706720d46b54e0ae364f4660892cceb1ad7 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 12:13:50 -0400 Subject: [PATCH] Address second round of Gadfly findings on #76 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - FillRegion's doc still said "half-pitch inset", left over from the first draft of the fix; the inset is radius - spacing/2. Two docs on the same function disagreeing is worse than either being terse. - Reject non-finite region bounds. They survive clamping and the inverted- region guard (NaN compares false both ways). Nothing corrupt reached the table — SQLite stores NaN as NULL and NOT NULL refuses it — but NaN surfaced as a raw store error and +Inf as a silent zero-plop success. - TestHexCentersTinyRegion used a region symmetric about the origin, so it could not distinguish "the middle of the region" from "the origin" and would have passed for an implementation that just returned (0,0). Added an off-centre case. Not taken: the finding that `make(..., rows*cols)` over-allocates ~12% because staggered rows hold cols-1. True, but the slice is capped at maxFillPlops (5000) and the exact count needs a ceil/floor split for no measurable gain. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/service/ops.go | 21 ++++++++++++--- internal/service/ops_test.go | 51 +++++++++++++++++++++++++++++++++--- 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/internal/service/ops.go b/internal/service/ops.go index f1e18be..4e3cab7 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -92,10 +92,11 @@ func defaultPlopRadius(spacingCM float64) float64 { // FillRegion lays a hex-packed field of plops of one plant across a region of a // plantable object the actor can edit. Plop radius comes from the plant's spacing // (or spacingOverride) via defaultPlopRadius; centers sit on a hex lattice at 2× -// radius pitch, centered in the region with a half-pitch inset at each edge (see -// hexCenters for why that inset and not a full one). A candidate is skipped when -// its plop would sit entirely inside an existing active plop (so re-filling -// doesn't stack duplicates). Returns the plops it created. +// radius pitch, centered in the region, and set in from each edge by the plop's +// radius less half a spacing — see hexCenters for why that half-spacing is what +// the edge is owed. A candidate is skipped when its plop would sit entirely +// inside an existing active plop (so re-filling doesn't stack duplicates). +// Returns the plops it created. func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, region Region, plantID int64, spacingOverride *float64) ([]domain.Planting, error) { o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor) if err != nil { @@ -128,6 +129,18 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde return nil, domain.ErrInvalidInput } + // A caller-supplied region is arbitrary floats, and non-finite ones survive + // everything downstream: clamping keeps them, the inverted-region guard can't + // see NaN (it compares false both ways), and fitAxis centres on them happily. + // Nothing corrupt reaches the table — SQLite stores NaN as NULL and the NOT + // NULL constraint refuses it — but the caller gets an opaque store error for + // NaN, and for +Inf a silent zero-plop success. Both are lies about what went + // wrong; say "bad input" here instead. + if !isFinite(region.MinX) || !isFinite(region.MinY) || + !isFinite(region.MaxX) || !isFinite(region.MaxY) { + return nil, domain.ErrInvalidInput + } + region = region.clampTo(o.WidthCM/2, o.HeightCM/2) centers := hexCenters(region, radius, spacing) if len(centers) > maxFillPlops { diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index 18e6939..af84de1 100644 --- a/internal/service/ops_test.go +++ b/internal/service/ops_test.go @@ -154,10 +154,55 @@ func TestHexCentersEdgeInset(t *testing.T) { // TestHexCentersTinyRegion covers a region too small to hold a plop at the // half-pitch inset: planting one in the middle beats refusing to plant at all. +// +// The off-centre case earns its place — a region symmetric about the origin +// can't tell "the middle of the region" from "the origin", so on its own it +// would pass for an implementation that just returned (0,0). func TestHexCentersTinyRegion(t *testing.T) { - pts := hexCenters(rect(-5, -5, 5, 5), 15, 10) - if len(pts) != 1 || pts[0].x != 0 || pts[0].y != 0 { - t.Errorf("tiny region gave %+v, want a single centered plop", pts) + for _, tc := range []struct { + name string + r Region + wantX, wantY float64 + }{ + {"centred on the origin", rect(-5, -5, 5, 5), 0, 0}, + {"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) + 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) + } + }) + } +} + +// TestFillRegionRejectsNonFiniteRegion: non-finite bounds survive clamping and +// the inverted-region guard (NaN compares false both ways). Without the explicit +// check, NaN surfaced as a raw store error ("NOT NULL constraint failed") and +// +Inf as a silent success that planted nothing — neither of which tells the +// caller what it actually did wrong. +func TestFillRegionRejectsNonFiniteRegion(t *testing.T) { + ctx := context.Background() + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g, _ := s.CreateGarden(ctx, owner, GardenInput{Name: "Big", WidthCM: 2000, HeightCM: 2000}) + bed := seedFillBed(t, s, owner, g.ID, 100, 100) + plant := seedOwnPlant(t, s, owner, 10) + + nan := math.NaN() + for _, r := range []Region{ + {MinX: nan, MinY: -50, MaxX: 50, MaxY: 50}, + {MinX: -50, MinY: -50, MaxX: 50, MaxY: math.Inf(1)}, + } { + created, err := s.FillRegion(ctx, owner, bed.ID, r, plant.ID, nil) + if !errors.Is(err, domain.ErrInvalidInput) { + t.Errorf("FillRegion(%+v) err = %v, want ErrInvalidInput", r, err) + } + for _, p := range created { + if !isFinite(p.XCM) || !isFinite(p.YCM) { + t.Errorf("persisted a plop with non-finite coordinates: %+v", p) + } + } } }