Agent: catalog and garden tools, and a ready date on every describe group #130

Merged
steve merged 2 commits from feat/agent-catalog-tools into main 2026-08-23 06:12:26 +00:00
Owner

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

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)
steve added 1 commit 2026-08-23 06:05:49 +00:00
Agent: catalog and garden tools, and a ready date on every describe group
Build image / build-and-push (push) Successful in 11s
Gadfly review (reusable) / review (pull_request) Successful in 4m42s
Adversarial Review (Gadfly) / review (pull_request) Successful in 4m42s
b4c8007977
- 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 — 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

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>
gitea-actions bot reviewed 2026-08-23 06:10:32 +00:00
gitea-actions bot left a comment

🪰 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>
@@ -837,0 +887,4 @@
GerminationPct *float64 `json:"germinationPct" description:"optional germination rate, 0100"`
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 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>
@@ -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 — 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>
@@ -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

  • 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>
@@ -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 — 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>
@@ -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

  • 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>

🪰 Gadfly review — consensus across 4 models

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 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_plantDeletePlant (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_lotUpdateSeedLot/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_gardenCreateGarden (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 finalizeSeedLotvalidSourceURL (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.gogardenFromInput, 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.goSeedLotPatch) — 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 writablePlantErrForbidden (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 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 introducedreadyDate 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.

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 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–763dateRangeOf(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:

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.

🧯 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.
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_plantservice.DeletePlantwritablePlant (plants.go:219) rejects built-ins (OwnerID == nilErrForbidden) and other users' plants (ErrNotFound), and refuses plants referenced by plantings or seed lots. Verified.
  • update_seed_lot/delete_seed_lotownSeedLot (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_gardenservice.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-808readyDate 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 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.

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

  • 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>
steve added 1 commit 2026-08-23 06:11:28 +00:00
Address #130 review: one toolCaller helper for the tool tests
Build image / build-and-push (push) Successful in 7s
c9076e84c4
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 main 2026-08-23 06:12:26 +00:00
steve deleted branch feat/agent-catalog-tools 2026-08-23 06:12:26 +00:00
Sign in to join this conversation.