- read_journal now takes an offset, so the hasMore it returns is
actionable — an agent can page a journal longer than 50 entries.
- remove_planting goes through a new service RemovePlanting that stamps
removed_at from s.now() (the injectable clock ClearObject and the fill
path use), instead of the adapter computing the date off the wall clock.
It delegates to UpdatePlanting, so the role check, version guard and
history record are unchanged. Drops the now-unused time import.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The toolbox could create and move but not delete or resize; write the
journal but not read it; clear a whole bed but not pull one plant; report
seed remaining but not record a purchase. Close those gaps with thin
adapters over the SAME service methods the REST API uses, so they inherit
the permission checks unchanged:
read_journal → ListJournal (the write/read asymmetry, most visible)
update_object → UpdateObject (resize / rotate / rename / plantable)
delete_object → DeleteObject (counterpart to create_object)
remove_planting → UpdatePlanting (soft-remove ONE plop, like clear does)
list_seed_lots → ListSeedLots
record_seed_lot → CreateSeedLot (record a purchase; "I bought 2 packets")
To address a single plop the agent needs its id + version, so
DescribePlanting now carries both — the same way DescribeObject.Version
already lets it edit an object. remove_planting soft-removes (removed_at =
today), mirroring clear_object, so the plant stays in planting history and
the change is undoable.
Deferred deliberately: an undo/revert tool needs a way to list recent
change sets to get a changeSetId, which is a larger addition; noted on the
issue for a follow-up.
Tested through the tool layer (TestCorrectiveTools): resize, single-plop
removal, journal read-back, seed-lot record+list, and delete.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Gadfly on #103:
- (4 models) applyOrientation allocated a WxH RGBA then threw it away for a
quarter-turn to allocate HxW. Compute the output dims once, allocate one
buffer.
- (perf) The At/Set loop boxed a color.Color per pixel — millions of heap
allocs on a full-res rotation. Convert to *image.RGBA once (draw.Draw)
then copy 4 bytes per pixel by offset. No boxing.
- (2 findings) exifOrientation didn't skip 0xFF fill bytes before a marker,
so a spec-valid padded APP1 would be misread. Skip them.
- (2 findings) orientationFromApp1 read the tag's inline value without
checking its type/count — a mistyped LONG/offset would be read as a
bogus SHORT. Require type=SHORT, count=1; also validate the TIFF 0x2A
magic agrees with the byte order.
- Tests now cover all 8 orientations (added the mirror/transpose cases
2/4/5/7, the most error-prone switch arms) as t.Run subtests.
All imagenorm tests green; gofmt clean; still CGO_ENABLED=0.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
imagenorm decoded and re-encoded to JPEG but ignored the EXIF Orientation
tag. Phone cameras store the sensor pixels one way and set an EXIF flag to
rotate on display, so a "portrait" JPEG is really a landscape bitmap tagged
"rotate 90°" — and our re-encode strips EXIF, so without baking the rotation
in, a packet photographed in portrait reaches the vision model sideways
(bad OCR) and any future thumbnail is wrong.
- exifOrientation: a small pure-Go parser that walks the JPEG APP1/Exif
segment for tag 0x0112, returning 1 (normal) for non-JPEG or unparseable
input — never guess a rotation onto a correct image. No cgo, no new dep.
- applyOrientation: bakes in all 8 orientations (the 4 rotations + mirrors)
after downscale (cheaper to rotate the small image; a 90° turn swaps the
sides but not the longest edge, so the downscale bound still holds).
- Non-JPEG paths (HEIC/webp/png) are untouched — their decoders own
orientation and they carry no JPEG EXIF.
Tests build oriented JPEGs (a corner marker + a spliced Exif APP1) and
assert the marker lands where each orientation says, dims swapping for the
quarter-turns; plus parser defaults for non-JPEG / no-EXIF / garbage.
Stays CGO_ENABLED=0.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Gadfly findings on #95:
- Correctness (3 models): the shared inset formula radius-spacing/2
collapses to 0 in grid mode (radius = spacing/2), so grid's outer row
planted flush on / overhanging the bed edge instead of the half-spacing
in that the rule wants. The inset genuinely differs by layout — a grid
plant sits AT the plop centre (inset spacing/2), a clump's plants reach
its rim (inset radius-spacing/2, overhanging by a half). Split it into a
new edgeInset(radius, spacing, layout); hexCenters now takes a
precomputed inset and is pure geometry (no spacing/layout knowledge).
Regression guard: grid plants land at ±25 on a 60cm bed, not ±30.
- Performance (2 findings): the in-loop `existing = append(existing, *p)`
was dead — every plop in one fill shares a radius and sits on a distinct
lattice point, and a plop is "covered" only when wholly inside another,
impossible between equal-radius circles at different centres. Removing it
stops the coveredByExisting scan growing during the fill (an empty-bed
grid fill's check was needlessly quadratic in the plop count).
- Docs: FillRegion/fillLoaded/hexCenters comments and the DESIGN.md bullets
updated for plopRadiusFor/edgeInset (were still citing defaultPlopRadius
and "written out in hexCenters").
- Test hygiene: split the grid + bad-layout cases out of TestFillAndClearAPI
into TestFillLayoutAPI (one concern per test).
The enum-tag finding is a non-issue: majordomo's DefineTool derives its arg
schema from the same struct-tag reflection as Generate (proven by the vision
SeedPacket enum), and the service validates mode regardless.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Gadfly findings on #94, the real ones:
- scanSeedPacket extends only the READ deadline; a slow upload + a live
vision call runs past the server's absolute 30s WriteTimeout and the
successful response is silently dropped (the #78 failure mode). Extend
the write deadline too (scanWriteTimeout).
- An oversized upload tripping MaxBytesReader was mapped to 400; it's 413.
Detect *http.MaxBytesError and report IMAGE_TOO_LARGE.
- Split imagenorm error mapping: ErrTooLarge->413, ErrUnsupported->400,
genuine read/encode faults (and a failed file.Open)->500, not 400.
- CreateFromPacket discarded the plant it created when the lot then
failed, contradicting its own doc. Roll the new plant back instead so
the confirm is all-or-nothing (a fresh plant has no lots/plantings, so
the delete is safe; log-and-continue on cleanup failure).
- Dedup: packetLotRequest and seedLotCreateRequest shared every lot
field. Extract a seedLotFields base both use. validCategory now reuses
plantCategories. EffectiveConfig resolves agent+vision from one
settings-row read instead of two.
- capabilities swallowed an EffectiveVision error silently; log it.
- vision test hand-copied Extract's body (drift risk). Split generate()
out of Extract so the hermetic test drives the real request builder.
Tests: rollback-on-lot-failure (service), oversized->413 (api).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Steve chose option 3: keep clumps as the default primitive, add a grid/rows
fill mode, so sketching and planning are different operations with different
outputs rather than one model forced to be both.
A plop is a CLUMP, not a plant — great for "a few plops of garlic in a corner",
useless for drawing a plantable 8-rows-of-garlic bed (that came out as ~15
blobs, #77). FillLayout selects what a fill packs:
- clump (default, unchanged): radius 1.5×spacing, ~7 plants per plop.
- grid: radius spacing/2, pitch = spacing, ONE plant per plop — rows you could
actually plant from.
The geometry is the SAME hexCenters lattice and the SAME #75 half-spacing edge
rule; only the radius→spacing relationship differs (plopRadiusFor). Grid keeps
no 15cm floor — its whole point is true spacing — while clump keeps it so a
tiny-spacing plant doesn't make invisible clumps.
Threaded through FillRegion/FillNamedRegion (empty layout = clump, so existing
callers are unchanged; unknown layout = ErrInvalidInput), the REST /fill
endpoint (`layout`), and the agent's fill_region tool (`mode`, enum clump|grid),
so "plant the bed in rows" works.
Tests: grid produces many more, single-plant plops than clump on the same bed
(radius spacing/2, derived count 1); unknown layout is refused at both the
service and the API. maxFillPlops still caps a grid fill of a huge bed.
No frontend fill affordance exists yet (fill is agent-only in the UI; the fill
UI was deferred in #82), so the mode toggle rides along when that's built —
noted. Docs: DESIGN placement-model decision.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Photograph a seed packet → it fills in the plant and the purchase. This is the
backend; the scan UI is a follow-up PR.
Vision model config (mirrors the agent model from #79):
- Migration 0011 adds instance_settings.vision_model; PANSY_VISION_MODEL is the
env default. Precedence Settings → env → empty; the KEY stays in the env.
- EffectiveVision resolves it; /capabilities advertises "vision" only when a
model + key are configured, so the UI offers the scan button only when it works.
Extraction is one-shot, NOT an agent loop (internal/vision):
- majordomo.Generate[SeedPacket] derives a JSON schema from the struct tags and
hands the image to the vision model; it can't call a tool, so it can't touch
the garden — it only reads a picture and returns data. Numeric fields are
pointers, so a field the packet doesn't print comes back nil, not a made-up 0.
- Hermetic test: majordomo's fake provider returns canned packet JSON and
Generate unmarshals it, image + derived schema included. No live model.
The image is normalized to JPEG at the upload boundary (imagenorm from #80),
which is where an iPhone HEIC becomes readable — majordomo's media path can't
decode HEIC. imagenorm now links into the binary (~7 MB, the cost #80 deferred).
The hard part is catalog matching, not OCR (internal/service/seed_packet.go):
- A wrong auto-match splits a variety's seed-lot history across duplicate rows,
so the service NEVER auto-creates. matchPlants surfaces RANKED candidates
(exact name → variety-in-name → same species, conservative and name-based),
the user confirms, and CreateFromPacket makes the plant (new or existing) + the
lot. Exactly one of plantId/newPlant, refused otherwise.
- Plants/lots aren't in the undo history (catalog/inventory), so no change set.
- The extractor is injectable (service.WithPacketExtractor) so ExtractSeedPacket
and the /scan endpoint test end to end against a fake, no live model.
Endpoints: POST /seed-lots/scan (multipart image → proposal, reads only; extends
the read deadline for a slow phone upload, caps the body, maps too-large/unreadable
to clear statuses) and POST /seed-lots/from-packet (confirmed proposal → 201).
Docs: README (PANSY_VISION_MODEL), DESIGN (routes + the decision and why the
model can't touch the garden).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Security / correctness (multi-model):
- Wrap the decoders in a recover (decodeSafely): a malformed HEIC/WebP that
panics libheif-via-WASM or x/image/webp now fails this one request as
ErrUnsupported instead of taking the process down.
- Make the pixel-bomb guard overflow-safe: check each side against maxDimension
BEFORE multiplying, so a header claiming ~2^32 on a side can't wrap
int64(w)*int64(h) negative and slip past the pixel-count cap.
- Lower maxDecodePixels 100 MP → 50 MP (~200 MB peak), still above any current
phone sensor, capping the amplification a small hostile header can force.
- Sentinel errors use errors.New, not fmt.Errorf without a verb.
The finding 5 models agreed on: the decompression-bomb guard was UNTESTED and a
stale comment implied otherwise. Added TestNormalizeRejectsPixelBomb, which
crafts a ~40-byte PNG (valid IHDR + CRC) claiming a huge canvas and asserts
ErrTooLarge from DecodeConfig alone, before any bitmap is allocated — covering
both the per-side and the area trip. Fixed the stale comment.
Error-handling / docs:
- format is now "" on ALL error paths (was populated on some, empty on others).
- Doc rewritten to state the actual error contract (read/encode I/O → wrapped,
not a sentinel) and to stop referencing a non-existent TestNormalize / TODO.
- Guard io.LimitReader's +1 against an int64 overflow at an absurd MaxBytes.
Tests / provenance:
- Downscale test derives its numbers from DefaultMaxDim instead of hard-coding.
- Check the previously-ignored DecodeConfig error in the small-image case.
- testdata/README documents where sample.heic/webp came from.
Deferred to the #81 upload handler, documented in the Normalize doc: EXIF
orientation (best fixed and tested with a real oriented photo end-to-end) and
context cancellation (image.Decode isn't cancellable mid-decode; the caller
runs it under a timeout, and the size guards keep the work finite regardless).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Prerequisite for seed-packet capture (#81). majordomo's media pipeline is
stdlib-based and cannot decode HEIC or webp — and HEIC is the iPhone camera
default, so the capture feature's very first input would fail on the device
that motivates it. Normalizing at the upload boundary means everything
downstream only ever sees JPEG.
internal/imagenorm.Normalize(reader, opts) decodes any of jpeg/png/heic/webp,
downscales to fit MaxDim (2048, ollama-cloud's limit) on the longest edge with
Catmull-Rom (sharp on the text a packet is mostly made of), and re-encodes JPEG.
CGO stays off: github.com/gen2brain/heic runs libheif as WASM via wazero (pure
Go), golang.org/x/image/webp is pure Go. Both register with image.Decode. The
~5 MB these add only links into the binary when #81 imports this package — it's
not pulled into cmd/pansy yet, so main is unchanged in size for now.
Guards against hostile uploads: input capped at MaxBytes (ErrTooLarge) before a
full read, and the decoded pixel count checked from DecodeConfig BEFORE the
bitmap is allocated, so a small file claiming a huge canvas (a decompression
bomb) is refused up front.
TestNormalizeAllFormats round-trips all four formats to a valid JPEG — its real
job is catching a dropped blank import, where the intuition-breaking failure is
that the COMMON format (png) breaks while the exotic one (heic) still works.
heic/webp fixtures live in testdata (Go has no encoder for them); the webp is a
known-good 16x16 from Python's stdlib test corpus, verified to decode.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Closes#79. Agent model + on/off move into an admin-only Settings section, and
is_admin is enforced for the first time. The live Runner is hot-swapped behind an
atomic.Pointer with routes always registered, so a settings change takes effect
with no restart; /capabilities reads the pointer. Secrets stay in the env, never
the DB. Precedence: Settings → env → default. Verified live: swap on/off/model,
bad spec → 400, race-clean. Gadfly blocking round addressed (dead code removed,
error-swallowing fixed, frontend 503 handling, holder dedup).
Closes#82. Bed filling now exists without an LLM configured, and clear-bed is
ONE change set instead of one per plop (a 40-plop bed no longer needs 40 undos).
POST /objects/:id/fill (region by compass name or rect, degenerate rect → 400)
and /clear, thin adapters over the service methods the agent already uses. Also
fixed DESIGN.md's API block, which had dropped the unauthenticated public route.
Closes#83 (seed-lots half). The only handler file without a sibling API test —
the exact state the journal routes shipped unreachable in. Full CRUD through the
router, derived-remaining (incl. negative/over-planted), private-to-owner ACL
(404 not 403), and requireAuth. Verified the tests catch an unregistered route.
- Remove config.AgentConfig.Ready() — dead after the refactor (no non-test
callers), and its doc comment ("route registration and capabilities gate on
this") was now false. EffectiveAgent.Ready() carries the same logic and IS
used, see below.
- agentHolder now uses eff.Ready() and eff.APIKey instead of an inline check and
a redundant apiKey field it held separately. This makes EffectiveAgent.APIKey/
Ready() production-used rather than test-only, drops the duplicated readiness
check, and simplifies newAgentHolder's signature.
- settingsPayload no longer swallows an EffectiveAgent error into a misleading
empty "effective" view (which would read as "nothing configured"). It returns
the error; the handlers surface it as a 500. EffectiveAgent re-reads the row
GetInstanceSettings just returned, so a failure there is a real DB fault.
- Frontend streamChat handles 503 (assistant disabled at runtime) distinctly
from 404 (endpoint absent) — the backend returns 503 AGENT_DISABLED now, which
the old code mislabelled.
- Fixed the api.go comment that still said a disabled request gets a 404 — it's
a 503.
- updateSettings uses the shared parseNullable[bool] instead of a bespoke
parseNullableBool (now removed).
- NewRunner drops its empty-spec guard; agentmodel.Resolve already owns that
check, so one place decides what a valid spec is.
- rebuild-on-resolve-error now documents WHY it keeps the current Runner rather
than tearing down a working assistant on a transient DB blip: the state is
persisted, the next rebuild reconciles, and killing a live assistant on a read
hiccup is worse than a brief stale window.
Not changed: the store's version-conflict fallback returning GetInstanceSettings'
error instead of ErrVersionConflict when that read also fails. That's the exact
pattern every other version-guarded update uses (gardens/objects/plants); making
only this one differ would be the inconsistency. A DB read failing immediately
after the guarded UPDATE on the same local SQLite file is a disk-fault edge case,
and surfacing that error is defensible.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- Rename fillRequest → objectFillRequest to match the package's
<resource><Action>Request convention (objectCreateRequest, gardenCreateRequest…).
- Add the missing anonymous-clear permission case (401), mirroring anonymous fill.
- Reword the makeFillPlant comment to stop referencing external branch state,
which won't make sense once merged — just note the consolidation follow-up.
- clearObject: send no body (undefined) rather than an empty {}, and validate
the {cleared} response with a zod schema instead of a raw type cast.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
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
Gadfly re-review: the regression test proved the stream outlives the server
WriteTimeout, but its whole run was under a second — far below the 30s
sseWriteTimeout — so it could NOT tell a per-frame refresh from a deadline set
once at open. A revert to set-once would reintroduce the unbounded-block risk
the per-frame refresh exists to prevent, and sail through the test.
Make sseWriteTimeout a var (production never reassigns it) so a test can shrink
it, and split into two focused cases sharing a streamFrames helper:
- TestEventStreamOutlivesServerWriteTimeout — server WriteTimeout 300ms, frames
straddling it, sseWriteTimeout left at 30s. Catches #78 (no deadline
management → server cuts the stream). Huge margin, so CI slowness only makes
it pass more surely — addresses the "timing margin tight" finding too.
- TestEventStreamRefreshesDeadlinePerFrame — sseWriteTimeout shrunk to 400ms,
8 frames at a 100ms tick (800ms total, 4× margin per gap). A per-frame refresh
delivers all 8; a set-once deadline expires mid-stream and cuts it.
Verified each fails against its own regression: removing the per-write refresh
gives 3/8 on the per-frame test (EOF at ~400ms); removing both deadline calls
gives 0/3 on the outlives test. Both pass on the real code, 3× under -count.
The "clear the deadline entirely" finding was already addressed by the previous
commit (per-frame refresh); this covers it against reintroduction.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- Cover the plantId filter's out-of-range branch (0, -1), not just the
non-numeric one — the handler rejects id < 1.
- Add the documented negative-remaining case: over-planting a lot (57 against
50 bought) drives remaining to -7 through the HTTP surface. The number is a
derived truth about what's committed, not a floor at zero, and that's the
signal a gardener wants.
- Rename objID → oid in the remaining test to match the package convention
(shares_test etc.).
Not taken: relocating createPlantAPI to plants_test.go — it's shared with #89's
branch and the move is cleanest once both land (consolidating with that branch's
makeFillPlant), noted on both PRs. The 409 "current" assertion style is
deliberate and reads clearly; matching journal_test.go's exact phrasing isn't
worth a divergence churn.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- Reject a degenerate (zero-area or inverted) fill rect with 400 instead of
silently planting a single plop at the object centre. An empty `"rect": {}`
decodes to all-zeros and is caught the same way. New fillRect.degenerate()
and a table test covering {}, zero-width, zero-height, and inverted.
- Make the rect a named type (fillRect) rather than an anonymous inline struct,
matching the rest of internal/api, and bind the pointer to a local in the
handler so the deref is visibly guarded rather than reading req.Rect.MinX
under an invariant from a line above.
- ops_test: drop gid from the unpack instead of the `_ = gid` shim.
- ClearBedModal: drop the `const n = plopCount` no-op alias.
Not taken: moving createPlantAPI/makeFillPlant to a shared helper — that
consolidation spans this branch and #88 and is cleanest once both land, as
noted in the PR. Reusing an objectPlantingsPath helper: there isn't one in this
package, and the one inline use doesn't earn a new helper on its own.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Moves the agent model out of env-only config into an admin-editable Settings
section, and enforces is_admin for the first time — it has been in the schema
since migration 0001, plumbed to the client, and checked nowhere.
Backend:
- Migration 0010: instance_settings, a single-row (CHECK id=1) table — pansy's
first instance-level state. Holds agent_model ('' = inherit env) and
agent_enabled (NULL = inherit env), version-guarded like every mutable row.
SECRETS STAY IN ENV: OLLAMA_CLOUD_API_KEY is never stored here.
- requireAdmin at the service seam (authoritative) plus a cheap middleware
early-403. Non-admin gets 403, not 404 — settings existence isn't masked.
- EffectiveAgent resolves DB-over-env (model, enabled); key always from env.
- The live Runner is hot-swapped, not built once. agentHolder holds it behind
an atomic.Pointer; the chat routes are now registered UNCONDITIONALLY and
nil-check agent.get(), so a settings change turns the assistant on/off/onto a
new model with no restart and no race against in-flight readers. /capabilities
reads the pointer, so it reports what's live, not what booted.
- internal/agentmodel is a new leaf package holding the one place that knows how
to turn a spec into a model. Both agent (to run) and service (to validate a
spec before storing it) import it; it can't live in agent, which imports
service. Settings PATCH validates the spec via Parse, so a typo is a 400 now
rather than a broken assistant on the next turn.
Frontend:
- /settings route (admin guard), a Settings page (model field, tri-state
enabled, live status), nav link shown only to admins.
- useCapabilities drops staleTime:Infinity — the assistant can now change under
a running page — and the settings save invalidates it.
Contract change: chat routes always exist, so "assistant off" is a runtime 503
+ capabilities:false, not a missing route. Updated the test that asserted the
old shape.
Verified live against the built binary: disable flips capabilities to false and
logs it; re-enable with a new model swaps it back; a bad spec is rejected 400;
the setting persists across a restart. Swap is race-clean under `go test -race`.
Docs: README (precedence + key-stays-in-env), DESIGN (decision + routes),
CLAUDE (don't re-add conditional route registration; key never in the DB).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Gadfly, security lens: clearing the write deadline outright traded the
truncation bug for an unbounded one. With no deadline, a client that stops
reading fills the socket buffer and blocks Write forever — pinning the agent
run goroutine and this stream's mutex, which also takes down the keep-alive
since it needs the same lock. The old 30s WriteTimeout at least bounded that.
Refresh the deadline per frame instead: the stream as a whole is unbounded,
but no single write is. That is the only shape that satisfies both ends, since
an absolute deadline cuts long turns and no deadline cannot be recovered from.
The probe stays in openEventStream so a writer that cannot take deadlines is
reported once rather than once per frame; the per-write call deliberately
ignores its error for the same reason.
Also widened the test's timing margins, per the second finding. tick is now
GREATER than writeTimeout, so every frame lands after the deadline has already
expired and the margin only ever grows — a loaded CI runner pushes this toward
passing rather than toward flaking. Sized the other way it would flake exactly
when CI is busiest. Passes 3/3 with -count=3.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
FillRegion and ClearObject were reachable only through the agent toolbox, so
on an instance with no model configured the most valuable bulk operation in a
garden planner — and the one carrying the most carefully reasoned geometry in
the codebase — did not exist at all. internal/service/ops.go said these lived
on *Service so "any future REST surface" would inherit the ACL checks; this is
that surface.
POST /objects/:id/fill takes a region EITHER by compass name ("all", "ne",
"south half") or as an explicit rect in the object's local frame, and refuses
both-or-neither: silently preferring one would make a client bug look like a
geometry bug. It answers 200 rather than 201 because a fill can legitimately
create nothing (the region is already planted) and there is no single resource
to point a Location at.
POST /objects/:id/clear replaces a client-side loop of PATCHes. That loop
wrote one change set per plop, so clearing a 40-plop bed took 40 presses of
Undo to put back — while the agent's clear_object, for the identical
user-facing action, undid in one click. CLAUDE.md states the rule it violated:
multi-row operations record together so they undo as one unit. Moving it
server-side also removes the partial-failure case the loop had to reconcile.
TestClearObjectIsOneChangeSetAPI pins that: fill a bed, count change sets,
clear it, assert the count rose by exactly one.
DESIGN.md's API block gains the two new routes, and the four it was already
missing: GET/POST/DELETE /gardens/:id/share-link and the unauthenticated
GET /public/gardens/:token. An unauthenticated surface absent from the
architecture doc is the one worth fixing on sight.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Every other handler file had a sibling API test; this group had none, which is
the state PATCH/DELETE /journal/:id shipped in — implemented, unit-tested
through the service, and completely unreachable. Service tests cannot see a
route that was never registered or one registered with the wrong :param.
Four tests, covering what a broken route would silently take with it:
- Full CRUD over HTTP, including GET/PATCH/DELETE by id, the ?plantId= filter,
a rejected non-numeric plantId, and a stale-version 409 carrying the current
row so the client can rebase.
- `remaining` is derived through the HTTP surface, not just in the service.
DESIGN.md makes derivation load-bearing, and the number is the whole reason
anyone opens the seed shelf.
- Lots are private to the buyer: another user gets 404 (not 403 — existence is
masked per the project convention) on get/patch/delete, sees none in their
listing, and the owner still has access afterwards.
- The group is behind requireAuth: unauthenticated calls 401 rather than
returning an empty list.
Verified the tests actually catch the failure they exist for by unregistering
GET /seed-lots/:id — all four fail with "404 page not found", the journal
signature.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
http.Server.WriteTimeout is an ABSOLUTE deadline from when the request header
was read, not an idle timeout. pansy sets it to 30s (cmd/pansy/main.go:65)
while an agent turn is budgeted 4 minutes, so every turn over 30 seconds was
cut mid-stream. The 4-minute runTimeout was unreachable; the real ceiling was
30 seconds.
That also meant the keep-alive added in #73 could never do its job. It ticks
at 20s, so it got exactly one tick before the connection it was pacing was
destroyed underneath it — a mechanism built to survive long silences that was
structurally incapable of surviving them.
The failure is invisible from the handler: writes made after the deadline
return err == nil and their bytes are discarded. There is no error to check
and none to log, which is why this survived a review that specifically looked
at the discarded write error. Only the client sees it, as an unexpected EOF,
which web/src/lib/agent.ts reports as "The connection dropped partway
through." — indistinguishable from a real network fault.
Because it is invisible server-side, the test drives a real http.Server with a
short WriteTimeout and asserts from the CLIENT side. Against the unfixed code
it gets 1 of 4 frames and an unexpected EOF; a test that inspected the return
of io.WriteString would have passed against the bug.
gin 1.10.1's responseWriter implements Unwrap, so ResponseController reaches
the underlying conn. A failure to clear the deadline IS worth logging: it
means the stream will be cut and we cannot prevent it.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- fillLoaded's doc listed what it does and omitted the non-finite-region
rejection this PR added to it.
- Trim the half-spacing rule's restatement in DESIGN.md to the decision and a
pointer. The rule, the square-foot arithmetic and the failure mode are
written out once, in hexCenters, rather than near-verbatim in four places.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- 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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- 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
- 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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- clampTo's doc justified itself by stopping hexCenters "looping forever",
which stopped being true when hexCenters became count-bounded. Say what it
actually does now, and note the inversion the new guard relies on.
- Trim the changelog prose from hexCenters' doc down to the one line that
earns its keep: don't re-anchor at the min corner, and why.
- Rename a test local from `max` so it stops shadowing the builtin.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
clampTo INVERTS a region lying wholly outside the object — Max clamps below
Min — rather than emptying it. The old loop-until-past-MaxX form handled that
for free by never entering the loop. Counting positions up front does not:
a region 500cm east of a bed with ±50cm local bounds produced 4 plops at
x=275, a couple of metres off the bed.
Caught by removing the guard and watching the new test fail, not by assuming
it would.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Filling a bed left the outer row too far from the edge, and staggered rows
worse still. Two defects, both from anchoring the lattice at the region's min
corner:
- Odd rows offset by `radius` started at `MinX + 2·radius`, leaving a bare
strip a whole plop wide down one side of every other row.
- All the leftover slack piled up on the far edge, where plops hung 13cm
outside the bed on a 4×8ft garlic bed. Nothing clips them, so they drew
over the bed outline.
Spacing is a constraint between neighbouring plants competing for the same
soil, light and water. A bed edge is not a competitor, so the outer row owes
it half the spacing — the arithmetic inside every square-foot-gardening chart
(4/square = 6" apart, 3" from the square's edge).
The wrinkle: a plop is a CLUMP, not a plant. defaultPlopRadius is 1.5×spacing,
so keeping the whole circle inside the bed insets the outer row by 1.5
spacings, three times what the rule allows. So centre the lattice and set the
minimum centre-inset to `radius - spacing/2`: the clump may cross the edge by
up to half a spacing, putting its outermost plants exactly the half-spacing
from the edge the rule asks for. Capped there — a clump mostly outside the bed
would be a drawing of plants in the path.
Same bed, same 15 plops, now symmetric with a deliberate 6.5cm overhang inside
the 7.5cm budget instead of an accidental 13cm on one side only. The stagger
falls out of the centring for free: an offset row holds one fewer plop, and
centring that run puts it exactly half a pitch off its neighbours.
TestFillRegionDeterministicPacking expected 4 plops in a 60×60 bed; the fourth
was centred ON the east edge with half of it outside, well past the budget.
It is 3 now — the fix working, not a regression in it.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Closes#44. Two independent grids (garden + per-bed), each with a size and a snap toggle; snapping defaults off. See PR #45 for details.
Co-authored-by: Steve Dudenhoeffer <[email protected]>
Per-garden public read-only link: unauthenticated GET /api/v1/public/gardens/:token (no requireAuth, no OIDC), owner-only enable/rotate/disable, and a /g/$token page rendering GardenCanvas read-only. Review fixes: redact plant owner ids, Cache-Control: no-store, 400 on malformed body, shared resetTransient store action.
Closes#41.
Co-authored-by: Steve Dudenhoeffer <[email protected]>
Fixes from the PR #28 adversarial review (considered; not graded).
Correctness / API
- PATCH /objects/:id can now clear nullable color/props back to NULL: the
request takes them as json.RawMessage, and ObjectPatch carries an explicit
Set flag so an explicit `null` (clear) is distinguished from an absent
field (unchanged) — the strongest cross-model finding (6 hits). New test.
Maintainability
- store/plantings.go + plants.go use explicit qualified column lists
(qualifyColumns helper) instead of SELECT *, matching gardens/objects and
surviving a future column add.
- Consolidated objectKinds + plantableByDefault into one kind→traits map.
- objectForRole factors the fetch-then-authorize shared by UpdateObject and
DeleteObject; dropped the redundant kind check in CreateObject
(finalizeObject is the single validation point).
- Request→service mapping via toInput()/toPatch() methods (matches gardens).
- Renamed handler gardenFull → getGardenFull (verbNoun); test helper
decodeGarden → decodeMap; generic bind-error messages.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steveandClaude Opus 4.8