From 958b90ebc655df5f312bbc7ed8a8fed0a6819524 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 14:03:41 -0400 Subject: [PATCH] Address fourth round of Gadfly findings on #76 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Split hexCenters' doc: the count/limit contract had run straight on from the #75 anti-regression paragraph with no separator, so its opening "It" read as referring to the wrong thing. - Write the stagger as pitch/2 rather than radius. Same value, but the intent is "half a pitch" and only incidentally "one radius". - fitAxis's step<=0 guard is unreachable from its only caller. Kept, and now says so: a helper this small shouldn't need its caller read to be shown safe, and the failure mode without it is ±Inf into an int conversion. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- CLAUDE.md | 7 ++++--- internal/service/ops.go | 19 ++++++++++++++----- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ce29833..c263429 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -140,9 +140,10 @@ 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. +was reviewed". Confirm a re-review actually ran by its **duration** — a real +pass takes ~10 minutes, a skip takes 2 seconds. Don't look for a new consensus +comment: Gadfly EDITS its existing status-board and consensus comments in place, +so their `created_at` stays at the first review and only `updated_at` moves. Workflow- and config-only changes (CI, this file, docs) go straight to `main` without the PR dance. diff --git a/internal/service/ops.go b/internal/service/ops.go index 2fa05df..08e4b5f 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -214,10 +214,13 @@ 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. -// 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. +// +// # Counting before building +// +// hexCenters returns 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 still refuse with the real number. func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int) { if radius <= 0 { return nil, 0 @@ -259,7 +262,7 @@ func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int // 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 = staggered, r.MinX+x0+radius + n, x = staggered, r.MinX+x0+pitch/2 } for i := 0; i < n; i++ { pts = append(pts, localPoint{x + float64(i)*pitch, y}) @@ -276,6 +279,12 @@ func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int // A span too small to hold even one position at that inset still gets one, in the // middle: filling a bed narrower than a single plop with one plop is a better // answer than refusing to plant it. +// +// The step<=0 half of that guard is currently unreachable — hexCenters, the only +// caller, returns early unless radius > 0, which makes both steps it passes +// positive. It stays because dividing by a non-positive step yields ±Inf and then +// a garbage int conversion, and a helper this small should not require reading +// its caller to know it is safe. Deliberate, not an oversight. func fitAxis(length, step, inset float64) (n int, start float64) { if step <= 0 || length < 2*inset { return 1, length / 2