Closes the agent-toolbox gaps from #85 item 3. The toolbox could create and move objects but not delete or resize them, write the journal but not read it, clear a whole bed but not pull one plant, and report seed-remaining but not record a purchase.
Each new tool is a thin adapter over the same service method the REST API uses, so it inherits pansy's permission checks unchanged (the whole point of the service seam):
Tool
Service method
Gap it closes
read_journal
ListJournal
the write-but-not-read asymmetry — "what did I note about the west bed?" (issue's "most user-visible")
update_object
UpdateObject
resize / rotate / rename / plantable — "make that bed 60cm wider"
delete_object
DeleteObject
the create-but-not-delete asymmetry — "remove the old grow bag"
remove_planting
UpdatePlanting
pull ONE plop instead of clearing the whole bed — "pull the basil out of the corner"
list_seed_lots
ListSeedLots
the detail behind find_plant's "remaining"
record_seed_lot
CreateSeedLot
record a purchase — "I bought two packets of Cherokee Purple"
Supporting change
To address a single plop, the agent needs its id + version — so DescribePlanting now carries both, exactly the way DescribeObject.Version already lets it edit an object. remove_plantingsoft-removes (removed_at = today), mirroring clear_object, so the plant stays in planting history and the change is undoable.
An undo/revert_change_set tool needs a way to list recent change sets to get a changeSetId — a larger addition (a history-listing tool). Left for a follow-up rather than shipped half-usable.
Tests
TestCorrectiveTools drives the new tools through the tool layer the way a model would: resize an object, place then single-plop-remove, add then read a journal entry, record then list a seed lot, and delete. Full GOWORK=off go test ./... green; gofmt clean.
Closes the agent-toolbox gaps from #85 item 3. The toolbox could create and move objects but not delete or resize them, write the journal but not read it, clear a whole bed but not pull one plant, and report seed-remaining but not record a purchase.
Each new tool is a thin adapter over the **same** service method the REST API uses, so it inherits pansy's permission checks unchanged (the whole point of the service seam):
| Tool | Service method | Gap it closes |
|---|---|---|
| `read_journal` | `ListJournal` | the write-but-not-read asymmetry — *"what did I note about the west bed?"* (issue's "most user-visible") |
| `update_object` | `UpdateObject` | resize / rotate / rename / plantable — *"make that bed 60cm wider"* |
| `delete_object` | `DeleteObject` | the create-but-not-delete asymmetry — *"remove the old grow bag"* |
| `remove_planting` | `UpdatePlanting` | pull ONE plop instead of clearing the whole bed — *"pull the basil out of the corner"* |
| `list_seed_lots` | `ListSeedLots` | the detail behind find_plant's "remaining" |
| `record_seed_lot` | `CreateSeedLot` | record a purchase — *"I bought two packets of Cherokee Purple"* |
### Supporting change
To address a single plop, the agent needs its id + version — so `DescribePlanting` now carries both, exactly the way `DescribeObject.Version` already lets it edit an object. `remove_planting` **soft-removes** (removed_at = today), mirroring `clear_object`, so the plant stays in planting history and the change is undoable.
### Deferred (noted on #85)
An undo/`revert_change_set` tool needs a way to *list* recent change sets to get a `changeSetId` — a larger addition (a history-listing tool). Left for a follow-up rather than shipped half-usable.
### Tests
`TestCorrectiveTools` drives the new tools through the tool layer the way a model would: resize an object, place then single-plop-remove, add then read a journal entry, record then list a seed lot, and delete. Full `GOWORK=off go test ./...` green; `gofmt` clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The toolbox could create and move but not delete or resize; write the
journal but not read it; clear a whole bed but not pull one plant; report
seed remaining but not record a purchase. Close those gaps with thin
adapters over the SAME service methods the REST API uses, so they inherit
the permission checks unchanged:
read_journal → ListJournal (the write/read asymmetry, most visible)
update_object → UpdateObject (resize / rotate / rename / plantable)
delete_object → DeleteObject (counterpart to create_object)
remove_planting → UpdatePlanting (soft-remove ONE plop, like clear does)
list_seed_lots → ListSeedLots
record_seed_lot → CreateSeedLot (record a purchase; "I bought 2 packets")
To address a single plop the agent needs its id + version, so
DescribePlanting now carries both — the same way DescribeObject.Version
already lets it edit an object. remove_planting soft-removes (removed_at =
today), mirroring clear_object, so the plant stays in planting history and
the change is undoable.
Deferred deliberately: an undo/revert tool needs a way to list recent
change sets to get a changeSetId, which is a larger addition; noted on the
issue for a follow-up.
Tested through the tool layer (TestCorrectiveTools): resize, single-plop
removal, journal read-back, seed-lot record+list, and delete.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
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
5/5 reviewers finished · updated 2026-07-23 01:34:12Z
#### `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** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ⚠️ **maintainability** — could not complete
- ✅ **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 — 7 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** — 7 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
⚪update_object sets absolute dimensions; a model conflating 'make 60cm wider' with 'set widthCm=60' passes finalizeObject's [1cm,100m] check and silently shrinks the bed
correctness · flagged by 1 model
internal/agent/tools.go:74 — update_object description is a plausible-magic-number trap.update_object sets absolute dimensions while move_object is separate. A model conflating "make 60cm wider" with "set widthCm=60" gets a 60cm bed instead of a 560cm one, and finalizeObject only rejects dimensions outside [1cm, 100m], so 60cm passes silently. The description does warn with the correct procedure (read width, add 60, pass sum), so this is a prompt-design risk, not a code bug;…
🪰 Gadfly · advisory
⚪ **update_object sets absolute dimensions; a model conflating 'make 60cm wider' with 'set widthCm=60' passes finalizeObject's [1cm,100m] check and silently shrinks the bed**
_correctness · flagged by 1 model_
- **`internal/agent/tools.go:74` — `update_object` description is a plausible-magic-number trap.** `update_object` sets *absolute* dimensions while `move_object` is separate. A model conflating "make 60cm wider" with "set widthCm=60" gets a 60cm bed instead of a 560cm one, and `finalizeObject` only rejects dimensions outside `[1cm, 100m]`, so 60cm passes silently. The description does warn with the correct procedure (read width, add 60, pass sum), so this is a prompt-design risk, not a code bug;…
<sub>🪰 Gadfly · advisory</sub>
🟠read_journal exposes hasMore without offset parameter, making pagination impossible
correctness, maintainability · flagged by 3 models
internal/agent/tools.go:222 — The read_journal tool hardcodes Limit: 50 and returns hasMore to the LLM, but does not expose the offset parameter that the underlying service.JournalQuery supports. Because the tool adapter omits offset, an LLM that sees hasMore: true has no way to request the next page, making the pagination signal functionally useless. The tool should either accept an offset argument so the LLM can page, or not return hasMore at all.
🪰 Gadfly · advisory
🟠 **read_journal exposes hasMore without offset parameter, making pagination impossible**
_correctness, maintainability · flagged by 3 models_
- `internal/agent/tools.go:222` — The `read_journal` tool hardcodes `Limit: 50` and returns `hasMore` to the LLM, but does not expose the `offset` parameter that the underlying `service.JournalQuery` supports. Because the tool adapter omits `offset`, an LLM that sees `hasMore: true` has no way to request the next page, making the pagination signal functionally useless. The tool should either accept an `offset` argument so the LLM can page, or not return `hasMore` at all.
<sub>🪰 Gadfly · advisory</sub>
🟠read_journal returns hasMore but exposes no pagination (offset/cursor), so journals over 50 entries are only partially readable via the agent
error-handling · flagged by 1 model
🪰 Gadfly · advisory
🟠 **read_journal returns hasMore but exposes no pagination (offset/cursor), so journals over 50 entries are only partially readable via the agent**
_error-handling · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🟡update_object overlaps move_object — two partial-patch tools over the same UpdateObject service method
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🟡 **update_object overlaps move_object — two partial-patch tools over the same UpdateObject service method**
_maintainability · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🟠remove_planting stamps removed_at from wall-clock time.Now() instead of the service's injectable s.now(), diverging from the clear_object path it claims to mirror
correctness, error-handling, maintainability · flagged by 3 models
internal/agent/tools.go:266 — removePlanting computes today := time.Now().UTC().Format("2006-01-02") from the wall clock, but every other date-stamping path in the service uses the injectable clock (s.now().UTC().Format(dateLayout)): CreatePlanting at internal/service/plantings.go:104, ClearObjectPlantings's soft-remove path at internal/service/ops.go:413, and the fill path at internal/service/ops.go:238. now is an injectable func() time.Time on Service (`internal/s…
🪰 Gadfly · advisory
🟠 **remove_planting stamps removed_at from wall-clock time.Now() instead of the service's injectable s.now(), diverging from the clear_object path it claims to mirror**
_correctness, error-handling, maintainability · flagged by 3 models_
- `internal/agent/tools.go:266` — `removePlanting` computes `today := time.Now().UTC().Format("2006-01-02")` from the **wall clock**, but every other date-stamping path in the service uses the injectable clock (`s.now().UTC().Format(dateLayout)`): `CreatePlanting` at `internal/service/plantings.go:104`, `ClearObjectPlantings`'s soft-remove path at `internal/service/ops.go:413`, and the fill path at `internal/service/ops.go:238`. `now` is an injectable `func() time.Time` on `Service` (`internal/s…
<sub>🪰 Gadfly · advisory</sub>
🟡list_seed_lots returns a bare array while read_journal wraps in {entries,hasMore}; inconsistent tool return shapes in the same PR
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🟡 **list_seed_lots returns a bare array while read_journal wraps in {entries,hasMore}; inconsistent tool return shapes in the same PR**
_maintainability · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
⚪TestCorrectiveTools bundles five unrelated tool checks into one fat test; a t.Fatalf in any step skips the rest
maintainability · flagged by 1 model
internal/agent/tools_test.go:354 — TestCorrectiveTools is one ~125-line function covering five unrelated tools. It's readable thanks to the call/describe helpers, and the per-step comments are good, but it conflates resize, single-plop-remove, journal read, seed-lot record/list, and delete into a single test. If one assertion fails early, the rest are skipped (t.Fatalf). Splitting into subtests (t.Run) would localize failures and keep each gap's coverage independent. Trivial, n…
🪰 Gadfly · advisory
⚪ **TestCorrectiveTools bundles five unrelated tool checks into one fat test; a t.Fatalf in any step skips the rest**
_maintainability · flagged by 1 model_
- **`internal/agent/tools_test.go:354` — `TestCorrectiveTools` is one ~125-line function covering five unrelated tools.** It's readable thanks to the `call`/`describe` helpers, and the per-step comments are good, but it conflates resize, single-plop-remove, journal read, seed-lot record/list, and delete into a single test. If one assertion fails early, the rest are skipped (`t.Fatalf`). Splitting into subtests (`t.Run`) would localize failures and keep each gap's coverage independent. Trivial, n…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Minor issues · 7 findings (2 with multi-model agreement)
Finding
Where
Models
Lens
🟠
read_journal exposes hasMore without offset parameter, making pagination impossible
internal/agent/tools.go:222
3/5
correctness, maintainability
🟠
remove_planting stamps removed_at from wall-clock time.Now() instead of the service's injectable s.now(), diverging from the clear_object path it claims to mirror
internal/agent/tools.go:266
3/5
correctness, error-handling, maintainability
5 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
read_journal returns hasMore but exposes no pagination (offset/cursor), so journals over 50 entries are only partially readable via the agent
internal/agent/tools.go:229
glm-5.2:cloud
error-handling
🟡
update_object overlaps move_object — two partial-patch tools over the same UpdateObject service method
internal/agent/tools.go:245
opencode/glm-5.2:cloud
maintainability
🟡
list_seed_lots returns a bare array while read_journal wraps in {entries,hasMore}; inconsistent tool return shapes in the same PR
internal/agent/tools.go:274
opencode/glm-5.2:cloud
maintainability
⚪
update_object sets absolute dimensions; a model conflating 'make 60cm wider' with 'set widthCm=60' passes finalizeObject's [1cm,100m] check and silently shrinks the bed
internal/agent/tools.go:74
opencode/glm-5.2:cloud
correctness
⚪
TestCorrectiveTools bundles five unrelated tool checks into one fat test; a t.Fatalf in any step skips the rest
internal/agent/tools_test.go:354
glm-5.2:cloud
maintainability
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
All new tool handlers use a.actor/a.svc, both fixed at toolbox construction — no user-controllable identity/authorization fields exist in any of the new tool argument structs, so an LLM cannot impersonate another user or garden actor through these tools.
Review
VERDICT: No material issues found
Walked each new tool (read_journal, update_object, delete_object, remove_planting, list_seed_lots, record_seed_lot) down to its service method and confirmed, by reading the actual code (not just the diff):
Every new adapter is a thin pass-through using the fixed a.actor/a.svc — no argument struct carries an actor/owner/user id the model could spoof (internal/agent/tools.go:19,106).
UpdateObject/DeleteObject route through objectForRole(..., roleEditor) (internal/service/objects.go:148-198), which enforces the garden-role check and masks no-access as ErrNotFound per the project's stated convention.
UpdatePlanting (used by remove_planting) loads the planting then re-derives its garden role via objectForRole (internal/service/plantings.go:125-134) — an attacker can't remove a plop in a garden they can't edit just by guessing a plantingId.
ListJournal/journal SQL is properly parameterized and always scoped WHERE j.garden_id = ? in addition to the optional object_id filter (internal/store/journal.go:78-104), so passing an objectId belonging to a different garden than gardenId returns nothing rather than leaking cross-garden entries — no authz bypass via mismatched IDs.
CreateSeedLot/ListSeedLots are properly owner-scoped (ownSeedLot, visiblePlant in internal/service/seed_lots.go:77-182) — a lot can only be created against a plant the actor can see (built-in or their own), and lots are always queried by ownerID.
record_seed_lot's SourceURL goes through the existing validSourceURL (absolute http/https, length-capped) validation in finalizeSeedLot (internal/service/seed_lots.go:291-336) — same path plants.go already uses; it's stored as an opaque string and never fetched server-side, so no SSRF vector.
The added DescribePlanting.ID/.Version fields (internal/service/ops.go) only surface within a DescribeGarden payload that's already gated by roleViewer on the whole garden — no new exposure.
Nothing here weakens the existing service-seam permission model; the PR's stated design goal (thin adapters over the same service methods) holds up under inspection.
🎯 Correctness — No material issues found
This is fine — permission enforcement happens inside CreateSeedLot/ListSeedLots via the service seam, as documented, so the thin adapters correctly inherit it.
I've verified all new service-method calls (ListJournal, UpdateObject, DeleteObject, UpdatePlanting, ListSeedLots, CreateSeedLot) against their actual signatures, confirmed the JournalQuery/ObjectPatch/PlantingPatch/SeedLotInput field names match, confirmed the date format used in removePlanting ("2006-01-02") matches dateLayout used throughout internal/service, and confirmed the test's tool-call argument names match the adapter struct tags. No correctness defects found.
VERDICT: No material issues found
The diff is a set of thin, mechanical adapters over existing, already-correct service methods (ListJournal, UpdateObject, DeleteObject, UpdatePlanting, ListSeedLots, CreateSeedLot), plus two new read-only fields (ID, Version) added to DescribePlanting. I checked each adapter's field names and types against the real service structs (JournalQuery, ObjectPatch, PlantingPatch, SeedLotInput) and they line up exactly. The one thing I looked at closely — removePlanting computing today := time.Now().UTC().Format("2006-01-02") instead of routing through the service's injectable s.now() clock (used everywhere else for "today", e.g. ClearObject at internal/service/ops.go:413) — is a minor style/testability inconsistency, not a functional bug: s.now() isn't overridden in production and isn't accessible from the agent package anyway, and the REST API (internal/api/plantings.go:70) already requires the caller to supply removedAt explicitly for single-planting updates, which is exactly the pattern this tool follows. Not reporting it as a correctness finding.
🧹 Code cleanliness & maintainability — Minor issues
Line number and all cited service-layer facts check out against the actual code: s.now() is injectable (service.go:40), dateLayout is defined once (plantings.go:14) and consistently reused in ClearObject (ops.go:413), FillClump (ops.go:238), CreatePlanting (plantings.go:104), and journal writes (journal.go:121). removePlanting in internal/agent/tools.go:266 is confirmed as the only call site that reimplements this with time.Now() and a bare "2006-01-02" literal instead of going through the service.
VERDICT: Minor issues
internal/agent/tools.go:266 — removePlanting computes "today" with time.Now().UTC().Format("2006-01-02") directly in the agent adapter, rather than delegating to the service layer the way every other soft-removal path does. The service package has an injectable now func() time.Time field (internal/service/service.go:40, "injectable so tests can advance time") and a shared dateLayout = "2006-01-02" const (internal/service/plantings.go:14), used consistently in ClearObject (internal/service/ops.go:413), CreatePlanting (internal/service/plantings.go:104), FillClump (internal/service/ops.go:238), and journal entries (internal/service/journal.go:121) — always as s.now().UTC().Format(dateLayout). This new adapter is the only place in the codebase that reimplements that computation outside the service, with the layout re-typed as a bare literal instead of reusing dateLayout. It's a leaky abstraction: the "what does 'today' mean for a soft-remove" decision now lives in two places with two clocks, and the service's test-clock injection can no longer control this call site. Since CreatePlanting already defaults PlantedAt internally via s.now() when omitted (internal/service/plantings.go:103-105), the cleaner fix is to let RemovedAt be optional in PlantingPatch/UpdatePlanting and default it via s.now() when omitted, mirroring PlantedAt, so the agent adapter (and any future caller) doesn't need to know about the date format or clock at all.
Everything else in the diff — the six new tool adapters, the DescribePlanting id/version addition, and the test — is thin, consistent with the existing adapter style, and free of dead code or unwarranted duplication.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens. Every new agent tool (read_journal, update_object, delete_object, remove_planting, list_seed_lots, record_seed_lot) is a thin one-call passthrough to an existing, already-shipped service method (ListJournal, UpdateObject, DeleteObject, UpdatePlanting, ListSeedLots, CreateSeedLot) — none of those service/store methods are modified by this diff, so any perf characteristics they have (verified: ListJournal already clamps to maxJournalPageSize=200 and uses a limit+1 "peek" trick instead of a second COUNT query at internal/service/journal.go:74-84; ListSeedLots/fillRemaining batches usage lookup with a single SeedLotUsage(ctx, actorID, ids) call rather than per-lot queries at internal/service/seed_lots.go:98-123) are pre-existing and out of scope for this PR.
The one genuinely new piece of runtime work is DescribeGarden's loop gaining two extra field assignments (ID, Version) per planting in internal/service/ops.go:524-532 — negligible, no new query, same O(n) shape as before.
No N+1s, no unbounded fetches, no new hot-loop work, no blocking calls introduced by this diff.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I read the six new adapter functions in internal/agent/tools.go, the DescribePlanting.ID/Version addition in internal/service/ops.go, and the service methods they call (UpdateObject, DeleteObject, UpdatePlanting, ListJournal, ListSeedLots, CreateSeedLot in internal/service/{objects,plantings,journal,seed_lots}.go).
Findings, through the error-handling/edge-case lens:
No swallowed errors. Every new adapter (readJournal, updateObject, deleteObject, removePlanting, listSeedLots, recordSeedLot) either returns the service error directly or checks it and returns immediately — consistent with the existing clearObject/moveObject pattern in the same file (internal/agent/tools.go:216-291).
Version-guarded mutations are correctly wired.update_object and remove_planting pass the caller-supplied version straight into UpdateObject/UpdatePlanting, which already implement optimistic-concurrency handling (internal/service/objects.go:162-181, internal/service/plantings.go:125-179) and return (current, ErrVersionConflict) on mismatch rather than silently overwriting. A model that omits version (leaving it 0) safely hits a conflict rather than corrupting data.
DescribePlanting.ID/Version are populated correctly. Verified plantingColumns/scanPlanting in internal/store/plantings.go:14-27 include and scan the version column, so remove_planting's version check isn't fed a bogus always-zero value from describe_garden.
Bounds/format validation is inherited, not bypassed.updateObject and recordSeedLot route through finalizeObject/finalizeSeedLot, which already reject non-finite dimensions, bad hex colors, invalid units, out-of-range years, and malformed URLs. readJournal's From/To go through validDatePtr, so a malformed date is a handled ErrInvalidInput, not silently ignored (internal/service/journal.go:58-60).
removePlanting computes "today" with time.Now().UTC() directly (internal/agent/tools.go:266) rather than through the service's injectable clock s.now() that every other "default to today" path uses (journal.go:121, plantings.go:104, ops.go:238,413). I looked for a concrete failure this could cause (e.g. a removed-before-planted rejection from clock skew) but couldn't construct one: the service's default clock is also time.Now, and the existing REST PATCH /plantings/:id endpoint already requires the caller to supply removedAt explicitly (internal/api/plantings.go:48-79), so this adapter's shape matches an established pattern rather than introducing a new one. Noting it, not flagging it as a finding, since it has no verified production impact.
No missing cleanup/defer, no nil-deref risk (all new struct fields are either value types or already-guarded pointers), and no unhandled boundary conditions found in the added code.
Through the security lens, every new tool is a thin pass-through to an existing service method, and I verified each one inherits its authorization and input-validation checks rather than bypassing them:
readJournal → ListJournal (journal.go:54): enforces requireGardenRole(..., roleViewer) on the caller-supplied gardenID; From/To run through validDatePtr; the store query (store/journal.go:78) is parameterized and scoped WHERE j.garden_id = ?, so a caller-supplied objectId from another garden just yields no rows — no cross-garden leak.
updateObject → UpdateObject (objects.go:162): gates on objectForRole(..., roleEditor) and is version-guarded.
deleteObject → DeleteObject (objects.go:185): same objectForRole(..., roleEditor) gate.
removePlanting → UpdatePlanting (plantings.go:125): loads the planting then re-checks the parent object via objectForRole(..., roleEditor). The today value is computed server-side (time.Now().UTC().Format("2006-01-02")) — not caller-controlled — and finalizePlanting validates it via validDatePtr and the planted-before-removed ordering. No injection path (parameterized store).
listSeedLots → ListSeedLots (seed_lots.go:77): scoped to ListSeedLotsForOwner(ctx, actorID, plantID) — only the actor's own lots; plantID is an optional narrowing filter, not an authz switch.
recordSeedLot → CreateSeedLot (seed_lots.go:152): visiblePlant check (built-in or own plant only), and finalizeSeedLot enforces unit allowlist (lotUnits), quantity bounds, text-length caps, packed-year range, and — importantly for injection/XSS — validSourceURL (seed_lots.go:332) which rejects anything that isn't an absolute http/https URL with a host, so a javascript: link can't be stored and later rendered as a clickable link.
The actor identity is bound once at NewToolbox (a.actor = actorID) and threaded into every service call; no tool lets the model override it, so there's no confused-deputy / horizontal-privilege path introduced here.
The only input that is caller-supplied and not length-validated at the tool seam (e.g. args.Notes, args.Vendor, args.Body) is bounded inside the corresponding finalize*/Create* service function, which I confirmed for the seed-lot and journal paths. Nothing in my lane is materially wrong.
🎯 Correctness — Minor issues
Verdict: Minor issues
internal/agent/tools.go:266 — removePlanting computes today := time.Now().UTC().Format("2006-01-02") from the wall clock, but every other date-stamping path in the service uses the injectable clock (s.now().UTC().Format(dateLayout)): CreatePlanting at internal/service/plantings.go:104, ClearObjectPlantings's soft-remove path at internal/service/ops.go:413, and the fill path at internal/service/ops.go:238. now is an injectable func() time.Time on Service (internal/service/service.go:40) specifically so tests can advance/freeze time. This is the one place in the new tools that bypasses that seam. Concrete impact: (a) it diverges from clear_object, which the PR description says it "mirrors" — the two "today" values can disagree across a midnight boundary or when the service clock is faked; (b) any test that freezes s.now() to assert a specific removed_at would see remove_planting stamp the real wall-clock date instead, silently breaking the invariant the soft-remove is supposed to preserve. The format string "2006-01-02" is byte-for-byte identical to dateLayout (internal/service/plantings.go:14), so the clock-source divergence is the substantive issue. Suggested fix: expose the formatted "today" through the service (e.g. func (s *Service) Today() string returning s.now().UTC().Format(dateLayout)) and have the adapter call that, so remove_planting and clear_object share one clock.
🧹 Code cleanliness & maintainability — Minor issues
All three findings verified. The corrected review:
Verdict: Minor issues
Through the cleanliness/maintainability lens, the adapters are thin and consistent with the existing tool style, and the test is well-structured with shared helpers. One pattern violation stands out, plus a couple of trivial nits.
internal/agent/tools.go:266 — removePlanting reinvents "today" and duplicates the date layout, breaking the established seam. Every service-side "soft-remove / default date" path uses s.now().UTC().Format(dateLayout) with an injectable clock (plantings.go:104, ops.go:238, ops.go:413, journal.go:121) and a single private dateLayout = "2006-01-02" constant (plantings.go:14). removePlanting instead hardcodes time.Now().UTC().Format("2006-01-02") inline, in a different package. Two maintainability consequences: (1) the format literal can drift from dateLayout, and (2) tests that advance the service clock via the now option won't move this tool's removed_at, so remove_planting and clear_object can disagree on "today" for the same logical soft-remove. The cleaner shape is the one clear_object/create_object already use: have the service compute today itself — e.g. let PlantingPatch{SetRemovedAt: true} with a nil RemovedAt default to s.now() inside UpdatePlanting — so the agent layer never formats dates at all. Verified against plantings.go:14 and the s.now() call sites.
internal/agent/tools.go:222 — readJournal hardcodes Limit: 50. The service already owns page-size policy (defaultJournalPageSize = 50, maxJournalPageSize = 200, applied at journal.go:61-67 when Limit <= 0); picking 50 in the agent layer duplicates that concern. Passing Limit: 0 would let the service apply its default, matching how every other caller leaves pagination to the seam. The default happens to equal 50 today, so the behavior is currently fine, but the layering point holds: the agent layer is encoding a policy constant that the service already owns.
internal/agent/tools_test.go:354 — TestCorrectiveTools is one ~125-line function covering five unrelated tools. It's readable thanks to the call/describe helpers, and the per-step comments are good, but it conflates resize, single-plop-remove, journal read, seed-lot record/list, and delete into a single test. If one assertion fails early, the rest are skipped (t.Fatalf). Splitting into subtests (t.Run) would localize failures and keep each gap's coverage independent. Trivial, not blocking.
No dead code, no copy-paste duplication of consequence, and the DescribePlantingID/Version addition mirrors DescribeObject.Version exactly as described.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, this change is clean. Verified by reading the underlying service/store code:
read_journal (tools.go:215) hard-codes Limit: 50 into the JournalQuery, and ListJournal (journal.go:61-67) re-clamps it against maxJournalPageSize (200). The store fetches limit+1 rows to answer hasMore without a separate COUNT. Bounded, single query, no pagination loop. No issue.
list_seed_lots (tools.go:278) delegates to ListSeedLots (seed_lots.go:77), which loads up to maxSeedLotsPerRead = 1000 rows (store/seed_lots.go:14,77) and then calls fillRemaining — that batches all lot IDs into a single SeedLotUsage query (seed_lots.go:106) and folds results into a map, so it is not N+1. Bounded and batched.
update_object, delete_object, remove_planting, record_seed_lot are all single-record adapters over existing service methods — one query each, no loops, no new allocation patterns.
DescribeGarden (ops.go:524-532) now copies two extra int64 fields (ID, Version) per planting into the result slice. This is a trivial constant-factor per plop on an already O(objects+plantings) describe path; no new query and no quadratic behavior. No material cost.
No N+1, no unbounded result sets, no missing caps, no hot-path regressions introduced by this PR.
🧯 Error handling & edge cases — Minor issues
Both findings verified against the actual source.
Verdict: Minor issues found
internal/agent/tools.go:266 — removePlanting uses time.Now() instead of the service's injectable s.now() clock. Confirmed: the adapter computes today := time.Now().UTC().Format("2006-01-02"), while the mirrored soft-remove in ClearObject uses s.now().UTC().Format(dateLayout) (internal/service/ops.go:413) and CreatePlanting does the same (plantings.go:104). Service.now is an injectable func() time.Time (service.go:40) so tests/scenarios that freeze or advance the service clock get a removed_at that diverges from how clear_object would date the same kind of removal. The agent layer has no access to s.now, so the two "today" values can disagree. Low-impact (not a crash), but a real consistency gap. Fix: have the service compute today itself when SetRemovedAt && RemovedAt == nil, or expose a Now() accessor.
internal/agent/tools.go:222-229 — readJournal returns hasMore but exposes no way to fetch the next page. Confirmed: the tool hardcodes Limit: 50 and the args struct has no offset/cursor field, yet it surfaces hasMore to the model. service.JournalQuery supports Offset (internal/service/journal.go:49) and ListJournal honors it. A garden whose journal exceeds 50 entries is only ever partially readable through the agent — the model sees hasMore: true with no way to continue, silently truncating the "what happened last spring?" use case the description advertises. Fix: add an Offset int (or page token) arg and thread it into q.Offset.
Both are edge-case/usability gaps rather than crash-causing, hence "Minor."
The new tools are thin adapters that correctly delegate to existing service methods, inheriting their authorization and validation unchanged:
read_journal → ListJournal: requires roleViewer on the garden; date strings are validated via validDatePtr; Limit is hardcoded to 50 in the adapter and clamped by the service.
update_object → UpdateObject: requires roleEditor via objectForRole; version-guarded; patch fields are validated by finalizeObject.
delete_object → DeleteObject: requires roleEditor via objectForRole.
remove_planting → UpdatePlanting: loads the planting, then requires roleEditor on its parent object; RemovedAt is generated server-side (time.Now().UTC().Format("2006-01-02")), not taken from agent input.
list_seed_lots → ListSeedLots: scoped to the authenticated owner via ListSeedLotsForOwner.
record_seed_lot → CreateSeedLot: checks visiblePlant and validates all fields (including SourceURL scheme/host) via finalizeSeedLot.
No injection surfaces, authz gaps, SSRF vectors, secret leakage, or unsafe deserialization were introduced by this diff.
🎯 Correctness — Minor issues
Verdict: Minor issues
internal/agent/tools.go:222 — The read_journal tool hardcodes Limit: 50 and returns hasMore to the LLM, but does not expose the offset parameter that the underlying service.JournalQuery supports. Because the tool adapter omits offset, an LLM that sees hasMore: true has no way to request the next page, making the pagination signal functionally useless. The tool should either accept an offset argument so the LLM can page, or not return hasMore at all.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
After reviewing through the code-cleanliness lens, I found two minor maintainability issues:
internal/agent/tools.go:222 — readJournal hardcodes Limit: 50 in the JournalQuery, which duplicates defaultJournalPageSize in the service layer. The service already handles limit <= 0 by defaulting to 50, so the explicit value is redundant. If the service default changes, the agent silently overrides it with the old value.
internal/agent/tools.go:266 — removePlanting duplicates the date-layout literal "2006-01-02" already defined as dateLayout in internal/service/plantings.go. The constant is unexported, so the agent cannot reference it; the duplication means a future format change in the service would silently break the agent's date construction.
internal/agent/tools.go:264-268 — removePlanting computes "today" in the agent layer (time.Now().UTC().Format(...)) and passes it to UpdatePlanting, whereas the adjacent clearObject tool delegates date computation entirely to the service's ClearObject method. This creates an inconsistency in where date/default logic lives; ideally both soft-remove paths would handle "today" in the same layer.
⚡ Performance — No material issues found
Verdict: No material issues found
From a performance perspective, the new tool adapters are thin and correctly delegate to existing service methods. No efficiency regressions are introduced:
readJournal hardcodes a Limit: 50, and the underlying ListJournal service enforces a ceiling of maxJournalPageSize = 200. No unbounded growth.
listSeedLots does not expose a limit parameter, but the store layer already caps reads at maxSeedLotsPerRead = 1000. The service's fillRemaining aggregates usage in a single SeedLotUsage query followed by one linear pass — no N+1 pattern.
removePlanting, updateObject, deleteObject, and recordSeedLot each make a single service call with straightforward argument construction. No extra allocations or blocking work on hot paths.
DescribeGarden gains ID/Version on DescribePlanting without adding queries; the fields are already present on the domain.Planting loaded by GardenFull.
The PR inherits the service layer's existing pagination limits, and the tool wrappers do not introduce avoidable copies, loops, or quadratic behavior.
🧯 Error handling & edge cases — No material issues found
No material issues found
internal/agent/tools.go:222 — read_journal hardcodes a 50-entry limit and exposes no pagination parameter, making the tool fail for gardens with more than 50 journal entries. The adapter sets Limit: 50 and returns hasMore: true when additional entries exist, but there is no offset or page argument the model can pass to retrieve them. The hasMore field becomes a dead-end: the agent knows more entries exist but has no tool mechanism to read them. This is an unhandled edge case for non-trivial journals. Fix: add an optional offset (or page) parameter to the tool schema and pass it through to JournalQuery.Offset.
The security lens is clean. Each new agent tool delegates directly to its corresponding service method, which performs the permission checks (garden role, ownership, "no-access is ErrNotFound"). The SourceURL field on recordSeedLot flows into CreateSeedLot, which validates it as an absolute http(s) URL with a host via validSourceURL — preventing the javascript: XSS vector. The date strings in read_journal pass through validDatePtr. No tainted input reaches SQL or rendering unvalidated. The time.Now().UTC() timestamp in removePlanting is server-generated, not user-controlled.
No material issues found.
🎯 Correctness — Minor issues
All three findings are confirmed:
removePlanting uses time.Now() at line 266 — verified the code, and ListJournal/CreateJournalEntry use s.now(). Finding stands.
readJournal hardcodes Limit: 50 and exposes no offset/cursor param — verified at line 222. The adapter only wires ObjectID, From, To. Finding stands.
updateObject description at line 74-84 of tools.go uses absolute dimensions — verified the description text and that finalizeObject only bounds [1cm, 100m]. This is a prompt-design concern. It's borderline; the description does warn with the correct procedure. Keeping as trivial.
## VERDICT: Minor issues found
- **`internal/agent/tools.go:266` — `remove_planting` uses `time.Now()` instead of the service's injectable clock.** Every service mutation that computes "today" uses `s.now().UTC().Format(dateLayout)` (e.g. `CreateJournalEntry` at `journal.go:121`), and `now` is injectable for tests. The agent adapter bypasses that and calls `time.Now()` directly. Harmless in production (service clock defaults to `time.Now`) but a latent determinism bug: a test wanting to assert `removed_at` can't pin it. `TestCorrectiveTools` doesn't check the date, so it slips through. The adapter has no access to `svc.now`; the clean fix is to let the service default `RemovedAt` to today when `SetRemovedAt` is true with a nil pointer, mirroring how `CreatePlanting` defaults `PlantedAt`.
- **`internal/agent/tools.go:222` — `read_journal` hardcodes `Limit: 50` with no pagination surface.** `JournalQuery` has an `Offset` field and `ListJournal` honors it (`journal.go:54-85`), but the tool exposes no `offset`/cursor argument and no way to follow the `hasMore` flag it returns. When a garden has >50 entries the agent reads the first 50, sees `hasMore: true`, and has no tool call to get the next page — "what happened last spring?" on a heavily-journaled garden dead-ends. Fix: add an `offset` argument and thread it into `JournalQuery.Offset`.
- **`internal/agent/tools.go:74` — `update_object` description is a plausible-magic-number trap.** `update_object` sets *absolute* dimensions while `move_object` is separate. A model conflating "make 60cm wider" with "set widthCm=60" gets a 60cm bed instead of a 560cm one, and `finalizeObject` only rejects dimensions outside `[1cm, 100m]`, so 60cm passes silently. The description does warn with the correct procedure (read width, add 60, pass sum), so this is a prompt-design risk, not a code bug; no code change required.
🧹 Code cleanliness & maintainability — Minor issues
I'll verify each finding against the actual code.
Now let me verify the soft-remove claims about dateLayout and the now injectable clock, and check ClearObject/CreatePlanting patterns.
All findings confirmed against the actual code. Let me also verify the test decode shapes and the move_object overlap claim — both already visible in the diff and confirmed in tools.go. Let me verify the test decode shapes.
All four findings are confirmed against the actual source code:
remove_planting soft-remove duplication — Confirmed: tools.go:266 uses time.Now().UTC().Format("2006-01-02") while every service-layer soft-remove uses s.now().UTC().Format(dateLayout) (ops.go:238, 413; plantings.go:104; journal.go:121). now is injectable (service.go:40), dateLayout is the unexported const (plantings.go:14). The adapter bypasses both.
Inconsistent return shapes — Confirmed: read_journal wraps as {entries, hasMore} (tools.go:233), list_seed_lots returns bare slice (tools.go:274). Test decodes differ (tools_test.go:429-433 vs 451-454).
update_object overlaps move_object — Confirmed: both call service.UpdateObject with ObjectPatch (tools.go:143-145 and 245-248); move_object is X/Y-only subset.
Verdict: Minor issues
remove_planting duplicates the soft-remove "today" logic and bypasses the service's injectable clock (internal/agent/tools.go:266-268). Every soft-remove in the service layer computes today as s.now().UTC().Format(dateLayout) (e.g. ClearObject at ops.go:413, CreatePlanting at plantings.go:104, also ops.go:238 and journal.go:121). The agent adapter instead hand-rolls time.Now().UTC().Format("2006-01-02") — a different time source (time.Now vs the injectable s.now, which service.go:40 documents as "injectable so tests can advance time") and a hardcoded format string that duplicates the unexported dateLayout const (plantings.go:14). The PR description calls these tools "thin adapters," but this one encodes pansy's soft-remove convention (which date layout, which clock) rather than delegating it. The cleanest fix is a dedicated SoftRemovePlanting(ctx, actorID, plantingID, version) service method that owns the today computation (mirroring ClearObject), so the adapter goes back to being a one-line pass-through.
read_journal hardcodes Limit: 50 (internal/agent/tools.go:222), duplicating defaultJournalPageSize = 50 (journal.go:23). If the page default ever changes, the agent silently drifts. Prefer importing/exposing the const, or leaving Limit: 0 so ListJournal applies its own default (it does — journal.go:62-63 treats limit <= 0 as defaultJournalPageSize).
Inconsistent return shape between two tools added in the same PR.read_journal wraps its result as {"entries": …, "hasMore": …} (tools.go:233) while list_seed_lots returns a bare []SeedLot (tools.go:274), and the test decodes them differently (tools_test.go:429-433 vs 451-454). There's a real reason for the difference (journal is paginated, seed lots aren't), but a bare array vs. a wrapped object is a shape inconsistency a model has to cope with. Minor; consider wrapping seed lots too, or documenting why one is bare.
update_object overlaps the existing move_object surface. Both call service.UpdateObject with an ObjectPatch (tools.go:143-145 and 245-248); move_object is now a strict subset (X/Y only) of what update_object could be if it accepted xCm/yCm. The split is deliberate (resize/rename vs. move), but it leaves two partial-patch tools over the same service method where one with all optional fields would do. Not dead code, just a slightly leaky/duplicated tool surface worth noting.
⚡ Performance — No material issues found
I'll review this through the performance lens, focusing on the DescribeGarden change and the new tool adapters.
The store query caps seed lots with LIMIT maxSeedLotsPerRead. The journal path sets Limit=50 in the tool. Both are bounded. The DescribeGarden change just adds two scalar fields per planting — negligible overhead. The new tool adapters are thin one-shot calls.
Let me check fillRemaining / SeedLotUsage for an N+1 pattern since list_seed_lots now exposes it to the agent.
No material issues found
Reviewed through the performance lens. All six new tool adapters are one-shot pass-throughs to existing service methods — no loops, no per-row work, no extra queries introduced by this PR.
Verified specifics:
read_journal (tools.go:215-226): sets Limit: 50; ListJournal (journal.go:61-77) clamps to maxJournalPageSize and uses the single LIMIT+1 query to compute hasMore — no second COUNT, bounded.
list_seed_lots (tools.go:280-282) → ListSeedLots → ListSeedLotsForOwner (store/seed_lots.go:77): capped by LIMIT maxSeedLotsPerRead. fillRemaining (seed_lots.go:98) is batched — one SeedLotUsage query for all lot ids, not per-lot (no N+1).
update_object, delete_object, remove_planting, record_seed_lot: each a single service/store call.
DescribeGarden change (ops.go:524-532): adds two scalar int64 fields (ID, Version) per planting to an already-built DescribePlanting struct. No new allocations, queries, or map lookups — pl.ID/pl.Version come from the already-fetched domain.Planting. Negligible overhead.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
The new tool adapters faithfully delegate to the existing service methods, which already handle the unhappy paths:
readJournal: empty/invalid From/To strings are only set as pointers when non-empty, and ListJournal validates them via validDatePtr (returns ErrInvalidInput). Empty GardenID is just a non-existent garden → ErrNotFound. hasMore + nil entries all handled.
updateObject: all optional fields are *-pointers, so omitting them leaves them nil and the service's applyObjectPatch treats nil as "leave unchanged". Zero Version falls through to the store's optimistic-concurrency check → ErrVersionConflict. Verified ObjectPatch semantics by reading the service.
deleteObject: passes through to DeleteObject; the error path returns the error, success returns a map. No swallowed errors.
removePlanting: time.Now().UTC().Format("2006-01-02") always produces a valid YYYY-MM-DD, and finalizePlanting validates the date plus the planted-before-removed ordering. args.Version is forwarded for the optimistic-concurrency guard. Soft-remove matches clear_object's behavior. No nil/zero edge cases unhandled.
listSeedLots: PlantID *int64 nil = "all my lots" (matches ListSeedLotsForOwner(actorID, nil)). Empty slice → fillRemaining short-circuits at len(lots) == 0. No panic risk.
recordSeedLot: Quantity/Unit/PackedForYear validated by finalizeSeedLot (isFinite, lotUnits map, year range). Unknown/non-owned plant → ErrInvalidInput via visiblePlant. No swallowed errors.
DescribePlanting now exposes ID/Version; existing readers of the JSON just gain two fields, no edge-case regression.
The only thing I noted outside the error-handling lens is that readJournal hard-codes Limit: 50 with no paging parameter exposed — a usability gap, not an error-handling defect, so I won't report it.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
All six new tools delegate to existing internal/service methods that enforce the same authorization checks (requireGardenRole/objectForRole with roleViewer/roleEditor, visiblePlant, ownSeedLot) used by the REST API. Verified by reading internal/service/journal.go:54, objects.go:162/185, plantings.go:125, and seed_lots.go:77/152.
Input validation remains on the service side: journal dates are checked with validDatePtr, object dimensions with finalizeObject, planting patches with finalizePlanting, and seed-lot fields (including SourceURL scheme/host validation) with finalizeSeedLot. No new injection surfaces are introduced.
read_journal hardcodes Limit: 50 and the service caps at maxJournalPageSize (200), so the agent cannot unboundedly enumerate entries.
record_seed_lot cannot attribute a purchase to another user's plant because visiblePlant requires the plant to be built-in or owned by the actor. list_seed_lots is scoped to owner_id = actorID in the store query.
remove_planting uses the same soft-removal pattern (removed_at = today) as clear_object, and the version-guarded UpdatePlanting path is already used by the REST API.
🎯 Correctness — No material issues found
No material issues found
Verified by reading the service methods (UpdatePlanting, DeleteObject, ListJournal, CreateSeedLot, ListSeedLots, UpdateObject), the domain types (Planting, SeedLot, JournalEntry), the patch application logic (applyPlantingPatch, finalizePlanting, finalizeSeedLot), and the test. All new tool adapters are thin, correct wrappers around existing service methods; they inherit the same ACL checks and validation. The remove_planting soft-remove path correctly sets SetRemovedAt + RemovedAt and passes finalizePlanting's date validation. The DescribePlanting fields are correctly populated from the domain row.
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
⚡ Performance — No material issues found
No material issues found
Each new tool is a thin adapter over an existing service method, inheriting the same query patterns and pagination limits already in place. No N+1 queries, no unbounded reads, no new allocations on hot paths, and no blocking work inside loops were introduced.
read_journal caps results at 50 entries (JournalQuery.Limit: 50), well within the service's maxJournalPageSize = 200. The underlying ListJournal uses limit+1 to compute hasMore in a single query — no extra COUNT.
list_seed_lots is capped at 1000 by the store's maxSeedLotsPerRead; fillRemaining batches usage lookup for all lots in one SeedLotUsage query.
update_object, delete_object, remove_planting, and record_seed_lot each issue a single service call matching the existing REST path's cost.
DescribePlanting gains two int64 fields; the payload size increase is negligible relative to what DescribeGarden already returns.
🧯 Error handling & edge cases — No material issues found
No material issues found
removePlanting soft-remove: The handler sets SetRemovedAt: true with RemovedAt: &today. Verified PlantingPatch supports both fields (service/plantings.go:61-75). The patch path hits applyPlantingPatch → finalizePlanting where validDatePtr(p.RemovedAt) is checked, and UpdatePlanting writes history with plantingEditSummary (correctly records a "Removed…" entry when before.RemovedAt == nil && after.RemovedAt != nil).
readJournal date validation: Passes q.From/q.To as pointers when non-empty. ListJournal validates them via validDatePtr and clamps limit to defaultJournalPageSize = 50 if unset, so the fixed Limit: 50 in the adapter is harmless (it gets capped or used as-is, both safe). Returns hasMore so pagination edge cases are visible to callers.
updateObject version guard: Passes args.Version straight through to UpdateObject, which does the store-level version check; no bypass.
deleteObject history recording: DeleteObject calls deleteObjectRecording (which snapshots cascade-deleted plantings) before the FK cascade erases them, so the undo path is coherent. No swallowed error: the actual store.DeleteObject happens inside deleteObjectRecording and its error propagates.
listSeedLots nil pointer: args.PlantID is *int64, matching ListSeedLots's plantID *int64 signature exactly — no risk of nil deref.
record_seed_lot fields: Quantity is float64 and Unit is string, matching SeedLotInput. finalizeSeedLot validates Unit against lotUnits, rejects negative/infinite quantity, and trims strings — all invariants enforced at the service seam.
GardenFull → DescribeGarden propagation: DescribeGarden calls GardenFull(ctx, actorID, gardenID, nil), so full.Plantings is the active-only set (year = nil). That is correct for a current-state description; a removed plop won't appear, which is what the tool's "0 active plops after remove" assertion expects.
No ignored errors, no nil derefs, no off-by-ones, no unhandled edge cases found in my lane.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 7 findings (2 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | read_journal exposes hasMore without offset parameter, making pagination impossible | `internal/agent/tools.go:222` | 3/5 | correctness, maintainability |
| 🟠 | remove_planting stamps removed_at from wall-clock time.Now() instead of the service's injectable s.now(), diverging from the clear_object path it claims to mirror | `internal/agent/tools.go:266` | 3/5 | correctness, error-handling, maintainability |
<details><summary>5 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | read_journal returns hasMore but exposes no pagination (offset/cursor), so journals over 50 entries are only partially readable via the agent | `internal/agent/tools.go:229` | glm-5.2:cloud | error-handling |
| 🟡 | update_object overlaps move_object — two partial-patch tools over the same UpdateObject service method | `internal/agent/tools.go:245` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | list_seed_lots returns a bare array while read_journal wraps in {entries,hasMore}; inconsistent tool return shapes in the same PR | `internal/agent/tools.go:274` | opencode/glm-5.2:cloud | maintainability |
| ⚪ | update_object sets absolute dimensions; a model conflating 'make 60cm wider' with 'set widthCm=60' passes finalizeObject's [1cm,100m] check and silently shrinks the bed | `internal/agent/tools.go:74` | opencode/glm-5.2:cloud | correctness |
| ⚪ | TestCorrectiveTools bundles five unrelated tool checks into one fat test; a t.Fatalf in any step skips the rest | `internal/agent/tools_test.go:354` | glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<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 new tool handlers use `a.actor`/`a.svc`, both fixed at toolbox construction — no user-controllable identity/authorization fields exist in any of the new tool argument structs, so an LLM cannot impersonate another user or garden actor through these tools.
## Review
**VERDICT: No material issues found**
Walked each new tool (`read_journal`, `update_object`, `delete_object`, `remove_planting`, `list_seed_lots`, `record_seed_lot`) down to its service method and confirmed, by reading the actual code (not just the diff):
- Every new adapter is a thin pass-through using the fixed `a.actor`/`a.svc` — no argument struct carries an actor/owner/user id the model could spoof (`internal/agent/tools.go:19,106`).
- `UpdateObject`/`DeleteObject` route through `objectForRole(..., roleEditor)` (`internal/service/objects.go:148-198`), which enforces the garden-role check and masks no-access as `ErrNotFound` per the project's stated convention.
- `UpdatePlanting` (used by `remove_planting`) loads the planting then re-derives its garden role via `objectForRole` (`internal/service/plantings.go:125-134`) — an attacker can't remove a plop in a garden they can't edit just by guessing a `plantingId`.
- `ListJournal`/journal SQL is properly parameterized and always scoped `WHERE j.garden_id = ?` in addition to the optional `object_id` filter (`internal/store/journal.go:78-104`), so passing an `objectId` belonging to a different garden than `gardenId` returns nothing rather than leaking cross-garden entries — no authz bypass via mismatched IDs.
- `CreateSeedLot`/`ListSeedLots` are properly owner-scoped (`ownSeedLot`, `visiblePlant` in `internal/service/seed_lots.go:77-182`) — a lot can only be created against a plant the actor can see (built-in or their own), and lots are always queried by `ownerID`.
- `record_seed_lot`'s `SourceURL` goes through the existing `validSourceURL` (absolute http/https, length-capped) validation in `finalizeSeedLot` (`internal/service/seed_lots.go:291-336`) — same path plants.go already uses; it's stored as an opaque string and never fetched server-side, so no SSRF vector.
- The added `DescribePlanting.ID`/`.Version` fields (`internal/service/ops.go`) only surface within a `DescribeGarden` payload that's already gated by `roleViewer` on the whole garden — no new exposure.
Nothing here weakens the existing service-seam permission model; the PR's stated design goal (thin adapters over the same service methods) holds up under inspection.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
This is fine — permission enforcement happens inside `CreateSeedLot`/`ListSeedLots` via the service seam, as documented, so the thin adapters correctly inherit it.
I've verified all new service-method calls (`ListJournal`, `UpdateObject`, `DeleteObject`, `UpdatePlanting`, `ListSeedLots`, `CreateSeedLot`) against their actual signatures, confirmed the `JournalQuery`/`ObjectPatch`/`PlantingPatch`/`SeedLotInput` field names match, confirmed the date format used in `removePlanting` (`"2006-01-02"`) matches `dateLayout` used throughout `internal/service`, and confirmed the test's tool-call argument names match the adapter struct tags. No correctness defects found.
**VERDICT: No material issues found**
The diff is a set of thin, mechanical adapters over existing, already-correct service methods (`ListJournal`, `UpdateObject`, `DeleteObject`, `UpdatePlanting`, `ListSeedLots`, `CreateSeedLot`), plus two new read-only fields (`ID`, `Version`) added to `DescribePlanting`. I checked each adapter's field names and types against the real service structs (`JournalQuery`, `ObjectPatch`, `PlantingPatch`, `SeedLotInput`) and they line up exactly. The one thing I looked at closely — `removePlanting` computing `today := time.Now().UTC().Format("2006-01-02")` instead of routing through the service's injectable `s.now()` clock (used everywhere else for "today", e.g. `ClearObject` at `internal/service/ops.go:413`) — is a minor style/testability inconsistency, not a functional bug: `s.now()` isn't overridden in production and isn't accessible from the `agent` package anyway, and the REST API (`internal/api/plantings.go:70`) already requires the *caller* to supply `removedAt` explicitly for single-planting updates, which is exactly the pattern this tool follows. Not reporting it as a correctness finding.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Line number and all cited service-layer facts check out against the actual code: `s.now()` is injectable (`service.go:40`), `dateLayout` is defined once (`plantings.go:14`) and consistently reused in `ClearObject` (`ops.go:413`), `FillClump` (`ops.go:238`), `CreatePlanting` (`plantings.go:104`), and journal writes (`journal.go:121`). `removePlanting` in `internal/agent/tools.go:266` is confirmed as the only call site that reimplements this with `time.Now()` and a bare `"2006-01-02"` literal instead of going through the service.
## VERDICT: Minor issues
- **`internal/agent/tools.go:266`** — `removePlanting` computes "today" with `time.Now().UTC().Format("2006-01-02")` directly in the agent adapter, rather than delegating to the service layer the way every other soft-removal path does. The service package has an injectable `now func() time.Time` field (`internal/service/service.go:40`, "injectable so tests can advance time") and a shared `dateLayout = "2006-01-02"` const (`internal/service/plantings.go:14`), used consistently in `ClearObject` (`internal/service/ops.go:413`), `CreatePlanting` (`internal/service/plantings.go:104`), `FillClump` (`internal/service/ops.go:238`), and journal entries (`internal/service/journal.go:121`) — always as `s.now().UTC().Format(dateLayout)`. This new adapter is the only place in the codebase that reimplements that computation outside the service, with the layout re-typed as a bare literal instead of reusing `dateLayout`. It's a leaky abstraction: the "what does 'today' mean for a soft-remove" decision now lives in two places with two clocks, and the service's test-clock injection can no longer control this call site. Since `CreatePlanting` already defaults `PlantedAt` internally via `s.now()` when omitted (`internal/service/plantings.go:103-105`), the cleaner fix is to let `RemovedAt` be optional in `PlantingPatch`/`UpdatePlanting` and default it via `s.now()` when omitted, mirroring `PlantedAt`, so the agent adapter (and any future caller) doesn't need to know about the date format or clock at all.
Everything else in the diff — the six new tool adapters, the `DescribePlanting` id/version addition, and the test — is thin, consistent with the existing adapter style, and free of dead code or unwarranted duplication.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
## VERDICT: No material issues found
Reviewed through the performance lens. Every new agent tool (`read_journal`, `update_object`, `delete_object`, `remove_planting`, `list_seed_lots`, `record_seed_lot`) is a thin one-call passthrough to an existing, already-shipped service method (`ListJournal`, `UpdateObject`, `DeleteObject`, `UpdatePlanting`, `ListSeedLots`, `CreateSeedLot`) — none of those service/store methods are modified by this diff, so any perf characteristics they have (verified: `ListJournal` already clamps to `maxJournalPageSize=200` and uses a limit+1 "peek" trick instead of a second COUNT query at `internal/service/journal.go:74-84`; `ListSeedLots`/`fillRemaining` batches usage lookup with a single `SeedLotUsage(ctx, actorID, ids)` call rather than per-lot queries at `internal/service/seed_lots.go:98-123`) are pre-existing and out of scope for this PR.
The one genuinely new piece of runtime work is `DescribeGarden`'s loop gaining two extra field assignments (`ID`, `Version`) per planting in `internal/service/ops.go:524-532` — negligible, no new query, same O(n) shape as before.
No N+1s, no unbounded fetches, no new hot-loop work, no blocking calls introduced by this diff.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
I read the six new adapter functions in `internal/agent/tools.go`, the `DescribePlanting.ID/Version` addition in `internal/service/ops.go`, and the service methods they call (`UpdateObject`, `DeleteObject`, `UpdatePlanting`, `ListJournal`, `ListSeedLots`, `CreateSeedLot` in `internal/service/{objects,plantings,journal,seed_lots}.go`).
Findings, through the error-handling/edge-case lens:
- **No swallowed errors.** Every new adapter (`readJournal`, `updateObject`, `deleteObject`, `removePlanting`, `listSeedLots`, `recordSeedLot`) either returns the service error directly or checks it and returns immediately — consistent with the existing `clearObject`/`moveObject` pattern in the same file (`internal/agent/tools.go:216-291`).
- **Version-guarded mutations are correctly wired.** `update_object` and `remove_planting` pass the caller-supplied `version` straight into `UpdateObject`/`UpdatePlanting`, which already implement optimistic-concurrency handling (`internal/service/objects.go:162-181`, `internal/service/plantings.go:125-179`) and return `(current, ErrVersionConflict)` on mismatch rather than silently overwriting. A model that omits `version` (leaving it `0`) safely hits a conflict rather than corrupting data.
- **`DescribePlanting.ID`/`Version` are populated correctly.** Verified `plantingColumns`/`scanPlanting` in `internal/store/plantings.go:14-27` include and scan the `version` column, so `remove_planting`'s version check isn't fed a bogus always-zero value from `describe_garden`.
- **Bounds/format validation is inherited, not bypassed.** `updateObject` and `recordSeedLot` route through `finalizeObject`/`finalizeSeedLot`, which already reject non-finite dimensions, bad hex colors, invalid units, out-of-range years, and malformed URLs. `readJournal`'s `From`/`To` go through `validDatePtr`, so a malformed date is a handled `ErrInvalidInput`, not silently ignored (`internal/service/journal.go:58-60`).
- **`removePlanting` computes "today" with `time.Now().UTC()` directly** (`internal/agent/tools.go:266`) rather than through the service's injectable clock `s.now()` that every other "default to today" path uses (`journal.go:121`, `plantings.go:104`, `ops.go:238,413`). I looked for a concrete failure this could cause (e.g. a removed-before-planted rejection from clock skew) but couldn't construct one: the service's default clock is also `time.Now`, and the existing REST `PATCH /plantings/:id` endpoint already requires the *caller* to supply `removedAt` explicitly (`internal/api/plantings.go:48-79`), so this adapter's shape matches an established pattern rather than introducing a new one. Noting it, not flagging it as a finding, since it has no verified production impact.
No missing cleanup/defer, no nil-deref risk (all new struct fields are either value types or already-guarded pointers), and no unhandled boundary conditions found in the added code.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the security lens, every new tool is a thin pass-through to an existing service method, and I verified each one inherits its authorization and input-validation checks rather than bypassing them:
- `readJournal` → `ListJournal` (journal.go:54): enforces `requireGardenRole(..., roleViewer)` on the caller-supplied `gardenID`; `From`/`To` run through `validDatePtr`; the store query (`store/journal.go:78`) is parameterized and scoped `WHERE j.garden_id = ?`, so a caller-supplied `objectId` from another garden just yields no rows — no cross-garden leak.
- `updateObject` → `UpdateObject` (objects.go:162): gates on `objectForRole(..., roleEditor)` and is version-guarded.
- `deleteObject` → `DeleteObject` (objects.go:185): same `objectForRole(..., roleEditor)` gate.
- `removePlanting` → `UpdatePlanting` (plantings.go:125): loads the planting then re-checks the parent object via `objectForRole(..., roleEditor)`. The `today` value is computed server-side (`time.Now().UTC().Format("2006-01-02")`) — not caller-controlled — and `finalizePlanting` validates it via `validDatePtr` and the planted-before-removed ordering. No injection path (parameterized store).
- `listSeedLots` → `ListSeedLots` (seed_lots.go:77): scoped to `ListSeedLotsForOwner(ctx, actorID, plantID)` — only the actor's own lots; `plantID` is an optional narrowing filter, not an authz switch.
- `recordSeedLot` → `CreateSeedLot` (seed_lots.go:152): `visiblePlant` check (built-in or own plant only), and `finalizeSeedLot` enforces unit allowlist (`lotUnits`), quantity bounds, text-length caps, packed-year range, and — importantly for injection/XSS — `validSourceURL` (seed_lots.go:332) which rejects anything that isn't an absolute `http`/`https` URL with a host, so a `javascript:` link can't be stored and later rendered as a clickable link.
The actor identity is bound once at `NewToolbox` (`a.actor = actorID`) and threaded into every service call; no tool lets the model override it, so there's no confused-deputy / horizontal-privilege path introduced here.
The only input that is caller-supplied and not length-validated at the tool seam (e.g. `args.Notes`, `args.Vendor`, `args.Body`) is bounded inside the corresponding `finalize*`/`Create*` service function, which I confirmed for the seed-lot and journal paths. Nothing in my lane is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict: Minor issues**
- `internal/agent/tools.go:266` — `removePlanting` computes `today := time.Now().UTC().Format("2006-01-02")` from the **wall clock**, but every other date-stamping path in the service uses the injectable clock (`s.now().UTC().Format(dateLayout)`): `CreatePlanting` at `internal/service/plantings.go:104`, `ClearObjectPlantings`'s soft-remove path at `internal/service/ops.go:413`, and the fill path at `internal/service/ops.go:238`. `now` is an injectable `func() time.Time` on `Service` (`internal/service/service.go:40`) specifically so tests can advance/freeze time. This is the one place in the new tools that bypasses that seam. Concrete impact: (a) it diverges from `clear_object`, which the PR description says it "mirrors" — the two "today" values can disagree across a midnight boundary or when the service clock is faked; (b) any test that freezes `s.now()` to assert a specific `removed_at` would see `remove_planting` stamp the real wall-clock date instead, silently breaking the invariant the soft-remove is supposed to preserve. The format string `"2006-01-02"` is byte-for-byte identical to `dateLayout` (`internal/service/plantings.go:14`), so the clock-source divergence is the substantive issue. Suggested fix: expose the formatted "today" through the service (e.g. `func (s *Service) Today() string` returning `s.now().UTC().Format(dateLayout)`) and have the adapter call that, so `remove_planting` and `clear_object` share one clock.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings verified. The corrected review:
**Verdict: Minor issues**
Through the cleanliness/maintainability lens, the adapters are thin and consistent with the existing tool style, and the test is well-structured with shared helpers. One pattern violation stands out, plus a couple of trivial nits.
- **`internal/agent/tools.go:266` — `removePlanting` reinvents "today" and duplicates the date layout, breaking the established seam.** Every service-side "soft-remove / default date" path uses `s.now().UTC().Format(dateLayout)` with an injectable clock (`plantings.go:104`, `ops.go:238`, `ops.go:413`, `journal.go:121`) and a single private `dateLayout = "2006-01-02"` constant (`plantings.go:14`). `removePlanting` instead hardcodes `time.Now().UTC().Format("2006-01-02")` inline, in a different package. Two maintainability consequences: (1) the format literal can drift from `dateLayout`, and (2) tests that advance the service clock via the `now` option won't move this tool's `removed_at`, so `remove_planting` and `clear_object` can disagree on "today" for the same logical soft-remove. The cleaner shape is the one `clear_object`/`create_object` already use: have the service compute today itself — e.g. let `PlantingPatch{SetRemovedAt: true}` with a nil `RemovedAt` default to `s.now()` inside `UpdatePlanting` — so the agent layer never formats dates at all. Verified against `plantings.go:14` and the `s.now()` call sites.
- **`internal/agent/tools.go:222` — `readJournal` hardcodes `Limit: 50`.** The service already owns page-size policy (`defaultJournalPageSize = 50`, `maxJournalPageSize = 200`, applied at `journal.go:61-67` when `Limit <= 0`); picking 50 in the agent layer duplicates that concern. Passing `Limit: 0` would let the service apply its default, matching how every other caller leaves pagination to the seam. The default happens to equal 50 today, so the behavior is currently fine, but the layering point holds: the agent layer is encoding a policy constant that the service already owns.
- **`internal/agent/tools_test.go:354` — `TestCorrectiveTools` is one ~125-line function covering five unrelated tools.** It's readable thanks to the `call`/`describe` helpers, and the per-step comments are good, but it conflates resize, single-plop-remove, journal read, seed-lot record/list, and delete into a single test. If one assertion fails early, the rest are skipped (`t.Fatalf`). Splitting into subtests (`t.Run`) would localize failures and keep each gap's coverage independent. Trivial, not blocking.
No dead code, no copy-paste duplication of consequence, and the `DescribePlanting` `ID`/`Version` addition mirrors `DescribeObject.Version` exactly as described.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the performance lens, this change is clean. Verified by reading the underlying service/store code:
- **`read_journal`** (tools.go:215) hard-codes `Limit: 50` into the `JournalQuery`, and `ListJournal` (journal.go:61-67) re-clamps it against `maxJournalPageSize` (200). The store fetches `limit+1` rows to answer `hasMore` without a separate COUNT. Bounded, single query, no pagination loop. No issue.
- **`list_seed_lots`** (tools.go:278) delegates to `ListSeedLots` (seed_lots.go:77), which loads up to `maxSeedLotsPerRead = 1000` rows (store/seed_lots.go:14,77) and then calls `fillRemaining` — that batches all lot IDs into a single `SeedLotUsage` query (seed_lots.go:106) and folds results into a map, so it is **not** N+1. Bounded and batched.
- **`update_object`, `delete_object`, `remove_planting`, `record_seed_lot`** are all single-record adapters over existing service methods — one query each, no loops, no new allocation patterns.
- **`DescribeGarden`** (ops.go:524-532) now copies two extra `int64` fields (`ID`, `Version`) per planting into the result slice. This is a trivial constant-factor per plop on an already O(objects+plantings) describe path; no new query and no quadratic behavior. No material cost.
No N+1, no unbounded result sets, no missing caps, no hot-path regressions introduced by this PR.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both findings verified against the actual source.
**Verdict: Minor issues found**
- **`internal/agent/tools.go:266` — `removePlanting` uses `time.Now()` instead of the service's injectable `s.now()` clock.** Confirmed: the adapter computes `today := time.Now().UTC().Format("2006-01-02")`, while the mirrored soft-remove in `ClearObject` uses `s.now().UTC().Format(dateLayout)` (`internal/service/ops.go:413`) and `CreatePlanting` does the same (`plantings.go:104`). `Service.now` is an injectable `func() time.Time` (`service.go:40`) so tests/scenarios that freeze or advance the service clock get a `removed_at` that diverges from how `clear_object` would date the same kind of removal. The agent layer has no access to `s.now`, so the two "today" values can disagree. Low-impact (not a crash), but a real consistency gap. Fix: have the service compute `today` itself when `SetRemovedAt && RemovedAt == nil`, or expose a `Now()` accessor.
- **`internal/agent/tools.go:222-229` — `readJournal` returns `hasMore` but exposes no way to fetch the next page.** Confirmed: the tool hardcodes `Limit: 50` and the args struct has no offset/cursor field, yet it surfaces `hasMore` to the model. `service.JournalQuery` supports `Offset` (`internal/service/journal.go:49`) and `ListJournal` honors it. A garden whose journal exceeds 50 entries is only ever partially readable through the agent — the model sees `hasMore: true` with no way to continue, silently truncating the "what happened last spring?" use case the description advertises. Fix: add an `Offset int` (or page token) arg and thread it into `q.Offset`.
Both are edge-case/usability gaps rather than crash-causing, hence "Minor."
</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
The new tools are thin adapters that correctly delegate to existing service methods, inheriting their authorization and validation unchanged:
- `read_journal` → `ListJournal`: requires `roleViewer` on the garden; date strings are validated via `validDatePtr`; `Limit` is hardcoded to 50 in the adapter and clamped by the service.
- `update_object` → `UpdateObject`: requires `roleEditor` via `objectForRole`; version-guarded; patch fields are validated by `finalizeObject`.
- `delete_object` → `DeleteObject`: requires `roleEditor` via `objectForRole`.
- `remove_planting` → `UpdatePlanting`: loads the planting, then requires `roleEditor` on its parent object; `RemovedAt` is generated server-side (`time.Now().UTC().Format("2006-01-02")`), not taken from agent input.
- `list_seed_lots` → `ListSeedLots`: scoped to the authenticated owner via `ListSeedLotsForOwner`.
- `record_seed_lot` → `CreateSeedLot`: checks `visiblePlant` and validates all fields (including `SourceURL` scheme/host) via `finalizeSeedLot`.
No injection surfaces, authz gaps, SSRF vectors, secret leakage, or unsafe deserialization were introduced by this diff.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict: Minor issues**
- `internal/agent/tools.go:222` — The `read_journal` tool hardcodes `Limit: 50` and returns `hasMore` to the LLM, but does not expose the `offset` parameter that the underlying `service.JournalQuery` supports. Because the tool adapter omits `offset`, an LLM that sees `hasMore: true` has no way to request the next page, making the pagination signal functionally useless. The tool should either accept an `offset` argument so the LLM can page, or not return `hasMore` at all.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
After reviewing through the code-cleanliness lens, I found two minor maintainability issues:
- **`internal/agent/tools.go:222`** — `readJournal` hardcodes `Limit: 50` in the `JournalQuery`, which duplicates `defaultJournalPageSize` in the service layer. The service already handles `limit <= 0` by defaulting to 50, so the explicit value is redundant. If the service default changes, the agent silently overrides it with the old value.
- **`internal/agent/tools.go:266`** — `removePlanting` duplicates the date-layout literal `"2006-01-02"` already defined as `dateLayout` in `internal/service/plantings.go`. The constant is unexported, so the agent cannot reference it; the duplication means a future format change in the service would silently break the agent's date construction.
- **`internal/agent/tools.go:264-268`** — `removePlanting` computes "today" in the agent layer (`time.Now().UTC().Format(...)`) and passes it to `UpdatePlanting`, whereas the adjacent `clearObject` tool delegates date computation entirely to the service's `ClearObject` method. This creates an inconsistency in where date/default logic lives; ideally both soft-remove paths would handle "today" in the same layer.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
From a performance perspective, the new tool adapters are thin and correctly delegate to existing service methods. No efficiency regressions are introduced:
- **`readJournal`** hardcodes a `Limit: 50`, and the underlying `ListJournal` service enforces a ceiling of `maxJournalPageSize = 200`. No unbounded growth.
- **`listSeedLots`** does not expose a limit parameter, but the store layer already caps reads at `maxSeedLotsPerRead = 1000`. The service's `fillRemaining` aggregates usage in a single `SeedLotUsage` query followed by one linear pass — no N+1 pattern.
- **`removePlanting`**, **`updateObject`**, **`deleteObject`**, and **`recordSeedLot`** each make a single service call with straightforward argument construction. No extra allocations or blocking work on hot paths.
- **`DescribeGarden`** gains `ID`/`Version` on `DescribePlanting` without adding queries; the fields are already present on the `domain.Planting` loaded by `GardenFull`.
The PR inherits the service layer's existing pagination limits, and the tool wrappers do not introduce avoidable copies, loops, or quadratic behavior.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
- **`internal/agent/tools.go:222` — `read_journal` hardcodes a 50-entry limit and exposes no pagination parameter, making the tool fail for gardens with more than 50 journal entries.** The adapter sets `Limit: 50` and returns `hasMore: true` when additional entries exist, but there is no `offset` or `page` argument the model can pass to retrieve them. The `hasMore` field becomes a dead-end: the agent knows more entries exist but has no tool mechanism to read them. This is an unhandled edge case for non-trivial journals. **Fix:** add an optional `offset` (or `page`) parameter to the tool schema and pass it through to `JournalQuery.Offset`.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
The security lens is clean. Each new agent tool delegates directly to its corresponding service method, which performs the permission checks (garden role, ownership, "no-access is ErrNotFound"). The `SourceURL` field on `recordSeedLot` flows into `CreateSeedLot`, which validates it as an absolute http(s) URL with a host via `validSourceURL` — preventing the `javascript:` XSS vector. The date strings in `read_journal` pass through `validDatePtr`. No tainted input reaches SQL or rendering unvalidated. The `time.Now().UTC()` timestamp in `removePlanting` is server-generated, not user-controlled.
No material issues found.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
All three findings are confirmed:
1. `removePlanting` uses `time.Now()` at line 266 — verified the code, and `ListJournal`/`CreateJournalEntry` use `s.now()`. Finding stands.
2. `readJournal` hardcodes `Limit: 50` and exposes no `offset`/`cursor` param — verified at line 222. The adapter only wires `ObjectID`, `From`, `To`. Finding stands.
3. `updateObject` description at line 74-84 of tools.go uses absolute dimensions — verified the description text and that `finalizeObject` only bounds `[1cm, 100m]`. This is a prompt-design concern. It's borderline; the description *does* warn with the correct procedure. Keeping as trivial.
```markdown
## VERDICT: Minor issues found
- **`internal/agent/tools.go:266` — `remove_planting` uses `time.Now()` instead of the service's injectable clock.** Every service mutation that computes "today" uses `s.now().UTC().Format(dateLayout)` (e.g. `CreateJournalEntry` at `journal.go:121`), and `now` is injectable for tests. The agent adapter bypasses that and calls `time.Now()` directly. Harmless in production (service clock defaults to `time.Now`) but a latent determinism bug: a test wanting to assert `removed_at` can't pin it. `TestCorrectiveTools` doesn't check the date, so it slips through. The adapter has no access to `svc.now`; the clean fix is to let the service default `RemovedAt` to today when `SetRemovedAt` is true with a nil pointer, mirroring how `CreatePlanting` defaults `PlantedAt`.
- **`internal/agent/tools.go:222` — `read_journal` hardcodes `Limit: 50` with no pagination surface.** `JournalQuery` has an `Offset` field and `ListJournal` honors it (`journal.go:54-85`), but the tool exposes no `offset`/cursor argument and no way to follow the `hasMore` flag it returns. When a garden has >50 entries the agent reads the first 50, sees `hasMore: true`, and has no tool call to get the next page — "what happened last spring?" on a heavily-journaled garden dead-ends. Fix: add an `offset` argument and thread it into `JournalQuery.Offset`.
- **`internal/agent/tools.go:74` — `update_object` description is a plausible-magic-number trap.** `update_object` sets *absolute* dimensions while `move_object` is separate. A model conflating "make 60cm wider" with "set widthCm=60" gets a 60cm bed instead of a 560cm one, and `finalizeObject` only rejects dimensions outside `[1cm, 100m]`, so 60cm passes silently. The description does warn with the correct procedure (read width, add 60, pass sum), so this is a prompt-design risk, not a code bug; no code change required.
```
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I'll verify each finding against the actual code.
Now let me verify the soft-remove claims about `dateLayout` and the `now` injectable clock, and check `ClearObject`/`CreatePlanting` patterns.
All findings confirmed against the actual code. Let me also verify the test decode shapes and the `move_object` overlap claim — both already visible in the diff and confirmed in tools.go. Let me verify the test decode shapes.
All four findings are confirmed against the actual source code:
1. **`remove_planting` soft-remove duplication** — Confirmed: `tools.go:266` uses `time.Now().UTC().Format("2006-01-02")` while every service-layer soft-remove uses `s.now().UTC().Format(dateLayout)` (ops.go:238, 413; plantings.go:104; journal.go:121). `now` is injectable (service.go:40), `dateLayout` is the unexported const (plantings.go:14). The adapter bypasses both.
2. **`read_journal` hardcoded `Limit: 50`** — Confirmed: `tools.go:222` hardcodes `Limit: 50`, duplicating `defaultJournalPageSize = 50` (journal.go:23). `ListJournal` treats `limit <= 0` as `defaultJournalPageSize` (journal.go:62-63).
3. **Inconsistent return shapes** — Confirmed: `read_journal` wraps as `{entries, hasMore}` (tools.go:233), `list_seed_lots` returns bare slice (tools.go:274). Test decodes differ (tools_test.go:429-433 vs 451-454).
4. **`update_object` overlaps `move_object`** — Confirmed: both call `service.UpdateObject` with `ObjectPatch` (tools.go:143-145 and 245-248); `move_object` is X/Y-only subset.
## Verdict: Minor issues
- **`remove_planting` duplicates the soft-remove "today" logic and bypasses the service's injectable clock** (`internal/agent/tools.go:266-268`). Every soft-remove in the service layer computes today as `s.now().UTC().Format(dateLayout)` (e.g. `ClearObject` at `ops.go:413`, `CreatePlanting` at `plantings.go:104`, also `ops.go:238` and `journal.go:121`). The agent adapter instead hand-rolls `time.Now().UTC().Format("2006-01-02")` — a different time source (`time.Now` vs the injectable `s.now`, which `service.go:40` documents as "injectable so tests can advance time") and a hardcoded format string that duplicates the unexported `dateLayout` const (`plantings.go:14`). The PR description calls these tools "thin adapters," but this one encodes pansy's soft-remove convention (which date layout, which clock) rather than delegating it. The cleanest fix is a dedicated `SoftRemovePlanting(ctx, actorID, plantingID, version)` service method that owns the `today` computation (mirroring `ClearObject`), so the adapter goes back to being a one-line pass-through.
- **`read_journal` hardcodes `Limit: 50`** (`internal/agent/tools.go:222`), duplicating `defaultJournalPageSize = 50` (`journal.go:23`). If the page default ever changes, the agent silently drifts. Prefer importing/exposing the const, or leaving `Limit: 0` so `ListJournal` applies its own default (it does — `journal.go:62-63` treats `limit <= 0` as `defaultJournalPageSize`).
- **Inconsistent return shape between two tools added in the same PR.** `read_journal` wraps its result as `{"entries": …, "hasMore": …}` (`tools.go:233`) while `list_seed_lots` returns a bare `[]SeedLot` (`tools.go:274`), and the test decodes them differently (`tools_test.go:429-433` vs `451-454`). There's a real reason for the difference (journal is paginated, seed lots aren't), but a bare array vs. a wrapped object is a shape inconsistency a model has to cope with. Minor; consider wrapping seed lots too, or documenting why one is bare.
- **`update_object` overlaps the existing `move_object` surface.** Both call `service.UpdateObject` with an `ObjectPatch` (`tools.go:143-145` and `245-248`); `move_object` is now a strict subset (X/Y only) of what `update_object` could be if it accepted `xCm/yCm`. The split is deliberate (resize/rename vs. move), but it leaves two partial-patch tools over the same service method where one with all optional fields would do. Not dead code, just a slightly leaky/duplicated tool surface worth noting.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I'll review this through the performance lens, focusing on the DescribeGarden change and the new tool adapters.
The store query caps seed lots with `LIMIT maxSeedLotsPerRead`. The journal path sets Limit=50 in the tool. Both are bounded. The DescribeGarden change just adds two scalar fields per planting — negligible overhead. The new tool adapters are thin one-shot calls.
Let me check `fillRemaining` / `SeedLotUsage` for an N+1 pattern since `list_seed_lots` now exposes it to the agent.
No material issues found
Reviewed through the performance lens. All six new tool adapters are one-shot pass-throughs to existing service methods — no loops, no per-row work, no extra queries introduced by this PR.
Verified specifics:
- `read_journal` (tools.go:215-226): sets `Limit: 50`; `ListJournal` (journal.go:61-77) clamps to `maxJournalPageSize` and uses the single `LIMIT+1` query to compute `hasMore` — no second COUNT, bounded.
- `list_seed_lots` (tools.go:280-282) → `ListSeedLots` → `ListSeedLotsForOwner` (store/seed_lots.go:77): capped by `LIMIT maxSeedLotsPerRead`. `fillRemaining` (seed_lots.go:98) is batched — one `SeedLotUsage` query for all lot ids, not per-lot (no N+1).
- `update_object`, `delete_object`, `remove_planting`, `record_seed_lot`: each a single service/store call.
- `DescribeGarden` change (ops.go:524-532): adds two scalar `int64` fields (`ID`, `Version`) per planting to an already-built `DescribePlanting` struct. No new allocations, queries, or map lookups — `pl.ID`/`pl.Version` come from the already-fetched `domain.Planting`. Negligible overhead.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
The new tool adapters faithfully delegate to the existing service methods, which already handle the unhappy paths:
- `readJournal`: empty/invalid `From`/`To` strings are only set as pointers when non-empty, and `ListJournal` validates them via `validDatePtr` (returns `ErrInvalidInput`). Empty `GardenID` is just a non-existent garden → `ErrNotFound`. `hasMore` + nil entries all handled.
- `updateObject`: all optional fields are `*`-pointers, so omitting them leaves them nil and the service's `applyObjectPatch` treats nil as "leave unchanged". Zero `Version` falls through to the store's optimistic-concurrency check → `ErrVersionConflict`. Verified `ObjectPatch` semantics by reading the service.
- `deleteObject`: passes through to `DeleteObject`; the error path returns the error, success returns a map. No swallowed errors.
- `removePlanting`: `time.Now().UTC().Format("2006-01-02")` always produces a valid `YYYY-MM-DD`, and `finalizePlanting` validates the date plus the planted-before-removed ordering. `args.Version` is forwarded for the optimistic-concurrency guard. Soft-remove matches `clear_object`'s behavior. No nil/zero edge cases unhandled.
- `listSeedLots`: `PlantID *int64` nil = "all my lots" (matches `ListSeedLotsForOwner(actorID, nil)`). Empty slice → `fillRemaining` short-circuits at `len(lots) == 0`. No panic risk.
- `recordSeedLot`: `Quantity`/`Unit`/`PackedForYear` validated by `finalizeSeedLot` (`isFinite`, `lotUnits` map, year range). Unknown/non-owned plant → `ErrInvalidInput` via `visiblePlant`. No swallowed errors.
`DescribePlanting` now exposes `ID`/`Version`; existing readers of the JSON just gain two fields, no edge-case regression.
The only thing I noted outside the error-handling lens is that `readJournal` hard-codes `Limit: 50` with no paging parameter exposed — a usability gap, not an error-handling defect, so I won't report it.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
- All six new tools delegate to existing `internal/service` methods that enforce the same authorization checks (`requireGardenRole`/`objectForRole` with `roleViewer`/`roleEditor`, `visiblePlant`, `ownSeedLot`) used by the REST API. Verified by reading `internal/service/journal.go:54`, `objects.go:162/185`, `plantings.go:125`, and `seed_lots.go:77/152`.
- Input validation remains on the service side: journal dates are checked with `validDatePtr`, object dimensions with `finalizeObject`, planting patches with `finalizePlanting`, and seed-lot fields (including `SourceURL` scheme/host validation) with `finalizeSeedLot`. No new injection surfaces are introduced.
- `read_journal` hardcodes `Limit: 50` and the service caps at `maxJournalPageSize` (200), so the agent cannot unboundedly enumerate entries.
- `record_seed_lot` cannot attribute a purchase to another user's plant because `visiblePlant` requires the plant to be built-in or owned by the actor. `list_seed_lots` is scoped to `owner_id = actorID` in the store query.
- `remove_planting` uses the same soft-removal pattern (`removed_at = today`) as `clear_object`, and the version-guarded `UpdatePlanting` path is already used by the REST API.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
*Verified by reading the service methods (`UpdatePlanting`, `DeleteObject`, `ListJournal`, `CreateSeedLot`, `ListSeedLots`, `UpdateObject`), the domain types (`Planting`, `SeedLot`, `JournalEntry`), the patch application logic (`applyPlantingPatch`, `finalizePlanting`, `finalizeSeedLot`), and the test. All new tool adapters are thin, correct wrappers around existing service methods; they inherit the same ACL checks and validation. The `remove_planting` soft-remove path correctly sets `SetRemovedAt` + `RemovedAt` and passes `finalizePlanting`'s date validation. The `DescribePlanting` fields are correctly populated from the domain row.*
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
Each new tool is a thin adapter over an existing service method, inheriting the same query patterns and pagination limits already in place. No N+1 queries, no unbounded reads, no new allocations on hot paths, and no blocking work inside loops were introduced.
- `read_journal` caps results at 50 entries (`JournalQuery.Limit: 50`), well within the service's `maxJournalPageSize = 200`. The underlying `ListJournal` uses `limit+1` to compute `hasMore` in a single query — no extra COUNT.
- `list_seed_lots` is capped at 1000 by the store's `maxSeedLotsPerRead`; `fillRemaining` batches usage lookup for all lots in one `SeedLotUsage` query.
- `update_object`, `delete_object`, `remove_planting`, and `record_seed_lot` each issue a single service call matching the existing REST path's cost.
- `DescribePlanting` gains two `int64` fields; the payload size increase is negligible relative to what `DescribeGarden` already returns.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
- **`removePlanting` soft-remove**: The handler sets `SetRemovedAt: true` with `RemovedAt: &today`. Verified `PlantingPatch` supports both fields (`service/plantings.go:61-75`). The patch path hits `applyPlantingPatch` → `finalizePlanting` where `validDatePtr(p.RemovedAt)` is checked, and `UpdatePlanting` writes history with `plantingEditSummary` (correctly records a "Removed…" entry when `before.RemovedAt == nil && after.RemovedAt != nil`).
- **`readJournal` date validation**: Passes `q.From`/`q.To` as pointers when non-empty. `ListJournal` validates them via `validDatePtr` and clamps limit to `defaultJournalPageSize = 50` if unset, so the fixed `Limit: 50` in the adapter is harmless (it gets capped or used as-is, both safe). Returns `hasMore` so pagination edge cases are visible to callers.
- **`updateObject` version guard**: Passes `args.Version` straight through to `UpdateObject`, which does the store-level version check; no bypass.
- **`deleteObject` history recording**: `DeleteObject` calls `deleteObjectRecording` (which snapshots cascade-deleted plantings) before the FK cascade erases them, so the undo path is coherent. No swallowed error: the actual `store.DeleteObject` happens inside `deleteObjectRecording` and its error propagates.
- **`listSeedLots` nil pointer**: `args.PlantID` is `*int64`, matching `ListSeedLots`'s `plantID *int64` signature exactly — no risk of nil deref.
- **`record_seed_lot` fields**: `Quantity` is `float64` and `Unit` is `string`, matching `SeedLotInput`. `finalizeSeedLot` validates `Unit` against `lotUnits`, rejects negative/infinite quantity, and trims strings — all invariants enforced at the service seam.
- **`GardenFull` → `DescribeGarden` propagation**: `DescribeGarden` calls `GardenFull(ctx, actorID, gardenID, nil)`, so `full.Plantings` is the active-only set (year = nil). That is correct for a current-state description; a removed plop won't appear, which is what the tool's "0 active plops after remove" assertion expects.
No ignored errors, no nil derefs, no off-by-ones, no unhandled edge cases found in my lane.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- read_journal now takes an offset, so the hasMore it returns is
actionable — an agent can page a journal longer than 50 entries.
- remove_planting goes through a new service RemovePlanting that stamps
removed_at from s.now() (the injectable clock ClearObject and the fill
path use), instead of the adapter computing the date off the wall clock.
It delegates to UpdatePlanting, so the role check, version guard and
history record are unchanged. Drops the now-unused time import.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit c432fe9199 into main2026-07-23 01:41:38 +00:00
steve
deleted branch feat/agent-corrective-tools2026-07-23 01:41:38 +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.
Closes the agent-toolbox gaps from #85 item 3. The toolbox could create and move objects but not delete or resize them, write the journal but not read it, clear a whole bed but not pull one plant, and report seed-remaining but not record a purchase.
Each new tool is a thin adapter over the same service method the REST API uses, so it inherits pansy's permission checks unchanged (the whole point of the service seam):
read_journalListJournalupdate_objectUpdateObjectdelete_objectDeleteObjectremove_plantingUpdatePlantinglist_seed_lotsListSeedLotsrecord_seed_lotCreateSeedLotSupporting change
To address a single plop, the agent needs its id + version — so
DescribePlantingnow carries both, exactly the wayDescribeObject.Versionalready lets it edit an object.remove_plantingsoft-removes (removed_at = today), mirroringclear_object, so the plant stays in planting history and the change is undoable.Deferred (noted on #85)
An undo/
revert_change_settool needs a way to list recent change sets to get achangeSetId— a larger addition (a history-listing tool). Left for a follow-up rather than shipped half-usable.Tests
TestCorrectiveToolsdrives the new tools through the tool layer the way a model would: resize an object, place then single-plop-remove, add then read a journal entry, record then list a seed lot, and delete. FullGOWORK=off go test ./...green;gofmtclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-23 01:34:12Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 7 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -67,0 +71,4 @@"recently observed first. Use this to answer \"what did I note about the west bed?\" "+"or \"what happened last spring?\".",a.readJournal),llm.DefineTool("update_object",⚪ update_object sets absolute dimensions; a model conflating 'make 60cm wider' with 'set widthCm=60' passes finalizeObject's [1cm,100m] check and silently shrinks the bed
correctness · flagged by 1 model
internal/agent/tools.go:74—update_objectdescription is a plausible-magic-number trap.update_objectsets absolute dimensions whilemove_objectis separate. A model conflating "make 60cm wider" with "set widthCm=60" gets a 60cm bed instead of a 560cm one, andfinalizeObjectonly rejects dimensions outside[1cm, 100m], so 60cm passes silently. The description does warn with the correct procedure (read width, add 60, pass sum), so this is a prompt-design risk, not a code bug;…🪰 Gadfly · advisory
@@ -180,0 +219,4 @@From string `json:"from" description:"optional earliest observed date, YYYY-MM-DD"`To string `json:"to" description:"optional latest observed date, YYYY-MM-DD"`}) (any, error) {q := service.JournalQuery{ObjectID: args.ObjectID, Limit: 50}🟠 read_journal exposes hasMore without offset parameter, making pagination impossible
correctness, maintainability · flagged by 3 models
internal/agent/tools.go:222— Theread_journaltool hardcodesLimit: 50and returnshasMoreto the LLM, but does not expose theoffsetparameter that the underlyingservice.JournalQuerysupports. Because the tool adapter omitsoffset, an LLM that seeshasMore: truehas no way to request the next page, making the pagination signal functionally useless. The tool should either accept anoffsetargument so the LLM can page, or not returnhasMoreat all.🪰 Gadfly · advisory
@@ -180,0 +226,4 @@if args.To != "" {q.To = &args.To}entries, hasMore, err := a.svc.ListJournal(ctx, a.actor, args.GardenID, q)🟠 read_journal returns hasMore but exposes no pagination (offset/cursor), so journals over 50 entries are only partially readable via the agent
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -180,0 +242,4 @@RotationDeg *float64 `json:"rotationDeg" description:"optional new rotation in degrees"`Plantable *bool `json:"plantable" description:"optional: whether the object can hold plants"`}) (any, error) {return a.svc.UpdateObject(ctx, a.actor, args.ObjectID, service.ObjectPatch{🟡 update_object overlaps move_object — two partial-patch tools over the same UpdateObject service method
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -180,0 +263,4 @@}) (any, error) {// Soft-remove: set removed_at to today, mirroring clear_object, so the plop// stays in the planting history and the change is undoable.today := time.Now().UTC().Format("2006-01-02")🟠 remove_planting stamps removed_at from wall-clock time.Now() instead of the service's injectable s.now(), diverging from the clear_object path it claims to mirror
correctness, error-handling, maintainability · flagged by 3 models
internal/agent/tools.go:266—removePlantingcomputestoday := time.Now().UTC().Format("2006-01-02")from the wall clock, but every other date-stamping path in the service uses the injectable clock (s.now().UTC().Format(dateLayout)):CreatePlantingatinternal/service/plantings.go:104,ClearObjectPlantings's soft-remove path atinternal/service/ops.go:413, and the fill path atinternal/service/ops.go:238.nowis an injectablefunc() time.TimeonService(`internal/s…🪰 Gadfly · advisory
@@ -180,0 +271,4 @@func (a *adapter) listSeedLots(ctx context.Context, args struct {PlantID *int64 `json:"plantId" description:"optional: only lots for this plant"`}) (any, error) {return a.svc.ListSeedLots(ctx, a.actor, args.PlantID)🟡 list_seed_lots returns a bare array while read_journal wraps in {entries,hasMore}; inconsistent tool return shapes in the same PR
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -350,0 +351,4 @@// could only write, resize and delete an object it could only create and move,// pull a single plop instead of clearing the whole bed, and record/read seed// lots. Each is driven through the tool layer the way a model would run it.func TestCorrectiveTools(t *testing.T) {⚪ TestCorrectiveTools bundles five unrelated tool checks into one fat test; a t.Fatalf in any step skips the rest
maintainability · flagged by 1 model
internal/agent/tools_test.go:354—TestCorrectiveToolsis one ~125-line function covering five unrelated tools. It's readable thanks to thecall/describehelpers, and the per-step comments are good, but it conflates resize, single-plop-remove, journal read, seed-lot record/list, and delete into a single test. If one assertion fails early, the rest are skipped (t.Fatalf). Splitting into subtests (t.Run) would localize failures and keep each gap's coverage independent. Trivial, n…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 7 findings (2 with multi-model agreement)
internal/agent/tools.go:222internal/agent/tools.go:2665 single-model findings (lower confidence)
internal/agent/tools.go:229internal/agent/tools.go:245internal/agent/tools.go:274internal/agent/tools.go:74internal/agent/tools_test.go:354Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
All new tool handlers use
a.actor/a.svc, both fixed at toolbox construction — no user-controllable identity/authorization fields exist in any of the new tool argument structs, so an LLM cannot impersonate another user or garden actor through these tools.Review
VERDICT: No material issues found
Walked each new tool (
read_journal,update_object,delete_object,remove_planting,list_seed_lots,record_seed_lot) down to its service method and confirmed, by reading the actual code (not just the diff):a.actor/a.svc— no argument struct carries an actor/owner/user id the model could spoof (internal/agent/tools.go:19,106).UpdateObject/DeleteObjectroute throughobjectForRole(..., roleEditor)(internal/service/objects.go:148-198), which enforces the garden-role check and masks no-access asErrNotFoundper the project's stated convention.UpdatePlanting(used byremove_planting) loads the planting then re-derives its garden role viaobjectForRole(internal/service/plantings.go:125-134) — an attacker can't remove a plop in a garden they can't edit just by guessing aplantingId.ListJournal/journal SQL is properly parameterized and always scopedWHERE j.garden_id = ?in addition to the optionalobject_idfilter (internal/store/journal.go:78-104), so passing anobjectIdbelonging to a different garden thangardenIdreturns nothing rather than leaking cross-garden entries — no authz bypass via mismatched IDs.CreateSeedLot/ListSeedLotsare properly owner-scoped (ownSeedLot,visiblePlantininternal/service/seed_lots.go:77-182) — a lot can only be created against a plant the actor can see (built-in or their own), and lots are always queried byownerID.record_seed_lot'sSourceURLgoes through the existingvalidSourceURL(absolute http/https, length-capped) validation infinalizeSeedLot(internal/service/seed_lots.go:291-336) — same path plants.go already uses; it's stored as an opaque string and never fetched server-side, so no SSRF vector.DescribePlanting.ID/.Versionfields (internal/service/ops.go) only surface within aDescribeGardenpayload that's already gated byroleVieweron the whole garden — no new exposure.Nothing here weakens the existing service-seam permission model; the PR's stated design goal (thin adapters over the same service methods) holds up under inspection.
🎯 Correctness — No material issues found
This is fine — permission enforcement happens inside
CreateSeedLot/ListSeedLotsvia the service seam, as documented, so the thin adapters correctly inherit it.I've verified all new service-method calls (
ListJournal,UpdateObject,DeleteObject,UpdatePlanting,ListSeedLots,CreateSeedLot) against their actual signatures, confirmed theJournalQuery/ObjectPatch/PlantingPatch/SeedLotInputfield names match, confirmed the date format used inremovePlanting("2006-01-02") matchesdateLayoutused throughoutinternal/service, and confirmed the test's tool-call argument names match the adapter struct tags. No correctness defects found.VERDICT: No material issues found
The diff is a set of thin, mechanical adapters over existing, already-correct service methods (
ListJournal,UpdateObject,DeleteObject,UpdatePlanting,ListSeedLots,CreateSeedLot), plus two new read-only fields (ID,Version) added toDescribePlanting. I checked each adapter's field names and types against the real service structs (JournalQuery,ObjectPatch,PlantingPatch,SeedLotInput) and they line up exactly. The one thing I looked at closely —removePlantingcomputingtoday := time.Now().UTC().Format("2006-01-02")instead of routing through the service's injectables.now()clock (used everywhere else for "today", e.g.ClearObjectatinternal/service/ops.go:413) — is a minor style/testability inconsistency, not a functional bug:s.now()isn't overridden in production and isn't accessible from theagentpackage anyway, and the REST API (internal/api/plantings.go:70) already requires the caller to supplyremovedAtexplicitly for single-planting updates, which is exactly the pattern this tool follows. Not reporting it as a correctness finding.🧹 Code cleanliness & maintainability — Minor issues
Line number and all cited service-layer facts check out against the actual code:
s.now()is injectable (service.go:40),dateLayoutis defined once (plantings.go:14) and consistently reused inClearObject(ops.go:413),FillClump(ops.go:238),CreatePlanting(plantings.go:104), and journal writes (journal.go:121).removePlantingininternal/agent/tools.go:266is confirmed as the only call site that reimplements this withtime.Now()and a bare"2006-01-02"literal instead of going through the service.VERDICT: Minor issues
internal/agent/tools.go:266—removePlantingcomputes "today" withtime.Now().UTC().Format("2006-01-02")directly in the agent adapter, rather than delegating to the service layer the way every other soft-removal path does. The service package has an injectablenow func() time.Timefield (internal/service/service.go:40, "injectable so tests can advance time") and a shareddateLayout = "2006-01-02"const (internal/service/plantings.go:14), used consistently inClearObject(internal/service/ops.go:413),CreatePlanting(internal/service/plantings.go:104),FillClump(internal/service/ops.go:238), and journal entries (internal/service/journal.go:121) — always ass.now().UTC().Format(dateLayout). This new adapter is the only place in the codebase that reimplements that computation outside the service, with the layout re-typed as a bare literal instead of reusingdateLayout. It's a leaky abstraction: the "what does 'today' mean for a soft-remove" decision now lives in two places with two clocks, and the service's test-clock injection can no longer control this call site. SinceCreatePlantingalready defaultsPlantedAtinternally vias.now()when omitted (internal/service/plantings.go:103-105), the cleaner fix is to letRemovedAtbe optional inPlantingPatch/UpdatePlantingand default it vias.now()when omitted, mirroringPlantedAt, so the agent adapter (and any future caller) doesn't need to know about the date format or clock at all.Everything else in the diff — the six new tool adapters, the
DescribePlantingid/version addition, and the test — is thin, consistent with the existing adapter style, and free of dead code or unwarranted duplication.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens. Every new agent tool (
read_journal,update_object,delete_object,remove_planting,list_seed_lots,record_seed_lot) is a thin one-call passthrough to an existing, already-shipped service method (ListJournal,UpdateObject,DeleteObject,UpdatePlanting,ListSeedLots,CreateSeedLot) — none of those service/store methods are modified by this diff, so any perf characteristics they have (verified:ListJournalalready clamps tomaxJournalPageSize=200and uses a limit+1 "peek" trick instead of a second COUNT query atinternal/service/journal.go:74-84;ListSeedLots/fillRemainingbatches usage lookup with a singleSeedLotUsage(ctx, actorID, ids)call rather than per-lot queries atinternal/service/seed_lots.go:98-123) are pre-existing and out of scope for this PR.The one genuinely new piece of runtime work is
DescribeGarden's loop gaining two extra field assignments (ID,Version) per planting ininternal/service/ops.go:524-532— negligible, no new query, same O(n) shape as before.No N+1s, no unbounded fetches, no new hot-loop work, no blocking calls introduced by this diff.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I read the six new adapter functions in
internal/agent/tools.go, theDescribePlanting.ID/Versionaddition ininternal/service/ops.go, and the service methods they call (UpdateObject,DeleteObject,UpdatePlanting,ListJournal,ListSeedLots,CreateSeedLotininternal/service/{objects,plantings,journal,seed_lots}.go).Findings, through the error-handling/edge-case lens:
readJournal,updateObject,deleteObject,removePlanting,listSeedLots,recordSeedLot) either returns the service error directly or checks it and returns immediately — consistent with the existingclearObject/moveObjectpattern in the same file (internal/agent/tools.go:216-291).update_objectandremove_plantingpass the caller-suppliedversionstraight intoUpdateObject/UpdatePlanting, which already implement optimistic-concurrency handling (internal/service/objects.go:162-181,internal/service/plantings.go:125-179) and return(current, ErrVersionConflict)on mismatch rather than silently overwriting. A model that omitsversion(leaving it0) safely hits a conflict rather than corrupting data.DescribePlanting.ID/Versionare populated correctly. VerifiedplantingColumns/scanPlantingininternal/store/plantings.go:14-27include and scan theversioncolumn, soremove_planting's version check isn't fed a bogus always-zero value fromdescribe_garden.updateObjectandrecordSeedLotroute throughfinalizeObject/finalizeSeedLot, which already reject non-finite dimensions, bad hex colors, invalid units, out-of-range years, and malformed URLs.readJournal'sFrom/Togo throughvalidDatePtr, so a malformed date is a handledErrInvalidInput, not silently ignored (internal/service/journal.go:58-60).removePlantingcomputes "today" withtime.Now().UTC()directly (internal/agent/tools.go:266) rather than through the service's injectable clocks.now()that every other "default to today" path uses (journal.go:121,plantings.go:104,ops.go:238,413). I looked for a concrete failure this could cause (e.g. a removed-before-planted rejection from clock skew) but couldn't construct one: the service's default clock is alsotime.Now, and the existing RESTPATCH /plantings/:idendpoint already requires the caller to supplyremovedAtexplicitly (internal/api/plantings.go:48-79), so this adapter's shape matches an established pattern rather than introducing a new one. Noting it, not flagging it as a finding, since it has no verified production impact.No missing cleanup/defer, no nil-deref risk (all new struct fields are either value types or already-guarded pointers), and no unhandled boundary conditions found in the added code.
glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Through the security lens, every new tool is a thin pass-through to an existing service method, and I verified each one inherits its authorization and input-validation checks rather than bypassing them:
readJournal→ListJournal(journal.go:54): enforcesrequireGardenRole(..., roleViewer)on the caller-suppliedgardenID;From/Torun throughvalidDatePtr; the store query (store/journal.go:78) is parameterized and scopedWHERE j.garden_id = ?, so a caller-suppliedobjectIdfrom another garden just yields no rows — no cross-garden leak.updateObject→UpdateObject(objects.go:162): gates onobjectForRole(..., roleEditor)and is version-guarded.deleteObject→DeleteObject(objects.go:185): sameobjectForRole(..., roleEditor)gate.removePlanting→UpdatePlanting(plantings.go:125): loads the planting then re-checks the parent object viaobjectForRole(..., roleEditor). Thetodayvalue is computed server-side (time.Now().UTC().Format("2006-01-02")) — not caller-controlled — andfinalizePlantingvalidates it viavalidDatePtrand the planted-before-removed ordering. No injection path (parameterized store).listSeedLots→ListSeedLots(seed_lots.go:77): scoped toListSeedLotsForOwner(ctx, actorID, plantID)— only the actor's own lots;plantIDis an optional narrowing filter, not an authz switch.recordSeedLot→CreateSeedLot(seed_lots.go:152):visiblePlantcheck (built-in or own plant only), andfinalizeSeedLotenforces unit allowlist (lotUnits), quantity bounds, text-length caps, packed-year range, and — importantly for injection/XSS —validSourceURL(seed_lots.go:332) which rejects anything that isn't an absolutehttp/httpsURL with a host, so ajavascript:link can't be stored and later rendered as a clickable link.The actor identity is bound once at
NewToolbox(a.actor = actorID) and threaded into every service call; no tool lets the model override it, so there's no confused-deputy / horizontal-privilege path introduced here.The only input that is caller-supplied and not length-validated at the tool seam (e.g.
args.Notes,args.Vendor,args.Body) is bounded inside the correspondingfinalize*/Create*service function, which I confirmed for the seed-lot and journal paths. Nothing in my lane is materially wrong.🎯 Correctness — Minor issues
Verdict: Minor issues
internal/agent/tools.go:266—removePlantingcomputestoday := time.Now().UTC().Format("2006-01-02")from the wall clock, but every other date-stamping path in the service uses the injectable clock (s.now().UTC().Format(dateLayout)):CreatePlantingatinternal/service/plantings.go:104,ClearObjectPlantings's soft-remove path atinternal/service/ops.go:413, and the fill path atinternal/service/ops.go:238.nowis an injectablefunc() time.TimeonService(internal/service/service.go:40) specifically so tests can advance/freeze time. This is the one place in the new tools that bypasses that seam. Concrete impact: (a) it diverges fromclear_object, which the PR description says it "mirrors" — the two "today" values can disagree across a midnight boundary or when the service clock is faked; (b) any test that freezess.now()to assert a specificremoved_atwould seeremove_plantingstamp the real wall-clock date instead, silently breaking the invariant the soft-remove is supposed to preserve. The format string"2006-01-02"is byte-for-byte identical todateLayout(internal/service/plantings.go:14), so the clock-source divergence is the substantive issue. Suggested fix: expose the formatted "today" through the service (e.g.func (s *Service) Today() stringreturnings.now().UTC().Format(dateLayout)) and have the adapter call that, soremove_plantingandclear_objectshare one clock.🧹 Code cleanliness & maintainability — Minor issues
All three findings verified. The corrected review:
Verdict: Minor issues
Through the cleanliness/maintainability lens, the adapters are thin and consistent with the existing tool style, and the test is well-structured with shared helpers. One pattern violation stands out, plus a couple of trivial nits.
internal/agent/tools.go:266—removePlantingreinvents "today" and duplicates the date layout, breaking the established seam. Every service-side "soft-remove / default date" path usess.now().UTC().Format(dateLayout)with an injectable clock (plantings.go:104,ops.go:238,ops.go:413,journal.go:121) and a single privatedateLayout = "2006-01-02"constant (plantings.go:14).removePlantinginstead hardcodestime.Now().UTC().Format("2006-01-02")inline, in a different package. Two maintainability consequences: (1) the format literal can drift fromdateLayout, and (2) tests that advance the service clock via thenowoption won't move this tool'sremoved_at, soremove_plantingandclear_objectcan disagree on "today" for the same logical soft-remove. The cleaner shape is the oneclear_object/create_objectalready use: have the service compute today itself — e.g. letPlantingPatch{SetRemovedAt: true}with a nilRemovedAtdefault tos.now()insideUpdatePlanting— so the agent layer never formats dates at all. Verified againstplantings.go:14and thes.now()call sites.internal/agent/tools.go:222—readJournalhardcodesLimit: 50. The service already owns page-size policy (defaultJournalPageSize = 50,maxJournalPageSize = 200, applied atjournal.go:61-67whenLimit <= 0); picking 50 in the agent layer duplicates that concern. PassingLimit: 0would let the service apply its default, matching how every other caller leaves pagination to the seam. The default happens to equal 50 today, so the behavior is currently fine, but the layering point holds: the agent layer is encoding a policy constant that the service already owns.internal/agent/tools_test.go:354—TestCorrectiveToolsis one ~125-line function covering five unrelated tools. It's readable thanks to thecall/describehelpers, and the per-step comments are good, but it conflates resize, single-plop-remove, journal read, seed-lot record/list, and delete into a single test. If one assertion fails early, the rest are skipped (t.Fatalf). Splitting into subtests (t.Run) would localize failures and keep each gap's coverage independent. Trivial, not blocking.No dead code, no copy-paste duplication of consequence, and the
DescribePlantingID/Versionaddition mirrorsDescribeObject.Versionexactly as described.⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, this change is clean. Verified by reading the underlying service/store code:
read_journal(tools.go:215) hard-codesLimit: 50into theJournalQuery, andListJournal(journal.go:61-67) re-clamps it againstmaxJournalPageSize(200). The store fetcheslimit+1rows to answerhasMorewithout a separate COUNT. Bounded, single query, no pagination loop. No issue.list_seed_lots(tools.go:278) delegates toListSeedLots(seed_lots.go:77), which loads up tomaxSeedLotsPerRead = 1000rows (store/seed_lots.go:14,77) and then callsfillRemaining— that batches all lot IDs into a singleSeedLotUsagequery (seed_lots.go:106) and folds results into a map, so it is not N+1. Bounded and batched.update_object,delete_object,remove_planting,record_seed_lotare all single-record adapters over existing service methods — one query each, no loops, no new allocation patterns.DescribeGarden(ops.go:524-532) now copies two extraint64fields (ID,Version) per planting into the result slice. This is a trivial constant-factor per plop on an already O(objects+plantings) describe path; no new query and no quadratic behavior. No material cost.No N+1, no unbounded result sets, no missing caps, no hot-path regressions introduced by this PR.
🧯 Error handling & edge cases — Minor issues
Both findings verified against the actual source.
Verdict: Minor issues found
internal/agent/tools.go:266—removePlantingusestime.Now()instead of the service's injectables.now()clock. Confirmed: the adapter computestoday := time.Now().UTC().Format("2006-01-02"), while the mirrored soft-remove inClearObjectusess.now().UTC().Format(dateLayout)(internal/service/ops.go:413) andCreatePlantingdoes the same (plantings.go:104).Service.nowis an injectablefunc() time.Time(service.go:40) so tests/scenarios that freeze or advance the service clock get aremoved_atthat diverges from howclear_objectwould date the same kind of removal. The agent layer has no access tos.now, so the two "today" values can disagree. Low-impact (not a crash), but a real consistency gap. Fix: have the service computetodayitself whenSetRemovedAt && RemovedAt == nil, or expose aNow()accessor.internal/agent/tools.go:222-229—readJournalreturnshasMorebut exposes no way to fetch the next page. Confirmed: the tool hardcodesLimit: 50and the args struct has no offset/cursor field, yet it surfaceshasMoreto the model.service.JournalQuerysupportsOffset(internal/service/journal.go:49) andListJournalhonors it. A garden whose journal exceeds 50 entries is only ever partially readable through the agent — the model seeshasMore: truewith no way to continue, silently truncating the "what happened last spring?" use case the description advertises. Fix: add anOffset int(or page token) arg and thread it intoq.Offset.Both are edge-case/usability gaps rather than crash-causing, hence "Minor."
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
The new tools are thin adapters that correctly delegate to existing service methods, inheriting their authorization and validation unchanged:
read_journal→ListJournal: requiresroleVieweron the garden; date strings are validated viavalidDatePtr;Limitis hardcoded to 50 in the adapter and clamped by the service.update_object→UpdateObject: requiresroleEditorviaobjectForRole; version-guarded; patch fields are validated byfinalizeObject.delete_object→DeleteObject: requiresroleEditorviaobjectForRole.remove_planting→UpdatePlanting: loads the planting, then requiresroleEditoron its parent object;RemovedAtis generated server-side (time.Now().UTC().Format("2006-01-02")), not taken from agent input.list_seed_lots→ListSeedLots: scoped to the authenticated owner viaListSeedLotsForOwner.record_seed_lot→CreateSeedLot: checksvisiblePlantand validates all fields (includingSourceURLscheme/host) viafinalizeSeedLot.No injection surfaces, authz gaps, SSRF vectors, secret leakage, or unsafe deserialization were introduced by this diff.
🎯 Correctness — Minor issues
Verdict: Minor issues
internal/agent/tools.go:222— Theread_journaltool hardcodesLimit: 50and returnshasMoreto the LLM, but does not expose theoffsetparameter that the underlyingservice.JournalQuerysupports. Because the tool adapter omitsoffset, an LLM that seeshasMore: truehas no way to request the next page, making the pagination signal functionally useless. The tool should either accept anoffsetargument so the LLM can page, or not returnhasMoreat all.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
After reviewing through the code-cleanliness lens, I found two minor maintainability issues:
internal/agent/tools.go:222—readJournalhardcodesLimit: 50in theJournalQuery, which duplicatesdefaultJournalPageSizein the service layer. The service already handleslimit <= 0by defaulting to 50, so the explicit value is redundant. If the service default changes, the agent silently overrides it with the old value.internal/agent/tools.go:266—removePlantingduplicates the date-layout literal"2006-01-02"already defined asdateLayoutininternal/service/plantings.go. The constant is unexported, so the agent cannot reference it; the duplication means a future format change in the service would silently break the agent's date construction.internal/agent/tools.go:264-268—removePlantingcomputes "today" in the agent layer (time.Now().UTC().Format(...)) and passes it toUpdatePlanting, whereas the adjacentclearObjecttool delegates date computation entirely to the service'sClearObjectmethod. This creates an inconsistency in where date/default logic lives; ideally both soft-remove paths would handle "today" in the same layer.⚡ Performance — No material issues found
Verdict: No material issues found
From a performance perspective, the new tool adapters are thin and correctly delegate to existing service methods. No efficiency regressions are introduced:
readJournalhardcodes aLimit: 50, and the underlyingListJournalservice enforces a ceiling ofmaxJournalPageSize = 200. No unbounded growth.listSeedLotsdoes not expose a limit parameter, but the store layer already caps reads atmaxSeedLotsPerRead = 1000. The service'sfillRemainingaggregates usage in a singleSeedLotUsagequery followed by one linear pass — no N+1 pattern.removePlanting,updateObject,deleteObject, andrecordSeedLoteach make a single service call with straightforward argument construction. No extra allocations or blocking work on hot paths.DescribeGardengainsID/VersiononDescribePlantingwithout adding queries; the fields are already present on thedomain.Plantingloaded byGardenFull.The PR inherits the service layer's existing pagination limits, and the tool wrappers do not introduce avoidable copies, loops, or quadratic behavior.
🧯 Error handling & edge cases — No material issues found
No material issues found
internal/agent/tools.go:222—read_journalhardcodes a 50-entry limit and exposes no pagination parameter, making the tool fail for gardens with more than 50 journal entries. The adapter setsLimit: 50and returnshasMore: truewhen additional entries exist, but there is nooffsetorpageargument the model can pass to retrieve them. ThehasMorefield becomes a dead-end: the agent knows more entries exist but has no tool mechanism to read them. This is an unhandled edge case for non-trivial journals. Fix: add an optionaloffset(orpage) parameter to the tool schema and pass it through toJournalQuery.Offset.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
The security lens is clean. Each new agent tool delegates directly to its corresponding service method, which performs the permission checks (garden role, ownership, "no-access is ErrNotFound"). The
SourceURLfield onrecordSeedLotflows intoCreateSeedLot, which validates it as an absolute http(s) URL with a host viavalidSourceURL— preventing thejavascript:XSS vector. The date strings inread_journalpass throughvalidDatePtr. No tainted input reaches SQL or rendering unvalidated. Thetime.Now().UTC()timestamp inremovePlantingis server-generated, not user-controlled.No material issues found.
🎯 Correctness — Minor issues
All three findings are confirmed:
removePlantingusestime.Now()at line 266 — verified the code, andListJournal/CreateJournalEntryuses.now(). Finding stands.readJournalhardcodesLimit: 50and exposes nooffset/cursorparam — verified at line 222. The adapter only wiresObjectID,From,To. Finding stands.updateObjectdescription at line 74-84 of tools.go uses absolute dimensions — verified the description text and thatfinalizeObjectonly bounds[1cm, 100m]. This is a prompt-design concern. It's borderline; the description does warn with the correct procedure. Keeping as trivial.🧹 Code cleanliness & maintainability — Minor issues
I'll verify each finding against the actual code.
Now let me verify the soft-remove claims about
dateLayoutand thenowinjectable clock, and checkClearObject/CreatePlantingpatterns.All findings confirmed against the actual code. Let me also verify the test decode shapes and the
move_objectoverlap claim — both already visible in the diff and confirmed in tools.go. Let me verify the test decode shapes.All four findings are confirmed against the actual source code:
remove_plantingsoft-remove duplication — Confirmed:tools.go:266usestime.Now().UTC().Format("2006-01-02")while every service-layer soft-remove usess.now().UTC().Format(dateLayout)(ops.go:238, 413; plantings.go:104; journal.go:121).nowis injectable (service.go:40),dateLayoutis the unexported const (plantings.go:14). The adapter bypasses both.read_journalhardcodedLimit: 50— Confirmed:tools.go:222hardcodesLimit: 50, duplicatingdefaultJournalPageSize = 50(journal.go:23).ListJournaltreatslimit <= 0asdefaultJournalPageSize(journal.go:62-63).Inconsistent return shapes — Confirmed:
read_journalwraps as{entries, hasMore}(tools.go:233),list_seed_lotsreturns bare slice (tools.go:274). Test decodes differ (tools_test.go:429-433 vs 451-454).update_objectoverlapsmove_object— Confirmed: both callservice.UpdateObjectwithObjectPatch(tools.go:143-145 and 245-248);move_objectis X/Y-only subset.Verdict: Minor issues
remove_plantingduplicates the soft-remove "today" logic and bypasses the service's injectable clock (internal/agent/tools.go:266-268). Every soft-remove in the service layer computes today ass.now().UTC().Format(dateLayout)(e.g.ClearObjectatops.go:413,CreatePlantingatplantings.go:104, alsoops.go:238andjournal.go:121). The agent adapter instead hand-rollstime.Now().UTC().Format("2006-01-02")— a different time source (time.Nowvs the injectables.now, whichservice.go:40documents as "injectable so tests can advance time") and a hardcoded format string that duplicates the unexporteddateLayoutconst (plantings.go:14). The PR description calls these tools "thin adapters," but this one encodes pansy's soft-remove convention (which date layout, which clock) rather than delegating it. The cleanest fix is a dedicatedSoftRemovePlanting(ctx, actorID, plantingID, version)service method that owns thetodaycomputation (mirroringClearObject), so the adapter goes back to being a one-line pass-through.read_journalhardcodesLimit: 50(internal/agent/tools.go:222), duplicatingdefaultJournalPageSize = 50(journal.go:23). If the page default ever changes, the agent silently drifts. Prefer importing/exposing the const, or leavingLimit: 0soListJournalapplies its own default (it does —journal.go:62-63treatslimit <= 0asdefaultJournalPageSize).Inconsistent return shape between two tools added in the same PR.
read_journalwraps its result as{"entries": …, "hasMore": …}(tools.go:233) whilelist_seed_lotsreturns a bare[]SeedLot(tools.go:274), and the test decodes them differently (tools_test.go:429-433vs451-454). There's a real reason for the difference (journal is paginated, seed lots aren't), but a bare array vs. a wrapped object is a shape inconsistency a model has to cope with. Minor; consider wrapping seed lots too, or documenting why one is bare.update_objectoverlaps the existingmove_objectsurface. Both callservice.UpdateObjectwith anObjectPatch(tools.go:143-145and245-248);move_objectis now a strict subset (X/Y only) of whatupdate_objectcould be if it acceptedxCm/yCm. The split is deliberate (resize/rename vs. move), but it leaves two partial-patch tools over the same service method where one with all optional fields would do. Not dead code, just a slightly leaky/duplicated tool surface worth noting.⚡ Performance — No material issues found
I'll review this through the performance lens, focusing on the DescribeGarden change and the new tool adapters.
The store query caps seed lots with
LIMIT maxSeedLotsPerRead. The journal path sets Limit=50 in the tool. Both are bounded. The DescribeGarden change just adds two scalar fields per planting — negligible overhead. The new tool adapters are thin one-shot calls.Let me check
fillRemaining/SeedLotUsagefor an N+1 pattern sincelist_seed_lotsnow exposes it to the agent.No material issues found
Reviewed through the performance lens. All six new tool adapters are one-shot pass-throughs to existing service methods — no loops, no per-row work, no extra queries introduced by this PR.
Verified specifics:
read_journal(tools.go:215-226): setsLimit: 50;ListJournal(journal.go:61-77) clamps tomaxJournalPageSizeand uses the singleLIMIT+1query to computehasMore— no second COUNT, bounded.list_seed_lots(tools.go:280-282) →ListSeedLots→ListSeedLotsForOwner(store/seed_lots.go:77): capped byLIMIT maxSeedLotsPerRead.fillRemaining(seed_lots.go:98) is batched — oneSeedLotUsagequery for all lot ids, not per-lot (no N+1).update_object,delete_object,remove_planting,record_seed_lot: each a single service/store call.DescribeGardenchange (ops.go:524-532): adds two scalarint64fields (ID,Version) per planting to an already-builtDescribePlantingstruct. No new allocations, queries, or map lookups —pl.ID/pl.Versioncome from the already-fetcheddomain.Planting. Negligible overhead.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
The new tool adapters faithfully delegate to the existing service methods, which already handle the unhappy paths:
readJournal: empty/invalidFrom/Tostrings are only set as pointers when non-empty, andListJournalvalidates them viavalidDatePtr(returnsErrInvalidInput). EmptyGardenIDis just a non-existent garden →ErrNotFound.hasMore+ nil entries all handled.updateObject: all optional fields are*-pointers, so omitting them leaves them nil and the service'sapplyObjectPatchtreats nil as "leave unchanged". ZeroVersionfalls through to the store's optimistic-concurrency check →ErrVersionConflict. VerifiedObjectPatchsemantics by reading the service.deleteObject: passes through toDeleteObject; the error path returns the error, success returns a map. No swallowed errors.removePlanting:time.Now().UTC().Format("2006-01-02")always produces a validYYYY-MM-DD, andfinalizePlantingvalidates the date plus the planted-before-removed ordering.args.Versionis forwarded for the optimistic-concurrency guard. Soft-remove matchesclear_object's behavior. No nil/zero edge cases unhandled.listSeedLots:PlantID *int64nil = "all my lots" (matchesListSeedLotsForOwner(actorID, nil)). Empty slice →fillRemainingshort-circuits atlen(lots) == 0. No panic risk.recordSeedLot:Quantity/Unit/PackedForYearvalidated byfinalizeSeedLot(isFinite,lotUnitsmap, year range). Unknown/non-owned plant →ErrInvalidInputviavisiblePlant. No swallowed errors.DescribePlantingnow exposesID/Version; existing readers of the JSON just gain two fields, no edge-case regression.The only thing I noted outside the error-handling lens is that
readJournalhard-codesLimit: 50with no paging parameter exposed — a usability gap, not an error-handling defect, so I won't report it.opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
internal/servicemethods that enforce the same authorization checks (requireGardenRole/objectForRolewithroleViewer/roleEditor,visiblePlant,ownSeedLot) used by the REST API. Verified by readinginternal/service/journal.go:54,objects.go:162/185,plantings.go:125, andseed_lots.go:77/152.validDatePtr, object dimensions withfinalizeObject, planting patches withfinalizePlanting, and seed-lot fields (includingSourceURLscheme/host validation) withfinalizeSeedLot. No new injection surfaces are introduced.read_journalhardcodesLimit: 50and the service caps atmaxJournalPageSize(200), so the agent cannot unboundedly enumerate entries.record_seed_lotcannot attribute a purchase to another user's plant becausevisiblePlantrequires the plant to be built-in or owned by the actor.list_seed_lotsis scoped toowner_id = actorIDin the store query.remove_plantinguses the same soft-removal pattern (removed_at = today) asclear_object, and the version-guardedUpdatePlantingpath is already used by the REST API.🎯 Correctness — No material issues found
No material issues found
Verified by reading the service methods (
UpdatePlanting,DeleteObject,ListJournal,CreateSeedLot,ListSeedLots,UpdateObject), the domain types (Planting,SeedLot,JournalEntry), the patch application logic (applyPlantingPatch,finalizePlanting,finalizeSeedLot), and the test. All new tool adapters are thin, correct wrappers around existing service methods; they inherit the same ACL checks and validation. Theremove_plantingsoft-remove path correctly setsSetRemovedAt+RemovedAtand passesfinalizePlanting's date validation. TheDescribePlantingfields are correctly populated from the domain row.🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
⚡ Performance — No material issues found
No material issues found
Each new tool is a thin adapter over an existing service method, inheriting the same query patterns and pagination limits already in place. No N+1 queries, no unbounded reads, no new allocations on hot paths, and no blocking work inside loops were introduced.
read_journalcaps results at 50 entries (JournalQuery.Limit: 50), well within the service'smaxJournalPageSize = 200. The underlyingListJournaluseslimit+1to computehasMorein a single query — no extra COUNT.list_seed_lotsis capped at 1000 by the store'smaxSeedLotsPerRead;fillRemainingbatches usage lookup for all lots in oneSeedLotUsagequery.update_object,delete_object,remove_planting, andrecord_seed_loteach issue a single service call matching the existing REST path's cost.DescribePlantinggains twoint64fields; the payload size increase is negligible relative to whatDescribeGardenalready returns.🧯 Error handling & edge cases — No material issues found
No material issues found
removePlantingsoft-remove: The handler setsSetRemovedAt: truewithRemovedAt: &today. VerifiedPlantingPatchsupports both fields (service/plantings.go:61-75). The patch path hitsapplyPlantingPatch→finalizePlantingwherevalidDatePtr(p.RemovedAt)is checked, andUpdatePlantingwrites history withplantingEditSummary(correctly records a "Removed…" entry whenbefore.RemovedAt == nil && after.RemovedAt != nil).readJournaldate validation: Passesq.From/q.Toas pointers when non-empty.ListJournalvalidates them viavalidDatePtrand clamps limit todefaultJournalPageSize = 50if unset, so the fixedLimit: 50in the adapter is harmless (it gets capped or used as-is, both safe). ReturnshasMoreso pagination edge cases are visible to callers.updateObjectversion guard: Passesargs.Versionstraight through toUpdateObject, which does the store-level version check; no bypass.deleteObjecthistory recording:DeleteObjectcallsdeleteObjectRecording(which snapshots cascade-deleted plantings) before the FK cascade erases them, so the undo path is coherent. No swallowed error: the actualstore.DeleteObjecthappens insidedeleteObjectRecordingand its error propagates.listSeedLotsnil pointer:args.PlantIDis*int64, matchingListSeedLots'splantID *int64signature exactly — no risk of nil deref.record_seed_lotfields:Quantityisfloat64andUnitisstring, matchingSeedLotInput.finalizeSeedLotvalidatesUnitagainstlotUnits, rejects negative/infinite quantity, and trims strings — all invariants enforced at the service seam.GardenFull→DescribeGardenpropagation:DescribeGardencallsGardenFull(ctx, actorID, gardenID, nil), sofull.Plantingsis the active-only set (year = nil). That is correct for a current-state description; a removed plop won't appear, which is what the tool's "0 active plops after remove" assertion expects.No ignored errors, no nil derefs, no off-by-ones, no unhandled edge cases found in my lane.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.