Fixes items 1–8 from the post-#124 smoke sweep (scratch build of main driven through Playwright at 1280×800 and 390×844).
What changed
No-change Save rewrote dimensions. The garden and plant dialogs re-parsed the display string on submit, so opening Edit on an imperial garden and pressing Save turned 900×600 cm into 899.922×599.948 (version bump + a bogus "Edited garden settings" history entry), and a plant's 45 cm spacing into 44.958. Both dialogs now hold a LengthField (web/src/lib/units.ts): the text is a view, the centimeters change only when the person types, and a unit switch re-shows the same centimeters. A Save that changed nothing closes without sending anything at all. Unit tests cover the round trip.
Two different "today"s. Journal notes used the browser's local date; placed plops left plantedAt to the server, which stamps UTC today — "planted tomorrow" for anyone west of Greenwich after ~8 pm, and notes sorting under older entries. today() now lives in web/src/lib/dates.ts and the UI sends it for plop placement, fill and removal too. POST /objects/:id/fill gained an optional plantedAt (validated as YYYY-MM-DD); API and agent callers that omit it still get UTC today. The agent's own tool defaults remain server-side UTC — giving it the gardener's local day is a follow-up.
One-tap Remove on a planted object. The inspector's trash deleted an object and its plantings with no question (it undid as one unit, but on the phone it sits beside "Plant this"). It now asks when the object holds plants — "It has 9 plants in it, and they go with it. One Undo brings everything back." — and still removes an empty object straight away.
Plant card action row overflow. "Delete" was clipped to "Delet" at four columns; the row wraps now.
Monogram lettering on pale colors. Letters were always paper-white, unreadable on garlic's #d9d2c5 and cabbage green. monogramInk() picks a theme-stable dark ink above a luminance cutoff (~2.5:1 where paper stops working); tomato, sage and the other saturated colors keep paper. Applied on the Plants page and the canvas; one new token --color-marker-ink.
Copy-as-plan proposed an existing name. With "Back Yard — 2027" already there, Copy still offered "Back Yard — 2027". It now picks the next free year (nextPlanYear) and warns if the typed name already exists.
Plan cards lost their year to truncation ("Back Yard — 20…" + plan). A plan card shows the base name and a 2027 plan tag.
"invalid input" for a bad model spec. The service returned the bare sentinel and the API flattened every ErrInvalidInput to "invalid input". A wrapped ErrInvalidInput reason now reaches the client as the 400's message (inputMessage, with the contract documented on the sentinel), the settings service wraps with which model and why, and the Settings page shows it under the field instead of toasting.
Also defuses a clock bomb: TestRemainingReturnsWhenAPlantingIsRemoved hard-coded a removal date of 2026-08-01 against a plantedAt that defaulted to the real date, so it has failed since August 2nd.
Verification
GOWORK=off go build ./... && go test ./... green, gofmt -l clean (plus new tests: TestFillRegionPlantedAt, TestInputMessage, the bad-model 400 now asserts its message).
npx tsc --noEmit, vitest run (132 tests, 9 new), npm run build green.
Re-drove every fix on a scratch build of the branch: no-change Save sends no PATCH and a real change sends exactly one; a plop placed at 22:19 EDT is dated 2026-08-22; Remove on a planted bed asks first; Delete sits inside the card; garlic's "G" is dark ink; Copy proposes "Back Yard — 2028" and warns on a duplicate; the card reads "Back Yard" + "2027 plan"; a bad spec shows chat model "garbage": unknown provider: … under the field.
Docs: CLAUDE.md gains the three conventions (LengthField, local "today", wrapped ErrInvalidInput is user-facing); DESIGN.md notes plantedAt on the fill route.
Review
Gadfly's sweep: security/correctness/error-handling clean across all four models; 7 minor findings, all addressed in 0d95578 — monogramInk memoized by color (it ran per plop per frame), one shared FALLBACK_PLANT_COLOR instead of three raw literals, the Copy dialog's proposed name follows the gardens list until edited, two GardenCard nits, today with a single import path, and CLAUDE.md now describes the inspector's actual guard rather than claiming it uses LengthField.
Fixes items 1–8 from the post-#124 smoke sweep (scratch build of `main` driven through Playwright at 1280×800 and 390×844).
## What changed
1. **No-change Save rewrote dimensions.** The garden and plant dialogs re-parsed the *display string* on submit, so opening Edit on an imperial garden and pressing Save turned 900×600 cm into 899.922×599.948 (version bump + a bogus "Edited garden settings" history entry), and a plant's 45 cm spacing into 44.958. Both dialogs now hold a `LengthField` (`web/src/lib/units.ts`): the text is a view, the centimeters change only when the person types, and a unit switch re-shows the same centimeters. A Save that changed nothing closes without sending anything at all. Unit tests cover the round trip.
2. **Two different "today"s.** Journal notes used the browser's local date; placed plops left `plantedAt` to the server, which stamps UTC today — "planted tomorrow" for anyone west of Greenwich after ~8 pm, and notes sorting under older entries. `today()` now lives in `web/src/lib/dates.ts` and the UI sends it for plop placement, fill and removal too. `POST /objects/:id/fill` gained an optional `plantedAt` (validated as YYYY-MM-DD); API and agent callers that omit it still get UTC today. The agent's own tool defaults remain server-side UTC — giving it the gardener's local day is a follow-up.
3. **One-tap Remove on a planted object.** The inspector's trash deleted an object *and its plantings* with no question (it undid as one unit, but on the phone it sits beside "Plant this"). It now asks when the object holds plants — "It has 9 plants in it, and they go with it. One Undo brings everything back." — and still removes an empty object straight away.
4. **Plant card action row overflow.** "Delete" was clipped to "Delet" at four columns; the row wraps now.
5. **Monogram lettering on pale colors.** Letters were always paper-white, unreadable on garlic's `#d9d2c5` and cabbage green. `monogramInk()` picks a theme-stable dark ink above a luminance cutoff (~2.5:1 where paper stops working); tomato, sage and the other saturated colors keep paper. Applied on the Plants page and the canvas; one new token `--color-marker-ink`.
6. **Copy-as-plan proposed an existing name.** With "Back Yard — 2027" already there, Copy still offered "Back Yard — 2027". It now picks the next free year (`nextPlanYear`) and warns if the typed name already exists.
7. **Plan cards lost their year to truncation** ("Back Yard — 20…" + `plan`). A plan card shows the base name and a `2027 plan` tag.
8. **"invalid input" for a bad model spec.** The service returned the bare sentinel and the API flattened every `ErrInvalidInput` to "invalid input". A wrapped `ErrInvalidInput` reason now reaches the client as the 400's message (`inputMessage`, with the contract documented on the sentinel), the settings service wraps with which model and why, and the Settings page shows it under the field instead of toasting.
Also defuses a clock bomb: `TestRemainingReturnsWhenAPlantingIsRemoved` hard-coded a removal date of 2026-08-01 against a `plantedAt` that defaulted to the real date, so it has failed since August 2nd.
## Verification
- `GOWORK=off go build ./... && go test ./...` green, `gofmt -l` clean (plus new tests: `TestFillRegionPlantedAt`, `TestInputMessage`, the bad-model 400 now asserts its message).
- `npx tsc --noEmit`, `vitest run` (132 tests, 9 new), `npm run build` green.
- Re-drove every fix on a scratch build of the branch: no-change Save sends no PATCH and a real change sends exactly one; a plop placed at 22:19 EDT is dated 2026-08-22; Remove on a planted bed asks first; Delete sits inside the card; garlic's "G" is dark ink; Copy proposes "Back Yard — 2028" and warns on a duplicate; the card reads "Back Yard" + "2027 plan"; a bad spec shows `chat model "garbage": unknown provider: …` under the field.
- Docs: CLAUDE.md gains the three conventions (LengthField, local "today", wrapped `ErrInvalidInput` is user-facing); DESIGN.md notes `plantedAt` on the fill route.
## Review
Gadfly's sweep: security/correctness/error-handling clean across all four models; 7 minor findings, all addressed in `0d95578` — `monogramInk` memoized by color (it ran per plop per frame), one shared `FALLBACK_PLANT_COLOR` instead of three raw literals, the Copy dialog's proposed name follows the gardens list until edited, two GardenCard nits, `today` with a single import path, and CLAUDE.md now describes the inspector's actual guard rather than claiming it uses `LengthField`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
- Garden and plant dialogs keep centimeters as the source of truth
(LengthField in lib/units.ts): a no-change Save no longer rewrites
900 cm as 899.922 or a 45 cm spacing as 44.958, bumping versions and
writing bogus history entries on the way.
- The UI stamps every date with the browser's local day (lib/dates.ts).
Journal notes already did; plop placement, fill and removal now do too,
so a 9 pm placement isn't "planted tomorrow". The fill endpoint gained an
optional plantedAt; API and agent callers still default to UTC today.
- Removing an object that holds plants asks first and says how many go
with it. An empty one still goes straight away (one Undo restores it).
- The expanded plant card's action row wraps instead of clipping "Delete".
- Monogram lettering switches to a dark ink on pale marker colors (garlic,
cabbage, marigold) instead of near-white on near-white.
- Copy-as-plan proposes the next free year and warns when the typed name
already exists, so two gardens can't both read as "the 2027 plan".
- Plan cards show the base name with a "2027 plan" tag, so the year — the
point of the name — survives truncation.
- A rejected model spec now says which model and why: a wrapped
ErrInvalidInput's reason reaches the client as the 400's message, and the
Settings field shows it inline instead of toasting "invalid input".
Also defuses a clock bomb in TestRemainingReturnsWhenAPlantingIsRemoved,
which only passed while the real date was before 2026-08-01.
Co-Authored-By: Claude Fable 5 <[email protected]>
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 02:20:48Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
⚪JSDoc summary rewritten to wrap mid-thought, producing a 2-line run-on; cosmetic readability regression
maintainability · flagged by 1 model
web/src/components/gardens/GardenCard.tsx:18-21 — The JSDoc summary was rewritten to wrap awkwardly mid-thought ("name (a / plan copy shows its base name and a <year> plan tag), size, ..."), producing a 2-line run-on where the rest of the file uses terse one-line summaries. Cosmetic, but a readability regression in a file-level comment.
🪰 Gadfly · advisory
⚪ **JSDoc summary rewritten to wrap mid-thought, producing a 2-line run-on; cosmetic readability regression**
_maintainability · flagged by 1 model_
- **`web/src/components/gardens/GardenCard.tsx:18-21`** — The JSDoc summary was rewritten to wrap awkwardly mid-thought ("name (a / plan copy shows its base name and a `<year> plan` tag), size, ..."), producing a 2-line run-on where the rest of the file uses terse one-line summaries. Cosmetic, but a readability regression in a file-level comment.
<sub>🪰 Gadfly · advisory</sub>
⚪Dead optional-chain and nullish-fallback on parsePlanName when planYear is already non-null
maintainability · flagged by 1 model
web/src/components/gardens/GardenCard.tsx:46 — dead optional-chain fallback
🪰 Gadfly · advisory
⚪ **Dead optional-chain and nullish-fallback on parsePlanName when planYear is already non-null**
_maintainability · flagged by 1 model_
### `web/src/components/gardens/GardenCard.tsx:46` — dead optional-chain fallback
<sub>🪰 Gadfly · advisory</sub>
🟡Fallback plant color #97a97c named in Canvas but left raw in Inspector.tsx:359 and GardenThumb.tsx:75; extract one shared constant
maintainability · flagged by 1 model
web/src/editor/Canvas.tsx:40 — half-finished extraction of the fallback plant color. This PR names the unknown-plant fallback FALLBACK_PLANT_COLOR = '#97a97c' in Canvas (correctly reusing it for both the circle fill and monogramInk), but the identical literal stays raw at Inspector.tsx:359 (ColorDot color={plant?.color ?? '#97a97c'}) and GardenThumb.tsx:75 (fill={plantColor.get(...) ?? '#97a97c'}) — the same "plant missing from the catalog" default. The constant is module-loc…
🪰 Gadfly · advisory
🟡 **Fallback plant color #97a97c named in Canvas but left raw in Inspector.tsx:359 and GardenThumb.tsx:75; extract one shared constant**
_maintainability · flagged by 1 model_
- **`web/src/editor/Canvas.tsx:40` — half-finished extraction of the fallback plant color.** This PR names the unknown-plant fallback `FALLBACK_PLANT_COLOR = '#97a97c'` in Canvas (correctly reusing it for both the circle fill and `monogramInk`), but the identical literal stays raw at `Inspector.tsx:359` (`ColorDot color={plant?.color ?? '#97a97c'}`) and `GardenThumb.tsx:75` (`fill={plantColor.get(...) ?? '#97a97c'}`) — the same "plant missing from the catalog" default. The constant is module-loc…
<sub>🪰 Gadfly · advisory</sub>
🟠monogramInk() called once per plop per render frame with no caching; was previously a static string
performance · flagged by 2 models
web/src/editor/Canvas.tsx:650 — monogramInk() called once per plop, every frame, with no caching
🪰 Gadfly · advisory
🟠 **monogramInk() called once per plop per render frame with no caching; was previously a static string**
_performance · flagged by 2 models_
**`web/src/editor/Canvas.tsx:650` — `monogramInk()` called once per plop, every frame, with no caching**
<sub>🪰 Gadfly · advisory</sub>
⚪today re-exported from journal.ts only to satisfy JournalTab; objects.ts already imports from lib/dates — inconsistent indirection
maintainability · flagged by 1 model
web/src/lib/journal.ts:131 — today is re-exported (export { today } from './dates') purely so JournalTab.tsx:7-16 can keep importing it from @/lib/journal, while objects.ts:10 already imports today directly from @/lib/dates. With only one consumer, this is a needless second source of the symbol; re-pointing JournalTab's import at @/lib/dates and dropping the re-export would be consistent with the rest of the codebase.
🪰 Gadfly · advisory
⚪ **today re-exported from journal.ts only to satisfy JournalTab; objects.ts already imports from lib/dates — inconsistent indirection**
_maintainability · flagged by 1 model_
- **`web/src/lib/journal.ts:131`** — `today` is re-exported (`export { today } from './dates'`) purely so `JournalTab.tsx:7-16` can keep importing it from `@/lib/journal`, while `objects.ts:10` already imports `today` directly from `@/lib/dates`. With only one consumer, this is a needless second source of the symbol; re-pointing JournalTab's import at `@/lib/dates` and dropping the re-export would be consistent with the rest of the codebase.
<sub>🪰 Gadfly · advisory</sub>
I've verified the security-relevant surfaces. Here's my review.
VERDICT: No material issues found
Reviewed through the security lens. The change threads an optional plantedAt through the fill path and broadens ErrInvalidInput error messages to reach the client — both are the natural places a security problem would hide, so I traced them.
What I checked and cleared:
plantedAt as tainted input → DB (internal/service/ops.go:198, internal/service/plantings.go:320). validDatePtr runs time.Parse("2006-01-02", *s) and rejects anything non-conforming, and the value reaches the store as a bound parameter in a domain.Planting, not string-concatenated SQL. Even a malformed date can't inject; the format check is redundant defense, not the only barrier. TestFillRegionPlantedAt asserts the reject path. No injection.
New client-facing error text (internal/api/errors.go:53,122). inputMessage now surfaces any wrapped ErrInvalidInput verbatim instead of a flat "invalid input". I grepped every wrap site in internal/ — only two exist (instance_settings.go:74, ops.go:199), both new and both intentional; neither echoes internal state beyond the caller's own input. No pre-existing wrap gets newly exposed.
Credential leakage via the model-spec error (internal/service/instance_settings.go:74 → specReason → agentmodel.Validate). I read agentmodel.go: Resolve wraps as agentmodel: resolve %q: %w, and specReason unwraps to the majordomo Parse error (e.g. "unknown provider"). The OllamaCloudAPIKey is passed to registry(apiKey) but never appears in any error value — only the admin's own spec string is echoed back. No key disclosure.
Authz on that error path (internal/api/api.go:158). The settings group is behind requireAuth() + requireAdmin(), so the more detailed spec/provider message is admin-only anyway; no info-exposure to ordinary users.
Frontend (monogram.ts, Canvas.tsx, CopyDialog.tsx). monogramInk returns only the fixed var(--color-*) constants after a strict hex regex — no way for plant.color to inject CSS through it. background: color was already raw and is unchanged. Garden-name handling in CopyDialog is plain string compare, no sink.
Nothing in the authn/authz, injection, SSRF, deserialization, secret-leakage, or input-validation categories is materially wrong here.
One forward-looking note (not a finding, since no current site triggers it): the inputMessage broadening makes every future %w-wrap of ErrInvalidInput client-visible. CLAUDE.md and the domain.ErrInvalidInput doc comment both now warn "write it for the person, not the log," which is the right guardrail — worth honoring for anyone adding wraps that might carry paths or internal identifiers.
🎯 Correctness — No material issues found
I've verified the correctness-sensitive parts of this PR against the actual checked-out code. Here is my review through the Correctness lens.
VERDICT: No material issues found
The change is careful and well-tested. I verified the following, and each holds up:
Date validation is strict, not just format-shaped.fillLoaded guards plantedAt with validDatePtr (internal/service/plantings.go:320), which calls time.Parse("2006-01-02", …) — that rejects out-of-range dates like 2026-13-45, not merely malformed strings. "April 1st" → ErrInvalidInput, as the new test asserts. The default path (s.now().UTC() when plantedAt == nil) is preserved for agent/API callers (internal/service/ops.go:243-245). All ~15 FillRegion/FillNamedRegion call sites were updated to the new signature (verified across ops_test.go, revisions_test.go, agent/tools.go, agent/*_test.go).
The luminance math is correct — I re-derived it.web/src/lib/monogram.ts:73 uses the WCAG formula (sRGB linearization with the 0.03928 knee and 2.4 exponent; coefficients 0.2126/0.7152/0.0722 mapped to R@slice0, G@slice2, B@slice4). Recomputing: sage #7a8a5e → L≈0.231 (< 0.37 → paper ✓); cabbage #8bc98b → L≈0.491 (> 0.37 → ink ✓). Matches the tests and the inline claims.
inputMessage/specReason compose correctly.agentmodel.Validate returns fmt.Errorf("agentmodel: resolve %q: %w", …) (agentmodel.go:43), so specReason's single errors.Unwrap (instance_settings.go:177) strips exactly that wrapper, and inputMessage (api/errors.go:122) trims the "invalid input: " prefix. Front-of-string wraps like loading: %w correctly fall through to the generic text — covered by TestInputMessage.
nextPlanYear and the GardenCard title logic are consistent.nextPlanYear (plan.ts:47) skips every taken year for the base from from upward; planYearOf gates the tag/title on year >= currentYear, so a past-year plan falls back to the full name with no tag — no mismatch between title and the {planYear} plan tag.
LengthField invariant is preserved.cm only moves on editDimensionField/editSpacingField (a keystroke); convertDimensionField re-derives text from the same cm; Save reads field.cm directly. A no-change Save now sends the loaded centimeters — the 900→899.922 drift is genuinely fixed.
The Remove-confirmation gate can't silently misfire.plantCountIn (Inspector.tsx:45) sums plopCount, which falls back to p.derivedCount when the plant is absent (shared.ts:98) — so the count is always a real number, never NaN, and plantCount > 0 reliably triggers the confirm. Wiring at GardenEditorPage.tsx:393 is correct.
CreatePlanting's { plantedAt: today(), ...body } merge is safe — TypeScript optional-omitted keys are absent from body, so the default isn't clobbered by an undefined.
No correctness defects to report in this lens.
🧹 Code cleanliness & maintainability — Minor issues
Both findings check out against the actual code:
Finding 1:FALLBACK_PLANT_COLOR = '#97a97c' is defined at Canvas.tsx:40; the identical literal appears raw at Inspector.tsx:359 and GardenThumb.tsx:75 (same "plant missing from catalog" default), while ColorSwatches.tsx:5CURATED_SWATCHES[0] is a semantically distinct swatch. Confirmed.
Finding 2:GardenDialog/PlantDialog use LengthField; Inspector.tsx (commitDim, lines 106–143) still keeps display strings via formatDimensionInput and re-parses with a string-compare guard. Two mechanisms for one concern, matching the draft. Confirmed.
VERDICT: Minor issues
The refactors in this PR are, on the whole, clean and well-targeted through the maintainability lens: the LengthField consolidation removes the ad-hoc convert/parseDimension juggling in GardenDialog, today() becomes a single source with journal.ts re-exporting it (no duplicated impl), the old dead imports in both dialogs are fully removed, and inputMessage/specReason/nextPlanYear are small, single-purpose, well-documented helpers. The --color-marker-ink token is deliberately single-placed (documented as theme-stable, matching the existing --color-paper pattern, so the "color in both places" convention doesn't apply).
A few small things:
web/src/editor/Canvas.tsx:40 — half-finished extraction of the fallback plant color. This PR names the unknown-plant fallback FALLBACK_PLANT_COLOR = '#97a97c' in Canvas (correctly reusing it for both the circle fill and monogramInk), but the identical literal stays raw at Inspector.tsx:359 (ColorDot color={plant?.color ?? '#97a97c'}) and GardenThumb.tsx:75 (fill={plantColor.get(...) ?? '#97a97c'}) — the same "plant missing from the catalog" default. The constant is module-local to Canvas, so the magic value is now named in one place and raw in two others that mean exactly the same thing, which is the opposite of what naming it was supposed to buy. Low-churn fix: export a single FALLBACK_PLANT_COLOR (e.g. from lib/plants.ts) and use it in all three. (ColorSwatches.tsx:5CURATED_SWATCHES[0] also equals #97a97c but is semantically "first curated swatch," not the fallback — leave it alone.)
web/src/editor/Inspector.tsx:106-143 — the inspector didn't adopt the LengthField pattern this PR standardizes. The dialogs now hold LengthFields so cm is the source of truth; the inspector still keeps display strings (formatDimensionInput) and re-parses in commitDim, guarding no-change edits with a string compare (raw.trim() === formatDimensionInput(current, unit)) instead. It achieves the same "don't rewrite on a no-op Save" goal, so this isn't a bug — but the CLAUDE.md note added here ("The inspector guards the same way") papers over the fact that there are now two mechanisms for one concern. Converting the inspector to LengthField is more churn than this PR's scope, so this is a maintainability heads-up, not a required change.
Nothing blocking in my lens.
⚡ Performance — No material issues found
I've reviewed the changes through the Performance lens. Let me confirm my read of the two spots that touch render/query paths.
The plantCountIn addition (Inspector.tsx:45) is called only for the single selected object (GardenEditorPage.tsx:393), right alongside the pre-existing rosterText which already does the identical O(plantings) pass — so it's one extra linear scan over one object's inspector render, not per-object work. monogramInk/luminance in Canvas.tsx:650 runs only in the label loop, gated by focus (p.objectId !== focusId returns null) and by r * s < 9, and the work is a single regex match plus a few parseInts — negligible even for a full focused bed. The today() calls and the service-side validDatePtr check are one-shot per mutation.
VERDICT: No material issues found
No N+1s, unbounded growth, or new quadratic behavior. The fill path still batches inserts in one transaction; the only addition is a constant-time date validation.
The one new per-render computation (monogramInk on the canvas) is bounded by focus + zoom gating and is cheap per call — not a material regression versus the previous constant string.
The new plantCountIn scan is single-object, same order as the adjacent existing rosterText scan; not worth memoizing.
useGardens() in CopyDialog reads an already-cached list on the gardens page — no extra fetch on the hot path.
Nothing in my lane rises to a finding.
🧯 Error handling & edge cases — No material issues found
I've verified the error-handling and edge-case paths thoroughly across both the Go service layer and the frontend. Here is my review.
VERDICT: No material issues found
I reviewed this change strictly through the error handling & edge cases lens and verified each concern against the checked-out code. The unhappy paths this PR introduces are all handled:
plantedAt validation (server).fillLoaded calls validDatePtr(plantedAt)before using it (internal/service/ops.go:198), and validDatePtr (internal/service/plantings.go:320) is nil-safe (returns true for nil) and rejects malformed strings via time.Parse(dateLayout, …). The nil default correctly falls through to s.now().UTC() at ops.go:243-246. TestFillRegionPlantedAt exercises both the good and bad-date branches. Go's time.Parse also range-checks (e.g. 2026-02-30 is rejected), so no out-of-range date leaks through.
inputMessage (server). Handles the bare-sentinel case (msg == base → returns generic "invalid input") and only strips a "invalid input: " prefix otherwise (internal/api/errors.go), so a suffix-wrapped error is returned verbatim rather than mangled. TestInputMessage covers all three shapes. specReason is nil-safe (errors.Unwrap guarded).
LengthField (frontend).editDimensionField / editSpacingField return cm: null on unparseable/empty input (web/src/lib/units.ts:251,267), and every consumer guards it: GardenDialog checks widthCm === null || heightCm === null then isValidDimensionCm(...) (rejects negative/zero), and PlantDialog checks spacingCm === null || spacingCm < 1. convertDimensionField preserves the raw text when cm is null so a typo stays visible on a unit switch.
Confirm-remove (frontend).ConfirmDialog wraps onConfirm in try/catch, surfaces failures inline (e.g. a 409), and re-enables the button for retry; the Inspector's onConfirm awaits del.mutateAsync before onDeleted(), so a failed remove doesn't deselect. plantCount is computed via plantCountIn → plopCount, which falls back to p.derivedCount when the plant is missing from the catalog (web/src/editor/shared.ts:96), so the confirm gate isn't skipped for a private plant in a shared garden.
Monogram color (frontend).luminance() returns null for anything that isn't #rgb/#rrggbb, and monogramInk falls back to paper on null (web/src/lib/monogram.ts); the canvas and Monogram both feed it a FALLBACK_PLANT_COLOR when plant is undefined. Tested for named colors and empty strings.
nextPlanYear (frontend). The while (taken.has(year)) loop iterates over a finite set and always terminates (web/src/lib/plan.ts:53).
The test clock-bomb fix (seed_lots_test.go) pins both plantedAt (2026-07-01) and the removal (2026-08-01) to fixed dates in the correct order, so it no longer depends on the wall clock.
Nothing in my lane is materially wrong here — the diff is genuinely careful about its unhappy paths.
plantedAt string (API input → DB): Validated in fillLoaded via validDatePtr, which calls time.Parse("2006-01-02", ...) — only strict YYYY-MM-DD strings pass. It reaches the database exclusively through parameterized ? placeholders (plantingInsert in internal/store/plantings.go). No injection surface.
inputMessage / wrapped ErrInvalidInput: The function strips the sentinel prefix and returns the wrapped reason verbatim. Verified all two fmt.Errorf("%w: …", domain.ErrInvalidInput) calls in internal/service/ place the sentinel first (the required contract). The third test case ("loading: %w") documents what happens when the contract is broken, but no production code hits that path today.
Model spec echoed in 400 body (instance_settings.go:74): The spec is the admin's own submitted value, returned to that same admin. requireAdmin gates the route. Intentional and safe.
React rendering of user-controlled strings (CopyDialog, GardenCard): Garden names and the warning message interpolate user data via JSX ({name.trim()}), which React escapes. No XSS vector.
monogramInk / luminance: User-controlled plant colors flow through a hex regex; the function returns only one of two fixed CSS variable strings regardless of input. No injection possible.
No new routes, no auth model changes, no credential handling, no SSRF surface introduced.
🎯 Correctness — No material issues found
I've now verified all the correctness-sensitive pieces. Let me write the review.
No material issues found
I checked every formula, conversion, and domain invariant introduced by this PR:
luminance in monogram.ts:87 — verified. channel(0) → hex.slice(0,2) = R, channel(2) → hex.slice(2,4) = G, channel(4) → hex.slice(4,6) = B; coefficients 0.2126/0.7152/0.0722 sum to 1.0 and match WCAG 2.1. Threshold 0.37 checks out: paper contrast = 1.05/(0.37+0.05) ≈ 2.5:1. Verified all six test fixtures by hand against the formula; they pass.
today() in dates.ts:8 — correct. getMonth()+1 gives 1–12, getDate() gives 1–31, getFullYear() is unambiguous. Both test cases verified.
LengthField in GardenDialog, PlantDialog — the invariant holds. dimensionField(cm, unit) stores the loaded cm directly; editDimensionField(text, unit) stores the parsed cm alongside the raw text; convertDimensionField reformats from cm without re-parsing. Submit path reads .cm directly — no re-parse of a display string on save.
inputMessage in errors.go:119 — all three test cases verified: bare sentinel → returns unchanged; %w-prefixed wrapping → TrimPrefix strips "invalid input: " correctly; %w-suffixed wrapping → TrimPrefix is a no-op and the full message is returned.
validDatePtr in plantings.go:320 — nil is allowed, otherwise time.Parse(dateLayout, *s) enforces YYYY-MM-DD strictly.
&plantedOn aliasing in ops.go:259 — plantedOn is set once before the loop and never mutated inside it; all pointers are live when CreatePlantings is called, which dereferences each in its own loop body. The old code used &today identically; not a new risk.
nextPlanYear in plan.ts:42 — correctly builds a set of taken years for the named base, then increments from from until it finds a free slot. Logic is sound.
specReason in instance_settings.go:172 — agentmodel.Validate wraps as fmt.Errorf("agentmodel: resolve %q: %w", spec, innerErr), so errors.Unwrap returns the inner parse error, stripping the redundant spec from the message.
plantCountIn in Inspector.tsx:44 — correct. The plantings array fed to it is the editor's active-only slice (soft-removed entries are filtered out in useRemovePlanting's onMutate).
🧹 Code cleanliness & maintainability — Minor issues
The finding checks out. When planYearOf returns non-null, it has already called parsePlanName and confirmed a non-null result with a valid year — so the second parsePlanName call on line 46 is guaranteed to return non-null, making both ?.base and ?? garden.name unreachable.
Verdict: Minor issues
One verified cleanliness finding; the rest of the PR is tidy through this lens.
web/src/components/gardens/GardenCard.tsx:46 — dead optional-chain fallback
constplanYear=planYearOf(garden.name)// line 43
consttitle=planYear!=null?(parsePlanName(garden.name)?.base??garden.name)// ← ?.base and ?? unreachable
:garden.name
planYearOf (in lib/plan.ts) does const p = parsePlanName(name); return p && p.year >= now.getFullYear() ? p.year : null, so planYear != null is only true when parsePlanName already returned a non-null result for the same input. Calling parsePlanName(garden.name) again inside the true-branch is guaranteed to succeed; the ?. optional chain and ?? garden.name fallback are unreachable dead code. A reader seeing ?.base ?? garden.name naturally wonders what case produces a null — there isn't one.
Fix:parsePlanName(garden.name)!.base (non-null assertion) or factor it out:
Everything else looks clean for this lens: the LengthField abstraction is applied consistently across the two dialogs; today() was correctly centralised in dates.ts with a re-export from journal.ts for existing callers; plantCountIn is a fresh export used in exactly one call site with no duplication of rosterText's logic; the inputMessage/specReason pair are narrow, well-tested helpers; and the constant extraction of FALLBACK_PLANT_COLOR in Canvas.tsx is a straightforward cleanup over the inline literal.
⚡ Performance — Minor issues
The finding is confirmed. At Canvas.tsx:650, monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR) is called inside renderedPlops.map() on every render. The luminance() function in monogram.ts:79–88 does the regex, spread, parseInt, and ** 2.4 work the draft describes. The only cached per-plant lookup in that loop is letters (a Map<number, string> prop from the parent); no equivalent ink cache exists.
Minor issues
The monogramInk call introduced by this PR to support dark/light ink selection runs once per plop on every canvas frame, with no caching.
web/src/editor/Canvas.tsx:650 — monogramInk() called once per plop, every frame, with no caching
monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR) sits inside renderedPlops.map(). monogramInk() calls luminance(), which does a regex match, an array spread (3-digit hex expansion), three parseInt calls, and three ** 2.4 floating-point powers. For a garden with 200 visible plops across a handful of plant species, every canvas frame recomputes the same 5–10 luminance values 200 times instead of once. Canvas re-renders on every pointer move during pan/zoom — up to 60 fps — so a full planting can mean ~12,000 redundant luminance computations per second, all returning identical results.
The letters prop already shows the right pattern: compute new Map(plants.map(p => [p.id, monogram(p)])) in the parent with useMemo([plants]), pass it as a prop, and look up by plantId in the loop. An inks map of plantId → monogramInk(p.color) follows the same shape and fixes the issue entirely.
🧯 Error handling & edge cases — No material issues found
I now have enough information to complete my review.
No material issues found
I traced every new unhappy path introduced by this PR through the actual checked-out code.
inputMessage() (internal/api/errors.go:115): The three cases are all correct and covered by the new unit test — bare sentinel returns "invalid input", the %w: reason wrap strips the prefix, and a "outer: %w" wrap returns the full message. The string-stripping logic is sound because writeServiceError only calls it after errors.Is has already confirmed the sentinel is present.
specReason() (internal/service/instance_settings.go:179): agentmodel.Resolve always wraps its errors as fmt.Errorf("agentmodel: resolve %q: %w", spec, underlying), so errors.Unwrap reliably returns the underlying majordomo error. The only other path — an empty spec — is guarded by the if f.spec == "" { continue } check before Validate is ever called, so specReason never sees a non-wrapping error from this code.
validDatePtr in fillLoaded (internal/service/ops.go:197): Placed correctly before validFillLayout and before computing lattice centers. Returns nil, wrapped ErrInvalidInput with a user-readable message that inputMessage will properly strip. The existing validDatePtr shared with plantings.go, journal.go, and seed_lots.go uses time.Parse("2006-01-02", …) which is strict enough for the stated contract.
plantedOn pointer aliasing in the batch loop (internal/service/ops.go:255): All domain.Planting entries point to &plantedOn — the same address. This was the pre-existing pattern (today before, plantedOn now); the store reads the string value at insertion time, not later, so there is no aliasing hazard introduced here.
plopCount with undefined plant (Inspector.tsx/plantCountIn): When plantsById.get(p.plantId) is undefined (a shared garden's private plant), plopCount falls through to p.derivedCount (verified in shared.ts:98). No panic path.
Confirm-dialog remove flow (Inspector.tsx:290): The confirmed path uses mutateAsync + onDeleted() called only on success; ConfirmDialog surfaces errorFallback on throw and keeps the dialog open for retry. The empty-object path uses the existing optimistic onDeleted() → mutate() order, unchanged from before this PR.
luminance() channel indices (monogram.ts:84): Verified: channel(0)→R, channel(2)→G, channel(4)→B for a 6-char hex string. WCAG weights are correctly ordered.
plantedAt input validation is present and correct.fillLoaded (internal/service/ops.go:198) guards the new pointer with validDatePtr, which uses time.Parse(dateLayout, *s) against "2006-01-02" (internal/service/plantings.go:320). time.Parse rejects out-of-range month/day values, so a malicious client can't push an arbitrary string through the fill route — it gets a wrapped ErrInvalidInput → 400. Verified by reading validDatePtr and the ops.go call site.
No injection surface. The date is stored as a string and only ever re-parsed with the same strict layout; it never reaches SQL or a template. The today() helper (web/src/lib/dates.ts) is a pure local-date formatter with no tainted input.
Reflected error message is safe.inputMessage (internal/api/errors.go:125) echoes the wrapped reason — which includes the user-supplied model spec — into the 400 body. That body is serialized by gin's c.JSON (JSON-escaped), and on the client it's surfaced under a field in React (text-escaped by default), so there is no HTML/script injection path. No dangerouslySetInnerHTML was indicated anywhere in the diff. Low risk, not a finding.
monogramInk luminance math is correct (verified web/src/lib/monogram.ts:78-88): channel indexing 0/2/4 maps R/G/B and the WCAG coefficients 0.2126/0.7152/0.0722 are applied to the right channels with the standard linearization. Not a security issue, but the magic number checks out.
Remove confirmation (Inspector.tsx) introduces no authz change — it gates a UI action behind a confirm dialog and still routes through the existing authorized useDeleteObject mutation; no new privileged path was opened.
Nothing in my lane is materially wrong.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I traced every correctness-sensitive path in this PR through the actual checked-out code.
Verified clean:
monogramInk luminance math (web/src/lib/monogram.ts:79-93): channel indices are correct — channel(0)→red weighted 0.2126, channel(2)→green weighted 0.7152, channel(4)→blue weighted 0.0722 (standard WCAG). The gamma transform v ≤ 0.03928 ? v/12.92 : ((v+0.055)/1.055)**2.4 is the correct WCAG sRGB→linear formula. The PAPER_MAX_LUMINANCE = 0.37 threshold is derivable: paper (~L=1.0) on a background of L gives contrast (1.05)/(L+0.05); at L=0.37 that's 1.05/0.42 = 2.5:1 — exactly the stated cutoff. Sage (#7a8a5e, ~0.23 < 0.37) keeps paper; cabbage (#8bc98b, ~0.49 > 0.37) flips to ink. The 3-digit hex expansion c+c is correct.
LengthField no-change-save fix (web/src/lib/units.ts, GardenDialog.tsx, PlantDialog.tsx): dimensionField/spacingField store the original cm verbatim; save reads field.cm directly, never re-parsing the display text. convertDimensionField regenerates text from the unchanged cm. editDimensionField/editSpacingField only re-parse on actual typing. This is the precise fix for the 899.922 / 44.958 drift. Round-trip verified against formatDimensionInput/parseDimension and spacingFromCm/cmFromSpacing.
today() local date (web/src/lib/dates.ts): new Date(2026, 7, 22, 21, 30) → 2026-08-22 (month 7 = August, 0-indexed, correct); new Date(2026, 0, 5, 0, 1) → 2026-01-05. Uses local getters only. Correct.
plantedAt plumbing (internal/service/ops.go:198,243-245): validDatePtr (plantings.go:320) uses time.Parse(dateLayout="2006-01-02", …), matching the format today() emits. Nil → UTC today default; valid string used verbatim; invalid → wrapped ErrInvalidInput. useCreatePlanting spreads { plantedAt: today(), ...body } so an explicit caller value still wins. Correct.
inputMessage prefix handling (internal/api/errors.go:125-131): bare sentinel → "invalid input"; fmt.Errorf("%w: …", ErrInvalidInput) → trimmed reason; a leading wrap like fmt.Errorf("loading: %w", ErrInvalidInput) → .Error() is "loading: invalid input", TrimPrefix finds no "invalid input: " prefix, returns the whole string unchanged. Matches all three test cases.
specReason unwrap (instance_settings.go:175-180): agentmodel.Validate returns fmt.Errorf("agentmodel: resolve %q: %w", spec, innerErr); errors.Unwrap yields innerErr, so the message reads the provider's own reason without repeating the spec — the test asserts chat model "nonesuch/model": <reason>. Consistent.
nextPlanYear (web/src/lib/plan.ts:47-55): builds a Set of years taken by plans whose base matches, then increments from from. The taken duplicate-name guard in CopyDialog is a superset (any exact name match), so it never false-positives on the auto-proposed (free-year) name. Correct.
No correctness defects found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
All three draft findings are confirmed against the actual code. Producing the corrected review with all surviving findings.
VERDICT: Minor issues
**web/src/editor/Inspector.tsx:104-112 (and 217-220)** — CLAUDE.md (line 137) states "The inspector guards the same way" as part of the new LengthField convention, but the Inspector's own dimension/grid fields still hold plain string state (useState(formatDimensionInput(...))) and still re-parse the display string on blur via commitDim/commitGrid(lines 137-149, applied at 217-220). TheLengthFieldpattern the PR introduces inGardenDialog.tsxandPlantDialog.tsx` (cm as source of truth, text as a view) is not actually applied here, so the same round-trip-drift class of bug the PR fixes elsewhere is left in place. This is an incomplete migration / an inaccurate doc claim, not a newly introduced bug, but it's the obvious cleanliness gap given the stated convention.
web/src/lib/journal.ts:131 — today is re-exported (export { today } from './dates') purely so JournalTab.tsx:7-16 can keep importing it from @/lib/journal, while objects.ts:10 already imports today directly from @/lib/dates. With only one consumer, this is a needless second source of the symbol; re-pointing JournalTab's import at @/lib/dates and dropping the re-export would be consistent with the rest of the codebase.
web/src/components/gardens/GardenCard.tsx:18-21 — The JSDoc summary was rewritten to wrap awkwardly mid-thought ("name (a / plan copy shows its base name and a <year> plan tag), size, ..."), producing a 2-line run-on where the rest of the file uses terse one-line summaries. Cosmetic, but a readability regression in a file-level comment.
⚡ Performance — Minor issues
I've verified the finding against the actual code. The diff confirms Canvas.tsx:650 changed from a constant fill="var(--color-paper)" to fill={monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)}, called inside renderedPlops.map(...) (line 635). monogramInk (in web/src/lib/monogram.ts:91) is a pure function delegating to luminance (line 79), which does the regex/hex-expansion/parseInt/exponentiation math described. The canvas already memoizes renderedPlops/byId but does NOT memoize the color→ink mapping — monogramInk is invoked inline per plop per render, and the canvas re-renders on pan/zoom/animation frames (ANIM_MS=520). Colors come from a small finite catalog set, so the work is repeatable and avoidable. The finding is accurate.
VERDICT: Minor issues
Performance lens review. The change is mostly clean efficiency-wise (batch insert preserved, no new N+1, fill date computed once per batch). One real, if minor, hot-path regression:
web/src/editor/Canvas.tsx:650 — monogramInk() recomputed per plop per render frame. The canvas is a hot path: it re-renders on every pan/zoom/animation tick (ANIM_MS=520ms of refit/transition frames), and renderedPlops.map(...) calls monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR) for each visible plop. monogramInk → luminance does a trim() + regex match + conditional [...hex].map(c => c+c).join() + three parseInt/division + three **2.4 exponentiations. Previously this was a constant "var(--color-paper)". The result depends only on the plant's color, which is a small finite set (the catalog), so a plop-heavy bed re-running this at 60fps across the animation is avoidable repeated work. Verified by reading web/src/lib/monogram.ts (pure function of color) and Canvas.tsx render loop (the component memoizes renderedPlops/byId but not the color→ink mapping).
Fix: memoize the mapping, e.g. a module-level Map<string, string> keyed by color, or useMemo building {plantId → ink} from plantsById once per render, so the luminance math runs once per distinct color rather than once per plop per frame.
Nothing else in the performance lane rose to material concern: nextPlanYear/names in CopyDialog.tsx run over the gardens list each render but that list is small and the dialog is not a hot path; plantCountIn/plantCount is a single linear scan over plantings in the inspector (not per-frame); the service fill path computes plantedOn once per batch, not per plop.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed the change through the error-handling & edge-cases lens, verifying the unhappy paths the diff introduces:
inputMessage (errors.go): Verified internal/api/errors.go:33-40 and its test. The bare-sentinel short-circuit returns "invalid input"; wrapped errors get strings.TrimPrefix(msg, "invalid input: "), which is a no-op when the sentinel isn't the prefix (e.g. "loading: invalid input" stays whole). Both branches behave as the test asserts; no panic on a nil-ish message (the function only receives a non-nil error from errors.Is).
Fill plantedAt validation (service/ops.go:194-200): Confirmed validDatePtr (plantings.go:319-326) accepts nil or strict YYYY-MM-DD; a bad string returns a wrapped ErrInvalidInput with a user-facing reason. The nil path correctly falls back to s.now().UTC(). No empty-string edge case slips through (time.Parse rejects "").
Create/fill UI defaults (objects.ts): { plantedAt: today(), ...body } ordering lets a caller-supplied plantedAt override the default intentionally; today() handles any Date (verified dates.test.ts), no NaN/undefined risk.
Inspector Remove confirm (Inspector.tsx:192-202, 281-296): plantCount > 0 gates the confirm; the ConfirmDialog owns its busy/error state and keeps the dialog open on throw (verified ConfirmDialog.tsx:37-47), so a 409 is surfaced not swallowed. The empty-object path still deletes directly. No double-delete race — setConfirmRemove(true); return short-circuits before del.mutate.
CopyDialog taken/names (CopyDialog.tsx:24-33): gardens.data ?? [] avoids a crash when the query is loading; taken is a non-blocking advisory, not a submit guard, so a stale list can't block a valid copy.
One thing I could not fully verify (out of budget): plantCountIn (Inspector.tsx:45-49) sums over every planting passed in without filtering by removed-state. If the editor's plantings array includes soft-removed plops, the confirm could fire for an object whose plants are all already removed. I didn't reach the call site to confirm what that array contains, so I'm not reporting it as a finding — and even if true, hard-deleting the object removes those rows too, so the warning is arguably still correct.
Authn/authz: No changes to auth boundaries; FillRegion/FillNamedRegion still gate through objectForRole with roleEditor, and UpdateInstanceSettings still requires admin.
Input validation: The new optional plantedAt parameter is validated with the existing validDatePtr (parses against 2006-01-02 layout), rejecting malformed dates with a clean ErrInvalidInput.
Error exposure: The inputMessage / wrapped-ErrInvalidInput design is intentional and documented (CLAUDE.md). User-controlled specs are %q-quoted before inclusion, and specReason strips one layer of wrapping to avoid doubling the spec in the message. No secret material (e.g., the Ollama API key) is incorporated into the formatted error.
No injection surfaces: No SQL, command, path, or template injection introduced. LengthField and date handling operate on already-validated or client-side data without new server-side parsing risks.
🎯 Correctness — No material issues found
No material issues found.
After verifying the key correctness-sensitive changes:
today() in web/src/lib/dates.ts correctly uses local-time getFullYear/getMonth+1/getDate, fixing the UTC-tomorrow bug for west-of-Greenwich gardeners. The test confirms zero-padded YYYY-MM-DD output.
validDatePtr in internal/service/plantings.go:320 accepts nil (API/agent default) and validates YYYY-MM-DD format. The fillLoaded flow: nil → UTC today; valid string → passed through; invalid → ErrInvalidInput. Verified correct.
inputMessage in internal/api/errors.go strips only a leading "invalid input: " prefix; the test suite covers bare sentinel, standard wrapped form, and non-standard wrapping. The logic is correct for the documented convention.
specReason in internal/service/instance_settings.go:177 unwraps one level to strip the agentmodel.Resolve wrapper ("agentmodel: resolve %q: …"), so the final user-facing message doesn't repeat the spec. Verified against the agentmodel package structure.
monogramInk threshold PAPER_MAX_LUMINANCE = 0.37 correctly implements the WCAG relative-luminance formula. At L = 0.37, white-on-marker contrast is (1.05)/(0.42) = 2.5:1, matching the documented cutoff. Verified against the test values.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The PR is clean and well-structured through the maintainability lens:
Good abstraction: LengthField in web/src/lib/units.ts cleanly separates display text from canonical centimeters, with dimensionField/editDimensionField/convertDimensionField and spacingField/editSpacingField providing a consistent pattern used by both GardenDialog.tsx and PlantDialog.tsx.
Consistent patterns: today() in web/src/lib/dates.ts centralizes local-date generation; monogramInk() in web/src/lib/monogram.ts encapsulates the luminance threshold logic with clear documentation; nextPlanYear() in web/src/lib/plan.ts extracts the collision-avoidance logic from CopyDialog.tsx.
No dead code: The old formatDimensionInput/parseDimension helpers remain appropriately in use by Inspector.tsx, which guards against re-parsing via its commitDim early-return check.
Clear naming: plantCountIn and rosterText describe what they compute; inputMessage and specReason accurately describe their purpose.
⚡ Performance — No material issues found
No material issues found
Nothing in this change introduces a performance regression I can justify:
The LengthField refactor (web/src/lib/units.ts) replaces string re-parsing on save with a stored numeric value — that's fewer allocations, not more.
monogramInk runs per plop label on the SVG canvas. It does a short regex and a few arithmetic ops; at expected garden scales this adds well under a millisecond per frame and is dwarfed by DOM paint cost. Not material.
plantCountIn duplicates the linear scan already done by rosterText for the selected object, but both are O(n) over the plantings array and run only when an object is selected. For typical counts this is sub-millisecond. Not material.
No N+1 queries, unbounded growth, missing pagination, or blocking hot-path calls were introduced.
🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/components/gardens/CopyDialog.tsx: useState(() => planNameFor(base, year)) captures the initial year value once. If useGardens() data is still loading when the dialog opens, names is empty, nextPlanYear returns from (which may already be taken), and the suggested name stays stale even after the list loads. The taken warning catches this on the next render, but the initial default is wrong and the user can submit before noticing. Compute the name inside a useEffect (or derive state) so it reacts when gardens.data arrives.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 4 models
**Verdict: Minor issues** · 7 findings (2 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | monogramInk() called once per plop per render frame with no caching; was previously a static string | `web/src/editor/Canvas.tsx:650` | 2/4 | performance |
| 🟠 | Inspector dimension/grid fields still hold plain strings and re-parse on save; the LengthField convention claimed in CLAUDE.md isn't applied here | `web/src/editor/Inspector.tsx:104` | 2/4 | maintainability |
<details><summary>5 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | CopyDialog useState initializer captures stale year when gardens list loads asynchronously | `web/src/components/gardens/CopyDialog.tsx:41` | kimi-k2.6:cloud | error-handling |
| 🟡 | Fallback plant color #97a97c named in Canvas but left raw in Inspector.tsx:359 and GardenThumb.tsx:75; extract one shared constant | `web/src/editor/Canvas.tsx:40` | claude-code/opus | maintainability |
| ⚪ | JSDoc summary rewritten to wrap mid-thought, producing a 2-line run-on; cosmetic readability regression | `web/src/components/gardens/GardenCard.tsx:18` | glm-5.2:cloud | maintainability |
| ⚪ | Dead optional-chain and nullish-fallback on parsePlanName when planYear is already non-null | `web/src/components/gardens/GardenCard.tsx:46` | claude-code/sonnet | maintainability |
| ⚪ | today re-exported from journal.ts only to satisfy JournalTab; objects.ts already imports from lib/dates — inconsistent indirection | `web/src/lib/journal.ts:131` | glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've verified the security-relevant surfaces. Here's my review.
## VERDICT: No material issues found
Reviewed through the security lens. The change threads an optional `plantedAt` through the fill path and broadens `ErrInvalidInput` error messages to reach the client — both are the natural places a security problem would hide, so I traced them.
**What I checked and cleared:**
- **`plantedAt` as tainted input → DB** (`internal/service/ops.go:198`, `internal/service/plantings.go:320`). `validDatePtr` runs `time.Parse("2006-01-02", *s)` and rejects anything non-conforming, and the value reaches the store as a bound parameter in a `domain.Planting`, not string-concatenated SQL. Even a malformed date can't inject; the format check is redundant defense, not the only barrier. `TestFillRegionPlantedAt` asserts the reject path. No injection.
- **New client-facing error text** (`internal/api/errors.go:53,122`). `inputMessage` now surfaces any wrapped `ErrInvalidInput` verbatim instead of a flat "invalid input". I grepped every wrap site in `internal/` — only two exist (`instance_settings.go:74`, `ops.go:199`), both new and both intentional; neither echoes internal state beyond the caller's own input. No pre-existing wrap gets newly exposed.
- **Credential leakage via the model-spec error** (`internal/service/instance_settings.go:74` → `specReason` → `agentmodel.Validate`). I read `agentmodel.go`: `Resolve` wraps as `agentmodel: resolve %q: %w`, and `specReason` unwraps to the majordomo `Parse` error (e.g. "unknown provider"). The `OllamaCloudAPIKey` is passed to `registry(apiKey)` but never appears in any error value — only the admin's own spec string is echoed back. No key disclosure.
- **Authz on that error path** (`internal/api/api.go:158`). The settings group is behind `requireAuth() + requireAdmin()`, so the more detailed spec/provider message is admin-only anyway; no info-exposure to ordinary users.
- **Frontend** (`monogram.ts`, `Canvas.tsx`, `CopyDialog.tsx`). `monogramInk` returns only the fixed `var(--color-*)` constants after a strict hex regex — no way for `plant.color` to inject CSS through it. `background: color` was already raw and is unchanged. Garden-name handling in `CopyDialog` is plain string compare, no sink.
Nothing in the authn/authz, injection, SSRF, deserialization, secret-leakage, or input-validation categories is materially wrong here.
One forward-looking note (not a finding, since no current site triggers it): the `inputMessage` broadening makes *every* future `%w`-wrap of `ErrInvalidInput` client-visible. CLAUDE.md and the `domain.ErrInvalidInput` doc comment both now warn "write it for the person, not the log," which is the right guardrail — worth honoring for anyone adding wraps that might carry paths or internal identifiers.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified the correctness-sensitive parts of this PR against the actual checked-out code. Here is my review through the **Correctness** lens.
## VERDICT: No material issues found
The change is careful and well-tested. I verified the following, and each holds up:
- **Date validation is strict, not just format-shaped.** `fillLoaded` guards `plantedAt` with `validDatePtr` (`internal/service/plantings.go:320`), which calls `time.Parse("2006-01-02", …)` — that rejects out-of-range dates like `2026-13-45`, not merely malformed strings. `"April 1st"` → `ErrInvalidInput`, as the new test asserts. The default path (`s.now().UTC()` when `plantedAt == nil`) is preserved for agent/API callers (`internal/service/ops.go:243-245`). All ~15 `FillRegion`/`FillNamedRegion` call sites were updated to the new signature (verified across `ops_test.go`, `revisions_test.go`, `agent/tools.go`, `agent/*_test.go`).
- **The luminance math is correct — I re-derived it.** `web/src/lib/monogram.ts:73` uses the WCAG formula (sRGB linearization with the 0.03928 knee and 2.4 exponent; coefficients 0.2126/0.7152/0.0722 mapped to R@slice0, G@slice2, B@slice4). Recomputing: sage `#7a8a5e` → L≈0.231 (< 0.37 → paper ✓); cabbage `#8bc98b` → L≈0.491 (> 0.37 → ink ✓). Matches the tests and the inline claims.
- **`inputMessage`/`specReason` compose correctly.** `agentmodel.Validate` returns `fmt.Errorf("agentmodel: resolve %q: %w", …)` (`agentmodel.go:43`), so `specReason`'s single `errors.Unwrap` (`instance_settings.go:177`) strips exactly that wrapper, and `inputMessage` (`api/errors.go:122`) trims the `"invalid input: "` prefix. Front-of-string wraps like `loading: %w` correctly fall through to the generic text — covered by `TestInputMessage`.
- **`nextPlanYear` and the GardenCard title logic are consistent.** `nextPlanYear` (`plan.ts:47`) skips every taken year for the base from `from` upward; `planYearOf` gates the tag/title on `year >= currentYear`, so a past-year plan falls back to the full name with no tag — no mismatch between `title` and the `{planYear} plan` tag.
- **`LengthField` invariant is preserved.** `cm` only moves on `editDimensionField`/`editSpacingField` (a keystroke); `convertDimensionField` re-derives text from the same `cm`; Save reads `field.cm` directly. A no-change Save now sends the loaded centimeters — the 900→899.922 drift is genuinely fixed.
- **The Remove-confirmation gate can't silently misfire.** `plantCountIn` (`Inspector.tsx:45`) sums `plopCount`, which falls back to `p.derivedCount` when the plant is absent (`shared.ts:98`) — so the count is always a real number, never `NaN`, and `plantCount > 0` reliably triggers the confirm. Wiring at `GardenEditorPage.tsx:393` is correct.
- **`CreatePlanting`'s `{ plantedAt: today(), ...body }` merge is safe** — TypeScript optional-omitted keys are absent from `body`, so the default isn't clobbered by an `undefined`.
No correctness defects to report in this lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings check out against the actual code:
- **Finding 1:** `FALLBACK_PLANT_COLOR = '#97a97c'` is defined at `Canvas.tsx:40`; the identical literal appears raw at `Inspector.tsx:359` and `GardenThumb.tsx:75` (same "plant missing from catalog" default), while `ColorSwatches.tsx:5` `CURATED_SWATCHES[0]` is a semantically distinct swatch. Confirmed.
- **Finding 2:** `GardenDialog`/`PlantDialog` use `LengthField`; `Inspector.tsx` (`commitDim`, lines 106–143) still keeps display strings via `formatDimensionInput` and re-parses with a string-compare guard. Two mechanisms for one concern, matching the draft. Confirmed.
## VERDICT: Minor issues
The refactors in this PR are, on the whole, clean and well-targeted through the maintainability lens: the `LengthField` consolidation removes the ad-hoc `convert`/`parseDimension` juggling in `GardenDialog`, `today()` becomes a single source with `journal.ts` re-exporting it (no duplicated impl), the old dead imports in both dialogs are fully removed, and `inputMessage`/`specReason`/`nextPlanYear` are small, single-purpose, well-documented helpers. The `--color-marker-ink` token is deliberately single-placed (documented as theme-stable, matching the existing `--color-paper` pattern, so the "color in both places" convention doesn't apply).
A few small things:
- **`web/src/editor/Canvas.tsx:40` — half-finished extraction of the fallback plant color.** This PR names the unknown-plant fallback `FALLBACK_PLANT_COLOR = '#97a97c'` in Canvas (correctly reusing it for both the circle fill and `monogramInk`), but the identical literal stays raw at `Inspector.tsx:359` (`ColorDot color={plant?.color ?? '#97a97c'}`) and `GardenThumb.tsx:75` (`fill={plantColor.get(...) ?? '#97a97c'}`) — the same "plant missing from the catalog" default. The constant is module-local to Canvas, so the magic value is now named in one place and raw in two others that mean exactly the same thing, which is the opposite of what naming it was supposed to buy. Low-churn fix: export a single `FALLBACK_PLANT_COLOR` (e.g. from `lib/plants.ts`) and use it in all three. (`ColorSwatches.tsx:5` `CURATED_SWATCHES[0]` also equals `#97a97c` but is semantically "first curated swatch," not the fallback — leave it alone.)
- **`web/src/editor/Inspector.tsx:106-143` — the inspector didn't adopt the `LengthField` pattern this PR standardizes.** The dialogs now hold `LengthField`s so cm is the source of truth; the inspector still keeps display strings (`formatDimensionInput`) and re-parses in `commitDim`, guarding no-change edits with a string compare (`raw.trim() === formatDimensionInput(current, unit)`) instead. It achieves the same "don't rewrite on a no-op Save" goal, so this isn't a bug — but the CLAUDE.md note added here ("The inspector guards the same way") papers over the fact that there are now two mechanisms for one concern. Converting the inspector to `LengthField` is more churn than this PR's scope, so this is a maintainability heads-up, not a required change.
Nothing blocking in my lens.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've reviewed the changes through the Performance lens. Let me confirm my read of the two spots that touch render/query paths.
The `plantCountIn` addition (`Inspector.tsx:45`) is called only for the single selected object (`GardenEditorPage.tsx:393`), right alongside the pre-existing `rosterText` which already does the identical O(plantings) pass — so it's one extra linear scan over one object's inspector render, not per-object work. `monogramInk`/`luminance` in `Canvas.tsx:650` runs only in the label loop, gated by focus (`p.objectId !== focusId` returns null) and by `r * s < 9`, and the work is a single regex match plus a few `parseInt`s — negligible even for a full focused bed. The `today()` calls and the service-side `validDatePtr` check are one-shot per mutation.
VERDICT: **No material issues found**
- No N+1s, unbounded growth, or new quadratic behavior. The fill path still batches inserts in one transaction; the only addition is a constant-time date validation.
- The one new per-render computation (`monogramInk` on the canvas) is bounded by focus + zoom gating and is cheap per call — not a material regression versus the previous constant string.
- The new `plantCountIn` scan is single-object, same order as the adjacent existing `rosterText` scan; not worth memoizing.
- `useGardens()` in `CopyDialog` reads an already-cached list on the gardens page — no extra fetch on the hot path.
Nothing in my lane rises to a finding.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've verified the error-handling and edge-case paths thoroughly across both the Go service layer and the frontend. Here is my review.
## VERDICT: No material issues found
I reviewed this change strictly through the **error handling & edge cases** lens and verified each concern against the checked-out code. The unhappy paths this PR introduces are all handled:
- **`plantedAt` validation (server).** `fillLoaded` calls `validDatePtr(plantedAt)` *before* using it (`internal/service/ops.go:198`), and `validDatePtr` (`internal/service/plantings.go:320`) is nil-safe (returns `true` for nil) and rejects malformed strings via `time.Parse(dateLayout, …)`. The nil default correctly falls through to `s.now().UTC()` at `ops.go:243-246`. `TestFillRegionPlantedAt` exercises both the good and bad-date branches. Go's `time.Parse` also range-checks (e.g. `2026-02-30` is rejected), so no out-of-range date leaks through.
- **`inputMessage` (server).** Handles the bare-sentinel case (`msg == base` → returns generic "invalid input") and only strips a `"invalid input: "` prefix otherwise (`internal/api/errors.go`), so a suffix-wrapped error is returned verbatim rather than mangled. `TestInputMessage` covers all three shapes. `specReason` is nil-safe (`errors.Unwrap` guarded).
- **LengthField (frontend).** `editDimensionField` / `editSpacingField` return `cm: null` on unparseable/empty input (`web/src/lib/units.ts:251,267`), and every consumer guards it: `GardenDialog` checks `widthCm === null || heightCm === null` then `isValidDimensionCm(...)` (rejects negative/zero), and `PlantDialog` checks `spacingCm === null || spacingCm < 1`. `convertDimensionField` preserves the raw text when `cm` is null so a typo stays visible on a unit switch.
- **Confirm-remove (frontend).** `ConfirmDialog` wraps `onConfirm` in try/catch, surfaces failures inline (e.g. a 409), and re-enables the button for retry; the Inspector's `onConfirm` awaits `del.mutateAsync` before `onDeleted()`, so a failed remove doesn't deselect. `plantCount` is computed via `plantCountIn` → `plopCount`, which falls back to `p.derivedCount` when the plant is missing from the catalog (`web/src/editor/shared.ts:96`), so the confirm gate isn't skipped for a private plant in a shared garden.
- **Monogram color (frontend).** `luminance()` returns `null` for anything that isn't `#rgb`/`#rrggbb`, and `monogramInk` falls back to paper on null (`web/src/lib/monogram.ts`); the canvas and Monogram both feed it a `FALLBACK_PLANT_COLOR` when `plant` is undefined. Tested for named colors and empty strings.
- **`nextPlanYear` (frontend).** The `while (taken.has(year))` loop iterates over a finite set and always terminates (`web/src/lib/plan.ts:53`).
The test clock-bomb fix (`seed_lots_test.go`) pins both `plantedAt` (2026-07-01) and the removal (2026-08-01) to fixed dates in the correct order, so it no longer depends on the wall clock.
Nothing in my lane is materially wrong here — the diff is genuinely careful about its unhappy paths.
</details>
</details>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
I traced all tainted-data paths in this PR:
- **`plantedAt` string (API input → DB):** Validated in `fillLoaded` via `validDatePtr`, which calls `time.Parse("2006-01-02", ...)` — only strict YYYY-MM-DD strings pass. It reaches the database exclusively through parameterized `?` placeholders (`plantingInsert` in `internal/store/plantings.go`). No injection surface.
- **`inputMessage` / wrapped `ErrInvalidInput`:** The function strips the sentinel prefix and returns the wrapped reason verbatim. Verified all two `fmt.Errorf("%w: …", domain.ErrInvalidInput)` calls in `internal/service/` place the sentinel first (the required contract). The third test case (`"loading: %w"`) documents what happens when the contract is broken, but no production code hits that path today.
- **Model spec echoed in 400 body (`instance_settings.go:74`):** The spec is the admin's own submitted value, returned to that same admin. `requireAdmin` gates the route. Intentional and safe.
- **React rendering of user-controlled strings (`CopyDialog`, `GardenCard`):** Garden names and the warning message interpolate user data via JSX (`{name.trim()}`), which React escapes. No XSS vector.
- **`monogramInk` / `luminance`:** User-controlled plant colors flow through a hex regex; the function returns only one of two fixed CSS variable strings regardless of input. No injection possible.
- **No new routes, no auth model changes, no credential handling, no SSRF surface introduced.**
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've now verified all the correctness-sensitive pieces. Let me write the review.
---
**No material issues found**
I checked every formula, conversion, and domain invariant introduced by this PR:
**`luminance` in `monogram.ts:87`** — verified. `channel(0)` → `hex.slice(0,2)` = R, `channel(2)` → `hex.slice(2,4)` = G, `channel(4)` → `hex.slice(4,6)` = B; coefficients 0.2126/0.7152/0.0722 sum to 1.0 and match WCAG 2.1. Threshold 0.37 checks out: paper contrast = 1.05/(0.37+0.05) ≈ 2.5:1. Verified all six test fixtures by hand against the formula; they pass.
**`today()` in `dates.ts:8`** — correct. `getMonth()+1` gives 1–12, `getDate()` gives 1–31, `getFullYear()` is unambiguous. Both test cases verified.
**`LengthField` in `GardenDialog`, `PlantDialog`** — the invariant holds. `dimensionField(cm, unit)` stores the loaded cm directly; `editDimensionField(text, unit)` stores the parsed cm alongside the raw text; `convertDimensionField` reformats from cm without re-parsing. Submit path reads `.cm` directly — no re-parse of a display string on save.
**`inputMessage` in `errors.go:119`** — all three test cases verified: bare sentinel → returns unchanged; `%w`-prefixed wrapping → `TrimPrefix` strips `"invalid input: "` correctly; `%w`-suffixed wrapping → TrimPrefix is a no-op and the full message is returned.
**`validDatePtr` in `plantings.go:320`** — nil is allowed, otherwise `time.Parse(dateLayout, *s)` enforces YYYY-MM-DD strictly.
**`&plantedOn` aliasing in `ops.go:259`** — `plantedOn` is set once before the loop and never mutated inside it; all pointers are live when `CreatePlantings` is called, which dereferences each in its own loop body. The old code used `&today` identically; not a new risk.
**`nextPlanYear` in `plan.ts:42`** — correctly builds a set of taken years for the named base, then increments from `from` until it finds a free slot. Logic is sound.
**`specReason` in `instance_settings.go:172`** — `agentmodel.Validate` wraps as `fmt.Errorf("agentmodel: resolve %q: %w", spec, innerErr)`, so `errors.Unwrap` returns the inner parse error, stripping the redundant spec from the message.
**`plantCountIn` in `Inspector.tsx:44`** — correct. The `plantings` array fed to it is the editor's active-only slice (soft-removed entries are filtered out in `useRemovePlanting`'s `onMutate`).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
The finding checks out. When `planYearOf` returns non-null, it has already called `parsePlanName` and confirmed a non-null result with a valid year — so the second `parsePlanName` call on line 46 is guaranteed to return non-null, making both `?.base` and `?? garden.name` unreachable.
**Verdict: Minor issues**
One verified cleanliness finding; the rest of the PR is tidy through this lens.
---
### `web/src/components/gardens/GardenCard.tsx:46` — dead optional-chain fallback
```ts
const planYear = planYearOf(garden.name) // line 43
const title = planYear != null
? (parsePlanName(garden.name)?.base ?? garden.name) // ← ?.base and ?? unreachable
: garden.name
```
`planYearOf` (in `lib/plan.ts`) does `const p = parsePlanName(name); return p && p.year >= now.getFullYear() ? p.year : null`, so `planYear != null` is only true when `parsePlanName` already returned a non-null result for the same input. Calling `parsePlanName(garden.name)` again inside the true-branch is guaranteed to succeed; the `?.` optional chain and `?? garden.name` fallback are unreachable dead code. A reader seeing `?.base ?? garden.name` naturally wonders what case produces a null — there isn't one.
**Fix:** `parsePlanName(garden.name)!.base` (non-null assertion) or factor it out:
```ts
const plan = parsePlanName(garden.name)
const planYear = plan != null && plan.year >= new Date().getFullYear() ? plan.year : null
const title = plan != null ? plan.base : garden.name
```
---
Everything else looks clean for this lens: the `LengthField` abstraction is applied consistently across the two dialogs; `today()` was correctly centralised in `dates.ts` with a re-export from `journal.ts` for existing callers; `plantCountIn` is a fresh export used in exactly one call site with no duplication of `rosterText`'s logic; the `inputMessage`/`specReason` pair are narrow, well-tested helpers; and the constant extraction of `FALLBACK_PLANT_COLOR` in `Canvas.tsx` is a straightforward cleanup over the inline literal.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
The finding is confirmed. At `Canvas.tsx:650`, `monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)` is called inside `renderedPlops.map()` on every render. The `luminance()` function in `monogram.ts:79–88` does the regex, spread, parseInt, and `** 2.4` work the draft describes. The only cached per-plant lookup in that loop is `letters` (a `Map<number, string>` prop from the parent); no equivalent ink cache exists.
---
**Minor issues**
The monogramInk call introduced by this PR to support dark/light ink selection runs once per plop on every canvas frame, with no caching.
**`web/src/editor/Canvas.tsx:650` — `monogramInk()` called once per plop, every frame, with no caching**
`monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)` sits inside `renderedPlops.map()`. `monogramInk()` calls `luminance()`, which does a regex match, an array spread (3-digit hex expansion), three `parseInt` calls, and three `** 2.4` floating-point powers. For a garden with 200 visible plops across a handful of plant species, every canvas frame recomputes the same 5–10 luminance values 200 times instead of once. Canvas re-renders on every pointer move during pan/zoom — up to 60 fps — so a full planting can mean ~12,000 redundant luminance computations per second, all returning identical results.
The `letters` prop already shows the right pattern: compute `new Map(plants.map(p => [p.id, monogram(p)]))` in the parent with `useMemo([plants])`, pass it as a prop, and look up by `plantId` in the loop. An `inks` map of `plantId → monogramInk(p.color)` follows the same shape and fixes the issue entirely.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I now have enough information to complete my review.
---
**No material issues found**
I traced every new unhappy path introduced by this PR through the actual checked-out code.
**`inputMessage()`** (`internal/api/errors.go:115`): The three cases are all correct and covered by the new unit test — bare sentinel returns "invalid input", the `%w: reason` wrap strips the prefix, and a `"outer: %w"` wrap returns the full message. The string-stripping logic is sound because `writeServiceError` only calls it after `errors.Is` has already confirmed the sentinel is present.
**`specReason()`** (`internal/service/instance_settings.go:179`): `agentmodel.Resolve` always wraps its errors as `fmt.Errorf("agentmodel: resolve %q: %w", spec, underlying)`, so `errors.Unwrap` reliably returns the underlying majordomo error. The only other path — an empty spec — is guarded by the `if f.spec == "" { continue }` check before `Validate` is ever called, so `specReason` never sees a non-wrapping error from this code.
**`validDatePtr` in `fillLoaded`** (`internal/service/ops.go:197`): Placed correctly before `validFillLayout` and before computing lattice centers. Returns `nil, wrapped ErrInvalidInput` with a user-readable message that `inputMessage` will properly strip. The existing `validDatePtr` shared with `plantings.go`, `journal.go`, and `seed_lots.go` uses `time.Parse("2006-01-02", …)` which is strict enough for the stated contract.
**`plantedOn` pointer aliasing in the batch loop** (`internal/service/ops.go:255`): All `domain.Planting` entries point to `&plantedOn` — the same address. This was the pre-existing pattern (`today` before, `plantedOn` now); the store reads the string value at insertion time, not later, so there is no aliasing hazard introduced here.
**`plopCount` with undefined plant** (`Inspector.tsx`/`plantCountIn`): When `plantsById.get(p.plantId)` is undefined (a shared garden's private plant), `plopCount` falls through to `p.derivedCount` (verified in `shared.ts:98`). No panic path.
**Confirm-dialog remove flow** (`Inspector.tsx:290`): The confirmed path uses `mutateAsync` + `onDeleted()` called only on success; ConfirmDialog surfaces `errorFallback` on throw and keeps the dialog open for retry. The empty-object path uses the existing optimistic `onDeleted()` → `mutate()` order, unchanged from before this PR.
**`luminance()` channel indices** (`monogram.ts:84`): Verified: `channel(0)`→R, `channel(2)`→G, `channel(4)`→B for a 6-char hex string. WCAG weights are correctly ordered.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## VERDICT: No material issues found
Through the security lens, the change holds up:
- **`plantedAt` input validation is present and correct.** `fillLoaded` (`internal/service/ops.go:198`) guards the new pointer with `validDatePtr`, which uses `time.Parse(dateLayout, *s)` against `"2006-01-02"` (`internal/service/plantings.go:320`). `time.Parse` rejects out-of-range month/day values, so a malicious client can't push an arbitrary string through the fill route — it gets a wrapped `ErrInvalidInput` → 400. Verified by reading `validDatePtr` and the `ops.go` call site.
- **No injection surface.** The date is stored as a string and only ever re-parsed with the same strict layout; it never reaches SQL or a template. The `today()` helper (`web/src/lib/dates.ts`) is a pure local-date formatter with no tainted input.
- **Reflected error message is safe.** `inputMessage` (`internal/api/errors.go:125`) echoes the wrapped reason — which includes the user-supplied model spec — into the 400 body. That body is serialized by gin's `c.JSON` (JSON-escaped), and on the client it's surfaced under a field in React (text-escaped by default), so there is no HTML/script injection path. No `dangerouslySetInnerHTML` was indicated anywhere in the diff. Low risk, not a finding.
- **`monogramInk` luminance math is correct** (verified `web/src/lib/monogram.ts:78-88`): channel indexing `0/2/4` maps R/G/B and the WCAG coefficients `0.2126/0.7152/0.0722` are applied to the right channels with the standard linearization. Not a security issue, but the magic number checks out.
- **Remove confirmation (`Inspector.tsx`) introduces no authz change** — it gates a UI action behind a confirm dialog and still routes through the existing authorized `useDeleteObject` mutation; no new privileged path was opened.
Nothing in my lane is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
## VERDICT: No material issues found
I traced every correctness-sensitive path in this PR through the actual checked-out code.
**Verified clean:**
- **`monogramInk` luminance math** (`web/src/lib/monogram.ts:79-93`): channel indices are correct — `channel(0)`→red weighted 0.2126, `channel(2)`→green weighted 0.7152, `channel(4)`→blue weighted 0.0722 (standard WCAG). The gamma transform `v ≤ 0.03928 ? v/12.92 : ((v+0.055)/1.055)**2.4` is the correct WCAG sRGB→linear formula. The `PAPER_MAX_LUMINANCE = 0.37` threshold is derivable: paper (~L=1.0) on a background of L gives contrast (1.05)/(L+0.05); at L=0.37 that's 1.05/0.42 = 2.5:1 — exactly the stated cutoff. Sage (#7a8a5e, ~0.23 < 0.37) keeps paper; cabbage (#8bc98b, ~0.49 > 0.37) flips to ink. The 3-digit hex expansion `c+c` is correct.
- **`LengthField` no-change-save fix** (`web/src/lib/units.ts`, `GardenDialog.tsx`, `PlantDialog.tsx`): `dimensionField`/`spacingField` store the original `cm` verbatim; save reads `field.cm` directly, never re-parsing the display text. `convertDimensionField` regenerates text from the unchanged `cm`. `editDimensionField`/`editSpacingField` only re-parse on actual typing. This is the precise fix for the 899.922 / 44.958 drift. Round-trip verified against `formatDimensionInput`/`parseDimension` and `spacingFromCm`/`cmFromSpacing`.
- **`today()` local date** (`web/src/lib/dates.ts`): `new Date(2026, 7, 22, 21, 30)` → `2026-08-22` (month 7 = August, 0-indexed, correct); `new Date(2026, 0, 5, 0, 1)` → `2026-01-05`. Uses local getters only. Correct.
- **`plantedAt` plumbing** (`internal/service/ops.go:198,243-245`): `validDatePtr` (plantings.go:320) uses `time.Parse(dateLayout="2006-01-02", …)`, matching the format `today()` emits. Nil → UTC today default; valid string used verbatim; invalid → wrapped `ErrInvalidInput`. `useCreatePlanting` spreads `{ plantedAt: today(), ...body }` so an explicit caller value still wins. Correct.
- **`inputMessage` prefix handling** (`internal/api/errors.go:125-131`): bare sentinel → "invalid input"; `fmt.Errorf("%w: …", ErrInvalidInput)` → trimmed reason; a leading wrap like `fmt.Errorf("loading: %w", ErrInvalidInput)` → `.Error()` is "loading: invalid input", `TrimPrefix` finds no "invalid input: " prefix, returns the whole string unchanged. Matches all three test cases.
- **`specReason` unwrap** (`instance_settings.go:175-180`): `agentmodel.Validate` returns `fmt.Errorf("agentmodel: resolve %q: %w", spec, innerErr)`; `errors.Unwrap` yields `innerErr`, so the message reads the provider's own reason without repeating the spec — the test asserts `chat model "nonesuch/model": <reason>`. Consistent.
- **`nextPlanYear`** (`web/src/lib/plan.ts:47-55`): builds a `Set` of years taken by plans whose `base` matches, then increments from `from`. The `taken` duplicate-name guard in `CopyDialog` is a superset (any exact name match), so it never false-positives on the auto-proposed (free-year) name. Correct.
No correctness defects found in this lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three draft findings are confirmed against the actual code. Producing the corrected review with all surviving findings.
VERDICT: Minor issues
- **`web/src/editor/Inspector.tsx:104-112` (and 217-220)`** — CLAUDE.md (line 137) states "The inspector guards the same way" as part of the new LengthField convention, but the Inspector's own dimension/grid fields still hold plain `string` state (`useState(formatDimensionInput(...))`) and still re-parse the display string on blur via `commitDim`/`commitGrid` (lines 137-149, applied at 217-220). The `LengthField` pattern the PR introduces in `GardenDialog.tsx` and `PlantDialog.tsx` (cm as source of truth, text as a view) is not actually applied here, so the same round-trip-drift class of bug the PR fixes elsewhere is left in place. This is an incomplete migration / an inaccurate doc claim, not a newly introduced bug, but it's the obvious cleanliness gap given the stated convention.
- **`web/src/lib/journal.ts:131`** — `today` is re-exported (`export { today } from './dates'`) purely so `JournalTab.tsx:7-16` can keep importing it from `@/lib/journal`, while `objects.ts:10` already imports `today` directly from `@/lib/dates`. With only one consumer, this is a needless second source of the symbol; re-pointing JournalTab's import at `@/lib/dates` and dropping the re-export would be consistent with the rest of the codebase.
- **`web/src/components/gardens/GardenCard.tsx:18-21`** — The JSDoc summary was rewritten to wrap awkwardly mid-thought ("name (a / plan copy shows its base name and a `<year> plan` tag), size, ..."), producing a 2-line run-on where the rest of the file uses terse one-line summaries. Cosmetic, but a readability regression in a file-level comment.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
I've verified the finding against the actual code. The diff confirms `Canvas.tsx:650` changed from a constant `fill="var(--color-paper)"` to `fill={monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)}`, called inside `renderedPlops.map(...)` (line 635). `monogramInk` (in `web/src/lib/monogram.ts:91`) is a pure function delegating to `luminance` (line 79), which does the regex/hex-expansion/parseInt/exponentiation math described. The canvas already memoizes `renderedPlops`/`byId` but does NOT memoize the color→ink mapping — `monogramInk` is invoked inline per plop per render, and the canvas re-renders on pan/zoom/animation frames (ANIM_MS=520). Colors come from a small finite catalog set, so the work is repeatable and avoidable. The finding is accurate.
## VERDICT: Minor issues
**Performance lens review.** The change is mostly clean efficiency-wise (batch insert preserved, no new N+1, fill date computed once per batch). One real, if minor, hot-path regression:
- **`web/src/editor/Canvas.tsx:650` — `monogramInk()` recomputed per plop per render frame.** The canvas is a hot path: it re-renders on every pan/zoom/animation tick (ANIM_MS=520ms of refit/transition frames), and `renderedPlops.map(...)` calls `monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)` for each visible plop. `monogramInk` → `luminance` does a `trim()` + regex match + conditional `[...hex].map(c => c+c).join()` + three `parseInt`/division + three `**2.4` exponentiations. Previously this was a constant `"var(--color-paper)"`. The result depends only on the plant's color, which is a small finite set (the catalog), so a plop-heavy bed re-running this at 60fps across the animation is avoidable repeated work. Verified by reading `web/src/lib/monogram.ts` (pure function of `color`) and `Canvas.tsx` render loop (the component memoizes `renderedPlops`/`byId` but not the color→ink mapping).
- **Fix:** memoize the mapping, e.g. a module-level `Map<string, string>` keyed by color, or `useMemo` building `{plantId → ink}` from `plantsById` once per render, so the luminance math runs once per distinct color rather than once per plop per frame.
Nothing else in the performance lane rose to material concern: `nextPlanYear`/`names` in `CopyDialog.tsx` run over the gardens list each render but that list is small and the dialog is not a hot path; `plantCountIn`/`plantCount` is a single linear scan over plantings in the inspector (not per-frame); the service fill path computes `plantedOn` once per batch, not per plop.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed the change through the error-handling & edge-cases lens, verifying the unhappy paths the diff introduces:
- **`inputMessage` (errors.go)**: Verified `internal/api/errors.go:33-40` and its test. The bare-sentinel short-circuit returns `"invalid input"`; wrapped errors get `strings.TrimPrefix(msg, "invalid input: ")`, which is a no-op when the sentinel isn't the prefix (e.g. `"loading: invalid input"` stays whole). Both branches behave as the test asserts; no panic on a nil-ish message (the function only receives a non-nil error from `errors.Is`).
- **Fill `plantedAt` validation (service/ops.go:194-200)**: Confirmed `validDatePtr` (plantings.go:319-326) accepts nil or strict `YYYY-MM-DD`; a bad string returns a wrapped `ErrInvalidInput` with a user-facing reason. The nil path correctly falls back to `s.now().UTC()`. No empty-string edge case slips through (`time.Parse` rejects `""`).
- **Create/fill UI defaults (objects.ts)**: `{ plantedAt: today(), ...body }` ordering lets a caller-supplied `plantedAt` override the default intentionally; `today()` handles any `Date` (verified dates.test.ts), no NaN/undefined risk.
- **Inspector Remove confirm (Inspector.tsx:192-202, 281-296)**: `plantCount > 0` gates the confirm; the `ConfirmDialog` owns its busy/error state and keeps the dialog open on throw (verified ConfirmDialog.tsx:37-47), so a 409 is surfaced not swallowed. The empty-object path still deletes directly. No double-delete race — `setConfirmRemove(true); return` short-circuits before `del.mutate`.
- **CopyDialog `taken`/`names` (CopyDialog.tsx:24-33)**: `gardens.data ?? []` avoids a crash when the query is loading; `taken` is a non-blocking advisory, not a submit guard, so a stale list can't block a valid copy.
One thing I could not fully verify (out of budget): `plantCountIn` (Inspector.tsx:45-49) sums over every planting passed in without filtering by removed-state. If the editor's `plantings` array includes soft-removed plops, the confirm could fire for an object whose plants are all already removed. I didn't reach the call site to confirm what that array contains, so I'm not reporting it as a finding — and even if true, hard-deleting the object removes those rows too, so the warning is arguably still correct.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
- **Authn/authz**: No changes to auth boundaries; `FillRegion`/`FillNamedRegion` still gate through `objectForRole` with `roleEditor`, and `UpdateInstanceSettings` still requires admin.
- **Input validation**: The new optional `plantedAt` parameter is validated with the existing `validDatePtr` (parses against `2006-01-02` layout), rejecting malformed dates with a clean `ErrInvalidInput`.
- **Error exposure**: The `inputMessage` / wrapped-`ErrInvalidInput` design is intentional and documented (CLAUDE.md). User-controlled specs are `%q`-quoted before inclusion, and `specReason` strips one layer of wrapping to avoid doubling the spec in the message. No secret material (e.g., the Ollama API key) is incorporated into the formatted error.
- **No injection surfaces**: No SQL, command, path, or template injection introduced. `LengthField` and date handling operate on already-validated or client-side data without new server-side parsing risks.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found.
After verifying the key correctness-sensitive changes:
- **`today()` in `web/src/lib/dates.ts`** correctly uses local-time `getFullYear`/`getMonth`+1/`getDate`, fixing the UTC-tomorrow bug for west-of-Greenwich gardeners. The test confirms zero-padded `YYYY-MM-DD` output.
- **`validDatePtr` in `internal/service/plantings.go:320`** accepts `nil` (API/agent default) and validates `YYYY-MM-DD` format. The `fillLoaded` flow: nil → UTC today; valid string → passed through; invalid → `ErrInvalidInput`. Verified correct.
- **`inputMessage` in `internal/api/errors.go`** strips only a leading `"invalid input: "` prefix; the test suite covers bare sentinel, standard wrapped form, and non-standard wrapping. The logic is correct for the documented convention.
- **`specReason` in `internal/service/instance_settings.go:177`** unwraps one level to strip the `agentmodel.Resolve` wrapper (`"agentmodel: resolve %q: …"`), so the final user-facing message doesn't repeat the spec. Verified against the `agentmodel` package structure.
- **`monogramInk` threshold `PAPER_MAX_LUMINANCE = 0.37`** correctly implements the WCAG relative-luminance formula. At `L = 0.37`, white-on-marker contrast is `(1.05)/(0.42) = 2.5:1`, matching the documented cutoff. Verified against the test values.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
The PR is clean and well-structured through the maintainability lens:
- **Good abstraction**: `LengthField` in `web/src/lib/units.ts` cleanly separates display text from canonical centimeters, with `dimensionField`/`editDimensionField`/`convertDimensionField` and `spacingField`/`editSpacingField` providing a consistent pattern used by both `GardenDialog.tsx` and `PlantDialog.tsx`.
- **Consistent patterns**: `today()` in `web/src/lib/dates.ts` centralizes local-date generation; `monogramInk()` in `web/src/lib/monogram.ts` encapsulates the luminance threshold logic with clear documentation; `nextPlanYear()` in `web/src/lib/plan.ts` extracts the collision-avoidance logic from `CopyDialog.tsx`.
- **No dead code**: The old `formatDimensionInput`/`parseDimension` helpers remain appropriately in use by `Inspector.tsx`, which guards against re-parsing via its `commitDim` early-return check.
- **Clear naming**: `plantCountIn` and `rosterText` describe what they compute; `inputMessage` and `specReason` accurately describe their purpose.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
Nothing in this change introduces a performance regression I can justify:
- The `LengthField` refactor (web/src/lib/units.ts) replaces string re-parsing on save with a stored numeric value — that's fewer allocations, not more.
- `monogramInk` runs per plop label on the SVG canvas. It does a short regex and a few arithmetic ops; at expected garden scales this adds well under a millisecond per frame and is dwarfed by DOM paint cost. Not material.
- `plantCountIn` duplicates the linear scan already done by `rosterText` for the selected object, but both are O(n) over the plantings array and run only when an object is selected. For typical counts this is sub-millisecond. Not material.
- No N+1 queries, unbounded growth, missing pagination, or blocking hot-path calls were introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`web/src/components/gardens/CopyDialog.tsx`**: `useState(() => planNameFor(base, year))` captures the initial `year` value once. If `useGardens()` data is still loading when the dialog opens, `names` is empty, `nextPlanYear` returns `from` (which may already be taken), and the suggested name stays stale even after the list loads. The `taken` warning catches this on the next render, but the initial default is wrong and the user can submit before noticing. Compute the name inside a `useEffect` (or derive state) so it reacts when `gardens.data` arrives.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
A Save that changed nothing still sent a PATCH, which bumped the row's
version and landed an "Edited garden settings" step in History that undid
nothing — the drift is gone since the last commit, but the write was still
there. Both dialogs now close without a request when every field matches
the loaded row.
In Settings, a rejected model spec's reason stayed under the field after
the field was blanked back to the saved value; committing an unchanged
value now clears it.
Co-Authored-By: Claude Fable 5 <[email protected]>
- monogramInk is memoized by color string; the canvas asks for every
visible plop on every frame of a pan (Gadfly, 2/4 models).
- FALLBACK_PLANT_COLOR lives in lib/plants and is used by the canvas, the
inspector and the garden thumbnail instead of three raw '#97a97c's.
- CopyDialog keeps its proposed "<base> — <year>" in step with the gardens
list until the person edits the name, so a list that loads after the
dialog opens can't leave a taken year in the field.
- GardenCard: reflowed the summary comment; no dead fallback on a plan
name that's already known to parse.
- today() has one import path (lib/dates); the journal re-export is gone.
- CLAUDE.md says what the inspector actually does (a text-compare guard)
rather than claiming it uses LengthField.
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit 5622b1accd into main2026-08-23 02:26:06 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes items 1–8 from the post-#124 smoke sweep (scratch build of
maindriven through Playwright at 1280×800 and 390×844).What changed
LengthField(web/src/lib/units.ts): the text is a view, the centimeters change only when the person types, and a unit switch re-shows the same centimeters. A Save that changed nothing closes without sending anything at all. Unit tests cover the round trip.plantedAtto the server, which stamps UTC today — "planted tomorrow" for anyone west of Greenwich after ~8 pm, and notes sorting under older entries.today()now lives inweb/src/lib/dates.tsand the UI sends it for plop placement, fill and removal too.POST /objects/:id/fillgained an optionalplantedAt(validated as YYYY-MM-DD); API and agent callers that omit it still get UTC today. The agent's own tool defaults remain server-side UTC — giving it the gardener's local day is a follow-up.#d9d2c5and cabbage green.monogramInk()picks a theme-stable dark ink above a luminance cutoff (~2.5:1 where paper stops working); tomato, sage and the other saturated colors keep paper. Applied on the Plants page and the canvas; one new token--color-marker-ink.nextPlanYear) and warns if the typed name already exists.plan). A plan card shows the base name and a2027 plantag.ErrInvalidInputto "invalid input". A wrappedErrInvalidInputreason now reaches the client as the 400's message (inputMessage, with the contract documented on the sentinel), the settings service wraps with which model and why, and the Settings page shows it under the field instead of toasting.Also defuses a clock bomb:
TestRemainingReturnsWhenAPlantingIsRemovedhard-coded a removal date of 2026-08-01 against aplantedAtthat defaulted to the real date, so it has failed since August 2nd.Verification
GOWORK=off go build ./... && go test ./...green,gofmt -lclean (plus new tests:TestFillRegionPlantedAt,TestInputMessage, the bad-model 400 now asserts its message).npx tsc --noEmit,vitest run(132 tests, 9 new),npm run buildgreen.chat model "garbage": unknown provider: …under the field.ErrInvalidInputis user-facing); DESIGN.md notesplantedAton the fill route.Review
Gadfly's sweep: security/correctness/error-handling clean across all four models; 7 minor findings, all addressed in
0d95578—monogramInkmemoized by color (it ran per plop per frame), one sharedFALLBACK_PLANT_COLORinstead of three raw literals, the Copy dialog's proposed name follows the gardens list until edited, two GardenCard nits,todaywith a single import path, and CLAUDE.md now describes the inspector's actual guard rather than claiming it usesLengthField.🤖 Generated with Claude Code
🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 02:20:48Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -17,3 +17,2 @@/*** One garden as a card: the plot thumbnail (a link into the editor), name + an* optional `plan` tag, size, a counts line, who it's shared with, and a footer* One garden as a card: the plot thumbnail (a link into the editor), name (a⚪ JSDoc summary rewritten to wrap mid-thought, producing a 2-line run-on; cosmetic readability regression
maintainability · flagged by 1 model
web/src/components/gardens/GardenCard.tsx:18-21— The JSDoc summary was rewritten to wrap awkwardly mid-thought ("name (a / plan copy shows its base name and a<year> plantag), size, ..."), producing a 2-line run-on where the rest of the file uses terse one-line summaries. Cosmetic, but a readability regression in a file-level comment.🪰 Gadfly · advisory
@@ -43,1 +43,4 @@const planYear = planYearOf(garden.name)// A plan's year is the point of its name, and the first thing truncation// would eat ("Back Yard — 20…"); show the base name and put the year on the tag.const title = planYear != null ? (parsePlanName(garden.name)?.base ?? garden.name) : garden.name⚪ Dead optional-chain and nullish-fallback on parsePlanName when planYear is already non-null
maintainability · flagged by 1 model
web/src/components/gardens/GardenCard.tsx:46— dead optional-chain fallback🪰 Gadfly · advisory
@@ -36,1 +37,4 @@// A plop whose plant is missing from the catalog (a shared garden's private// plant) still needs a color to be drawn in.const FALLBACK_PLANT_COLOR = '#97a97c'🟡 Fallback plant color #97a97c named in Canvas but left raw in Inspector.tsx:359 and GardenThumb.tsx:75; extract one shared constant
maintainability · flagged by 1 model
web/src/editor/Canvas.tsx:40— half-finished extraction of the fallback plant color. This PR names the unknown-plant fallbackFALLBACK_PLANT_COLOR = '#97a97c'in Canvas (correctly reusing it for both the circle fill andmonogramInk), but the identical literal stays raw atInspector.tsx:359(ColorDot color={plant?.color ?? '#97a97c'}) andGardenThumb.tsx:75(fill={plantColor.get(...) ?? '#97a97c'}) — the same "plant missing from the catalog" default. The constant is module-loc…🪰 Gadfly · advisory
@@ -643,3 +648,3 @@dominantBaseline="central"fontSize={r * 1.05}fill="var(--color-paper)"fill={monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)}🟠 monogramInk() called once per plop per render frame with no caching; was previously a static string
performance · flagged by 2 models
web/src/editor/Canvas.tsx:650—monogramInk()called once per plop, every frame, with no caching🪰 Gadfly · advisory
@@ -135,2 +130,2 @@return `${now.getFullYear()}-${pad(now.getMonth() + 1)}-${pad(now.getDate())}`}// "Today" for a note is the same local day everything else stamps (lib/dates.ts).export { today } from './dates'⚪ today re-exported from journal.ts only to satisfy JournalTab; objects.ts already imports from lib/dates — inconsistent indirection
maintainability · flagged by 1 model
web/src/lib/journal.ts:131—todayis re-exported (export { today } from './dates') purely soJournalTab.tsx:7-16can keep importing it from@/lib/journal, whileobjects.ts:10already importstodaydirectly from@/lib/dates. With only one consumer, this is a needless second source of the symbol; re-pointing JournalTab's import at@/lib/datesand dropping the re-export would be consistent with the rest of the codebase.🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 7 findings (2 with multi-model agreement)
web/src/editor/Canvas.tsx:650web/src/editor/Inspector.tsx:1045 single-model findings (lower confidence)
web/src/components/gardens/CopyDialog.tsx:41web/src/editor/Canvas.tsx:40web/src/components/gardens/GardenCard.tsx:18web/src/components/gardens/GardenCard.tsx:46web/src/lib/journal.ts:131Per-model detail
claude-code/opus (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've verified the security-relevant surfaces. Here's my review.
VERDICT: No material issues found
Reviewed through the security lens. The change threads an optional
plantedAtthrough the fill path and broadensErrInvalidInputerror messages to reach the client — both are the natural places a security problem would hide, so I traced them.What I checked and cleared:
plantedAtas tainted input → DB (internal/service/ops.go:198,internal/service/plantings.go:320).validDatePtrrunstime.Parse("2006-01-02", *s)and rejects anything non-conforming, and the value reaches the store as a bound parameter in adomain.Planting, not string-concatenated SQL. Even a malformed date can't inject; the format check is redundant defense, not the only barrier.TestFillRegionPlantedAtasserts the reject path. No injection.New client-facing error text (
internal/api/errors.go:53,122).inputMessagenow surfaces any wrappedErrInvalidInputverbatim instead of a flat "invalid input". I grepped every wrap site ininternal/— only two exist (instance_settings.go:74,ops.go:199), both new and both intentional; neither echoes internal state beyond the caller's own input. No pre-existing wrap gets newly exposed.Credential leakage via the model-spec error (
internal/service/instance_settings.go:74→specReason→agentmodel.Validate). I readagentmodel.go:Resolvewraps asagentmodel: resolve %q: %w, andspecReasonunwraps to the majordomoParseerror (e.g. "unknown provider"). TheOllamaCloudAPIKeyis passed toregistry(apiKey)but never appears in any error value — only the admin's own spec string is echoed back. No key disclosure.Authz on that error path (
internal/api/api.go:158). The settings group is behindrequireAuth() + requireAdmin(), so the more detailed spec/provider message is admin-only anyway; no info-exposure to ordinary users.Frontend (
monogram.ts,Canvas.tsx,CopyDialog.tsx).monogramInkreturns only the fixedvar(--color-*)constants after a strict hex regex — no way forplant.colorto inject CSS through it.background: colorwas already raw and is unchanged. Garden-name handling inCopyDialogis plain string compare, no sink.Nothing in the authn/authz, injection, SSRF, deserialization, secret-leakage, or input-validation categories is materially wrong here.
One forward-looking note (not a finding, since no current site triggers it): the
inputMessagebroadening makes every future%w-wrap ofErrInvalidInputclient-visible. CLAUDE.md and thedomain.ErrInvalidInputdoc comment both now warn "write it for the person, not the log," which is the right guardrail — worth honoring for anyone adding wraps that might carry paths or internal identifiers.🎯 Correctness — No material issues found
I've verified the correctness-sensitive parts of this PR against the actual checked-out code. Here is my review through the Correctness lens.
VERDICT: No material issues found
The change is careful and well-tested. I verified the following, and each holds up:
Date validation is strict, not just format-shaped.
fillLoadedguardsplantedAtwithvalidDatePtr(internal/service/plantings.go:320), which callstime.Parse("2006-01-02", …)— that rejects out-of-range dates like2026-13-45, not merely malformed strings."April 1st"→ErrInvalidInput, as the new test asserts. The default path (s.now().UTC()whenplantedAt == nil) is preserved for agent/API callers (internal/service/ops.go:243-245). All ~15FillRegion/FillNamedRegioncall sites were updated to the new signature (verified acrossops_test.go,revisions_test.go,agent/tools.go,agent/*_test.go).The luminance math is correct — I re-derived it.
web/src/lib/monogram.ts:73uses the WCAG formula (sRGB linearization with the 0.03928 knee and 2.4 exponent; coefficients 0.2126/0.7152/0.0722 mapped to R@slice0, G@slice2, B@slice4). Recomputing: sage#7a8a5e→ L≈0.231 (< 0.37 → paper ✓); cabbage#8bc98b→ L≈0.491 (> 0.37 → ink ✓). Matches the tests and the inline claims.inputMessage/specReasoncompose correctly.agentmodel.Validatereturnsfmt.Errorf("agentmodel: resolve %q: %w", …)(agentmodel.go:43), sospecReason's singleerrors.Unwrap(instance_settings.go:177) strips exactly that wrapper, andinputMessage(api/errors.go:122) trims the"invalid input: "prefix. Front-of-string wraps likeloading: %wcorrectly fall through to the generic text — covered byTestInputMessage.nextPlanYearand the GardenCard title logic are consistent.nextPlanYear(plan.ts:47) skips every taken year for the base fromfromupward;planYearOfgates the tag/title onyear >= currentYear, so a past-year plan falls back to the full name with no tag — no mismatch betweentitleand the{planYear} plantag.LengthFieldinvariant is preserved.cmonly moves oneditDimensionField/editSpacingField(a keystroke);convertDimensionFieldre-derives text from the samecm; Save readsfield.cmdirectly. A no-change Save now sends the loaded centimeters — the 900→899.922 drift is genuinely fixed.The Remove-confirmation gate can't silently misfire.
plantCountIn(Inspector.tsx:45) sumsplopCount, which falls back top.derivedCountwhen the plant is absent (shared.ts:98) — so the count is always a real number, neverNaN, andplantCount > 0reliably triggers the confirm. Wiring atGardenEditorPage.tsx:393is correct.CreatePlanting's{ plantedAt: today(), ...body }merge is safe — TypeScript optional-omitted keys are absent frombody, so the default isn't clobbered by anundefined.No correctness defects to report in this lens.
🧹 Code cleanliness & maintainability — Minor issues
Both findings check out against the actual code:
FALLBACK_PLANT_COLOR = '#97a97c'is defined atCanvas.tsx:40; the identical literal appears raw atInspector.tsx:359andGardenThumb.tsx:75(same "plant missing from catalog" default), whileColorSwatches.tsx:5CURATED_SWATCHES[0]is a semantically distinct swatch. Confirmed.GardenDialog/PlantDialoguseLengthField;Inspector.tsx(commitDim, lines 106–143) still keeps display strings viaformatDimensionInputand re-parses with a string-compare guard. Two mechanisms for one concern, matching the draft. Confirmed.VERDICT: Minor issues
The refactors in this PR are, on the whole, clean and well-targeted through the maintainability lens: the
LengthFieldconsolidation removes the ad-hocconvert/parseDimensionjuggling inGardenDialog,today()becomes a single source withjournal.tsre-exporting it (no duplicated impl), the old dead imports in both dialogs are fully removed, andinputMessage/specReason/nextPlanYearare small, single-purpose, well-documented helpers. The--color-marker-inktoken is deliberately single-placed (documented as theme-stable, matching the existing--color-paperpattern, so the "color in both places" convention doesn't apply).A few small things:
web/src/editor/Canvas.tsx:40— half-finished extraction of the fallback plant color. This PR names the unknown-plant fallbackFALLBACK_PLANT_COLOR = '#97a97c'in Canvas (correctly reusing it for both the circle fill andmonogramInk), but the identical literal stays raw atInspector.tsx:359(ColorDot color={plant?.color ?? '#97a97c'}) andGardenThumb.tsx:75(fill={plantColor.get(...) ?? '#97a97c'}) — the same "plant missing from the catalog" default. The constant is module-local to Canvas, so the magic value is now named in one place and raw in two others that mean exactly the same thing, which is the opposite of what naming it was supposed to buy. Low-churn fix: export a singleFALLBACK_PLANT_COLOR(e.g. fromlib/plants.ts) and use it in all three. (ColorSwatches.tsx:5CURATED_SWATCHES[0]also equals#97a97cbut is semantically "first curated swatch," not the fallback — leave it alone.)web/src/editor/Inspector.tsx:106-143— the inspector didn't adopt theLengthFieldpattern this PR standardizes. The dialogs now holdLengthFields so cm is the source of truth; the inspector still keeps display strings (formatDimensionInput) and re-parses incommitDim, guarding no-change edits with a string compare (raw.trim() === formatDimensionInput(current, unit)) instead. It achieves the same "don't rewrite on a no-op Save" goal, so this isn't a bug — but the CLAUDE.md note added here ("The inspector guards the same way") papers over the fact that there are now two mechanisms for one concern. Converting the inspector toLengthFieldis more churn than this PR's scope, so this is a maintainability heads-up, not a required change.Nothing blocking in my lens.
⚡ Performance — No material issues found
I've reviewed the changes through the Performance lens. Let me confirm my read of the two spots that touch render/query paths.
The
plantCountInaddition (Inspector.tsx:45) is called only for the single selected object (GardenEditorPage.tsx:393), right alongside the pre-existingrosterTextwhich already does the identical O(plantings) pass — so it's one extra linear scan over one object's inspector render, not per-object work.monogramInk/luminanceinCanvas.tsx:650runs only in the label loop, gated by focus (p.objectId !== focusIdreturns null) and byr * s < 9, and the work is a single regex match plus a fewparseInts — negligible even for a full focused bed. Thetoday()calls and the service-sidevalidDatePtrcheck are one-shot per mutation.VERDICT: No material issues found
monogramInkon the canvas) is bounded by focus + zoom gating and is cheap per call — not a material regression versus the previous constant string.plantCountInscan is single-object, same order as the adjacent existingrosterTextscan; not worth memoizing.useGardens()inCopyDialogreads an already-cached list on the gardens page — no extra fetch on the hot path.Nothing in my lane rises to a finding.
🧯 Error handling & edge cases — No material issues found
I've verified the error-handling and edge-case paths thoroughly across both the Go service layer and the frontend. Here is my review.
VERDICT: No material issues found
I reviewed this change strictly through the error handling & edge cases lens and verified each concern against the checked-out code. The unhappy paths this PR introduces are all handled:
plantedAtvalidation (server).fillLoadedcallsvalidDatePtr(plantedAt)before using it (internal/service/ops.go:198), andvalidDatePtr(internal/service/plantings.go:320) is nil-safe (returnstruefor nil) and rejects malformed strings viatime.Parse(dateLayout, …). The nil default correctly falls through tos.now().UTC()atops.go:243-246.TestFillRegionPlantedAtexercises both the good and bad-date branches. Go'stime.Parsealso range-checks (e.g.2026-02-30is rejected), so no out-of-range date leaks through.inputMessage(server). Handles the bare-sentinel case (msg == base→ returns generic "invalid input") and only strips a"invalid input: "prefix otherwise (internal/api/errors.go), so a suffix-wrapped error is returned verbatim rather than mangled.TestInputMessagecovers all three shapes.specReasonis nil-safe (errors.Unwrapguarded).editDimensionField/editSpacingFieldreturncm: nullon unparseable/empty input (web/src/lib/units.ts:251,267), and every consumer guards it:GardenDialogcheckswidthCm === null || heightCm === nullthenisValidDimensionCm(...)(rejects negative/zero), andPlantDialogchecksspacingCm === null || spacingCm < 1.convertDimensionFieldpreserves the raw text whencmis null so a typo stays visible on a unit switch.ConfirmDialogwrapsonConfirmin try/catch, surfaces failures inline (e.g. a 409), and re-enables the button for retry; the Inspector'sonConfirmawaitsdel.mutateAsyncbeforeonDeleted(), so a failed remove doesn't deselect.plantCountis computed viaplantCountIn→plopCount, which falls back top.derivedCountwhen the plant is missing from the catalog (web/src/editor/shared.ts:96), so the confirm gate isn't skipped for a private plant in a shared garden.luminance()returnsnullfor anything that isn't#rgb/#rrggbb, andmonogramInkfalls back to paper on null (web/src/lib/monogram.ts); the canvas and Monogram both feed it aFALLBACK_PLANT_COLORwhenplantis undefined. Tested for named colors and empty strings.nextPlanYear(frontend). Thewhile (taken.has(year))loop iterates over a finite set and always terminates (web/src/lib/plan.ts:53).The test clock-bomb fix (
seed_lots_test.go) pins bothplantedAt(2026-07-01) and the removal (2026-08-01) to fixed dates in the correct order, so it no longer depends on the wall clock.Nothing in my lane is materially wrong here — the diff is genuinely careful about its unhappy paths.
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
I traced all tainted-data paths in this PR:
plantedAtstring (API input → DB): Validated infillLoadedviavalidDatePtr, which callstime.Parse("2006-01-02", ...)— only strict YYYY-MM-DD strings pass. It reaches the database exclusively through parameterized?placeholders (plantingInsertininternal/store/plantings.go). No injection surface.inputMessage/ wrappedErrInvalidInput: The function strips the sentinel prefix and returns the wrapped reason verbatim. Verified all twofmt.Errorf("%w: …", domain.ErrInvalidInput)calls ininternal/service/place the sentinel first (the required contract). The third test case ("loading: %w") documents what happens when the contract is broken, but no production code hits that path today.Model spec echoed in 400 body (
instance_settings.go:74): The spec is the admin's own submitted value, returned to that same admin.requireAdmingates the route. Intentional and safe.React rendering of user-controlled strings (
CopyDialog,GardenCard): Garden names and the warning message interpolate user data via JSX ({name.trim()}), which React escapes. No XSS vector.monogramInk/luminance: User-controlled plant colors flow through a hex regex; the function returns only one of two fixed CSS variable strings regardless of input. No injection possible.No new routes, no auth model changes, no credential handling, no SSRF surface introduced.
🎯 Correctness — No material issues found
I've now verified all the correctness-sensitive pieces. Let me write the review.
No material issues found
I checked every formula, conversion, and domain invariant introduced by this PR:
luminanceinmonogram.ts:87— verified.channel(0)→hex.slice(0,2)= R,channel(2)→hex.slice(2,4)= G,channel(4)→hex.slice(4,6)= B; coefficients 0.2126/0.7152/0.0722 sum to 1.0 and match WCAG 2.1. Threshold 0.37 checks out: paper contrast = 1.05/(0.37+0.05) ≈ 2.5:1. Verified all six test fixtures by hand against the formula; they pass.today()indates.ts:8— correct.getMonth()+1gives 1–12,getDate()gives 1–31,getFullYear()is unambiguous. Both test cases verified.LengthFieldinGardenDialog,PlantDialog— the invariant holds.dimensionField(cm, unit)stores the loaded cm directly;editDimensionField(text, unit)stores the parsed cm alongside the raw text;convertDimensionFieldreformats from cm without re-parsing. Submit path reads.cmdirectly — no re-parse of a display string on save.inputMessageinerrors.go:119— all three test cases verified: bare sentinel → returns unchanged;%w-prefixed wrapping →TrimPrefixstrips"invalid input: "correctly;%w-suffixed wrapping → TrimPrefix is a no-op and the full message is returned.validDatePtrinplantings.go:320— nil is allowed, otherwisetime.Parse(dateLayout, *s)enforces YYYY-MM-DD strictly.&plantedOnaliasing inops.go:259—plantedOnis set once before the loop and never mutated inside it; all pointers are live whenCreatePlantingsis called, which dereferences each in its own loop body. The old code used&todayidentically; not a new risk.nextPlanYearinplan.ts:42— correctly builds a set of taken years for the named base, then increments fromfromuntil it finds a free slot. Logic is sound.specReasonininstance_settings.go:172—agentmodel.Validatewraps asfmt.Errorf("agentmodel: resolve %q: %w", spec, innerErr), soerrors.Unwrapreturns the inner parse error, stripping the redundant spec from the message.plantCountIninInspector.tsx:44— correct. Theplantingsarray fed to it is the editor's active-only slice (soft-removed entries are filtered out inuseRemovePlanting'sonMutate).🧹 Code cleanliness & maintainability — Minor issues
The finding checks out. When
planYearOfreturns non-null, it has already calledparsePlanNameand confirmed a non-null result with a valid year — so the secondparsePlanNamecall on line 46 is guaranteed to return non-null, making both?.baseand?? garden.nameunreachable.Verdict: Minor issues
One verified cleanliness finding; the rest of the PR is tidy through this lens.
web/src/components/gardens/GardenCard.tsx:46— dead optional-chain fallbackplanYearOf(inlib/plan.ts) doesconst p = parsePlanName(name); return p && p.year >= now.getFullYear() ? p.year : null, soplanYear != nullis only true whenparsePlanNamealready returned a non-null result for the same input. CallingparsePlanName(garden.name)again inside the true-branch is guaranteed to succeed; the?.optional chain and?? garden.namefallback are unreachable dead code. A reader seeing?.base ?? garden.namenaturally wonders what case produces a null — there isn't one.Fix:
parsePlanName(garden.name)!.base(non-null assertion) or factor it out:Everything else looks clean for this lens: the
LengthFieldabstraction is applied consistently across the two dialogs;today()was correctly centralised indates.tswith a re-export fromjournal.tsfor existing callers;plantCountInis a fresh export used in exactly one call site with no duplication ofrosterText's logic; theinputMessage/specReasonpair are narrow, well-tested helpers; and the constant extraction ofFALLBACK_PLANT_COLORinCanvas.tsxis a straightforward cleanup over the inline literal.⚡ Performance — Minor issues
The finding is confirmed. At
Canvas.tsx:650,monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)is called insiderenderedPlops.map()on every render. Theluminance()function inmonogram.ts:79–88does the regex, spread, parseInt, and** 2.4work the draft describes. The only cached per-plant lookup in that loop isletters(aMap<number, string>prop from the parent); no equivalent ink cache exists.Minor issues
The monogramInk call introduced by this PR to support dark/light ink selection runs once per plop on every canvas frame, with no caching.
web/src/editor/Canvas.tsx:650—monogramInk()called once per plop, every frame, with no cachingmonogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)sits insiderenderedPlops.map().monogramInk()callsluminance(), which does a regex match, an array spread (3-digit hex expansion), threeparseIntcalls, and three** 2.4floating-point powers. For a garden with 200 visible plops across a handful of plant species, every canvas frame recomputes the same 5–10 luminance values 200 times instead of once. Canvas re-renders on every pointer move during pan/zoom — up to 60 fps — so a full planting can mean ~12,000 redundant luminance computations per second, all returning identical results.The
lettersprop already shows the right pattern: computenew Map(plants.map(p => [p.id, monogram(p)]))in the parent withuseMemo([plants]), pass it as a prop, and look up byplantIdin the loop. Aninksmap ofplantId → monogramInk(p.color)follows the same shape and fixes the issue entirely.🧯 Error handling & edge cases — No material issues found
I now have enough information to complete my review.
No material issues found
I traced every new unhappy path introduced by this PR through the actual checked-out code.
inputMessage()(internal/api/errors.go:115): The three cases are all correct and covered by the new unit test — bare sentinel returns "invalid input", the%w: reasonwrap strips the prefix, and a"outer: %w"wrap returns the full message. The string-stripping logic is sound becausewriteServiceErroronly calls it aftererrors.Ishas already confirmed the sentinel is present.specReason()(internal/service/instance_settings.go:179):agentmodel.Resolvealways wraps its errors asfmt.Errorf("agentmodel: resolve %q: %w", spec, underlying), soerrors.Unwrapreliably returns the underlying majordomo error. The only other path — an empty spec — is guarded by theif f.spec == "" { continue }check beforeValidateis ever called, sospecReasonnever sees a non-wrapping error from this code.validDatePtrinfillLoaded(internal/service/ops.go:197): Placed correctly beforevalidFillLayoutand before computing lattice centers. Returnsnil, wrapped ErrInvalidInputwith a user-readable message thatinputMessagewill properly strip. The existingvalidDatePtrshared withplantings.go,journal.go, andseed_lots.gousestime.Parse("2006-01-02", …)which is strict enough for the stated contract.plantedOnpointer aliasing in the batch loop (internal/service/ops.go:255): Alldomain.Plantingentries point to&plantedOn— the same address. This was the pre-existing pattern (todaybefore,plantedOnnow); the store reads the string value at insertion time, not later, so there is no aliasing hazard introduced here.plopCountwith undefined plant (Inspector.tsx/plantCountIn): WhenplantsById.get(p.plantId)is undefined (a shared garden's private plant),plopCountfalls through top.derivedCount(verified inshared.ts:98). No panic path.Confirm-dialog remove flow (
Inspector.tsx:290): The confirmed path usesmutateAsync+onDeleted()called only on success; ConfirmDialog surfaceserrorFallbackon throw and keeps the dialog open for retry. The empty-object path uses the existing optimisticonDeleted()→mutate()order, unchanged from before this PR.luminance()channel indices (monogram.ts:84): Verified:channel(0)→R,channel(2)→G,channel(4)→B for a 6-char hex string. WCAG weights are correctly ordered.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Through the security lens, the change holds up:
plantedAtinput validation is present and correct.fillLoaded(internal/service/ops.go:198) guards the new pointer withvalidDatePtr, which usestime.Parse(dateLayout, *s)against"2006-01-02"(internal/service/plantings.go:320).time.Parserejects out-of-range month/day values, so a malicious client can't push an arbitrary string through the fill route — it gets a wrappedErrInvalidInput→ 400. Verified by readingvalidDatePtrand theops.gocall site.today()helper (web/src/lib/dates.ts) is a pure local-date formatter with no tainted input.inputMessage(internal/api/errors.go:125) echoes the wrapped reason — which includes the user-supplied model spec — into the 400 body. That body is serialized by gin'sc.JSON(JSON-escaped), and on the client it's surfaced under a field in React (text-escaped by default), so there is no HTML/script injection path. NodangerouslySetInnerHTMLwas indicated anywhere in the diff. Low risk, not a finding.monogramInkluminance math is correct (verifiedweb/src/lib/monogram.ts:78-88): channel indexing0/2/4maps R/G/B and the WCAG coefficients0.2126/0.7152/0.0722are applied to the right channels with the standard linearization. Not a security issue, but the magic number checks out.Inspector.tsx) introduces no authz change — it gates a UI action behind a confirm dialog and still routes through the existing authorizeduseDeleteObjectmutation; no new privileged path was opened.Nothing in my lane is materially wrong.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I traced every correctness-sensitive path in this PR through the actual checked-out code.
Verified clean:
monogramInkluminance math (web/src/lib/monogram.ts:79-93): channel indices are correct —channel(0)→red weighted 0.2126,channel(2)→green weighted 0.7152,channel(4)→blue weighted 0.0722 (standard WCAG). The gamma transformv ≤ 0.03928 ? v/12.92 : ((v+0.055)/1.055)**2.4is the correct WCAG sRGB→linear formula. ThePAPER_MAX_LUMINANCE = 0.37threshold is derivable: paper (~L=1.0) on a background of L gives contrast (1.05)/(L+0.05); at L=0.37 that's 1.05/0.42 = 2.5:1 — exactly the stated cutoff. Sage (#7a8a5e, ~0.23 < 0.37) keeps paper; cabbage (#8bc98b, ~0.49 > 0.37) flips to ink. The 3-digit hex expansionc+cis correct.LengthFieldno-change-save fix (web/src/lib/units.ts,GardenDialog.tsx,PlantDialog.tsx):dimensionField/spacingFieldstore the originalcmverbatim; save readsfield.cmdirectly, never re-parsing the display text.convertDimensionFieldregenerates text from the unchangedcm.editDimensionField/editSpacingFieldonly re-parse on actual typing. This is the precise fix for the 899.922 / 44.958 drift. Round-trip verified againstformatDimensionInput/parseDimensionandspacingFromCm/cmFromSpacing.today()local date (web/src/lib/dates.ts):new Date(2026, 7, 22, 21, 30)→2026-08-22(month 7 = August, 0-indexed, correct);new Date(2026, 0, 5, 0, 1)→2026-01-05. Uses local getters only. Correct.plantedAtplumbing (internal/service/ops.go:198,243-245):validDatePtr(plantings.go:320) usestime.Parse(dateLayout="2006-01-02", …), matching the formattoday()emits. Nil → UTC today default; valid string used verbatim; invalid → wrappedErrInvalidInput.useCreatePlantingspreads{ plantedAt: today(), ...body }so an explicit caller value still wins. Correct.inputMessageprefix handling (internal/api/errors.go:125-131): bare sentinel → "invalid input";fmt.Errorf("%w: …", ErrInvalidInput)→ trimmed reason; a leading wrap likefmt.Errorf("loading: %w", ErrInvalidInput)→.Error()is "loading: invalid input",TrimPrefixfinds no "invalid input: " prefix, returns the whole string unchanged. Matches all three test cases.specReasonunwrap (instance_settings.go:175-180):agentmodel.Validatereturnsfmt.Errorf("agentmodel: resolve %q: %w", spec, innerErr);errors.UnwrapyieldsinnerErr, so the message reads the provider's own reason without repeating the spec — the test assertschat model "nonesuch/model": <reason>. Consistent.nextPlanYear(web/src/lib/plan.ts:47-55): builds aSetof years taken by plans whosebasematches, then increments fromfrom. Thetakenduplicate-name guard inCopyDialogis a superset (any exact name match), so it never false-positives on the auto-proposed (free-year) name. Correct.No correctness defects found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
All three draft findings are confirmed against the actual code. Producing the corrected review with all surviving findings.
VERDICT: Minor issues
**
web/src/editor/Inspector.tsx:104-112(and 217-220)** — CLAUDE.md (line 137) states "The inspector guards the same way" as part of the new LengthField convention, but the Inspector's own dimension/grid fields still hold plainstringstate (useState(formatDimensionInput(...))) and still re-parse the display string on blur viacommitDim/commitGrid(lines 137-149, applied at 217-220). TheLengthFieldpattern the PR introduces inGardenDialog.tsxandPlantDialog.tsx` (cm as source of truth, text as a view) is not actually applied here, so the same round-trip-drift class of bug the PR fixes elsewhere is left in place. This is an incomplete migration / an inaccurate doc claim, not a newly introduced bug, but it's the obvious cleanliness gap given the stated convention.web/src/lib/journal.ts:131—todayis re-exported (export { today } from './dates') purely soJournalTab.tsx:7-16can keep importing it from@/lib/journal, whileobjects.ts:10already importstodaydirectly from@/lib/dates. With only one consumer, this is a needless second source of the symbol; re-pointing JournalTab's import at@/lib/datesand dropping the re-export would be consistent with the rest of the codebase.web/src/components/gardens/GardenCard.tsx:18-21— The JSDoc summary was rewritten to wrap awkwardly mid-thought ("name (a / plan copy shows its base name and a<year> plantag), size, ..."), producing a 2-line run-on where the rest of the file uses terse one-line summaries. Cosmetic, but a readability regression in a file-level comment.⚡ Performance — Minor issues
I've verified the finding against the actual code. The diff confirms
Canvas.tsx:650changed from a constantfill="var(--color-paper)"tofill={monogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)}, called insiderenderedPlops.map(...)(line 635).monogramInk(inweb/src/lib/monogram.ts:91) is a pure function delegating toluminance(line 79), which does the regex/hex-expansion/parseInt/exponentiation math described. The canvas already memoizesrenderedPlops/byIdbut does NOT memoize the color→ink mapping —monogramInkis invoked inline per plop per render, and the canvas re-renders on pan/zoom/animation frames (ANIM_MS=520). Colors come from a small finite catalog set, so the work is repeatable and avoidable. The finding is accurate.VERDICT: Minor issues
Performance lens review. The change is mostly clean efficiency-wise (batch insert preserved, no new N+1, fill date computed once per batch). One real, if minor, hot-path regression:
web/src/editor/Canvas.tsx:650—monogramInk()recomputed per plop per render frame. The canvas is a hot path: it re-renders on every pan/zoom/animation tick (ANIM_MS=520ms of refit/transition frames), andrenderedPlops.map(...)callsmonogramInk(plant?.color ?? FALLBACK_PLANT_COLOR)for each visible plop.monogramInk→luminancedoes atrim()+ regex match + conditional[...hex].map(c => c+c).join()+ threeparseInt/division + three**2.4exponentiations. Previously this was a constant"var(--color-paper)". The result depends only on the plant's color, which is a small finite set (the catalog), so a plop-heavy bed re-running this at 60fps across the animation is avoidable repeated work. Verified by readingweb/src/lib/monogram.ts(pure function ofcolor) andCanvas.tsxrender loop (the component memoizesrenderedPlops/byIdbut not the color→ink mapping).Map<string, string>keyed by color, oruseMemobuilding{plantId → ink}fromplantsByIdonce per render, so the luminance math runs once per distinct color rather than once per plop per frame.Nothing else in the performance lane rose to material concern:
nextPlanYear/namesinCopyDialog.tsxrun over the gardens list each render but that list is small and the dialog is not a hot path;plantCountIn/plantCountis a single linear scan over plantings in the inspector (not per-frame); the service fill path computesplantedOnonce per batch, not per plop.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed the change through the error-handling & edge-cases lens, verifying the unhappy paths the diff introduces:
inputMessage(errors.go): Verifiedinternal/api/errors.go:33-40and its test. The bare-sentinel short-circuit returns"invalid input"; wrapped errors getstrings.TrimPrefix(msg, "invalid input: "), which is a no-op when the sentinel isn't the prefix (e.g."loading: invalid input"stays whole). Both branches behave as the test asserts; no panic on a nil-ish message (the function only receives a non-nil error fromerrors.Is).plantedAtvalidation (service/ops.go:194-200): ConfirmedvalidDatePtr(plantings.go:319-326) accepts nil or strictYYYY-MM-DD; a bad string returns a wrappedErrInvalidInputwith a user-facing reason. The nil path correctly falls back tos.now().UTC(). No empty-string edge case slips through (time.Parserejects"").{ plantedAt: today(), ...body }ordering lets a caller-suppliedplantedAtoverride the default intentionally;today()handles anyDate(verified dates.test.ts), no NaN/undefined risk.plantCount > 0gates the confirm; theConfirmDialogowns its busy/error state and keeps the dialog open on throw (verified ConfirmDialog.tsx:37-47), so a 409 is surfaced not swallowed. The empty-object path still deletes directly. No double-delete race —setConfirmRemove(true); returnshort-circuits beforedel.mutate.taken/names(CopyDialog.tsx:24-33):gardens.data ?? []avoids a crash when the query is loading;takenis a non-blocking advisory, not a submit guard, so a stale list can't block a valid copy.One thing I could not fully verify (out of budget):
plantCountIn(Inspector.tsx:45-49) sums over every planting passed in without filtering by removed-state. If the editor'splantingsarray includes soft-removed plops, the confirm could fire for an object whose plants are all already removed. I didn't reach the call site to confirm what that array contains, so I'm not reporting it as a finding — and even if true, hard-deleting the object removes those rows too, so the warning is arguably still correct.kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
FillRegion/FillNamedRegionstill gate throughobjectForRolewithroleEditor, andUpdateInstanceSettingsstill requires admin.plantedAtparameter is validated with the existingvalidDatePtr(parses against2006-01-02layout), rejecting malformed dates with a cleanErrInvalidInput.inputMessage/ wrapped-ErrInvalidInputdesign is intentional and documented (CLAUDE.md). User-controlled specs are%q-quoted before inclusion, andspecReasonstrips one layer of wrapping to avoid doubling the spec in the message. No secret material (e.g., the Ollama API key) is incorporated into the formatted error.LengthFieldand date handling operate on already-validated or client-side data without new server-side parsing risks.🎯 Correctness — No material issues found
No material issues found.
After verifying the key correctness-sensitive changes:
today()inweb/src/lib/dates.tscorrectly uses local-timegetFullYear/getMonth+1/getDate, fixing the UTC-tomorrow bug for west-of-Greenwich gardeners. The test confirms zero-paddedYYYY-MM-DDoutput.validDatePtrininternal/service/plantings.go:320acceptsnil(API/agent default) and validatesYYYY-MM-DDformat. ThefillLoadedflow: nil → UTC today; valid string → passed through; invalid →ErrInvalidInput. Verified correct.inputMessageininternal/api/errors.gostrips only a leading"invalid input: "prefix; the test suite covers bare sentinel, standard wrapped form, and non-standard wrapping. The logic is correct for the documented convention.specReasonininternal/service/instance_settings.go:177unwraps one level to strip theagentmodel.Resolvewrapper ("agentmodel: resolve %q: …"), so the final user-facing message doesn't repeat the spec. Verified against theagentmodelpackage structure.monogramInkthresholdPAPER_MAX_LUMINANCE = 0.37correctly implements the WCAG relative-luminance formula. AtL = 0.37, white-on-marker contrast is(1.05)/(0.42) = 2.5:1, matching the documented cutoff. Verified against the test values.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The PR is clean and well-structured through the maintainability lens:
LengthFieldinweb/src/lib/units.tscleanly separates display text from canonical centimeters, withdimensionField/editDimensionField/convertDimensionFieldandspacingField/editSpacingFieldproviding a consistent pattern used by bothGardenDialog.tsxandPlantDialog.tsx.today()inweb/src/lib/dates.tscentralizes local-date generation;monogramInk()inweb/src/lib/monogram.tsencapsulates the luminance threshold logic with clear documentation;nextPlanYear()inweb/src/lib/plan.tsextracts the collision-avoidance logic fromCopyDialog.tsx.formatDimensionInput/parseDimensionhelpers remain appropriately in use byInspector.tsx, which guards against re-parsing via itscommitDimearly-return check.plantCountInandrosterTextdescribe what they compute;inputMessageandspecReasonaccurately describe their purpose.⚡ Performance — No material issues found
No material issues found
Nothing in this change introduces a performance regression I can justify:
LengthFieldrefactor (web/src/lib/units.ts) replaces string re-parsing on save with a stored numeric value — that's fewer allocations, not more.monogramInkruns per plop label on the SVG canvas. It does a short regex and a few arithmetic ops; at expected garden scales this adds well under a millisecond per frame and is dwarfed by DOM paint cost. Not material.plantCountInduplicates the linear scan already done byrosterTextfor the selected object, but both are O(n) over the plantings array and run only when an object is selected. For typical counts this is sub-millisecond. Not material.🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/components/gardens/CopyDialog.tsx:useState(() => planNameFor(base, year))captures the initialyearvalue once. IfuseGardens()data is still loading when the dialog opens,namesis empty,nextPlanYearreturnsfrom(which may already be taken), and the suggested name stays stale even after the list loads. Thetakenwarning catches this on the next render, but the initial default is wrong and the user can submit before noticing. Compute the name inside auseEffect(or derive state) so it reacts whengardens.dataarrives.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.