Tier 2 of the agent-tool round (Tier 1 was #129). The catalog side of the record, plus the one describe field the model kept mis-computing.
What's new
update_seed_lot / delete_seed_lot — correct or drop a recorded purchase ("it was three packets, not two"). Quantity, unit, vendor, link, purchase date, packed-for year, germination rate, notes; the plant a lot is for stays fixed (the service never re-points a purchase). Dates go through the same parseDay path as every other dated argument, trimmed value stored.
delete_plant — remove a duplicate from the user's catalog. The service already refuses while plantings (past seasons included) or a seed lot reference it; the tool turns ErrPlantInUse into words the model can pass on, and tells it not to clear those references to get its way. Built-ins stay undeletable.
create_garden — a new place (front yard, allotment) with the service's defaults; the prompt says a season plan is still a copy_garden, and that the conversation stays with the garden it started in.
readyAround on every DescribeGroup — planting date + the plant's days to maturity, for the plops still in the ground, as one date or first…last. Live, "what can I pick this week?" got the arithmetic wrong; now it is a lookup. Absent when the catalog has no days for the plant.
Tests
TestDescribeGroupSaysWhenReady (service): range across two planting dates, nothing for a plant without days, and a pulled plop excluded from the range even in the season view.
TestCatalogAndGardenTools (agent): create_garden defaults and an imperial one; record → correct → delete a lot; delete_plant refused for a lot, refused for a pulled planting (with the "past seasons count" wording), allowed once nothing references it, refused for a built-in.
DESIGN.md roadmap note and UI step labels updated.
Tier 2 of the agent-tool round (Tier 1 was #129). The catalog side of the record, plus the one describe field the model kept mis-computing.
## What's new
- **`update_seed_lot` / `delete_seed_lot`** — correct or drop a recorded purchase ("it was three packets, not two"). Quantity, unit, vendor, link, purchase date, packed-for year, germination rate, notes; the plant a lot is for stays fixed (the service never re-points a purchase). Dates go through the same `parseDay` path as every other dated argument, trimmed value stored.
- **`delete_plant`** — remove a duplicate from the user's catalog. The service already refuses while plantings (past seasons included) or a seed lot reference it; the tool turns `ErrPlantInUse` into words the model can pass on, and tells it *not* to clear those references to get its way. Built-ins stay undeletable.
- **`create_garden`** — a new place (front yard, allotment) with the service's defaults; the prompt says a season plan is still a `copy_garden`, and that the conversation stays with the garden it started in.
- **`readyAround` on every `DescribeGroup`** — planting date + the plant's days to maturity, for the plops still in the ground, as one date or `first…last`. Live, "what can I pick this week?" got the arithmetic wrong; now it is a lookup. Absent when the catalog has no days for the plant.
## Tests
- `TestDescribeGroupSaysWhenReady` (service): range across two planting dates, nothing for a plant without days, and a pulled plop excluded from the range even in the season view.
- `TestCatalogAndGardenTools` (agent): create_garden defaults and an imperial one; record → correct → delete a lot; delete_plant refused for a lot, refused for a pulled planting (with the "past seasons count" wording), allowed once nothing references it, refused for a built-in.
DESIGN.md roadmap note and UI step labels updated.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
- update_seed_lot / delete_seed_lot: correct or drop a recorded purchase
("it was three packets, not two"); the plant a lot is for stays fixed.
- delete_plant: remove a duplicate from the user's catalog. The service
already refuses while plantings (past seasons included) or a lot reference
it; the tool turns that sentinel into words the model can pass on, and
tells it not to clear those references to get its way.
- create_garden: a new place, with the service's defaults; the prompt says a
plan is still a copy_garden.
- describe_garden groups carry readyAround — planting date plus days to
maturity for the plops still in the ground — so "what can I pick this
week?" is a lookup rather than arithmetic the model got wrong live.
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 06:10:31Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **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** — No material issues found
- ✅ **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** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ 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
<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>
🟡updateSeedLot 'nothing to change' guard hand-enumerates every field; must be kept in sync with the args struct when new fields are added
maintainability · flagged by 1 model
internal/agent/tools.go:890-892 — The "nothing to change" guard in updateSeedLot hand-enumerates every nullable field (args.Quantity == nil && args.Unit == nil && …). This faithfully mirrors the established pattern in updateGarden (tools.go:823-825), so it is consistent with the surrounding code, but it is a maintenance hazard: the next field added to the updateSeedLot args struct / SeedLotPatch must also be appended to this boolean chain in lockstep, or a valid single-(new-)field…
🪰 Gadfly · advisory
🟡 **updateSeedLot 'nothing to change' guard hand-enumerates every field; must be kept in sync with the args struct when new fields are added**
_maintainability · flagged by 1 model_
- `internal/agent/tools.go:890-892` — The "nothing to change" guard in `updateSeedLot` hand-enumerates every nullable field (`args.Quantity == nil && args.Unit == nil && …`). This faithfully mirrors the established pattern in `updateGarden` (tools.go:823-825), so it is consistent with the surrounding code, but it is a maintenance hazard: the next field added to the `updateSeedLot` args struct / `SeedLotPatch` must also be appended to this boolean chain in lockstep, or a valid single-(new-)field…
<sub>🪰 Gadfly · advisory</sub>
// The sentinel's text is for a log line; the model needs to know what
// to tell the person, and what not to do about it.
returnnil,fmt.Errorf("%w: the plant is still used — by plantings (past seasons count) or a seed lot — so it stays; tell the user rather than removing those",domain.ErrPlantInUse)
🟡Wrapping the same sentinel error with %w is an unusual pattern
maintainability · flagged by 1 model
internal/agent/tools.go:927 — The deletePlant adapter wraps domain.ErrPlantInUse with %w and the same sentinel, layering the error. The comment says the sentinel text is “for a log line,” but wrapping the same sentinel rather than returning a plain string or a distinct error type is an unusual pattern that could confuse future maintainers about error identity. A plain fmt.Errorf("...") (without %w) or a dedicated agent-facing error would be cleaner, since the agent layer is intenti…
🪰 Gadfly · advisory
🟡 **Wrapping the same sentinel error with %w is an unusual pattern**
_maintainability · flagged by 1 model_
- `internal/agent/tools.go:927` — The `deletePlant` adapter wraps `domain.ErrPlantInUse` with `%w` and the same sentinel, layering the error. The comment says the sentinel text is “for a log line,” but wrapping the same sentinel rather than returning a plain string or a distinct error type is an unusual pattern that could confuse future maintainers about error identity. A plain `fmt.Errorf("...")` (without `%w`) or a dedicated agent-facing error would be cleaner, since the agent layer is intenti…
<sub>🪰 Gadfly · advisory</sub>
⚪TestCatalogAndGardenTools conflates three unrelated tool scenarios; splitting would give more precise failure attribution
maintainability · flagged by 1 model
TestCatalogAndGardenTools conflates three distinct tool areas into one ~100-line function (internal/agent/tools_test.go:938). The // --- comments acknowledge the length. create_garden is independent of the seed-lot and delete_plant scenarios; a failure in the garden-creation section suppresses all downstream signal. Splitting at the natural // --- seams (one test for garden creation, one for catalog lifecycle) would give more precise failure attribution on a red run. Minor in a…
🪰 Gadfly · advisory
⚪ **TestCatalogAndGardenTools conflates three unrelated tool scenarios; splitting would give more precise failure attribution**
_maintainability · flagged by 1 model_
- **`TestCatalogAndGardenTools` conflates three distinct tool areas into one ~100-line function** (`internal/agent/tools_test.go:938`). The `// ---` comments acknowledge the length. `create_garden` is independent of the seed-lot and `delete_plant` scenarios; a failure in the garden-creation section suppresses all downstream signal. Splitting at the natural `// ---` seams (one test for garden creation, one for catalog lifecycle) would give more precise failure attribution on a red run. Minor in a…
<sub>🪰 Gadfly · advisory</sub>
🟠Duplicated call/mustCall test helpers should be extracted
maintainability · flagged by 1 model
internal/agent/tools_test.go:943 — The call and mustCall closures in TestCatalogAndGardenTools are identical to those in TestRecordKeepingTools (line 753). Across the file there are six call and two mustCall inline redefinitions. This copy-paste clutter makes tests harder to maintain and drift-prone. Extract them once as package-level helpers (e.g., execTool(t, box, name, args) and execToolOK(t, box, name, args, into)) and reuse them.
🪰 Gadfly · advisory
🟠 **Duplicated call/mustCall test helpers should be extracted**
_maintainability · flagged by 1 model_
- `internal/agent/tools_test.go:943` — The `call` and `mustCall` closures in `TestCatalogAndGardenTools` are identical to those in `TestRecordKeepingTools` (line 753). Across the file there are six `call` and two `mustCall` inline redefinitions. This copy-paste clutter makes tests harder to maintain and drift-prone. Extract them once as package-level helpers (e.g., `execTool(t, box, name, args)` and `execToolOK(t, box, name, args, into)`) and reuse them.
<sub>🪰 Gadfly · advisory</sub>
🟡mustCall closure copied verbatim from TestRecordKeepingTools — candidate for a file-level helper
maintainability · flagged by 1 model
mustCall closure duplicated verbatim between TestRecordKeepingTools and TestCatalogAndGardenTools (internal/agent/tools_test.go:947). The body is identical to the one introduced at line 757 — same signature, same t.Fatalf messages, same json.Unmarshal call. The call closure (capturing ctx and box) appears multiple times across the file and was never extracted, so this follows the existing style, but mustCall is a richer wrapper that a third test would copy again. Extrac…
🪰 Gadfly · advisory
🟡 **mustCall closure copied verbatim from TestRecordKeepingTools — candidate for a file-level helper**
_maintainability · flagged by 1 model_
- **`mustCall` closure duplicated verbatim between `TestRecordKeepingTools` and `TestCatalogAndGardenTools`** (`internal/agent/tools_test.go:947`). The body is identical to the one introduced at line 757 — same signature, same `t.Fatalf` messages, same `json.Unmarshal` call. The `call` closure (capturing `ctx` and `box`) appears multiple times across the file and was never extracted, so this follows the existing style, but `mustCall` is a richer wrapper that a third test would copy again. Extrac…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Minor issues · 5 findings (0 with multi-model agreement)
5 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
Duplicated call/mustCall test helpers should be extracted
internal/agent/tools_test.go:943
kimi-k2.6:cloud
maintainability
🟡
updateSeedLot 'nothing to change' guard hand-enumerates every field; must be kept in sync with the args struct when new fields are added
internal/agent/tools.go:890
glm-5.2:cloud
maintainability
🟡
Wrapping the same sentinel error with %w is an unusual pattern
internal/agent/tools.go:927
kimi-k2.6:cloud
maintainability
🟡
mustCall closure copied verbatim from TestRecordKeepingTools — candidate for a file-level helper
internal/agent/tools_test.go:947
claude-code/sonnet
maintainability
⚪
TestCatalogAndGardenTools conflates three unrelated tool scenarios; splitting would give more precise failure attribution
internal/agent/tools_test.go:938
claude-code/sonnet
maintainability
Per-model detailclaude-code/opus (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've traced each new agent tool to its service method and verified the authorization and validation controls.
VERDICT: No material issues found
The new tools are thin adapters over service methods that already enforce every control that matters for this lens. I verified each:
delete_plant → DeletePlant (plants.go:258): gated by writablePlant (plants.go:219), which returns ErrForbidden for built-ins (OwnerID == nil) and ErrNotFound for another user's plant. No cross-tenant deletion, built-ins protected. The ErrPlantInUse rewrap in the tool only changes the message string, not authz.
update_seed_lot / delete_seed_lot → UpdateSeedLot/DeleteSeedLot (seed_lots.go:194,229): both gated by ownSeedLot (seed_lots.go:129), which masks any lot not owned by the actor as ErrNotFound. Version-guarded on update. No IDOR.
create_garden → CreateGarden (gardens.go:113): forces g.OwnerID = actorID — the caller cannot set ownership.
SourceURL on a seed lot (potential stored-XSS via a javascript: link, since it renders as a clickable link): the update path runs through finalizeSeedLot → validSourceURL (seed_lots.go:304,332), which rejects any non-http(s)/hostless scheme. The vector is closed for the update tool too.
Dates (purchasedAt) go through parseDay, and the tool rejects free-text like "last spring" (test-covered) before it reaches the store.
All tools use a.actor (the authenticated session actor) as actorID; there is no way for the model to pass an arbitrary actor. No SSRF (URLs are stored, never fetched server-side), no injection surface (parameterized store), no secret handling in this diff.
Authorization lives entirely at the service seam exactly as the architecture intends, and the adapters add no bypass. Nothing in the security lens is materially wrong.
🎯 Correctness — No material issues found
I've verified all the correctness-critical paths. Let me summarize.
VERDICT: No material issues found
I reviewed this PR through the Correctness lens, verifying each numeric/formula path against the checked-out code.
What I checked and confirmed correct:
readyDate arithmetic (internal/service/ops.go:798) — re-derived the two test dates from first principles using Go's AddDate(0,0,days) normalization:
2026-05-01 + 60d → day 61 of May → June 30 (2026-06-30) ✓
2026-05-11 + 60d → day 71 of May → July 10 (2026-07-10) ✓
Matches the test's expected "2026-06-30…2026-07-10". dateLayout is "2006-01-02" in both service and agent packages, so parse/format round-trips cleanly.
Range min/max (dateRangeOf, ops.go:828) — uses lexical string compare; valid for fixed-width ISO dates. readyDate always emits a 10-char YYYY-MM-DD, so ordering is sound. Nils (undated / unparseable) are skipped, not treated as empty-string extremes.
Pulled-plop exclusion (ops.go:757-762) — the closure returns nil for RemovedAt != nil, so the season view's range covers only live plops. Verified against TestDescribeGroupSaysWhenReady: the pulled radish (2026-03-01, ready 2026-04-30) is correctly dropped from the range in the year=2026 view. Guard is also correct: ReadyAround only computed when plant.DaysToMaturity != nil, and omitempty hides an all-nil range.
createGarden defaults (tools.go → gardenFromInput, gardens.go:226) — the tool passes 0 for omitted widthCm/heightCm/gridSizeCm; applyDefaults=true fills 1000/1000 (defaultGardenCM = 1000) and grid 100, empty units → metric. Blank name → ErrInvalidInput. "Imperial" is lowercased to "imperial" before the enum check. All match the test.
updateSeedLot patch mapping (tools.go → SeedLotPatch) — field-for-field consistent with applySeedLotPatch/finalizeSeedLot. Nullable columns (PurchasedAt, PackedForYear, GerminationPct) correctly use Set* flags gated on != nil; non-null columns (Quantity, Unit, Vendor, SourceURL, Notes) use pointer-presence. PurchasedAt routes through parseDay (normalizes/validates YYYY-MM-DD) before storage. No pointer-aliasing bug in the purchased := args.PurchasedAt; purchased = &on reassignment.
deletePlant (tools.go) — service refuses on CountPlantingsForPlant > 0 (past seasons included, since it counts all plantings) or CountSeedLotsForPlant > 0, both mapped to ErrPlantInUse; built-ins refused earlier via writablePlant → ErrForbidden (plants.go:224). The errors.Is remap only rewrites ErrPlantInUse, passing other errors through unchanged.
No correctness defects surfaced in my lane.
🧹 Code cleanliness & maintainability — No material issues found
I've verified the changes against the checked-out code. Findings below.
VERDICT: No material issues found
I reviewed this PR through the code cleanliness & maintainability lens and checked the new code against the surrounding patterns it's supposed to follow. It's clean and idiomatic.
What I verified:
readyDate (internal/service/ops.go:798) reuses the shared dateLayout const (internal/service/plantings.go:15) and the existing dateRangeOf helper rather than reinventing either. The ReadyAround closure at ops.go:757 mirrors the RemovedAt: dateRangeOf(members, func…) line two lines above it — same idiom, so it reads consistently. Since readyDate emits ISO YYYY-MM-DD, the string min/max in dateRangeOf stays valid.
createGarden (internal/agent/tools.go) delegates defaults to the service (gardenFromInput, gardens.go:230+): 0 → defaultGardenCM/defaultGardenGridCM, blank UnitPref → metric. The tool's "default 1000" / "default 100" descriptions match the service, so the docs aren't lying. UnitPref normalization (ToLower+TrimSpace) is the same treatment updateGarden gives it — consistent.
updateSeedLot builds service.SeedLotPatch respecting the Set*-flag convention (SetPurchasedAt/SetPackedForYear/SetGerminationPct) exactly as the struct doc at seed_lots.go:53 prescribes; plain *string fields (Vendor/SourceURL/Notes) map straight through as nil=unchanged. The empty-args guard and %w: … domain.ErrInvalidInput refusal mirror updateGarden's guard verbatim.
deletePlant / deleteSeedLot are thin adapters; the errors.Is(err, domain.ErrPlantInUse) translation is the only branch, and the newly-added errors import is used. DeletePlant/DeleteSeedLot/ErrPlantInUse all exist in the service/domain.
web/src/lib/agent.ts has a TOOL_LABELS entry for all four new tools — no tool ships without a label.
Docs (DESIGN.md roadmap, runtime.go prompt) are updated in the same commit, per the repo's "keep the docs true" rule.
No dead code, no copy-paste that should be shared, no confusing names, no leaky abstraction introduced.
⚡ Performance — No material issues found
I've verified the changed code against the checked-out state. Let me assess the one performance-relevant addition — the ReadyAround computation in describeGroups.
VERDICT: No material issues found
Reviewed through the Performance lens only.
What I checked:
readyAround in describeGroups (internal/service/ops.go:755-763) — This adds one more dateRangeOf pass over each group's members, and readyDate (ops.go:798) calls time.Parse/AddDate/Format once per plop. I confirmed the work is strictly O(total plops), bounded, and computed once per describe_garden call — not inside any nested/quadratic loop. describeGroups already makes several linear passes over members (dateRange, the RemovedAtdateRangeOf, the Plants/Removed tally, and the Each build at ops.go:764-775); this is one additional pass of the same order. The gating if plant.DaysToMaturity != nil (ops.go:755) skips the parse work entirely for plants with no maturity data. Even on the previously-cited ~450-plop grid garden, a few hundred time.Parse calls per turn is microseconds against a model round-trip — a micro-cost, not a regression.
No N+1 introduced — readyDate reads pl.PlantedAt (already in memory) and plantByID is the map already loaded by the caller; no per-plop store call is added.
New agent tools (internal/agent/tools.go:858-933) — createGarden, updateSeedLot, deleteSeedLot, deletePlant are thin single-call adapters over existing service methods. Each is a one-shot mutation, not a loop or hot path; no allocations or iteration of concern.
update_seed_lot empty-patch guard (tools.go:889-892) — rejects a no-field call before doing any work, so the common "nothing to change" case does no I/O. Fine.
Nothing in my lane rises to a reportable issue.
🧯 Error handling & edge cases — No material issues found
I've verified the new code paths against the checked-out source. Here's my review through the error-handling & edge-cases lens.
VERDICT: No material issues found
The unhappy paths this PR introduces are handled carefully, and I confirmed each by reading the surrounding code rather than the diff alone:
readyDate (internal/service/ops.go:798) — guards nil/empty plantedAt and a time.Parse failure, returning nil in both cases (comment acknowledges the "shouldn't happen" path). AddDate(0,0,days) can't panic; string-ordered ISO output keeps dateRangeOf's min/max correct since days is constant per group. Verified.
ReadyAround range (ops.go:757) — the closure returns nil for pulled plops (RemovedAt != nil), so the season/year view correctly excludes a harvest that's behind us; when every member is pulled/undated the result is "" and omitempty drops it. Confirmed both the "now" and year views funnel through the single describeGroups call (ops.go:692), so the behavior is uniform — the test's season-view assertion isn't exercising a separate, un-updated path.
updateSeedLot (internal/agent/tools.go:874) — the all-nil guard rejects an empty patch with a keyboard-facing message; parseDay rejects prose dates before they reach the store; SetPurchasedAt/SetPackedForYear/SetGerminationPct sentinels are set correctly. Downstream finalizeSeedLot validates unit, quantity (finite, 0..max), germination (0..100), URL scheme, and date — so bad values can't slip through the tool.
deletePlant (tools.go:923) — errors.Is wrap is nil-safe (nil err skips both branches and returns the success map); ErrPlantInUse covers both the planting and seed-lot cases the service checks (plants.go:266,274), and non-ErrPlantInUse errors (e.g. built-in/unwritable via writablePlant) propagate to the if err != nil return.
createGarden (tools.go:862) — blank name and default dimensions/grid are enforced by gardenFromInput(in, true); the tool adds no path that bypasses that validation.
No swallowed errors, missing cleanup, panics on bad input, or unhandled nil/empty/boundary cases in the diff's new code.
All my checks confirm clean. Let me write up the verdict.
No material issues found
The four new agent tools (create_garden, update_seed_lot, delete_seed_lot, delete_plant) follow the established authz pattern: each service method calls a sentinel that verifies ownership and returns ErrNotFound (masking existence) for anything that isn't the caller's — ownSeedLot for the lot ops, writablePlant for catalog ops. No gaps.
SourceURL (verified):UpdateSeedLot runs finalizeSeedLot after applying the patch (seed_lots.go:200), and finalizeSeedLot calls validSourceURL (line 304). That function rejects anything that isn't an empty string or a valid http(s)://host/... URL — javascript: and relative paths are explicitly blocked (the inline comment names the XSS reason). No path in the codebase fetches the stored URL server-side, so SSRF is not in scope.
parseDay error message (verified): The model-provided date string is formatted with %q (tools.go:590), which Go-escapes the value before embedding it in the ErrInvalidInput message. The sentinel is stripped at the API layer; the %q-quoted value is what reaches the client — no injection surface.
createGarden unit preference (verified): The service normalises the string to lowercase before a strict two-value check (domain.UnitMetric / domain.UnitImperial) and returns ErrInvalidInput on anything else (gardens.go:252-256). The agent wrapper calls strings.ToLower(strings.TrimSpace(...)) before passing it in, but the service-layer gate is the real guard.
ErrPlantInUse wrapping (verified): The custom-text error stays in the agent/tool layer; the model sees the tool error text and relays it in natural language. If it somehow propagated to the HTTP handler, the ErrPlantInUse case there returns a hardcoded 409 message and ignores the wrapped detail — no unintended disclosure.
🎯 Correctness — No material issues found
I've now verified all the key correctness claims. Let me compile the findings.
Verified:
dateRangeOf correctly skips nil returns from the callback (line 832: if d == nil || *d == "" { continue }), so pulled plops with pl.RemovedAt != nil returning nil are safely excluded from the readyAround range.
dateLayout = "2006-01-02" is YYYY-MM-DD; string comparison (<, >) is lexicographically correct for ISO 8601 dates.
readyDate arithmetic: time.AddDate(0, 0, 60) on "2026-05-01" → "2026-06-30" (30 days to end of May + 30 into June = 60); on "2026-05-11" → "2026-07-10" (20 to May 31 + 30 to June 30 + 10 into July = 60). Both match the test expectations exactly.
SeedLotPatch has SetPurchasedAt/PurchasedAt, SetPackedForYear/PackedForYear, SetGerminationPct/GerminationPct — all correctly set in updateSeedLot. The local variable purchased is pre-parsed before the Set* flag is derived, so SetPurchasedAt: purchased != nil is semantically correct (only true when the caller actually supplied and validated a date).
The "nothing to change" guard in updateSeedLot covers all eight optional fields.
Verdict: No material issues found
The readyAround arithmetic is correct, nil propagation through dateRangeOf works as expected, and the SeedLotPatch field mapping is complete and semantically accurate. No logic bugs in this lens.
🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed. The mustCall closure at lines 757–768 in TestRecordKeepingTools is byte-for-byte identical to the one at lines 947–958 in TestCatalogAndGardenTools (same signature, same t.Fatalf messages, same json.Unmarshal call). TestCatalogAndGardenTools runs from line 938 to ~line 1030 and covers three distinct tool areas (garden creation, seed-lot lifecycle, and plant deletion) inside a single function.
Minor issues
mustCall closure duplicated verbatim between TestRecordKeepingTools and TestCatalogAndGardenTools (internal/agent/tools_test.go:947). The body is identical to the one introduced at line 757 — same signature, same t.Fatalf messages, same json.Unmarshal call. The call closure (capturing ctx and box) appears multiple times across the file and was never extracted, so this follows the existing style, but mustCall is a richer wrapper that a third test would copy again. Extracting it as a file-level helper — func mustToolCall(t *testing.T, exec func(string, any) llm.ToolResult, name string, args any, into any) — would make future tests cost less.
TestCatalogAndGardenTools conflates three distinct tool areas into one ~100-line function (internal/agent/tools_test.go:938). The // --- comments acknowledge the length. create_garden is independent of the seed-lot and delete_plant scenarios; a failure in the garden-creation section suppresses all downstream signal. Splitting at the natural // --- seams (one test for garden creation, one for catalog lifecycle) would give more precise failure attribution on a red run. Minor in a test file, but the function is already noticeably long for a single named thing.
Neither finding blocks the merge; both are nits surfaced by the lens of code cleanliness.
⚡ Performance — No material issues found
No material issues found
The one noteworthy point is a minor redundancy in the ReadyAround computation, not a regression.
readyDate called N times when ≤2 suffice
internal/service/ops.go:755–763 — dateRangeOf(members, …) invokes the closure once per plop in members. The closure calls readyDate, which does time.Parse + time.AddDate + time.Format. For a grid-filled bed the CLAUDE.md history mentions 450 plops reaching this path; that becomes 450 parse+format round-trips per group per describe_garden call.
Because readyDate is monotone (PlantedAt + constant), the minimum and maximum ready dates correspond exactly to the minimum and maximum non-removed PlantedAt strings — and ISO dates sort correctly as strings, so no parsing is needed to find them. The fix reduces to finding min/max PlantedAt with string comparisons, then calling readyDate on at most those two values:
Impact is measured in tenths of a millisecond for typical gardens (< 1 ms total for most describe_garden calls), so this is not blocking, but it is wasteful given the easy fix. Verified by reading ops.go:755–808 and dateRangeOf:828–846.
🧯 Error handling & edge cases — No material issues found
I've now read all the relevant code paths. Here is the review.
No material issues found
All the new error paths in this PR handle unhappy cases correctly. Quick verification notes:
readyDate (ops.go:798): guards plantedAt == nil and empty string before parsing; silently returns nil on parse failure rather than panicking. Correct.
dateRangeOf for ReadyAround (ops.go:757): when all group members are pulled (picker returns nil for all), first and last stay "", first == last returns "", and the field is omitted via omitempty. Verified by reading dateRangeOf at line 828.
updateSeedLot nothing-to-change guard (tools.go:890): cross-checked against SeedLotPatch struct — all eight tool-exposed fields (Quantity, Unit, Vendor, SourceURL, PurchasedAt, PackedForYear, GerminationPct, Notes) are covered. SKU and LotCode exist in the patch but are deliberately unexposed. Guard is complete.
createGarden invalid inputs (tools.go:862): blank/whitespace name, negative/NaN/Inf dimensions, and unknown unit strings all reach gardenFromInput which returns ErrInvalidInput — confirmed at gardens.go:228, 248, 256. Test exercises the blank-name case.
deletePlant dual-cause ErrPlantInUse (tools.go:924): the service returns the same sentinel for "has plantings" and "has seed lots" (plants.go:267,274); the tool's replacement message names both cases explicitly, so the model gets actionable text in either path.
Negative/NaN quantity in updateSeedLot: finalizeSeedLot (seed_lots.go:295) rejects quantity < 0 and non-finite values; germination likewise validated at 316–320. No bypass path through the tool.
deleteSeedLot with no version: consistent with the stated design (seed lots outside undo history); ownSeedLot still enforces ownership before deletion.
Through the security lens, all four new agent tools (create_garden, update_seed_lot, delete_seed_lot, delete_plant) are thin adapters over service-layer methods that already enforce authorization and input validation:
delete_plant → service.DeletePlant → writablePlant (plants.go:219) rejects built-ins (OwnerID == nil → ErrForbidden) and other users' plants (ErrNotFound), and refuses plants referenced by plantings or seed lots. Verified.
update_seed_lot/delete_seed_lot → ownSeedLot (seed_lots.go:134) checks l.OwnerID == actorID else ErrNotFound; UpdateSeedLot runs finalizeSeedLot which validates unit, quantity range, text lengths, validSourceURL (http/https only, with host — blocking javascript:/schemeless URLs at seed_lots.go:332), date format, packed-year range, and germination-pct bounds. The adapter routes purchasedAt through parseDay (YYYY-MM-DD) before the service re-validates. Verified.
create_garden → service.CreateGarden hard-sets OwnerID = actorID (not from input) and runs gardenFromInput. Verified at gardens.go:113-125.
No tainted-input path reaches a sink unvalidated: SourceURL is scheme-checked before being stored/rendered, dated strings go through parseDay + finalizeSeedLot, numeric/enum fields are bounds-checked, and ownership is enforced server-side on every mutation. The readyDate helper in ops.go only parses already-stored, previously-validated planting dates — no untrusted input, no injection surface.
🎯 Correctness — No material issues found
I have verified the core logic. Let me re-derive the readyAround arithmetic from the test to confirm correctness, then write my final review.
Test: radish planted 2026-05-01 (60 days → 2026-06-30) and 2026-05-11 (60 days → 2026-07-10). Expected range "2026-06-30…2026-07-10". time.Parse("2006-01-02", "2026-05-01").AddDate(0,0,60) = 2026-06-30 ✓; 2026-05-11+60 = 2026-07-10 ✓. Pulled plop (planted 2026-03-01) returns nil from readyDate only when... wait, readyDate does NOT check RemovedAt — that check is in the dateRangeOf callback (if pl.RemovedAt != nil return nil). So pulled plop contributes nothing. Correct. Range stays 06-30…07-10. ✓
Verdict: No material issues found
readyAround arithmetic (ops.go:755-763, 798-808) — Verified correct. readyDate parses plantedAt with the same dateLayout ("2006-01-02") used everywhere else in the store, adds days via t.AddDate(0,0,days), and reformats. Pulled plops are excluded by the RemovedAt != nil guard in the dateRangeOf callback (not in readyDate itself, which is the right place — dateRangeOf skips nil returns). When all members are undated/pulled, dateRangeOf returns "", and the omitempty keeps the field absent. Matches the test's expected 2026-06-30…2026-07-10. No off-by-one, no unit confusion.
updateSeedLot date handling (tools.go:885-891) — Verified. purchasedAt goes through parseDay (trims + validates dateLayout), and SetPurchasedAt: purchased != nil correctly distinguishes "clear it" (pass empty → parseDay trims to ""... actually note: passing an empty string would make purchased != nil true, set SetPurchasedAt true, and PurchasedAt points to "" — that's a clear-to-empty path, but finalizeSeedLot's validDatePtr accepts nil/valid; an empty string would fail validation). Edge: the tool can't explicitly clear purchasedAt to NULL since an empty string fails validDatePtr. Minor, and not new incorrect behavior vs. existing semantics. Not blocking.
createGarden (tools.go:862-873) — Verified against gardenFromInput(in, true): zero WidthCM/HeightCM → defaultGardenCM (1000); blank UnitPref (lowercased/trimmed to "") → UnitMetric; name blank → ErrInvalidInput. The strings.ToLower normalizes "Imperial" → "imperial", matching domain.UnitImperial. Correct.
deletePlant (tools.go:927-933) — Verified domain.ErrPlantInUse is returned by DeletePlant when plantings or seed lots reference the plant; the errors.Is wrap preserves the sentinel while adding model-facing text. Built-ins return ErrForbidden from writablePlant (OwnerID nil), passed through unchanged. Correct.
deleteSeedLot — Delegates to DeleteSeedLot, which SET NULLs plantings. Matches description. No correctness issue.
No correctness defects found within my lens.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/agent/tools.go:890-892 — The "nothing to change" guard in updateSeedLot hand-enumerates every nullable field (args.Quantity == nil && args.Unit == nil && …). This faithfully mirrors the established pattern in updateGarden (tools.go:823-825), so it is consistent with the surrounding code, but it is a maintenance hazard: the next field added to the updateSeedLot args struct / SeedLotPatch must also be appended to this boolean chain in lockstep, or a valid single-(new-)field update will be rejected as "say what to change." Low severity because the pattern is established and the field set is small, but worth a comment or a helper if more patch tools appear. Verified by reading both guards.
internal/agent/tools.go:894-901 — The purchased := args.PurchasedAt / purchased = &on dance is slightly more indirect than necessary. parseDay (tools.go:587) already returns the trimmed/parsed string, and SetPurchasedAt: purchased != nil ends up equivalent to args.PurchasedAt != nil (when args.PurchasedAt is non-nil, purchased is always reassigned to a non-nil &on). The local variable earns its keep only by carrying the trimmed value that parseDay already returns, so the on, err := parseDay(*args.PurchasedAt); … SetPlantedAt, patch.PlantedAt = true, &on form used in updatePlanting (tools.go:551-556) is a touch cleaner. Minor readability nit only; not actionable.
internal/service/ops.go:798-808 — readyDate re-implements time.Parse(dateLayout, …) with a nil/empty guard, duplicating the parse already done by validDatePtr in plantings.go:396-402. They serve different purposes (one validates, one computes), so this isn't a real duplication to factor out now, but the date-parsing-with-guard idiom is now in two places; if a third appears, a small parseDayPtr(*string) (time.Time, bool) helper would be worth it. Not actionable now.
No dead code, no naming problems, no broken abstractions. The new deleteSeedLot/deletePlant return shapes (map[string]any{"deleted": id}) match the existing deleteJournalEntry/deleteObject tools, and ReadyAround cleanly reuses dateRangeOf with a picker that returns nil for pulled plops. The added tool prompt strings and TOOL_LABELS entries follow the established conventions.
⚡ Performance — No material issues found
No material issues found.
I reviewed the change through the performance lens, verifying the hot path (describeGroups in internal/service/ops.go) and the new agent tool adapters.
readyDate (ops.go:798) does a time.Parse per plop, but only for groups whose plant has DaysToMaturity, and only inside describeGroups — already an in-memory aggregation over plops fetched in one GardenFull query. No new queries, no N+1. The added dateRangeOf pass over members is one extra linear scan alongside the existing dateRange/RemovedAt/count/Each scans; all are O(plops-in-group) and dwarfed by the DB round-trip that produced them. Not material.
The new tool handlers (createGarden, updateSeedLot, deleteSeedLot, deletePlant) each issue a single service call; no loops, no per-record lookups, no unbounded growth.
updateSeedLot's "nothing to change" guard short-circuits before any parse or service call.
No efficiency regressions, unbounded growth, missing limits, or hot-path blocking introduced by this PR.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
The error-handling and edge-case paths in this diff are sound. I verified the key surfaces:
readyDate (internal/service/ops.go:798): handles nil/empty plantedAt (returns nil), swallows parse errors (returns nil rather than propagating), and AddDate(0,0,days) cannot overflow/panic. DaysToMaturity is guaranteed 1..3650 at write time (finalizePlant at plants.go:328), so no zero/negative reaches it.
dateRangeOf for ReadyAround (ops.go:757): plops with RemovedAt != nil or undated plops return nil from the picker and are skipped, so an all-pulled group yields "" (the omitempty then drops the field) rather than a malformed range. String comparison of ISO dates is chronologically correct.
updateSeedLot (tools.go:889): rejects an all-nil patch with ErrInvalidInput; routes purchasedAt through parseDay (which trims and rejects non-YYYY-MM-DD, verified at tools.go:587); LotID == 0 (omitted) resolves to ErrNotFound via ownSeedLot, not a panic. Service UpdateSeedLot propagates both store and finalizeSeedLot errors.
deleteSeedLot / deletePlant (tools.go:911/tools.go:919): errors are returned, not swallowed; the ErrPlantInUse wrap uses %w so the sentinel survives errors.Is, while adding model-facing text.
createGarden (tools.go:866): UnitPref is lowercased/trimmed; empty falls through to the service default (metric) on create; garbage units are rejected by gardenFromInput; WidthCM/HeightCM/GridSizeCM of 0 default, negatives rejected by validDimensionCM. No zero-value panic path.
One trivial note (not blocking)
updateSeedLot exposes no way to clearpurchasedAt to null. Passing "purchasedAt": "" fails at parseDay (empty is not a valid YYYY-MM-DD), and SetPurchasedAt is only ever true when a parseable date was supplied — so unlike sourceUrl (whose description says "empty clears it" and which the patch trims to ""), the purchase date is set-or-leave-unchanged only. This is consistent with the field's description (which promises no "clear" semantics), so it reads as a deliberate limitation rather than a bug, but a gardener who says "forget the purchase date, drop it" has no tool path to do it. internal/agent/tools.go:885 / :904. Confidence: low that it's unintended.
All four new tools (create_garden, update_seed_lot, delete_seed_lot, delete_plant) delegate authorization to the service layer (ownSeedLot, writablePlant, CreateGarden with OwnerID = actorID), so there are no authn/authz gaps.
update_seed_lot dates are passed through the existing parseDay validator before reaching the service, and the service validates all patch fields (quantity bounds, unit whitelist, URL scheme/host, date format, germination percentage range).
SourceURL is scrubbed in the service via validSourceURL, which rejects anything other than http:// or https:// with a non-empty host, preventing javascript: and other malicious schemes from being stored as clickable vendor links.
delete_plant correctly wraps domain.ErrPlantInUse without bypassing the ownership/built-in checks that happen first in the service; no information leakage beyond the already-sentinel-protected fact that the plant is referenced.
readyDate operates only on already-validated store data (plantedAt was validated on write, daysToMaturity is an int from the catalog), so there is no exposure to unfiltered user input.
🎯 Correctness — No material issues found
No material issues found
I examined the core correctness of the new logic through the diff and verified key declarations in the checked-out code:
readyDate arithmetic (internal/service/ops.go:798–807): time.Parse + AddDate(0, 0, days) + Format correctly computes planting-date + days-to-maturity. The test expectations are confirmed accurate:
2026-05-01 + 60 days = 2026-06-30 (May has 31 days: 30 days remaining in May + 30 days into June)
2026-05-11 + 60 days = 2026-07-10 (20 days remaining in May + 30 days in June + 10 days into July)
ReadyAround exclusion of pulled plops (ops.go:757–762): The callback returns nil when pl.RemovedAt != nil, so dateRangeOf skips them. The season-view test confirms the pulled radish (2026-03-01) does not affect the range even though it appears in members.
dateRangeOf empty-result handling (ops.go:828–846): When all callbacks return nil (no live dated plops), first == last == "" and it returns "". Coupled with omitempty on ReadyAround, the field is correctly absent from JSON.
update_seed_lot field guard (tools.go:890–893): The nil-check for all optional fields prevents no-op updates and returns ErrInvalidInput.
delete_plant sentinel wrapping (tools.go:923–928): errors.Is(err, domain.ErrPlantInUse) correctly matches the sentinel returned by DeletePlant, and the re-wrapped error is human-readable for the model while preserving the original cause chain.
No logic bugs, incorrect formulas, or semantic mis-computations were found.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/agent/tools_test.go:943 — The call and mustCall closures in TestCatalogAndGardenTools are identical to those in TestRecordKeepingTools (line 753). Across the file there are six call and two mustCall inline redefinitions. This copy-paste clutter makes tests harder to maintain and drift-prone. Extract them once as package-level helpers (e.g., execTool(t, box, name, args) and execToolOK(t, box, name, args, into)) and reuse them.
internal/agent/tools.go:927 — The deletePlant adapter wraps domain.ErrPlantInUse with %w and the same sentinel, layering the error. The comment says the sentinel text is “for a log line,” but wrapping the same sentinel rather than returning a plain string or a distinct error type is an unusual pattern that could confuse future maintainers about error identity. A plain fmt.Errorf("...") (without %w) or a dedicated agent-facing error would be cleaner, since the agent layer is intentionally translating service errors into model-readable prose anyway.
⚡ Performance — No material issues found
No material issues found.
🧯 Error handling & edge cases — No material issues found
No material issues found.
My lens is error-handling and edge cases. I verified the following:
createGarden passes raw Name to the service, but gardenFromInput trims and validates it (confirmed by reading internal/service/gardens.go:227–230); the test also checks whitespace-only names are rejected. Dimension defaults and UnitPref normalization are handled downstream or in the service.
updateSeedLot correctly guards against no-op updates, parses the optional PurchasedAt through parseDay (which trims and validates), and uses pointer presence (!= nil) to drive Set* booleans for the patch. The direct pointer fields (Quantity, Unit, etc.) safely distinguish “don’t change” (nil) from “set to zero/empty” (non-nil pointer).
deleteSeedLot simply proxies errors and returns a success payload; nothing swallowed.
deletePlant uses errors.Is to detect the ErrPlantInUse sentinel and re-wraps it with model-oriented wording while preserving the sentinel for upstream matching. Other errors pass through unchanged. The underlying service checks both plantings and seed-lot references (confirmed by reading internal/service/plants.go:258–277).
readyDate defensively handles nil/empty plantedAt, parse failure, and returns nil so undated or invalid-dated plantings are silently excluded from the range rather than panicking or producing garbage.
All unhappy paths I can reach are covered or delegated safely to the service layer.
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** · 5 findings (0 with multi-model agreement)
<details><summary>5 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Duplicated call/mustCall test helpers should be extracted | `internal/agent/tools_test.go:943` | kimi-k2.6:cloud | maintainability |
| 🟡 | updateSeedLot 'nothing to change' guard hand-enumerates every field; must be kept in sync with the args struct when new fields are added | `internal/agent/tools.go:890` | glm-5.2:cloud | maintainability |
| 🟡 | Wrapping the same sentinel error with %w is an unusual pattern | `internal/agent/tools.go:927` | kimi-k2.6:cloud | maintainability |
| 🟡 | mustCall closure copied verbatim from TestRecordKeepingTools — candidate for a file-level helper | `internal/agent/tools_test.go:947` | claude-code/sonnet | maintainability |
| ⚪ | TestCatalogAndGardenTools conflates three unrelated tool scenarios; splitting would give more precise failure attribution | `internal/agent/tools_test.go:938` | claude-code/sonnet | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've traced each new agent tool to its service method and verified the authorization and validation controls.
## VERDICT: No material issues found
The new tools are thin adapters over service methods that already enforce every control that matters for this lens. I verified each:
- **`delete_plant` → `DeletePlant`** (`plants.go:258`): gated by `writablePlant` (`plants.go:219`), which returns `ErrForbidden` for built-ins (`OwnerID == nil`) and `ErrNotFound` for another user's plant. No cross-tenant deletion, built-ins protected. The `ErrPlantInUse` rewrap in the tool only changes the message string, not authz.
- **`update_seed_lot` / `delete_seed_lot` → `UpdateSeedLot`/`DeleteSeedLot`** (`seed_lots.go:194,229`): both gated by `ownSeedLot` (`seed_lots.go:129`), which masks any lot not owned by the actor as `ErrNotFound`. Version-guarded on update. No IDOR.
- **`create_garden` → `CreateGarden`** (`gardens.go:113`): forces `g.OwnerID = actorID` — the caller cannot set ownership.
- **`SourceURL` on a seed lot** (potential stored-XSS via a `javascript:` link, since it renders as a clickable link): the update path runs through `finalizeSeedLot` → `validSourceURL` (`seed_lots.go:304,332`), which rejects any non-`http(s)`/hostless scheme. The vector is closed for the update tool too.
- **Dates** (`purchasedAt`) go through `parseDay`, and the tool rejects free-text like `"last spring"` (test-covered) before it reaches the store.
- All tools use `a.actor` (the authenticated session actor) as `actorID`; there is no way for the model to pass an arbitrary actor. No SSRF (URLs are stored, never fetched server-side), no injection surface (parameterized store), no secret handling in this diff.
Authorization lives entirely at the service seam exactly as the architecture intends, and the adapters add no bypass. Nothing in the security lens is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified all the correctness-critical paths. Let me summarize.
## VERDICT: No material issues found
I reviewed this PR through the **Correctness** lens, verifying each numeric/formula path against the checked-out code.
**What I checked and confirmed correct:**
- **`readyDate` arithmetic** (`internal/service/ops.go:798`) — re-derived the two test dates from first principles using Go's `AddDate(0,0,days)` normalization:
- `2026-05-01 + 60d` → day 61 of May → June 30 (`2026-06-30`) ✓
- `2026-05-11 + 60d` → day 71 of May → July 10 (`2026-07-10`) ✓
- Matches the test's expected `"2026-06-30…2026-07-10"`. `dateLayout` is `"2006-01-02"` in both `service` and `agent` packages, so parse/format round-trips cleanly.
- **Range min/max** (`dateRangeOf`, `ops.go:828`) — uses lexical string compare; valid for fixed-width ISO dates. `readyDate` always emits a 10-char `YYYY-MM-DD`, so ordering is sound. Nils (undated / unparseable) are skipped, not treated as empty-string extremes.
- **Pulled-plop exclusion** (`ops.go:757-762`) — the closure returns `nil` for `RemovedAt != nil`, so the season view's range covers only live plops. Verified against `TestDescribeGroupSaysWhenReady`: the pulled radish (`2026-03-01`, ready `2026-04-30`) is correctly dropped from the range in the `year=2026` view. Guard is also correct: `ReadyAround` only computed when `plant.DaysToMaturity != nil`, and `omitempty` hides an all-nil range.
- **`createGarden` defaults** (`tools.go` → `gardenFromInput`, `gardens.go:226`) — the tool passes `0` for omitted `widthCm`/`heightCm`/`gridSizeCm`; `applyDefaults=true` fills `1000`/`1000` (`defaultGardenCM = 1000`) and grid `100`, empty units → `metric`. Blank name → `ErrInvalidInput`. `"Imperial"` is lowercased to `"imperial"` before the enum check. All match the test.
- **`updateSeedLot` patch mapping** (`tools.go` → `SeedLotPatch`) — field-for-field consistent with `applySeedLotPatch`/`finalizeSeedLot`. Nullable columns (`PurchasedAt`, `PackedForYear`, `GerminationPct`) correctly use `Set*` flags gated on `!= nil`; non-null columns (`Quantity`, `Unit`, `Vendor`, `SourceURL`, `Notes`) use pointer-presence. `PurchasedAt` routes through `parseDay` (normalizes/validates `YYYY-MM-DD`) before storage. No pointer-aliasing bug in the `purchased := args.PurchasedAt; purchased = &on` reassignment.
- **`deletePlant`** (`tools.go`) — service refuses on `CountPlantingsForPlant > 0` (past seasons included, since it counts all plantings) or `CountSeedLotsForPlant > 0`, both mapped to `ErrPlantInUse`; built-ins refused earlier via `writablePlant` → `ErrForbidden` (`plants.go:224`). The `errors.Is` remap only rewrites `ErrPlantInUse`, passing other errors through unchanged.
No correctness defects surfaced in my lane.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
I've verified the changes against the checked-out code. Findings below.
## VERDICT: No material issues found
I reviewed this PR through the **code cleanliness & maintainability** lens and checked the new code against the surrounding patterns it's supposed to follow. It's clean and idiomatic.
What I verified:
- **`readyDate` (`internal/service/ops.go:798`)** reuses the shared `dateLayout` const (`internal/service/plantings.go:15`) and the existing `dateRangeOf` helper rather than reinventing either. The `ReadyAround` closure at `ops.go:757` mirrors the `RemovedAt: dateRangeOf(members, func…)` line two lines above it — same idiom, so it reads consistently. Since `readyDate` emits ISO `YYYY-MM-DD`, the string min/max in `dateRangeOf` stays valid.
- **`createGarden` (`internal/agent/tools.go`)** delegates defaults to the service (`gardenFromInput`, `gardens.go:230`+): `0 → defaultGardenCM/defaultGardenGridCM`, blank `UnitPref → metric`. The tool's `"default 1000"` / `"default 100"` descriptions match the service, so the docs aren't lying. `UnitPref` normalization (`ToLower`+`TrimSpace`) is the same treatment `updateGarden` gives it — consistent.
- **`updateSeedLot`** builds `service.SeedLotPatch` respecting the Set*-flag convention (`SetPurchasedAt/SetPackedForYear/SetGerminationPct`) exactly as the struct doc at `seed_lots.go:53` prescribes; plain `*string` fields (Vendor/SourceURL/Notes) map straight through as nil=unchanged. The empty-args guard and `%w: … domain.ErrInvalidInput` refusal mirror `updateGarden`'s guard verbatim.
- **`deletePlant` / `deleteSeedLot`** are thin adapters; the `errors.Is(err, domain.ErrPlantInUse)` translation is the only branch, and the newly-added `errors` import is used. `DeletePlant`/`DeleteSeedLot`/`ErrPlantInUse` all exist in the service/domain.
- **`web/src/lib/agent.ts`** has a `TOOL_LABELS` entry for all four new tools — no tool ships without a label.
- Docs (DESIGN.md roadmap, runtime.go prompt) are updated in the same commit, per the repo's "keep the docs true" rule.
No dead code, no copy-paste that should be shared, no confusing names, no leaky abstraction introduced.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've verified the changed code against the checked-out state. Let me assess the one performance-relevant addition — the `ReadyAround` computation in `describeGroups`.
## VERDICT: No material issues found
Reviewed through the Performance lens only.
**What I checked:**
- **`readyAround` in `describeGroups` (`internal/service/ops.go:755-763`)** — This adds one more `dateRangeOf` pass over each group's `members`, and `readyDate` (`ops.go:798`) calls `time.Parse`/`AddDate`/`Format` once per plop. I confirmed the work is strictly `O(total plops)`, bounded, and computed **once per `describe_garden` call** — not inside any nested/quadratic loop. `describeGroups` already makes several linear passes over `members` (`dateRange`, the `RemovedAt` `dateRangeOf`, the `Plants`/`Removed` tally, and the `Each` build at `ops.go:764-775`); this is one additional pass of the same order. The gating `if plant.DaysToMaturity != nil` (`ops.go:755`) skips the parse work entirely for plants with no maturity data. Even on the previously-cited ~450-plop grid garden, a few hundred `time.Parse` calls per turn is microseconds against a model round-trip — a micro-cost, not a regression.
- **No N+1 introduced** — `readyDate` reads `pl.PlantedAt` (already in memory) and `plantByID` is the map already loaded by the caller; no per-plop store call is added.
- **New agent tools (`internal/agent/tools.go:858-933`)** — `createGarden`, `updateSeedLot`, `deleteSeedLot`, `deletePlant` are thin single-call adapters over existing service methods. Each is a one-shot mutation, not a loop or hot path; no allocations or iteration of concern.
- **`update_seed_lot` empty-patch guard (`tools.go:889-892`)** — rejects a no-field call before doing any work, so the common "nothing to change" case does no I/O. Fine.
Nothing in my lane rises to a reportable issue.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've verified the new code paths against the checked-out source. Here's my review through the error-handling & edge-cases lens.
## VERDICT: No material issues found
The unhappy paths this PR introduces are handled carefully, and I confirmed each by reading the surrounding code rather than the diff alone:
- **`readyDate` (`internal/service/ops.go:798`)** — guards `nil`/empty `plantedAt` and a `time.Parse` failure, returning `nil` in both cases (comment acknowledges the "shouldn't happen" path). `AddDate(0,0,days)` can't panic; string-ordered ISO output keeps `dateRangeOf`'s min/max correct since `days` is constant per group. Verified.
- **`ReadyAround` range (`ops.go:757`)** — the closure returns `nil` for pulled plops (`RemovedAt != nil`), so the season/year view correctly excludes a harvest that's behind us; when every member is pulled/undated the result is `""` and `omitempty` drops it. Confirmed both the "now" and `year` views funnel through the single `describeGroups` call (`ops.go:692`), so the behavior is uniform — the test's season-view assertion isn't exercising a separate, un-updated path.
- **`updateSeedLot` (`internal/agent/tools.go:874`)** — the all-`nil` guard rejects an empty patch with a keyboard-facing message; `parseDay` rejects prose dates before they reach the store; `SetPurchasedAt`/`SetPackedForYear`/`SetGerminationPct` sentinels are set correctly. Downstream `finalizeSeedLot` validates unit, quantity (finite, `0..max`), germination (`0..100`), URL scheme, and date — so bad values can't slip through the tool.
- **`deletePlant` (`tools.go:923`)** — `errors.Is` wrap is `nil`-safe (nil err skips both branches and returns the success map); `ErrPlantInUse` covers both the planting and seed-lot cases the service checks (`plants.go:266,274`), and non-`ErrPlantInUse` errors (e.g. built-in/unwritable via `writablePlant`) propagate to the `if err != nil` return.
- **`createGarden` (`tools.go:862`)** — blank name and default dimensions/grid are enforced by `gardenFromInput(in, true)`; the tool adds no path that bypasses that validation.
No swallowed errors, missing cleanup, panics on bad input, or unhandled nil/empty/boundary cases in the diff's new code.
</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>
All my checks confirm clean. Let me write up the verdict.
**No material issues found**
The four new agent tools (`create_garden`, `update_seed_lot`, `delete_seed_lot`, `delete_plant`) follow the established authz pattern: each service method calls a sentinel that verifies ownership and returns `ErrNotFound` (masking existence) for anything that isn't the caller's — `ownSeedLot` for the lot ops, `writablePlant` for catalog ops. No gaps.
- **`SourceURL` (verified):** `UpdateSeedLot` runs `finalizeSeedLot` after applying the patch (seed_lots.go:200), and `finalizeSeedLot` calls `validSourceURL` (line 304). That function rejects anything that isn't an empty string or a valid `http(s)://host/...` URL — `javascript:` and relative paths are explicitly blocked (the inline comment names the XSS reason). No path in the codebase fetches the stored URL server-side, so SSRF is not in scope.
- **`parseDay` error message (verified):** The model-provided date string is formatted with `%q` (tools.go:590), which Go-escapes the value before embedding it in the `ErrInvalidInput` message. The sentinel is stripped at the API layer; the `%q`-quoted value is what reaches the client — no injection surface.
- **`createGarden` unit preference (verified):** The service normalises the string to lowercase before a strict two-value check (`domain.UnitMetric` / `domain.UnitImperial`) and returns `ErrInvalidInput` on anything else (gardens.go:252-256). The agent wrapper calls `strings.ToLower(strings.TrimSpace(...))` before passing it in, but the service-layer gate is the real guard.
- **`ErrPlantInUse` wrapping (verified):** The custom-text error stays in the agent/tool layer; the model sees the tool error text and relays it in natural language. If it somehow propagated to the HTTP handler, the `ErrPlantInUse` case there returns a hardcoded 409 message and ignores the wrapped detail — no unintended disclosure.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've now verified all the key correctness claims. Let me compile the findings.
**Verified:**
- `dateRangeOf` correctly skips nil returns from the callback (line 832: `if d == nil || *d == "" { continue }`), so pulled plops with `pl.RemovedAt != nil` returning nil are safely excluded from the `readyAround` range.
- `dateLayout = "2006-01-02"` is YYYY-MM-DD; string comparison (`<`, `>`) is lexicographically correct for ISO 8601 dates.
- `readyDate` arithmetic: `time.AddDate(0, 0, 60)` on "2026-05-01" → "2026-06-30" (30 days to end of May + 30 into June = 60); on "2026-05-11" → "2026-07-10" (20 to May 31 + 30 to June 30 + 10 into July = 60). Both match the test expectations exactly.
- `SeedLotPatch` has `SetPurchasedAt/PurchasedAt`, `SetPackedForYear/PackedForYear`, `SetGerminationPct/GerminationPct` — all correctly set in `updateSeedLot`. The local variable `purchased` is pre-parsed before the `Set*` flag is derived, so `SetPurchasedAt: purchased != nil` is semantically correct (only true when the caller actually supplied and validated a date).
- The "nothing to change" guard in `updateSeedLot` covers all eight optional fields.
---
**Verdict: No material issues found**
The `readyAround` arithmetic is correct, nil propagation through `dateRangeOf` works as expected, and the `SeedLotPatch` field mapping is complete and semantically accurate. No logic bugs in this lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings are confirmed. The `mustCall` closure at lines 757–768 in `TestRecordKeepingTools` is byte-for-byte identical to the one at lines 947–958 in `TestCatalogAndGardenTools` (same signature, same `t.Fatalf` messages, same `json.Unmarshal` call). `TestCatalogAndGardenTools` runs from line 938 to ~line 1030 and covers three distinct tool areas (garden creation, seed-lot lifecycle, and plant deletion) inside a single function.
---
**Minor issues**
- **`mustCall` closure duplicated verbatim between `TestRecordKeepingTools` and `TestCatalogAndGardenTools`** (`internal/agent/tools_test.go:947`). The body is identical to the one introduced at line 757 — same signature, same `t.Fatalf` messages, same `json.Unmarshal` call. The `call` closure (capturing `ctx` and `box`) appears multiple times across the file and was never extracted, so this follows the existing style, but `mustCall` is a richer wrapper that a third test would copy again. Extracting it as a file-level helper — `func mustToolCall(t *testing.T, exec func(string, any) llm.ToolResult, name string, args any, into any)` — would make future tests cost less.
- **`TestCatalogAndGardenTools` conflates three distinct tool areas into one ~100-line function** (`internal/agent/tools_test.go:938`). The `// ---` comments acknowledge the length. `create_garden` is independent of the seed-lot and `delete_plant` scenarios; a failure in the garden-creation section suppresses all downstream signal. Splitting at the natural `// ---` seams (one test for garden creation, one for catalog lifecycle) would give more precise failure attribution on a red run. Minor in a test file, but the function is already noticeably long for a single named thing.
Neither finding blocks the merge; both are nits surfaced by the lens of code cleanliness.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
The one noteworthy point is a minor redundancy in the `ReadyAround` computation, not a regression.
**`readyDate` called N times when ≤2 suffice**
`internal/service/ops.go:755–763` — `dateRangeOf(members, …)` invokes the closure once per plop in `members`. The closure calls `readyDate`, which does `time.Parse` + `time.AddDate` + `time.Format`. For a grid-filled bed the CLAUDE.md history mentions 450 plops reaching this path; that becomes 450 parse+format round-trips per group per `describe_garden` call.
Because `readyDate` is monotone (`PlantedAt + constant`), the minimum and maximum ready dates correspond exactly to the minimum and maximum non-removed `PlantedAt` strings — and ISO dates sort correctly as strings, so no parsing is needed to find them. The fix reduces to finding min/max `PlantedAt` with string comparisons, then calling `readyDate` on at most those two values:
```go
if plant.DaysToMaturity != nil {
days := *plant.DaysToMaturity
var first, last string
for _, pl := range members {
if pl.RemovedAt != nil || pl.PlantedAt == nil {
continue
}
if first == "" || *pl.PlantedAt < first { first = *pl.PlantedAt }
if *pl.PlantedAt > last { last = *pl.PlantedAt }
}
if first != "" {
fd := readyDate(&first, days)
if first == last {
g.ReadyAround = *fd
} else {
ld := readyDate(&last, days)
g.ReadyAround = *fd + "…" + *ld
}
}
}
```
Impact is measured in tenths of a millisecond for typical gardens (< 1 ms total for most `describe_garden` calls), so this is not blocking, but it is wasteful given the easy fix. Verified by reading `ops.go:755–808` and `dateRangeOf:828–846`.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've now read all the relevant code paths. Here is the review.
---
**No material issues found**
All the new error paths in this PR handle unhappy cases correctly. Quick verification notes:
- **`readyDate`** (`ops.go:798`): guards `plantedAt == nil` and empty string before parsing; silently returns `nil` on parse failure rather than panicking. Correct.
- **`dateRangeOf` for `ReadyAround`** (`ops.go:757`): when all group members are pulled (picker returns `nil` for all), `first` and `last` stay `""`, `first == last` returns `""`, and the field is omitted via `omitempty`. Verified by reading `dateRangeOf` at line 828.
- **`updateSeedLot` nothing-to-change guard** (`tools.go:890`): cross-checked against `SeedLotPatch` struct — all eight tool-exposed fields (`Quantity`, `Unit`, `Vendor`, `SourceURL`, `PurchasedAt`, `PackedForYear`, `GerminationPct`, `Notes`) are covered. `SKU` and `LotCode` exist in the patch but are deliberately unexposed. Guard is complete.
- **`createGarden` invalid inputs** (`tools.go:862`): blank/whitespace name, negative/NaN/Inf dimensions, and unknown unit strings all reach `gardenFromInput` which returns `ErrInvalidInput` — confirmed at `gardens.go:228`, `248`, `256`. Test exercises the blank-name case.
- **`deletePlant` dual-cause ErrPlantInUse** (`tools.go:924`): the service returns the same sentinel for "has plantings" and "has seed lots" (`plants.go:267,274`); the tool's replacement message names both cases explicitly, so the model gets actionable text in either path.
- **Negative/NaN quantity in `updateSeedLot`**: `finalizeSeedLot` (`seed_lots.go:295`) rejects `quantity < 0` and non-finite values; germination likewise validated at 316–320. No bypass path through the tool.
- **`deleteSeedLot` with no version**: consistent with the stated design (seed lots outside undo history); `ownSeedLot` still enforces ownership before deletion.
</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>
No material issues found
Through the security lens, all four new agent tools (`create_garden`, `update_seed_lot`, `delete_seed_lot`, `delete_plant`) are thin adapters over service-layer methods that already enforce authorization and input validation:
- **`delete_plant`** → `service.DeletePlant` → `writablePlant` (`plants.go:219`) rejects built-ins (`OwnerID == nil` → `ErrForbidden`) and other users' plants (`ErrNotFound`), and refuses plants referenced by plantings or seed lots. Verified.
- **`update_seed_lot`/`delete_seed_lot`** → `ownSeedLot` (`seed_lots.go:134`) checks `l.OwnerID == actorID` else `ErrNotFound`; `UpdateSeedLot` runs `finalizeSeedLot` which validates unit, quantity range, text lengths, `validSourceURL` (http/https only, with host — blocking `javascript:`/schemeless URLs at `seed_lots.go:332`), date format, packed-year range, and germination-pct bounds. The adapter routes `purchasedAt` through `parseDay` (YYYY-MM-DD) before the service re-validates. Verified.
- **`create_garden`** → `service.CreateGarden` hard-sets `OwnerID = actorID` (not from input) and runs `gardenFromInput`. Verified at `gardens.go:113-125`.
No tainted-input path reaches a sink unvalidated: `SourceURL` is scheme-checked before being stored/rendered, dated strings go through `parseDay` + `finalizeSeedLot`, numeric/enum fields are bounds-checked, and ownership is enforced server-side on every mutation. The `readyDate` helper in `ops.go` only parses already-stored, previously-validated planting dates — no untrusted input, no injection surface.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I have verified the core logic. Let me re-derive the `readyAround` arithmetic from the test to confirm correctness, then write my final review.
Test: radish planted 2026-05-01 (60 days → 2026-06-30) and 2026-05-11 (60 days → 2026-07-10). Expected range "2026-06-30…2026-07-10". `time.Parse("2006-01-02", "2026-05-01").AddDate(0,0,60)` = 2026-06-30 ✓; `2026-05-11`+60 = 2026-07-10 ✓. Pulled plop (planted 2026-03-01) returns nil from `readyDate` only when... wait, `readyDate` does NOT check `RemovedAt` — that check is in the `dateRangeOf` callback (`if pl.RemovedAt != nil return nil`). So pulled plop contributes nothing. Correct. Range stays 06-30…07-10. ✓
**Verdict: No material issues found**
- **`readyAround` arithmetic (ops.go:755-763, 798-808)** — Verified correct. `readyDate` parses `plantedAt` with the same `dateLayout` ("2006-01-02") used everywhere else in the store, adds `days` via `t.AddDate(0,0,days)`, and reformats. Pulled plops are excluded by the `RemovedAt != nil` guard in the `dateRangeOf` callback (not in `readyDate` itself, which is the right place — `dateRangeOf` skips nil returns). When all members are undated/pulled, `dateRangeOf` returns `""`, and the `omitempty` keeps the field absent. Matches the test's expected `2026-06-30…2026-07-10`. No off-by-one, no unit confusion.
- **`updateSeedLot` date handling (tools.go:885-891)** — Verified. `purchasedAt` goes through `parseDay` (trims + validates `dateLayout`), and `SetPurchasedAt: purchased != nil` correctly distinguishes "clear it" (pass empty → parseDay trims to ""... actually note: passing an empty string would make `purchased != nil` true, set `SetPurchasedAt` true, and `PurchasedAt` points to "" — that's a clear-to-empty path, but `finalizeSeedLot`'s `validDatePtr` accepts nil/valid; an empty string would fail validation). Edge: the tool can't explicitly clear `purchasedAt` to NULL since an empty string fails `validDatePtr`. Minor, and not new incorrect behavior vs. existing semantics. Not blocking.
- **`createGarden` (tools.go:862-873)** — Verified against `gardenFromInput(in, true)`: zero WidthCM/HeightCM → `defaultGardenCM` (1000); blank `UnitPref` (lowercased/trimmed to "") → `UnitMetric`; name blank → `ErrInvalidInput`. The `strings.ToLower` normalizes "Imperial" → "imperial", matching `domain.UnitImperial`. Correct.
- **`deletePlant` (tools.go:927-933)** — Verified `domain.ErrPlantInUse` is returned by `DeletePlant` when plantings or seed lots reference the plant; the `errors.Is` wrap preserves the sentinel while adding model-facing text. Built-ins return `ErrForbidden` from `writablePlant` (OwnerID nil), passed through unchanged. Correct.
- **`deleteSeedLot`** — Delegates to `DeleteSeedLot`, which SET NULLs plantings. Matches description. No correctness issue.
No correctness defects found within my lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
- `internal/agent/tools.go:890-892` — The "nothing to change" guard in `updateSeedLot` hand-enumerates every nullable field (`args.Quantity == nil && args.Unit == nil && …`). This faithfully mirrors the established pattern in `updateGarden` (tools.go:823-825), so it is consistent with the surrounding code, but it is a maintenance hazard: the next field added to the `updateSeedLot` args struct / `SeedLotPatch` must also be appended to this boolean chain in lockstep, or a valid single-(new-)field update will be rejected as "say what to change." Low severity because the pattern is established and the field set is small, but worth a comment or a helper if more patch tools appear. Verified by reading both guards.
- `internal/agent/tools.go:894-901` — The `purchased := args.PurchasedAt` / `purchased = &on` dance is slightly more indirect than necessary. `parseDay` (tools.go:587) already returns the trimmed/parsed string, and `SetPurchasedAt: purchased != nil` ends up equivalent to `args.PurchasedAt != nil` (when `args.PurchasedAt` is non-nil, `purchased` is always reassigned to a non-nil `&on`). The local variable earns its keep only by carrying the trimmed value that `parseDay` already returns, so the `on, err := parseDay(*args.PurchasedAt); … SetPlantedAt, patch.PlantedAt = true, &on` form used in `updatePlanting` (tools.go:551-556) is a touch cleaner. Minor readability nit only; not actionable.
- `internal/service/ops.go:798-808` — `readyDate` re-implements `time.Parse(dateLayout, …)` with a nil/empty guard, duplicating the parse already done by `validDatePtr` in `plantings.go:396-402`. They serve different purposes (one validates, one computes), so this isn't a real duplication to factor out now, but the date-parsing-with-guard idiom is now in two places; if a third appears, a small `parseDayPtr(*string) (time.Time, bool)` helper would be worth it. Not actionable now.
No dead code, no naming problems, no broken abstractions. The new `deleteSeedLot`/`deletePlant` return shapes (`map[string]any{"deleted": id}`) match the existing `deleteJournalEntry`/`deleteObject` tools, and `ReadyAround` cleanly reuses `dateRangeOf` with a picker that returns nil for pulled plops. The added tool prompt strings and `TOOL_LABELS` entries follow the established conventions.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
I reviewed the change through the performance lens, verifying the hot path (`describeGroups` in `internal/service/ops.go`) and the new agent tool adapters.
- `readyDate` (ops.go:798) does a `time.Parse` per plop, but only for groups whose plant has `DaysToMaturity`, and only inside `describeGroups` — already an in-memory aggregation over plops fetched in one `GardenFull` query. No new queries, no N+1. The added `dateRangeOf` pass over `members` is one extra linear scan alongside the existing `dateRange`/`RemovedAt`/count/`Each` scans; all are O(plops-in-group) and dwarfed by the DB round-trip that produced them. Not material.
- The new tool handlers (`createGarden`, `updateSeedLot`, `deleteSeedLot`, `deletePlant`) each issue a single service call; no loops, no per-record lookups, no unbounded growth.
- `updateSeedLot`'s "nothing to change" guard short-circuits before any parse or service call.
No efficiency regressions, unbounded growth, missing limits, or hot-path blocking introduced by this PR.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
## Verdict: No material issues found
The error-handling and edge-case paths in this diff are sound. I verified the key surfaces:
- **`readyDate` (`internal/service/ops.go:798`)**: handles `nil`/empty `plantedAt` (returns nil), swallows parse errors (returns nil rather than propagating), and `AddDate(0,0,days)` cannot overflow/panic. `DaysToMaturity` is guaranteed 1..3650 at write time (`finalizePlant` at `plants.go:328`), so no zero/negative reaches it.
- **`dateRangeOf` for `ReadyAround` (`ops.go:757`)**: plops with `RemovedAt != nil` or undated plops return nil from the picker and are skipped, so an all-pulled group yields `""` (the `omitempty` then drops the field) rather than a malformed range. String comparison of ISO dates is chronologically correct.
- **`updateSeedLot` (`tools.go:889`)**: rejects an all-nil patch with `ErrInvalidInput`; routes `purchasedAt` through `parseDay` (which trims and rejects non-`YYYY-MM-DD`, verified at `tools.go:587`); `LotID == 0` (omitted) resolves to `ErrNotFound` via `ownSeedLot`, not a panic. Service `UpdateSeedLot` propagates both store and `finalizeSeedLot` errors.
- **`deleteSeedLot` / `deletePlant` (`tools.go:911`/`tools.go:919`)**: errors are returned, not swallowed; the `ErrPlantInUse` wrap uses `%w` so the sentinel survives `errors.Is`, while adding model-facing text.
- **`createGarden` (`tools.go:866`)**: `UnitPref` is lowercased/trimmed; empty falls through to the service default (metric) on create; garbage units are rejected by `gardenFromInput`; `WidthCM`/`HeightCM`/`GridSizeCM` of 0 default, negatives rejected by `validDimensionCM`. No zero-value panic path.
### One trivial note (not blocking)
`updateSeedLot` exposes no way to *clear* `purchasedAt` to null. Passing `"purchasedAt": ""` fails at `parseDay` (empty is not a valid `YYYY-MM-DD`), and `SetPurchasedAt` is only ever true when a parseable date was supplied — so unlike `sourceUrl` (whose description says "empty clears it" and which the patch trims to `""`), the purchase date is set-or-leave-unchanged only. This is consistent with the field's description (which promises no "clear" semantics), so it reads as a deliberate limitation rather than a bug, but a gardener who says "forget the purchase date, drop it" has no tool path to do it. `internal/agent/tools.go:885` / `:904`. Confidence: low that it's unintended.
</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
- All four new tools (`create_garden`, `update_seed_lot`, `delete_seed_lot`, `delete_plant`) delegate authorization to the service layer (`ownSeedLot`, `writablePlant`, `CreateGarden` with `OwnerID = actorID`), so there are no authn/authz gaps.
- `update_seed_lot` dates are passed through the existing `parseDay` validator before reaching the service, and the service validates all patch fields (quantity bounds, unit whitelist, URL scheme/host, date format, germination percentage range).
- `SourceURL` is scrubbed in the service via `validSourceURL`, which rejects anything other than `http://` or `https://` with a non-empty host, preventing `javascript:` and other malicious schemes from being stored as clickable vendor links.
- `delete_plant` correctly wraps `domain.ErrPlantInUse` without bypassing the ownership/built-in checks that happen first in the service; no information leakage beyond the already-sentinel-protected fact that the plant is referenced.
- `readyDate` operates only on already-validated store data (`plantedAt` was validated on write, `daysToMaturity` is an int from the catalog), so there is no exposure to unfiltered user input.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I examined the core correctness of the new logic through the diff and verified key declarations in the checked-out code:
- **`readyDate` arithmetic** (`internal/service/ops.go:798–807`): `time.Parse` + `AddDate(0, 0, days)` + `Format` correctly computes planting-date + days-to-maturity. The test expectations are confirmed accurate:
- `2026-05-01 + 60 days = 2026-06-30` (May has 31 days: 30 days remaining in May + 30 days into June)
- `2026-05-11 + 60 days = 2026-07-10` (20 days remaining in May + 30 days in June + 10 days into July)
- **`ReadyAround` exclusion of pulled plops** (`ops.go:757–762`): The callback returns `nil` when `pl.RemovedAt != nil`, so `dateRangeOf` skips them. The season-view test confirms the pulled radish (`2026-03-01`) does not affect the range even though it appears in `members`.
- **`dateRangeOf` empty-result handling** (`ops.go:828–846`): When all callbacks return `nil` (no live dated plops), `first == last == ""` and it returns `""`. Coupled with `omitempty` on `ReadyAround`, the field is correctly absent from JSON.
- **`update_seed_lot` field guard** (`tools.go:890–893`): The nil-check for all optional fields prevents no-op updates and returns `ErrInvalidInput`.
- **`delete_plant` sentinel wrapping** (`tools.go:923–928`): `errors.Is(err, domain.ErrPlantInUse)` correctly matches the sentinel returned by `DeletePlant`, and the re-wrapped error is human-readable for the model while preserving the original cause chain.
No logic bugs, incorrect formulas, or semantic mis-computations were found.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `internal/agent/tools_test.go:943` — The `call` and `mustCall` closures in `TestCatalogAndGardenTools` are identical to those in `TestRecordKeepingTools` (line 753). Across the file there are six `call` and two `mustCall` inline redefinitions. This copy-paste clutter makes tests harder to maintain and drift-prone. Extract them once as package-level helpers (e.g., `execTool(t, box, name, args)` and `execToolOK(t, box, name, args, into)`) and reuse them.
- `internal/agent/tools.go:927` — The `deletePlant` adapter wraps `domain.ErrPlantInUse` with `%w` and the same sentinel, layering the error. The comment says the sentinel text is “for a log line,” but wrapping the same sentinel rather than returning a plain string or a distinct error type is an unusual pattern that could confuse future maintainers about error identity. A plain `fmt.Errorf("...")` (without `%w`) or a dedicated agent-facing error would be cleaner, since the agent layer is intentionally translating service errors into model-readable prose anyway.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found.
My lens is error-handling and edge cases. I verified the following:
- **`createGarden`** passes raw `Name` to the service, but `gardenFromInput` trims and validates it (confirmed by reading `internal/service/gardens.go:227–230`); the test also checks whitespace-only names are rejected. Dimension defaults and `UnitPref` normalization are handled downstream or in the service.
- **`updateSeedLot`** correctly guards against no-op updates, parses the optional `PurchasedAt` through `parseDay` (which trims and validates), and uses pointer presence (`!= nil`) to drive `Set*` booleans for the patch. The direct pointer fields (`Quantity`, `Unit`, etc.) safely distinguish “don’t change” (nil) from “set to zero/empty” (non-nil pointer).
- **`deleteSeedLot`** simply proxies errors and returns a success payload; nothing swallowed.
- **`deletePlant`** uses `errors.Is` to detect the `ErrPlantInUse` sentinel and re-wraps it with model-oriented wording while preserving the sentinel for upstream matching. Other errors pass through unchanged. The underlying service checks both plantings and seed-lot references (confirmed by reading `internal/service/plants.go:258–277`).
- **`readyDate`** defensively handles nil/empty `plantedAt`, parse failure, and returns `nil` so undated or invalid-dated plantings are silently excluded from the range rather than panicking or producing garbage.
All unhappy paths I can reach are covered or delegated safely to the service layer.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
The call/mustCall closures were copied between TestRecordKeepingTools and
TestCatalogAndGardenTools; both now use a file-level toolCaller. The other
notes are left as they are: the 'nothing to change' guard enumerates the
args on purpose (it is the tool's own contract, next to the struct it
checks), and wrapping a sentinel with %w is how every readable refusal in
this package is built.
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit 97008f5a92 into main2026-08-23 06:12:26 +00:00
steve
deleted branch feat/agent-catalog-tools2026-08-23 06:12:26 +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.
Tier 2 of the agent-tool round (Tier 1 was #129). The catalog side of the record, plus the one describe field the model kept mis-computing.
What's new
update_seed_lot/delete_seed_lot— correct or drop a recorded purchase ("it was three packets, not two"). Quantity, unit, vendor, link, purchase date, packed-for year, germination rate, notes; the plant a lot is for stays fixed (the service never re-points a purchase). Dates go through the sameparseDaypath as every other dated argument, trimmed value stored.delete_plant— remove a duplicate from the user's catalog. The service already refuses while plantings (past seasons included) or a seed lot reference it; the tool turnsErrPlantInUseinto words the model can pass on, and tells it not to clear those references to get its way. Built-ins stay undeletable.create_garden— a new place (front yard, allotment) with the service's defaults; the prompt says a season plan is still acopy_garden, and that the conversation stays with the garden it started in.readyAroundon everyDescribeGroup— planting date + the plant's days to maturity, for the plops still in the ground, as one date orfirst…last. Live, "what can I pick this week?" got the arithmetic wrong; now it is a lookup. Absent when the catalog has no days for the plant.Tests
TestDescribeGroupSaysWhenReady(service): range across two planting dates, nothing for a plant without days, and a pulled plop excluded from the range even in the season view.TestCatalogAndGardenTools(agent): create_garden defaults and an imperial one; record → correct → delete a lot; delete_plant refused for a lot, refused for a pulled planting (with the "past seasons count" wording), allowed once nothing references it, refused for a built-in.DESIGN.md roadmap note and UI step labels updated.
🤖 Generated with Claude Code
- update_seed_lot / delete_seed_lot: correct or drop a recorded purchase ("it was three packets, not two"); the plant a lot is for stays fixed. - delete_plant: remove a duplicate from the user's catalog. The service already refuses while plantings (past seasons included) or a lot reference it; the tool turns that sentinel into words the model can pass on, and tells it not to clear those references to get its way. - create_garden: a new place, with the service's defaults; the prompt says a plan is still a copy_garden. - describe_garden groups carry readyAround — planting date plus days to maturity for the plops still in the ground — so "what can I pick this week?" is a lookup rather than arithmetic the model got wrong live. Co-Authored-By: Claude Fable 5 <[email protected]>🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 06:10:31Z
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.
@@ -837,0 +887,4 @@GerminationPct *float64 `json:"germinationPct" description:"optional germination rate, 0–100"`Notes *string `json:"notes" description:"optional replacement notes"`}) (any, error) {if args.Quantity == nil && args.Unit == nil && args.Vendor == nil && args.SourceURL == nil &&🟡 updateSeedLot 'nothing to change' guard hand-enumerates every field; must be kept in sync with the args struct when new fields are added
maintainability · flagged by 1 model
internal/agent/tools.go:890-892— The "nothing to change" guard inupdateSeedLothand-enumerates every nullable field (args.Quantity == nil && args.Unit == nil && …). This faithfully mirrors the established pattern inupdateGarden(tools.go:823-825), so it is consistent with the surrounding code, but it is a maintenance hazard: the next field added to theupdateSeedLotargs struct /SeedLotPatchmust also be appended to this boolean chain in lockstep, or a valid single-(new-)field…🪰 Gadfly · advisory
@@ -837,0 +924,4 @@if errors.Is(err, domain.ErrPlantInUse) {// The sentinel's text is for a log line; the model needs to know what// to tell the person, and what not to do about it.return nil, fmt.Errorf("%w: the plant is still used — by plantings (past seasons count) or a seed lot — so it stays; tell the user rather than removing those", domain.ErrPlantInUse)🟡 Wrapping the same sentinel error with %w is an unusual pattern
maintainability · flagged by 1 model
internal/agent/tools.go:927— ThedeletePlantadapter wrapsdomain.ErrPlantInUsewith%wand the same sentinel, layering the error. The comment says the sentinel text is “for a log line,” but wrapping the same sentinel rather than returning a plain string or a distinct error type is an unusual pattern that could confuse future maintainers about error identity. A plainfmt.Errorf("...")(without%w) or a dedicated agent-facing error would be cleaner, since the agent layer is intenti…🪰 Gadfly · advisory
@@ -934,0 +935,4 @@// TestCatalogAndGardenTools — the catalog side of the record: correct or delete// a seed lot, delete a duplicate plant (refused while anything references it,// in words the model can pass on), and start a new garden with sane defaults.func TestCatalogAndGardenTools(t *testing.T) {⚪ TestCatalogAndGardenTools conflates three unrelated tool scenarios; splitting would give more precise failure attribution
maintainability · flagged by 1 model
TestCatalogAndGardenToolsconflates three distinct tool areas into one ~100-line function (internal/agent/tools_test.go:938). The// ---comments acknowledge the length.create_gardenis independent of the seed-lot anddelete_plantscenarios; a failure in the garden-creation section suppresses all downstream signal. Splitting at the natural// ---seams (one test for garden creation, one for catalog lifecycle) would give more precise failure attribution on a red run. Minor in a…🪰 Gadfly · advisory
@@ -934,0 +940,4 @@svc, owner := newAgentTestService(t)box := NewToolbox(svc, owner, "2026-08-23")call := func(name string, args any) llm.ToolResult {🟠 Duplicated call/mustCall test helpers should be extracted
maintainability · flagged by 1 model
internal/agent/tools_test.go:943— ThecallandmustCallclosures inTestCatalogAndGardenToolsare identical to those inTestRecordKeepingTools(line 753). Across the file there are sixcalland twomustCallinline redefinitions. This copy-paste clutter makes tests harder to maintain and drift-prone. Extract them once as package-level helpers (e.g.,execTool(t, box, name, args)andexecToolOK(t, box, name, args, into)) and reuse them.🪰 Gadfly · advisory
@@ -934,0 +944,4 @@t.Helper()return box.Execute(ctx, llm.ToolCall{ID: "1", Name: name, Arguments: mustJSON(t, args)})}mustCall := func(name string, args any, into any) {🟡 mustCall closure copied verbatim from TestRecordKeepingTools — candidate for a file-level helper
maintainability · flagged by 1 model
mustCallclosure duplicated verbatim betweenTestRecordKeepingToolsandTestCatalogAndGardenTools(internal/agent/tools_test.go:947). The body is identical to the one introduced at line 757 — same signature, samet.Fatalfmessages, samejson.Unmarshalcall. Thecallclosure (capturingctxandbox) appears multiple times across the file and was never extracted, so this follows the existing style, butmustCallis a richer wrapper that a third test would copy again. Extrac…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 5 findings (0 with multi-model agreement)
5 single-model findings (lower confidence)
internal/agent/tools_test.go:943internal/agent/tools.go:890internal/agent/tools.go:927internal/agent/tools_test.go:947internal/agent/tools_test.go:938Per-model detail
claude-code/opus (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've traced each new agent tool to its service method and verified the authorization and validation controls.
VERDICT: No material issues found
The new tools are thin adapters over service methods that already enforce every control that matters for this lens. I verified each:
delete_plant→DeletePlant(plants.go:258): gated bywritablePlant(plants.go:219), which returnsErrForbiddenfor built-ins (OwnerID == nil) andErrNotFoundfor another user's plant. No cross-tenant deletion, built-ins protected. TheErrPlantInUserewrap in the tool only changes the message string, not authz.update_seed_lot/delete_seed_lot→UpdateSeedLot/DeleteSeedLot(seed_lots.go:194,229): both gated byownSeedLot(seed_lots.go:129), which masks any lot not owned by the actor asErrNotFound. Version-guarded on update. No IDOR.create_garden→CreateGarden(gardens.go:113): forcesg.OwnerID = actorID— the caller cannot set ownership.SourceURLon a seed lot (potential stored-XSS via ajavascript:link, since it renders as a clickable link): the update path runs throughfinalizeSeedLot→validSourceURL(seed_lots.go:304,332), which rejects any non-http(s)/hostless scheme. The vector is closed for the update tool too.purchasedAt) go throughparseDay, and the tool rejects free-text like"last spring"(test-covered) before it reaches the store.a.actor(the authenticated session actor) asactorID; there is no way for the model to pass an arbitrary actor. No SSRF (URLs are stored, never fetched server-side), no injection surface (parameterized store), no secret handling in this diff.Authorization lives entirely at the service seam exactly as the architecture intends, and the adapters add no bypass. Nothing in the security lens is materially wrong.
🎯 Correctness — No material issues found
I've verified all the correctness-critical paths. Let me summarize.
VERDICT: No material issues found
I reviewed this PR through the Correctness lens, verifying each numeric/formula path against the checked-out code.
What I checked and confirmed correct:
readyDatearithmetic (internal/service/ops.go:798) — re-derived the two test dates from first principles using Go'sAddDate(0,0,days)normalization:2026-05-01 + 60d→ day 61 of May → June 30 (2026-06-30) ✓2026-05-11 + 60d→ day 71 of May → July 10 (2026-07-10) ✓"2026-06-30…2026-07-10".dateLayoutis"2006-01-02"in bothserviceandagentpackages, so parse/format round-trips cleanly.Range min/max (
dateRangeOf,ops.go:828) — uses lexical string compare; valid for fixed-width ISO dates.readyDatealways emits a 10-charYYYY-MM-DD, so ordering is sound. Nils (undated / unparseable) are skipped, not treated as empty-string extremes.Pulled-plop exclusion (
ops.go:757-762) — the closure returnsnilforRemovedAt != nil, so the season view's range covers only live plops. Verified againstTestDescribeGroupSaysWhenReady: the pulled radish (2026-03-01, ready2026-04-30) is correctly dropped from the range in theyear=2026view. Guard is also correct:ReadyAroundonly computed whenplant.DaysToMaturity != nil, andomitemptyhides an all-nil range.createGardendefaults (tools.go→gardenFromInput,gardens.go:226) — the tool passes0for omittedwidthCm/heightCm/gridSizeCm;applyDefaults=truefills1000/1000(defaultGardenCM = 1000) and grid100, empty units →metric. Blank name →ErrInvalidInput."Imperial"is lowercased to"imperial"before the enum check. All match the test.updateSeedLotpatch mapping (tools.go→SeedLotPatch) — field-for-field consistent withapplySeedLotPatch/finalizeSeedLot. Nullable columns (PurchasedAt,PackedForYear,GerminationPct) correctly useSet*flags gated on!= nil; non-null columns (Quantity,Unit,Vendor,SourceURL,Notes) use pointer-presence.PurchasedAtroutes throughparseDay(normalizes/validatesYYYY-MM-DD) before storage. No pointer-aliasing bug in thepurchased := args.PurchasedAt; purchased = &onreassignment.deletePlant(tools.go) — service refuses onCountPlantingsForPlant > 0(past seasons included, since it counts all plantings) orCountSeedLotsForPlant > 0, both mapped toErrPlantInUse; built-ins refused earlier viawritablePlant→ErrForbidden(plants.go:224). Theerrors.Isremap only rewritesErrPlantInUse, passing other errors through unchanged.No correctness defects surfaced in my lane.
🧹 Code cleanliness & maintainability — No material issues found
I've verified the changes against the checked-out code. Findings below.
VERDICT: No material issues found
I reviewed this PR through the code cleanliness & maintainability lens and checked the new code against the surrounding patterns it's supposed to follow. It's clean and idiomatic.
What I verified:
readyDate(internal/service/ops.go:798) reuses the shareddateLayoutconst (internal/service/plantings.go:15) and the existingdateRangeOfhelper rather than reinventing either. TheReadyAroundclosure atops.go:757mirrors theRemovedAt: dateRangeOf(members, func…)line two lines above it — same idiom, so it reads consistently. SincereadyDateemits ISOYYYY-MM-DD, the string min/max indateRangeOfstays valid.createGarden(internal/agent/tools.go) delegates defaults to the service (gardenFromInput,gardens.go:230+):0 → defaultGardenCM/defaultGardenGridCM, blankUnitPref → metric. The tool's"default 1000"/"default 100"descriptions match the service, so the docs aren't lying.UnitPrefnormalization (ToLower+TrimSpace) is the same treatmentupdateGardengives it — consistent.updateSeedLotbuildsservice.SeedLotPatchrespecting the Set*-flag convention (SetPurchasedAt/SetPackedForYear/SetGerminationPct) exactly as the struct doc atseed_lots.go:53prescribes; plain*stringfields (Vendor/SourceURL/Notes) map straight through as nil=unchanged. The empty-args guard and%w: … domain.ErrInvalidInputrefusal mirrorupdateGarden's guard verbatim.deletePlant/deleteSeedLotare thin adapters; theerrors.Is(err, domain.ErrPlantInUse)translation is the only branch, and the newly-addederrorsimport is used.DeletePlant/DeleteSeedLot/ErrPlantInUseall exist in the service/domain.web/src/lib/agent.tshas aTOOL_LABELSentry for all four new tools — no tool ships without a label.No dead code, no copy-paste that should be shared, no confusing names, no leaky abstraction introduced.
⚡ Performance — No material issues found
I've verified the changed code against the checked-out state. Let me assess the one performance-relevant addition — the
ReadyAroundcomputation indescribeGroups.VERDICT: No material issues found
Reviewed through the Performance lens only.
What I checked:
readyAroundindescribeGroups(internal/service/ops.go:755-763) — This adds one moredateRangeOfpass over each group'smembers, andreadyDate(ops.go:798) callstime.Parse/AddDate/Formatonce per plop. I confirmed the work is strictlyO(total plops), bounded, and computed once perdescribe_gardencall — not inside any nested/quadratic loop.describeGroupsalready makes several linear passes overmembers(dateRange, theRemovedAtdateRangeOf, thePlants/Removedtally, and theEachbuild atops.go:764-775); this is one additional pass of the same order. The gatingif plant.DaysToMaturity != nil(ops.go:755) skips the parse work entirely for plants with no maturity data. Even on the previously-cited ~450-plop grid garden, a few hundredtime.Parsecalls per turn is microseconds against a model round-trip — a micro-cost, not a regression.No N+1 introduced —
readyDatereadspl.PlantedAt(already in memory) andplantByIDis the map already loaded by the caller; no per-plop store call is added.New agent tools (
internal/agent/tools.go:858-933) —createGarden,updateSeedLot,deleteSeedLot,deletePlantare thin single-call adapters over existing service methods. Each is a one-shot mutation, not a loop or hot path; no allocations or iteration of concern.update_seed_lotempty-patch guard (tools.go:889-892) — rejects a no-field call before doing any work, so the common "nothing to change" case does no I/O. Fine.Nothing in my lane rises to a reportable issue.
🧯 Error handling & edge cases — No material issues found
I've verified the new code paths against the checked-out source. Here's my review through the error-handling & edge-cases lens.
VERDICT: No material issues found
The unhappy paths this PR introduces are handled carefully, and I confirmed each by reading the surrounding code rather than the diff alone:
readyDate(internal/service/ops.go:798) — guardsnil/emptyplantedAtand atime.Parsefailure, returningnilin both cases (comment acknowledges the "shouldn't happen" path).AddDate(0,0,days)can't panic; string-ordered ISO output keepsdateRangeOf's min/max correct sincedaysis constant per group. Verified.ReadyAroundrange (ops.go:757) — the closure returnsnilfor pulled plops (RemovedAt != nil), so the season/year view correctly excludes a harvest that's behind us; when every member is pulled/undated the result is""andomitemptydrops it. Confirmed both the "now" andyearviews funnel through the singledescribeGroupscall (ops.go:692), so the behavior is uniform — the test's season-view assertion isn't exercising a separate, un-updated path.updateSeedLot(internal/agent/tools.go:874) — the all-nilguard rejects an empty patch with a keyboard-facing message;parseDayrejects prose dates before they reach the store;SetPurchasedAt/SetPackedForYear/SetGerminationPctsentinels are set correctly. DownstreamfinalizeSeedLotvalidates unit, quantity (finite,0..max), germination (0..100), URL scheme, and date — so bad values can't slip through the tool.deletePlant(tools.go:923) —errors.Iswrap isnil-safe (nil err skips both branches and returns the success map);ErrPlantInUsecovers both the planting and seed-lot cases the service checks (plants.go:266,274), and non-ErrPlantInUseerrors (e.g. built-in/unwritable viawritablePlant) propagate to theif err != nilreturn.createGarden(tools.go:862) — blank name and default dimensions/grid are enforced bygardenFromInput(in, true); the tool adds no path that bypasses that validation.No swallowed errors, missing cleanup, panics on bad input, or unhandled nil/empty/boundary cases in the diff's new code.
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
All my checks confirm clean. Let me write up the verdict.
No material issues found
The four new agent tools (
create_garden,update_seed_lot,delete_seed_lot,delete_plant) follow the established authz pattern: each service method calls a sentinel that verifies ownership and returnsErrNotFound(masking existence) for anything that isn't the caller's —ownSeedLotfor the lot ops,writablePlantfor catalog ops. No gaps.SourceURL(verified):UpdateSeedLotrunsfinalizeSeedLotafter applying the patch (seed_lots.go:200), andfinalizeSeedLotcallsvalidSourceURL(line 304). That function rejects anything that isn't an empty string or a validhttp(s)://host/...URL —javascript:and relative paths are explicitly blocked (the inline comment names the XSS reason). No path in the codebase fetches the stored URL server-side, so SSRF is not in scope.parseDayerror message (verified): The model-provided date string is formatted with%q(tools.go:590), which Go-escapes the value before embedding it in theErrInvalidInputmessage. The sentinel is stripped at the API layer; the%q-quoted value is what reaches the client — no injection surface.createGardenunit preference (verified): The service normalises the string to lowercase before a strict two-value check (domain.UnitMetric/domain.UnitImperial) and returnsErrInvalidInputon anything else (gardens.go:252-256). The agent wrapper callsstrings.ToLower(strings.TrimSpace(...))before passing it in, but the service-layer gate is the real guard.ErrPlantInUsewrapping (verified): The custom-text error stays in the agent/tool layer; the model sees the tool error text and relays it in natural language. If it somehow propagated to the HTTP handler, theErrPlantInUsecase there returns a hardcoded 409 message and ignores the wrapped detail — no unintended disclosure.🎯 Correctness — No material issues found
I've now verified all the key correctness claims. Let me compile the findings.
Verified:
dateRangeOfcorrectly skips nil returns from the callback (line 832:if d == nil || *d == "" { continue }), so pulled plops withpl.RemovedAt != nilreturning nil are safely excluded from thereadyAroundrange.dateLayout = "2006-01-02"is YYYY-MM-DD; string comparison (<,>) is lexicographically correct for ISO 8601 dates.readyDatearithmetic:time.AddDate(0, 0, 60)on "2026-05-01" → "2026-06-30" (30 days to end of May + 30 into June = 60); on "2026-05-11" → "2026-07-10" (20 to May 31 + 30 to June 30 + 10 into July = 60). Both match the test expectations exactly.SeedLotPatchhasSetPurchasedAt/PurchasedAt,SetPackedForYear/PackedForYear,SetGerminationPct/GerminationPct— all correctly set inupdateSeedLot. The local variablepurchasedis pre-parsed before theSet*flag is derived, soSetPurchasedAt: purchased != nilis semantically correct (only true when the caller actually supplied and validated a date).updateSeedLotcovers all eight optional fields.Verdict: No material issues found
The
readyAroundarithmetic is correct, nil propagation throughdateRangeOfworks as expected, and theSeedLotPatchfield mapping is complete and semantically accurate. No logic bugs in this lens.🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed. The
mustCallclosure at lines 757–768 inTestRecordKeepingToolsis byte-for-byte identical to the one at lines 947–958 inTestCatalogAndGardenTools(same signature, samet.Fatalfmessages, samejson.Unmarshalcall).TestCatalogAndGardenToolsruns from line 938 to ~line 1030 and covers three distinct tool areas (garden creation, seed-lot lifecycle, and plant deletion) inside a single function.Minor issues
mustCallclosure duplicated verbatim betweenTestRecordKeepingToolsandTestCatalogAndGardenTools(internal/agent/tools_test.go:947). The body is identical to the one introduced at line 757 — same signature, samet.Fatalfmessages, samejson.Unmarshalcall. Thecallclosure (capturingctxandbox) appears multiple times across the file and was never extracted, so this follows the existing style, butmustCallis a richer wrapper that a third test would copy again. Extracting it as a file-level helper —func mustToolCall(t *testing.T, exec func(string, any) llm.ToolResult, name string, args any, into any)— would make future tests cost less.TestCatalogAndGardenToolsconflates three distinct tool areas into one ~100-line function (internal/agent/tools_test.go:938). The// ---comments acknowledge the length.create_gardenis independent of the seed-lot anddelete_plantscenarios; a failure in the garden-creation section suppresses all downstream signal. Splitting at the natural// ---seams (one test for garden creation, one for catalog lifecycle) would give more precise failure attribution on a red run. Minor in a test file, but the function is already noticeably long for a single named thing.Neither finding blocks the merge; both are nits surfaced by the lens of code cleanliness.
⚡ Performance — No material issues found
No material issues found
The one noteworthy point is a minor redundancy in the
ReadyAroundcomputation, not a regression.readyDatecalled N times when ≤2 sufficeinternal/service/ops.go:755–763—dateRangeOf(members, …)invokes the closure once per plop inmembers. The closure callsreadyDate, which doestime.Parse+time.AddDate+time.Format. For a grid-filled bed the CLAUDE.md history mentions 450 plops reaching this path; that becomes 450 parse+format round-trips per group perdescribe_gardencall.Because
readyDateis monotone (PlantedAt + constant), the minimum and maximum ready dates correspond exactly to the minimum and maximum non-removedPlantedAtstrings — and ISO dates sort correctly as strings, so no parsing is needed to find them. The fix reduces to finding min/maxPlantedAtwith string comparisons, then callingreadyDateon at most those two values:Impact is measured in tenths of a millisecond for typical gardens (< 1 ms total for most
describe_gardencalls), so this is not blocking, but it is wasteful given the easy fix. Verified by readingops.go:755–808anddateRangeOf:828–846.🧯 Error handling & edge cases — No material issues found
I've now read all the relevant code paths. Here is the review.
No material issues found
All the new error paths in this PR handle unhappy cases correctly. Quick verification notes:
readyDate(ops.go:798): guardsplantedAt == niland empty string before parsing; silently returnsnilon parse failure rather than panicking. Correct.dateRangeOfforReadyAround(ops.go:757): when all group members are pulled (picker returnsnilfor all),firstandlaststay"",first == lastreturns"", and the field is omitted viaomitempty. Verified by readingdateRangeOfat line 828.updateSeedLotnothing-to-change guard (tools.go:890): cross-checked againstSeedLotPatchstruct — all eight tool-exposed fields (Quantity,Unit,Vendor,SourceURL,PurchasedAt,PackedForYear,GerminationPct,Notes) are covered.SKUandLotCodeexist in the patch but are deliberately unexposed. Guard is complete.createGardeninvalid inputs (tools.go:862): blank/whitespace name, negative/NaN/Inf dimensions, and unknown unit strings all reachgardenFromInputwhich returnsErrInvalidInput— confirmed atgardens.go:228,248,256. Test exercises the blank-name case.deletePlantdual-cause ErrPlantInUse (tools.go:924): the service returns the same sentinel for "has plantings" and "has seed lots" (plants.go:267,274); the tool's replacement message names both cases explicitly, so the model gets actionable text in either path.updateSeedLot:finalizeSeedLot(seed_lots.go:295) rejectsquantity < 0and non-finite values; germination likewise validated at 316–320. No bypass path through the tool.deleteSeedLotwith no version: consistent with the stated design (seed lots outside undo history);ownSeedLotstill enforces ownership before deletion.glm-5.2: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
Through the security lens, all four new agent tools (
create_garden,update_seed_lot,delete_seed_lot,delete_plant) are thin adapters over service-layer methods that already enforce authorization and input validation:delete_plant→service.DeletePlant→writablePlant(plants.go:219) rejects built-ins (OwnerID == nil→ErrForbidden) and other users' plants (ErrNotFound), and refuses plants referenced by plantings or seed lots. Verified.update_seed_lot/delete_seed_lot→ownSeedLot(seed_lots.go:134) checksl.OwnerID == actorIDelseErrNotFound;UpdateSeedLotrunsfinalizeSeedLotwhich validates unit, quantity range, text lengths,validSourceURL(http/https only, with host — blockingjavascript:/schemeless URLs atseed_lots.go:332), date format, packed-year range, and germination-pct bounds. The adapter routespurchasedAtthroughparseDay(YYYY-MM-DD) before the service re-validates. Verified.create_garden→service.CreateGardenhard-setsOwnerID = actorID(not from input) and runsgardenFromInput. Verified atgardens.go:113-125.No tainted-input path reaches a sink unvalidated:
SourceURLis scheme-checked before being stored/rendered, dated strings go throughparseDay+finalizeSeedLot, numeric/enum fields are bounds-checked, and ownership is enforced server-side on every mutation. ThereadyDatehelper inops.goonly parses already-stored, previously-validated planting dates — no untrusted input, no injection surface.🎯 Correctness — No material issues found
I have verified the core logic. Let me re-derive the
readyAroundarithmetic from the test to confirm correctness, then write my final review.Test: radish planted 2026-05-01 (60 days → 2026-06-30) and 2026-05-11 (60 days → 2026-07-10). Expected range "2026-06-30…2026-07-10".
time.Parse("2006-01-02", "2026-05-01").AddDate(0,0,60)= 2026-06-30 ✓;2026-05-11+60 = 2026-07-10 ✓. Pulled plop (planted 2026-03-01) returns nil fromreadyDateonly when... wait,readyDatedoes NOT checkRemovedAt— that check is in thedateRangeOfcallback (if pl.RemovedAt != nil return nil). So pulled plop contributes nothing. Correct. Range stays 06-30…07-10. ✓Verdict: No material issues found
readyAroundarithmetic (ops.go:755-763, 798-808) — Verified correct.readyDateparsesplantedAtwith the samedateLayout("2006-01-02") used everywhere else in the store, addsdaysviat.AddDate(0,0,days), and reformats. Pulled plops are excluded by theRemovedAt != nilguard in thedateRangeOfcallback (not inreadyDateitself, which is the right place —dateRangeOfskips nil returns). When all members are undated/pulled,dateRangeOfreturns"", and theomitemptykeeps the field absent. Matches the test's expected2026-06-30…2026-07-10. No off-by-one, no unit confusion.updateSeedLotdate handling (tools.go:885-891) — Verified.purchasedAtgoes throughparseDay(trims + validatesdateLayout), andSetPurchasedAt: purchased != nilcorrectly distinguishes "clear it" (pass empty → parseDay trims to ""... actually note: passing an empty string would makepurchased != niltrue, setSetPurchasedAttrue, andPurchasedAtpoints to "" — that's a clear-to-empty path, butfinalizeSeedLot'svalidDatePtraccepts nil/valid; an empty string would fail validation). Edge: the tool can't explicitly clearpurchasedAtto NULL since an empty string failsvalidDatePtr. Minor, and not new incorrect behavior vs. existing semantics. Not blocking.createGarden(tools.go:862-873) — Verified againstgardenFromInput(in, true): zero WidthCM/HeightCM →defaultGardenCM(1000); blankUnitPref(lowercased/trimmed to "") →UnitMetric; name blank →ErrInvalidInput. Thestrings.ToLowernormalizes "Imperial" → "imperial", matchingdomain.UnitImperial. Correct.deletePlant(tools.go:927-933) — Verifieddomain.ErrPlantInUseis returned byDeletePlantwhen plantings or seed lots reference the plant; theerrors.Iswrap preserves the sentinel while adding model-facing text. Built-ins returnErrForbiddenfromwritablePlant(OwnerID nil), passed through unchanged. Correct.deleteSeedLot— Delegates toDeleteSeedLot, which SET NULLs plantings. Matches description. No correctness issue.No correctness defects found within my lens.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/agent/tools.go:890-892— The "nothing to change" guard inupdateSeedLothand-enumerates every nullable field (args.Quantity == nil && args.Unit == nil && …). This faithfully mirrors the established pattern inupdateGarden(tools.go:823-825), so it is consistent with the surrounding code, but it is a maintenance hazard: the next field added to theupdateSeedLotargs struct /SeedLotPatchmust also be appended to this boolean chain in lockstep, or a valid single-(new-)field update will be rejected as "say what to change." Low severity because the pattern is established and the field set is small, but worth a comment or a helper if more patch tools appear. Verified by reading both guards.internal/agent/tools.go:894-901— Thepurchased := args.PurchasedAt/purchased = &ondance is slightly more indirect than necessary.parseDay(tools.go:587) already returns the trimmed/parsed string, andSetPurchasedAt: purchased != nilends up equivalent toargs.PurchasedAt != nil(whenargs.PurchasedAtis non-nil,purchasedis always reassigned to a non-nil&on). The local variable earns its keep only by carrying the trimmed value thatparseDayalready returns, so theon, err := parseDay(*args.PurchasedAt); … SetPlantedAt, patch.PlantedAt = true, &onform used inupdatePlanting(tools.go:551-556) is a touch cleaner. Minor readability nit only; not actionable.internal/service/ops.go:798-808—readyDatere-implementstime.Parse(dateLayout, …)with a nil/empty guard, duplicating the parse already done byvalidDatePtrinplantings.go:396-402. They serve different purposes (one validates, one computes), so this isn't a real duplication to factor out now, but the date-parsing-with-guard idiom is now in two places; if a third appears, a smallparseDayPtr(*string) (time.Time, bool)helper would be worth it. Not actionable now.No dead code, no naming problems, no broken abstractions. The new
deleteSeedLot/deletePlantreturn shapes (map[string]any{"deleted": id}) match the existingdeleteJournalEntry/deleteObjecttools, andReadyAroundcleanly reusesdateRangeOfwith a picker that returns nil for pulled plops. The added tool prompt strings andTOOL_LABELSentries follow the established conventions.⚡ Performance — No material issues found
No material issues found.
I reviewed the change through the performance lens, verifying the hot path (
describeGroupsininternal/service/ops.go) and the new agent tool adapters.readyDate(ops.go:798) does atime.Parseper plop, but only for groups whose plant hasDaysToMaturity, and only insidedescribeGroups— already an in-memory aggregation over plops fetched in oneGardenFullquery. No new queries, no N+1. The addeddateRangeOfpass overmembersis one extra linear scan alongside the existingdateRange/RemovedAt/count/Eachscans; all are O(plops-in-group) and dwarfed by the DB round-trip that produced them. Not material.createGarden,updateSeedLot,deleteSeedLot,deletePlant) each issue a single service call; no loops, no per-record lookups, no unbounded growth.updateSeedLot's "nothing to change" guard short-circuits before any parse or service call.No efficiency regressions, unbounded growth, missing limits, or hot-path blocking introduced by this PR.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
The error-handling and edge-case paths in this diff are sound. I verified the key surfaces:
readyDate(internal/service/ops.go:798): handlesnil/emptyplantedAt(returns nil), swallows parse errors (returns nil rather than propagating), andAddDate(0,0,days)cannot overflow/panic.DaysToMaturityis guaranteed 1..3650 at write time (finalizePlantatplants.go:328), so no zero/negative reaches it.dateRangeOfforReadyAround(ops.go:757): plops withRemovedAt != nilor undated plops return nil from the picker and are skipped, so an all-pulled group yields""(theomitemptythen drops the field) rather than a malformed range. String comparison of ISO dates is chronologically correct.updateSeedLot(tools.go:889): rejects an all-nil patch withErrInvalidInput; routespurchasedAtthroughparseDay(which trims and rejects non-YYYY-MM-DD, verified attools.go:587);LotID == 0(omitted) resolves toErrNotFoundviaownSeedLot, not a panic. ServiceUpdateSeedLotpropagates both store andfinalizeSeedLoterrors.deleteSeedLot/deletePlant(tools.go:911/tools.go:919): errors are returned, not swallowed; theErrPlantInUsewrap uses%wso the sentinel surviveserrors.Is, while adding model-facing text.createGarden(tools.go:866):UnitPrefis lowercased/trimmed; empty falls through to the service default (metric) on create; garbage units are rejected bygardenFromInput;WidthCM/HeightCM/GridSizeCMof 0 default, negatives rejected byvalidDimensionCM. No zero-value panic path.One trivial note (not blocking)
updateSeedLotexposes no way to clearpurchasedAtto null. Passing"purchasedAt": ""fails atparseDay(empty is not a validYYYY-MM-DD), andSetPurchasedAtis only ever true when a parseable date was supplied — so unlikesourceUrl(whose description says "empty clears it" and which the patch trims to""), the purchase date is set-or-leave-unchanged only. This is consistent with the field's description (which promises no "clear" semantics), so it reads as a deliberate limitation rather than a bug, but a gardener who says "forget the purchase date, drop it" has no tool path to do it.internal/agent/tools.go:885/:904. Confidence: low that it's unintended.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
create_garden,update_seed_lot,delete_seed_lot,delete_plant) delegate authorization to the service layer (ownSeedLot,writablePlant,CreateGardenwithOwnerID = actorID), so there are no authn/authz gaps.update_seed_lotdates are passed through the existingparseDayvalidator before reaching the service, and the service validates all patch fields (quantity bounds, unit whitelist, URL scheme/host, date format, germination percentage range).SourceURLis scrubbed in the service viavalidSourceURL, which rejects anything other thanhttp://orhttps://with a non-empty host, preventingjavascript:and other malicious schemes from being stored as clickable vendor links.delete_plantcorrectly wrapsdomain.ErrPlantInUsewithout bypassing the ownership/built-in checks that happen first in the service; no information leakage beyond the already-sentinel-protected fact that the plant is referenced.readyDateoperates only on already-validated store data (plantedAtwas validated on write,daysToMaturityis an int from the catalog), so there is no exposure to unfiltered user input.🎯 Correctness — No material issues found
No material issues found
I examined the core correctness of the new logic through the diff and verified key declarations in the checked-out code:
readyDatearithmetic (internal/service/ops.go:798–807):time.Parse+AddDate(0, 0, days)+Formatcorrectly computes planting-date + days-to-maturity. The test expectations are confirmed accurate:2026-05-01 + 60 days = 2026-06-30(May has 31 days: 30 days remaining in May + 30 days into June)2026-05-11 + 60 days = 2026-07-10(20 days remaining in May + 30 days in June + 10 days into July)ReadyAroundexclusion of pulled plops (ops.go:757–762): The callback returnsnilwhenpl.RemovedAt != nil, sodateRangeOfskips them. The season-view test confirms the pulled radish (2026-03-01) does not affect the range even though it appears inmembers.dateRangeOfempty-result handling (ops.go:828–846): When all callbacks returnnil(no live dated plops),first == last == ""and it returns"". Coupled withomitemptyonReadyAround, the field is correctly absent from JSON.update_seed_lotfield guard (tools.go:890–893): The nil-check for all optional fields prevents no-op updates and returnsErrInvalidInput.delete_plantsentinel wrapping (tools.go:923–928):errors.Is(err, domain.ErrPlantInUse)correctly matches the sentinel returned byDeletePlant, and the re-wrapped error is human-readable for the model while preserving the original cause chain.No logic bugs, incorrect formulas, or semantic mis-computations were found.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/agent/tools_test.go:943— ThecallandmustCallclosures inTestCatalogAndGardenToolsare identical to those inTestRecordKeepingTools(line 753). Across the file there are sixcalland twomustCallinline redefinitions. This copy-paste clutter makes tests harder to maintain and drift-prone. Extract them once as package-level helpers (e.g.,execTool(t, box, name, args)andexecToolOK(t, box, name, args, into)) and reuse them.internal/agent/tools.go:927— ThedeletePlantadapter wrapsdomain.ErrPlantInUsewith%wand the same sentinel, layering the error. The comment says the sentinel text is “for a log line,” but wrapping the same sentinel rather than returning a plain string or a distinct error type is an unusual pattern that could confuse future maintainers about error identity. A plainfmt.Errorf("...")(without%w) or a dedicated agent-facing error would be cleaner, since the agent layer is intentionally translating service errors into model-readable prose anyway.⚡ Performance — No material issues found
No material issues found.
🧯 Error handling & edge cases — No material issues found
No material issues found.
My lens is error-handling and edge cases. I verified the following:
createGardenpasses rawNameto the service, butgardenFromInputtrims and validates it (confirmed by readinginternal/service/gardens.go:227–230); the test also checks whitespace-only names are rejected. Dimension defaults andUnitPrefnormalization are handled downstream or in the service.updateSeedLotcorrectly guards against no-op updates, parses the optionalPurchasedAtthroughparseDay(which trims and validates), and uses pointer presence (!= nil) to driveSet*booleans for the patch. The direct pointer fields (Quantity,Unit, etc.) safely distinguish “don’t change” (nil) from “set to zero/empty” (non-nil pointer).deleteSeedLotsimply proxies errors and returns a success payload; nothing swallowed.deletePlantuseserrors.Isto detect theErrPlantInUsesentinel and re-wraps it with model-oriented wording while preserving the sentinel for upstream matching. Other errors pass through unchanged. The underlying service checks both plantings and seed-lot references (confirmed by readinginternal/service/plants.go:258–277).readyDatedefensively handles nil/emptyplantedAt, parse failure, and returnsnilso undated or invalid-dated plantings are silently excluded from the range rather than panicking or producing garbage.All unhappy paths I can reach are covered or delegated safely to the service layer.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.