Tier 3 of the agent-tool round (#129, #130): the outward-facing set, and the one destructive tool that was still missing.
What's new
list_shares, share_garden, remove_share, public_link wrap the sharing service (ListShares/AddShare/UpdateShareRole/RemoveShare, Get/Enable/DisablePublicShareLink). They change who can see a garden beyond the screen, so they are gated twice:
the prompt tells the model to say exactly what it would do — who, which role, or that a link will start or stop working — and ask;
the tools refuse without confirmed=true, which their descriptions allow only after a yes in the conversation. The refusal names the action, so the question the model then asks is precise. This is a schema-level reminder, not a guarantee (a model could pass it anyway), which is why the prompt rule stays too.
share_garden changes the role of an existing share instead of failing on ErrShareExists; remove_share takes the email list_shares reports (the model has the address, not the id); an unknown email explains that the person must sign in once first (v1 has no invitation emails).
public_link returns the address, never a bare token — PANSY_BASE_URL + /g/<token> through the new Service.PublicShareURL, or the site-relative path when the base URL isn't configured.
delete_planting — the hard delete for a plop that was never really planted (a misplacement, a duplicate), as opposed to remove_planting's "it came out". Recorded in the history, so undoable.
CLAUDE.md gets the convention ("outward-facing tools ask first, and refuse without confirmed=true" — keep both halves); DESIGN.md roadmap note and UI step labels updated.
Tests
TestSharingToolsAskFirst (agent): each outward-facing action refused with the action named until confirmed; share → role change → no-op note; unknown email; a viewer can't list shares; remove by email, unknown email explained; public link get/enable (idempotent)/rotate/disable and a bad action; delete_planting gone from the season view, in the history, and restored by undo_change.
Tier 3 of the agent-tool round (#129, #130): the outward-facing set, and the one destructive tool that was still missing.
## What's new
- **`list_shares`, `share_garden`, `remove_share`, `public_link`** wrap the sharing service (`ListShares`/`AddShare`/`UpdateShareRole`/`RemoveShare`, `Get/Enable/DisablePublicShareLink`). They change who can see a garden beyond the screen, so they are gated twice:
- the prompt tells the model to say exactly what it would do — who, which role, or that a link will start or stop working — and ask;
- the tools refuse without `confirmed=true`, which their descriptions allow only after a yes in the conversation. The refusal names the action, so the question the model then asks is precise. This is a schema-level reminder, not a guarantee (a model could pass it anyway), which is why the prompt rule stays too.
- `share_garden` changes the role of an existing share instead of failing on `ErrShareExists`; `remove_share` takes the email `list_shares` reports (the model has the address, not the id); an unknown email explains that the person must sign in once first (v1 has no invitation emails).
- `public_link` returns the address, never a bare token — `PANSY_BASE_URL + /g/<token>` through the new `Service.PublicShareURL`, or the site-relative path when the base URL isn't configured.
- **`delete_planting`** — the hard delete for a plop that was never really planted (a misplacement, a duplicate), as opposed to `remove_planting`'s "it came out". Recorded in the history, so undoable.
CLAUDE.md gets the convention ("outward-facing tools ask first, and refuse without confirmed=true" — keep both halves); DESIGN.md roadmap note and UI step labels updated.
## Tests
`TestSharingToolsAskFirst` (agent): each outward-facing action refused with the action named until confirmed; share → role change → no-op note; unknown email; a viewer can't list shares; remove by email, unknown email explained; public link get/enable (idempotent)/rotate/disable and a bad action; delete_planting gone from the season view, in the history, and restored by `undo_change`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
list_shares, share_garden, remove_share and public_link (get / enable /
rotate / disable) wrap the sharing service. They change who can see a garden
beyond the screen, so they are gated twice: the prompt tells the model to say
exactly what it would do and ask, and the tools refuse without confirmed=true,
which their descriptions allow only after a yes in the conversation. The
refusal names the action, so the question the model asks is precise.
share_garden changes the role of an existing share instead of failing on it;
remove_share takes the email list_shares reports; an unknown email explains
that the person has to sign in once first. public_link returns the address
(PANSY_BASE_URL + /g/<token>, via the new Service.PublicShareURL), never a
bare token.
delete_planting is the hard delete for a plop that was never really planted,
as opposed to remove_planting's "it came out"; it is recorded, so undoable.
Co-Authored-By: Claude Fable 5 <[email protected]>
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 06:17:33Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ⚠️ **correctness** — could not complete
- ✅ **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** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡linkView returns untyped map[string]any while shareView is a typed struct — inconsistent representation of sibling view types
maintainability · flagged by 1 model
internal/agent/tools.go:1008 — linkView returns map[string]any while shareView is a typed struct. Both are "the view the tools report," and listShares returns them side by side in the same map (line 1003). The link gets an untyped map (so callers/tests reach in with publicLink["enabled"], see tools_test.go:1085–1088), the share gets a typed struct. Inconsistent for conceptually sibling output types. A small linkView struct (Enabled bool / URL string \json:"url,omitempty"\…
🪰 Gadfly · advisory
🟡 **linkView returns untyped map[string]any while shareView is a typed struct — inconsistent representation of sibling view types**
_maintainability · flagged by 1 model_
- **`internal/agent/tools.go:1008` — `linkView` returns `map[string]any` while `shareView` is a typed struct.** Both are "the view the tools report," and `listShares` returns them side by side in the same map (line 1003). The link gets an untyped map (so callers/tests reach in with `publicLink["enabled"]`, see tools_test.go:1085–1088), the share gets a typed struct. Inconsistent for conceptually sibling output types. A small `linkView` struct (`Enabled bool` / `URL string \`json:"url,omitempty"\…
<sub>🪰 Gadfly · advisory</sub>
🟡shareView is built inline five times; a toShareView helper would remove the copy-paste
maintainability · flagged by 1 model
internal/agent/tools.go:1041 — shareView is built inline five times with all four fields.listShares (line 997) constructs it in a loop; shareGarden builds it three different ways across its branches (lines 1041, 1048, 1053); removeShare builds it once (line 1090). Four of these repeat UserID/Email/DisplayName/Role field-by-field from a domain.ShareWithUser-shaped source. A tiny helper toShareView(s domain.ShareWithUser) shareView would remove the copy-paste and make the `E…
🪰 Gadfly · advisory
🟡 **shareView is built inline five times; a toShareView helper would remove the copy-paste**
_maintainability · flagged by 1 model_
- **`internal/agent/tools.go:1041` — `shareView` is built inline five times with all four fields.** `listShares` (line 997) constructs it in a loop; `shareGarden` builds it three different ways across its branches (lines 1041, 1048, 1053); `removeShare` builds it once (line 1090). Four of these repeat `UserID/Email/DisplayName/Role` field-by-field from a `domain.ShareWithUser`-shaped source. A tiny helper `toShareView(s domain.ShareWithUser) shareView` would remove the copy-paste and make the `E…
<sub>🪰 Gadfly · advisory</sub>
🟡share_garden new-share success path omits DisplayName, inconsistent with its other return paths and list_shares/remove_share
maintainability · flagged by 1 model
internal/agent/tools.go:1053 — share_garden's new-share success path omits DisplayName (and reports the input email, not the stored one), while every other return path in the same tool sets it. The ErrShareExists branches (lines 1041, 1048) build shareView{... DisplayName: existing.DisplayName}, and remove_share/list_shares both populate it too. A model that shares a brand-new person back gets a share object shaped differently from the one it would get for a re-share or fro…
🪰 Gadfly · advisory
🟡 **share_garden new-share success path omits DisplayName, inconsistent with its other return paths and list_shares/remove_share**
_maintainability · flagged by 1 model_
- **`internal/agent/tools.go:1053` — `share_garden`'s new-share success path omits `DisplayName` (and reports the input email, not the stored one), while every other return path in the same tool sets it.** The `ErrShareExists` branches (lines 1041, 1048) build `shareView{... DisplayName: existing.DisplayName}`, and `remove_share`/`list_shares` both populate it too. A model that shares a brand-new person back gets a `share` object shaped differently from the one it would get for a re-share or fro…
<sub>🪰 Gadfly · advisory</sub>
🟠Unknown publicLink action with confirmed=false produces empty-description errUnconfirmed; clear validation error is unreachable from that path
correctness, error-handling, maintainability · flagged by 4 models
internal/agent/tools.go:1102 — Unknown action + confirmed=false produces a garbled error. The map lookup at line 1103 has keys only for "enable", "rotate", and "disable"; any other action value (including a misspelled one) causes a missing-key lookup that returns "", so errUnconfirmed("") yields "not done — is outward-facing, …" with an empty action description. The default branch at line 1124 that would return the clear "action must be get, enable, rotate or disable"…
🪰 Gadfly · advisory
🟠 **Unknown publicLink action with confirmed=false produces empty-description errUnconfirmed; clear validation error is unreachable from that path**
_correctness, error-handling, maintainability · flagged by 4 models_
- **`internal/agent/tools.go:1102`** — Unknown action + `confirmed=false` produces a garbled error. The map lookup at line 1103 has keys only for `"enable"`, `"rotate"`, and `"disable"`; any other action value (including a misspelled one) causes a missing-key lookup that returns `""`, so `errUnconfirmed("")` yields `"not done — is outward-facing, …"` with an empty action description. The `default` branch at line 1124 that would return the clear `"action must be get, enable, rotate or disable"`…
<sub>🪰 Gadfly · advisory</sub>
I've verified the security-relevant paths against the checked-out code. My findings:
VERDICT: No material issues found
This PR is a set of thin agent-tool adapters over already-existing, permission-gated service methods. I traced every new tool to its service call and every service call to its authorization check:
Authorization — all enforced at the service seam (verified):
listShares → Service.ListShares requires roleOwner (shares.go:20); the test that "a viewer can't list shares" confirms this end-to-end.
shareGarden → Service.AddShare/UpdateShareRole, both roleOwner + isShareRole role validation (shares.go:31,54,34,57). A role:"owner" request is rejected as ErrInvalidInput — no privilege escalation to owner via a share row.
removeShare → Service.RemoveShare routes through requireGardenRole(roleViewer) then owner-or-self (shares.go:69-75), so a non-participant gets masked ErrNotFound.
publicLink → Get/Enable/DisablePublicShareLink, all roleOwner (public.go:55,70,105).
deletePlanting → Service.DeletePlanting requires roleEditor on the parent object via objectForRole (plantings.go:289).
Every adapter passes a.actor, which is the authenticated session user threaded through Runner.Run(actorID, …) (runtime.go:93,131) — the agent can only act with the caller's own permissions. gardenId is a tool argument, but each service call re-checks ownership of that garden, so a cross-user access is impossible; within-user scope matches every other existing tool.
Token / secret handling (verified):
newPublicToken uses crypto/rand (service.go:100), not math/rand.
linkView only surfaces url (which embeds the token) when the link is enabled, and only to the owner (GetPublicShareLink is owner-gated). Emitting the token to the owner is the intended purpose; no leak to a lesser role or anonymous path. PublicShareURL just prefixes PANSY_BASE_URL — no secret material added to logs/backups.
Input validation: email/role are trimmed; email lookup is by exact account match (GetUserByEmail), findShare compares case-insensitively — no injection sink (parameterized store), no path/command/template surface. The confirmed=true gate is a model-behavior guardrail, not an access-control boundary, and the real boundary (owner-only) holds regardless of it.
One defense-in-depth observation, not a defect in this diff: garden notes flow into the system prompt, so prompt-injection is a general concern for the agent — but it can't escalate sharing here, because the share tools are owner-only and a chat about a garden shared to you (where you'd read someone else's notes) fails the owner check.
Nothing in the security lens is materially wrong.
🎯 Correctness — Minor issues
I've confirmed the finding against the actual source. The code at internal/agent/tools.go:1098-1126 matches the draft's description exactly: for an invalid action (e.g. "foo") sent without confirmed, action != "get" is true, so the map lookup map[...]{...}["foo"] returns "" and errUnconfirmed("") produces a refusal with an empty action name. The default: case that says "action must be get, enable, rotate or disable" is only reachable once past the confirm gate. The finding holds.
VERDICT: Minor issues
I reviewed this PR through the Correctness lens, verifying each new tool against the actual service layer, domain types, routes, and revert path (not just the diff).
One minor logic gap:
internal/agent/tools.go:1102 (publicLink): the unconfirmed branch (action != "get" && !args.Confirmed) does a map lookup map[...]{...}[action] for the refusal text. For an invalid action (e.g. "foo") sent withoutconfirmed, the lookup returns "", so the refusal reads "not done — ⎵ is outward-facing…" with an empty action name. The proper "action must be get, enable, rotate or disable" validation in the default: switch case is only reachable when confirmed=true. Impact is small (a confusingly-worded refusal, still a refusal) and requires the model to ignore the enum, but the two validation orderings don't agree. A cheap fix: validate the action against the allowed set before the confirm check.
Nothing blocking in my lens.
🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code:
Finding 1 (tools.go:1053): The new-share success return is shareView{UserID: share.UserID, Email: email, Role: share.Role} — no DisplayName, and it echoes the input email. The three other return sites (1041, 1048, 1090) all populate DisplayName. AddShare returns *domain.GardenShare (domain.go:314), which has neither Email nor DisplayName, so the divergence is real. Confirmed.
Finding 2 (tools.go:1102-1107): The guard action != "get" && !args.Confirmed does a map lookup that returns "" for any unknown action, feeding errUnconfirmed(""). The clean "action must be get, enable, rotate or disable" (line 1125) is only reachable through the default: case on the confirmed path. Confirmed.
Both survive. The verdict stands.
VERDICT: Minor issues
Reviewed through the code cleanliness & maintainability lens. The new tools closely follow the existing adapter method patterns, comments are apt, and the test is thorough. Two small consistency nits, both verified by reading internal/agent/tools.go:
internal/agent/tools.go:1053 — share_garden's new-share success path omits DisplayName (and reports the input email, not the stored one), while every other return path in the same tool sets it. The ErrShareExists branches (lines 1041, 1048) build shareView{... DisplayName: existing.DisplayName}, and remove_share/list_shares both populate it too. A model that shares a brand-new person back gets a share object shaped differently from the one it would get for a re-share or from list_shares. AddShare returns *domain.GardenShare (no Email/DisplayName — confirmed at domain.go:314), so the populated fields aren't free, but the divergence is still a leaky, surprising output shape for one tool. Low-churn fix: resolve via the existing findShare (or accept the gap and drop DisplayName everywhere for symmetry).
internal/agent/tools.go:1102-1107 — an unknown action passed withoutconfirmed=true produces a garbled refusal. The guard does a map lookup map[...]{"enable":…,"rotate":…,"disable":…}[action]; for any action other than those three (e.g. "share") the lookup returns "", so errUnconfirmed("") yields "not done — is outward-facing, so say exactly what you would do…". The clean "action must be get, enable, rotate or disable" message (line 1125) is only reachable on the confirmed=true path, because the default: case sits after the guard. Since the default: case already exists precisely because the enum isn't trusted, the unconfirmed branch should validate the action too — e.g. reject an unknown action before the confirm check, or fall through to the same "action must be…" error.
Everything else in scope is clean: errUnconfirmed, shareView, linkView, and findShare are well-named single-purpose helpers; the doc updates (CLAUDE.md, DESIGN.md, runtime.go prompt) all match convention.
⚡ Performance — No material issues found
Verified. Through the performance lens, this change is clean.
VERDICT: No material issues found
The new sharing tools and delete_planting are agent-tool adapters over single-query service methods, invoked at most a handful of times per conversation turn — not a hot path.
Things I checked and found fine:
shareGarden's conflict path (tools.go) calls findShare → svc.ListShares a second time after AddShare returns ErrShareExists, then UpdateShareRole. That's an extra ListSharesForGarden query, but only on the already-shared branch, and share lists are per-garden and tiny (a handful of accounts). No meaningful cost. Not worth flagging.
findShare's linear scan over ListShares results (tools.go) — O(n) over a small per-garden list, no nested queries. Fine.
listShares issues two store reads (ListShares + GetPublicShareLink) — bounded, one-shot. Fine.
ListShares/AddShare/RemoveShare (internal/service/shares.go) are each a role check plus one store call; ListSharesForGarden is a single query, not an N+1 per recipient. Verified by reading shares.go:19-71.
PublicShareURL (internal/service/public.go:92) is pure string concatenation. Fine.
No new work inside loops, no unbounded growth, no missing pagination (share sets are inherently small), no blocking calls introduced.
Nothing in the performance lane is materially wrong here.
🧯 Error handling & edge cases — Minor issues
The test at tools_test.go:1137 is decisive: it calls public_link with action: "share" and confirmed: true, and expects the refusal to contain "get, enable, rotate or disable" (the switch default message). For that test to pass, an off-enum action value must actually reach the function body — proving the enum tag is advisory to the model, not enforced by the framework. That confirms the draft's premise: an invalid action can arrive at publicLink.
Tracing the finding's scenario (action="share", confirmed=false):
errUnconfirmed("") → "...: not done — is outward-facing, so say exactly what you would do..." — empty action name, wrong guidance
The correct switch default message is never reached because the gate short-circuits.
The finding is confirmed against the real code. Impact is low (a misleading message to the model, not a crash), severity small, confidence high. The verdict stands.
VERDICT: Minor issues
Findings
internal/agent/tools.go:1102-1108 — Invalid action + unconfirmed yields a nonsensical refusal. The confirmation gate runs before action validation. For a non-empty, non-get action that isn't in {enable,rotate,disable} (e.g. "share"), when confirmed is false the map lookup returns the zero value "", so errUnconfirmed("") produces the message "not done — is outward-facing, so say exactly what you would do and ask the user first…" — an empty action name and the wrong guidance. The clear error ("action must be get, enable, rotate or disable") only fires on the switch default, which is unreachable in this branch because the confirmation check short-circuits first. The enum tag is advisory to the model, not enforced by the framework (the PR's own test at tools_test.go:1137 passes action:"share" through to the switch default with confirmed=true), so an off-enum value genuinely reaches this code; that unconfirmed unhappy path is untested. Impact is low (a confusing message to the model, not a crash); fix is to validate action against the allowed set before the confirmation check. Small.
I've verified all the key security properties. Here's my assessment:
No material issues found
The security posture of this PR is sound. Here's what I checked:
Authorization chain — verified solid
listShares, shareGarden, removeShare, publicLink all delegate to service methods (ListShares, AddShare, UpdateShareRole, RemoveShare, GetPublicShareLink, EnablePublicShareLink, DisablePublicShareLink) that call requireGardenRole(roleOwner) — checked in internal/service/shares.go and internal/service/public.go. A non-owner gets ErrNotFound (masked).
deletePlanting delegates to svc.DeletePlanting which calls objectForRole(ctx, actorID, pl.ObjectID, roleEditor) before touching data.
The actor identity (a.actor) is always sourced from the HTTP session (actor := mustActor(c) → runner.Run(ctx, actor.ID, ...) → newToolbox(svc, actorID, today)), never from tool arguments.
Public link token — cryptographically secure
newPublicToken() calls randToken(18) which uses crypto/rand (verified at internal/service/service.go:84). 144 bits of entropy; not guessable.
Role validation — double-enforced
The enum:"viewer,editor" struct tag is advisory for the LLM. The service's isShareRole() is the actual gate. Passing "owner" with confirmed=true fails at svc.AddShare with ErrInvalidInput — the test at refused("share_garden", ..., "role": "owner", "confirmed": true) covers this.
errUnconfirmed with unknown public_link action — no bypass
If action="share" and confirmed=true, the map lookup yields "" so the confirmation block is skipped; the switch's default case then returns ErrInvalidInput. No security gap.
Email reflection in error messages — low-risk by design
User-supplied email appears in errUnconfirmed(fmt.Sprintf("sharing this garden with %s as %s", email, role)) and errUnconfirmed("removing " + email + " from this garden"). This is reflected back to the same user's AI session. React escapes text content by default; no XSS path. The data never crosses an authorization boundary.
Acknowledged design limitation: confirmed=true can be spoofed by the model
Explicitly documented in CLAUDE.md ("Neither is a guarantee, and a new outward-facing tool gets the same pair"). The defense is two-layered (prompt + schema) but not cryptographic. Any real damage still requires the service's owner check to pass — spoofing the flag doesn't grant access the actor doesn't already have.
🎯 Correctness — Minor issues
The code confirms the finding exactly as described. Lines 1102-1107: the confirmed guard fires before the switch, and an unknown action causes the map lookup to return "", producing a garbled errUnconfirmed("") message. The default branch at line 1124-1125 is only reachable once the confirmed guard is already satisfied.
Minor issues
internal/agent/tools.go:1102 — Unknown action + confirmed=false produces a garbled error. The map lookup at line 1103 has keys only for "enable", "rotate", and "disable"; any other action value (including a misspelled one) causes a missing-key lookup that returns "", so errUnconfirmed("") yields "not done — is outward-facing, …" with an empty action description. The default branch at line 1124 that would return the clear "action must be get, enable, rotate or disable" message is inside the switch at line 1113, which is only entered after the confirmed guard is already satisfied — so an unknown action with confirmed=false can never reach it. Fix: validate the action against the known set before the confirmed check and return ErrInvalidInput immediately for unknown values; then let the confirmed gate run only for the known non-get actions.
🧹 Code cleanliness & maintainability — Minor issues
The logic is confirmed. With an unrecognized action (e.g. "foo") and confirmed=false:
action != "get" is true and !args.Confirmed is true → enters the guard
map[string]string{...}["foo"] returns "" (Go zero-value for a missing key)
errUnconfirmed("") produces the garbled message with a blank action name
The default: branch with the correct error is never reached because the switch is after this guard
The finding is genuine and confirmed.
Minor issues
internal/agent/tools.go:1102–1108 — in publicLink, the !args.Confirmed guard runs before action validation. For an unrecognized action (e.g. "foo") with confirmed=false, the map lookup map[string]string{...}[action] returns "" (Go zero value), and errUnconfirmed("") produces the grammatically broken message "not done — is outward-facing, so say exactly what you would do and ask the user first..." — the action name is blank. The default: branch that gives the correct "action must be get, enable, rotate or disable" error is only reachable when confirmed=true or action=="get". Fix: move action validation (an early-return for unrecognized values) before the confirmed check, or add a fallback to the description map.
The test covers the invalid-action case only with confirmed=true, so the garbled message on confirmed=false is not caught. In practice the LLM is guided by the enum tag, but the guard is what breaks when it doesn't listen.
⚡ Performance — No material issues found
No material issues found
I traced every query path in the new tools:
listShares issues two separate requireGardenRole checks — one inside ListShares, one inside GetPublicShareLink — fetching the garden row twice for the same ownership assertion. At SQLite scale with these dataset sizes this is a micro-optimisation, not a material concern.
shareGarden on ErrShareExists calls findShare → ListShares to resolve the email to a user ID, even though AddShare already did GetUserByEmail internally. The role-change path therefore executes up to six round-trips where three or four would do. There is no loop and no realistic scale at which garden shares grow large enough for this to be perceptible — LIMIT 1000 on a table that typically holds single-digit rows per garden makes this a non-issue.
removeShare similarly calls findShare (one requireGardenRole + one JOIN) then RemoveShare (another requireGardenRole): two ownership checks for one delete. Same analysis applies.
None of these are in loops, none operate on unbounded data, and the extra SQLite queries are sub-millisecond. The thresholds for flagging ("impact you can justify, not micro-optimisations") are not met here.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed. An unknown action (not in {get, enable, rotate, disable}) combined with confirmed=false hits the map lookup at line 1103–1107 which returns "", passing an empty string to errUnconfirmed — producing a blank where the action description should appear. The default branch that gives the proper "must be get, enable, rotate or disable" error is only reached after the confirmation guard, so it can't correct the bad message in this path.
Verdict: Minor issues
publicLink: unknown action with confirmed=false produces a blank error message
internal/agent/tools.go:1102–1108
ifaction!="get"&&!args.Confirmed{returnnil,errUnconfirmed(map[string]string{"enable":"turning the public link on, so anyone with the link can see this garden","rotate":"rotating the public link, so the old link stops working","disable":"turning the public link off, so the link stops working",}[action])// unknown key → ""}
If the model supplies an action outside {get, enable, rotate, disable}andconfirmed is false, the map lookup returns "" and the error reads:
invalid input: not done — is outward-facing, so say exactly what you would do and ask the user first…
— blank where the action description should be. With confirmed: true the default branch correctly rejects the unknown action with a clear message. The schema enum is advisory and a model can pass anything; the empty-string path is reachable. The fix is to validate the action before the confirmation guard, returning the clear "must be get, enable, rotate or disable" error for any unrecognised value regardless of confirmed.
All sharing/public-link operations route through Service.{ListShares,AddShare,UpdateShareRole,RemoveShare,GetPublicShareLink,EnablePublicShareLink,DisablePublicShareLink}, each of which enforces owner-only via requireGardenRole(..., roleOwner) — so the adapter can't bypass authz even when a model passes confirmed=true (verified in internal/service/shares.go:19-79 and internal/service/public.go:54-108). A non-owner's list_shares is rejected at the service layer (the test confirms this), which is the correct behavior for an authz gate.
The token is never returned bare to the model; linkView only emits the assembled address PublicShareURL(token) = cfg.BaseURL + /g/<token> (or a relative path when BaseURL unset) — and only when enabled. No token leakage via a separate field. BaseURL is normalized at config load (TrimRight of trailing /, internal/config/config.go:108), and PublicShareLink.Token is omitempty, so a disabled link carries no token (verified internal/service/public.go:12-22,90-100).
shareGarden's role is lowered and validated against isShareRole (viewer/editor) inside AddShare/UpdateShareRole; an "owner" role is rejected with ErrInvalidInput (verified internal/service/shares.go:34-36,57-59; the test exercises the owner refusal). No role-injection/escalation path.
delete_planting delegates to Service.DeletePlanting and inherits its permission checks; no new untrusted-input sink introduced here (signature is just a plantingId int64).
Email/role inputs are trimmed and used as parameters to store lookups (GetUserByEmail, CreateShare), not interpolated into SQL; no injection surface in the adapter.
No security-relevant issues found within this lens.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: max steps reached without a final answer (max 14)
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/agent/tools.go:1041 — shareView is built inline five times with all four fields.listShares (line 997) constructs it in a loop; shareGarden builds it three different ways across its branches (lines 1041, 1048, 1053); removeShare builds it once (line 1090). Four of these repeat UserID/Email/DisplayName/Role field-by-field from a domain.ShareWithUser-shaped source. A tiny helper toShareView(s domain.ShareWithUser) shareView would remove the copy-paste and make the Email-vs-existing.Email / updated.Role-mixing in the ErrShareExists branch (lines 1048–1049) easier to follow at a glance.
internal/agent/tools.go:1008 — linkView returns map[string]any while shareView is a typed struct. Both are "the view the tools report," and listShares returns them side by side in the same map (line 1003). The link gets an untyped map (so callers/tests reach in with publicLink["enabled"], see tools_test.go:1085–1088), the share gets a typed struct. Inconsistent for conceptually sibling output types. A small linkView struct (Enabled bool / URL string \json:"url,omitempty"`) would match shareViewand keep theomitemptyrule the code now hand-implements with theif link.Enabled` guard.
internal/agent/tools.go:1103 — the per-action refusal map is rebuilt on every publicLink call, and the action vocabulary is re-spelled by hand in the default error. A package-level var publicLinkActions = map[string]string{...} would (a) stop rebuilding the map per call and (b) make the action list one place, shared with the default error message at line 1125 which currently re-spells "get, enable, rotate or disable" by hand. The enum tag makes errUnconfirmed("") with an empty key unreachable today, so this is purely a readability nit.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, the new tool handlers are all single-request, bounded paths:
listShares — one ListShares (capped at maxSharesListed=1000 in internal/store/shares.go:45) plus one GetPublicShareLink. No pagination concern at household scale, and the store already hard-limits the query.
removeShare / shareGarden's ErrShareExists path — call findShare, which re-runs ListShares (the full join) just to resolve an email to a UserID. This is one extra round-trip per outward-facing action, not in a loop, and bounded by the same 1000 cap. It's a mildly avoidable query (the service's AddShare already did GetUserByEmail and could in principle surface the existing share instead of forcing the adapter to re-scan the list), but the impact is negligible and it is not an N+1. Noted as trivial, not a finding.
deletePlanting — GetPlanting + objectForRole + DeletePlanting + record; no hot-loop or allocation concern.
publicLink — single service call per action; linkView allocates one small map. Fine.
I verified ListSharesForGarden's LIMIT, the service signatures, and the DeletePlanting/RemovePlanting implementations by reading internal/store/shares.go, internal/service/shares.go, internal/service/public.go, and internal/service/plantings.go.
🧯 Error handling & edge cases — Minor issues
Config loading normalizes BaseURL with strings.TrimRight(..., "/"), so the trailing-slash finding is invalid. The first finding about the empty action name stands.
Verdict: Minor issues
internal/agent/tools.go:1102-1108 — publicLink looks up the refusal text via a map[string]string indexed by action. For an action outside {get,enable,rotate,disable} (the enum is a schema hint, not a hard gate — the test itself passes "share"), with confirmed=false, the lookup returns the zero string "", so errUnconfirmed("") produces a malformed refusal: not done — is outward-facing, so say exactly what you would do... — a double space and no action named, defeating the point of naming the action. The default branch that would give a proper "action must be get, enable, rotate or disable" error is only reached after the confirmation check, so it never fires for an unconfirmed bad action. Suggested fix: validate the action against the known set before the confirmation check, or fall back to a generic "that action" string when the map lookup is absent.
I traced the new sharing tools and delete_planting through the service layer and verified:
Authz is enforced at the service layer: share_garden, remove_share, and public_link (all mutating actions) delegate to AddShare, RemoveShare, EnablePublicShareLink, DisablePublicShareLink, which all require roleOwner via requireGardenRole. list_shares is read-only and also owner-gated. The agent adapter's findShare calls ListShares, so an owner check happens before RemoveShare is ever reached.
confirmed=true gating works as designed: each outward-facing tool returns a precise refusal naming the action when confirmed is missing/false. This is the intended defense-in-depth (prompt + schema-level boolean), acknowledged in the PR description as not a cryptographic guarantee.
No injection vectors: email and role inputs are trimmed/lower-cased and passed to parameterized store queries (GetUserByEmail, CreateShare). Error messages that embed the email/role are returned to the LLM, not rendered as raw HTML.
publicLink map lookup is safe: a missing key in the action-description map returns the zero-value empty string (Go maps do not panic on missing keys), so an unknown action with confirmed=false yields a harmless generic refusal; an unknown action with confirmed=true hits the explicit default case in the switch.
delete_planting is consistent with existing destructive tools: it checks roleEditor via objectForRole, is recorded in the history for undo, and is correctly classified as non-outward-facing (it doesn't change who can see the garden).
🎯 Correctness — Minor issues
Minor issues found
PublicShareURL trusts cfg.BaseURL without trimming a trailing slash, risking a double-slash URL. internal/service/public.go:94-99 concatenates s.cfg.BaseURL + "/g/" + token. The config loader does strings.TrimRight(..., "/"), but nothing prevents a test or future code path from setting cfg.BaseURL directly to a value ending in /. The result would be https://example.com//g/<token>, a malformed URL that the model would hand to the user. Fix: Defensively trim before concatenation: strings.TrimRight(s.cfg.BaseURL, "/") + path.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/agent/tools.go:1102-1108 — publicLink allocates a 3-entry map[string]string on every outward-facing call just to index into it once. A small switch or inline if action == "enable" { … } would avoid the allocation, be easier to read, and prevent an invalid action from silently producing an empty description (map["foo"] → "") instead of the validation error the switch default handles later.
Suggested fix: replace the map literal with a switch that assigns the description string, then pass it to errUnconfirmed.
⚡ Performance — No material issues found
No material issues found.
The sharing tool wrappers added in this PR are bounded and do not introduce measurable regressions:
listShares fetches at most maxSharesListed = 1000 shares (already enforced in the store layer) and one public-link state — bounded, no pagination missing.
findShare does a linear scan over that bounded slice to resolve email→share. While a direct store lookup would be tighter, 1000 iterations is a hard cap and the tool is invoked one call at a time by the agent loop, so no N+1 or quadratic blow-up.
shareGarden on ErrShareExists pays one extra ListShares round-trip before updating the role, but this is an edge path (re-sharing with a different role), not a hot loop.
deletePlanting issues the same number of store calls as sibling mutation tools (GetPlanting, objectForRole, DeletePlanting, record) — no new alloc or copy path stands out.
🧯 Error handling & edge cases — No material issues found
No material issues found.
I reviewed the diff through the error-handling and edge-case lens. The outward-facing sharing tools (share_garden, remove_share, public_link) correctly refuse destructive actions without confirmed=true, return named errors so the model can ask precisely, and validate inputs before proceeding. No errors are swallowed, no nil pointers are dereferenced, and there are no missing cleanup paths. The delete_planting wrapper passes through to the service layer which records the change for undo, and the test verifies the round-trip. All edge cases I could identify (empty inputs, unknown actions, already-disabled links, already-shared users, case-insensitive email matching, non-owner access) are handled or gracefully returned as errors.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 4 models
**Verdict: Minor issues** · 5 findings (1 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | Unknown publicLink action with confirmed=false produces empty-description errUnconfirmed; clear validation error is unreachable from that path | `internal/agent/tools.go:1102` | 4/4 | correctness, error-handling, maintainability |
<details><summary>4 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | linkView returns untyped map[string]any while shareView is a typed struct — inconsistent representation of sibling view types | `internal/agent/tools.go:1008` | glm-5.2:cloud | maintainability |
| 🟡 | shareView is built inline five times; a toShareView helper would remove the copy-paste | `internal/agent/tools.go:1041` | glm-5.2:cloud | maintainability |
| 🟡 | share_garden new-share success path omits DisplayName, inconsistent with its other return paths and list_shares/remove_share | `internal/agent/tools.go:1053` | claude-code/opus | maintainability |
| 🟡 | PublicShareURL does not trim trailing slash from BaseURL before concatenation | `internal/service/public.go:96` | kimi-k2.6:cloud | correctness |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've verified the security-relevant paths against the checked-out code. My findings:
## VERDICT: No material issues found
This PR is a set of thin agent-tool adapters over already-existing, permission-gated service methods. I traced every new tool to its service call and every service call to its authorization check:
**Authorization — all enforced at the service seam (verified):**
- `listShares` → `Service.ListShares` requires `roleOwner` (`shares.go:20`); the test that "a viewer can't list shares" confirms this end-to-end.
- `shareGarden` → `Service.AddShare`/`UpdateShareRole`, both `roleOwner` + `isShareRole` role validation (`shares.go:31,54,34,57`). A `role:"owner"` request is rejected as `ErrInvalidInput` — no privilege escalation to owner via a share row.
- `removeShare` → `Service.RemoveShare` routes through `requireGardenRole(roleViewer)` then owner-or-self (`shares.go:69-75`), so a non-participant gets masked `ErrNotFound`.
- `publicLink` → `Get/Enable/DisablePublicShareLink`, all `roleOwner` (`public.go:55,70,105`).
- `deletePlanting` → `Service.DeletePlanting` requires `roleEditor` on the parent object via `objectForRole` (`plantings.go:289`).
Every adapter passes `a.actor`, which is the authenticated session user threaded through `Runner.Run(actorID, …)` (`runtime.go:93,131`) — the agent can only act with the caller's own permissions. `gardenId` is a tool argument, but each service call re-checks ownership of *that* garden, so a cross-user access is impossible; within-user scope matches every other existing tool.
**Token / secret handling (verified):**
- `newPublicToken` uses `crypto/rand` (`service.go:100`), not `math/rand`.
- `linkView` only surfaces `url` (which embeds the token) when the link is enabled, and only to the owner (`GetPublicShareLink` is owner-gated). Emitting the token to the owner is the intended purpose; no leak to a lesser role or anonymous path. `PublicShareURL` just prefixes `PANSY_BASE_URL` — no secret material added to logs/backups.
**Input validation:** email/role are trimmed; email lookup is by exact account match (`GetUserByEmail`), `findShare` compares case-insensitively — no injection sink (parameterized store), no path/command/template surface. The `confirmed=true` gate is a model-behavior guardrail, not an access-control boundary, and the real boundary (owner-only) holds regardless of it.
One defense-in-depth observation, not a defect in this diff: garden notes flow into the system prompt, so prompt-injection is a general concern for the agent — but it can't escalate sharing here, because the share tools are owner-only and a chat about a garden shared *to* you (where you'd read someone else's notes) fails the owner check.
Nothing in the security lens is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
I've confirmed the finding against the actual source. The code at `internal/agent/tools.go:1098-1126` matches the draft's description exactly: for an invalid action (e.g. `"foo"`) sent without `confirmed`, `action != "get"` is true, so the map lookup `map[...]{...}["foo"]` returns `""` and `errUnconfirmed("")` produces a refusal with an empty action name. The `default:` case that says "action must be get, enable, rotate or disable" is only reachable once past the confirm gate. The finding holds.
## VERDICT: Minor issues
I reviewed this PR through the **Correctness** lens, verifying each new tool against the actual service layer, domain types, routes, and revert path (not just the diff).
**One minor logic gap:**
- `internal/agent/tools.go:1102` (`publicLink`): the unconfirmed branch (`action != "get" && !args.Confirmed`) does a map lookup `map[...]{...}[action]` for the refusal text. For an *invalid* action (e.g. `"foo"`) sent **without** `confirmed`, the lookup returns `""`, so the refusal reads "not done — **⎵** is outward-facing…" with an empty action name. The proper "action must be get, enable, rotate or disable" validation in the `default:` switch case is only reachable when `confirmed=true`. Impact is small (a confusingly-worded refusal, still a refusal) and requires the model to ignore the `enum`, but the two validation orderings don't agree. A cheap fix: validate the action against the allowed set before the confirm check.
Nothing blocking in my lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings are confirmed against the actual code:
- **Finding 1** (`tools.go:1053`): The new-share success return is `shareView{UserID: share.UserID, Email: email, Role: share.Role}` — no `DisplayName`, and it echoes the input `email`. The three other return sites (1041, 1048, 1090) all populate `DisplayName`. `AddShare` returns `*domain.GardenShare` (domain.go:314), which has neither `Email` nor `DisplayName`, so the divergence is real. Confirmed.
- **Finding 2** (`tools.go:1102-1107`): The guard `action != "get" && !args.Confirmed` does a map lookup that returns `""` for any unknown action, feeding `errUnconfirmed("")`. The clean "action must be get, enable, rotate or disable" (line 1125) is only reachable through the `default:` case on the confirmed path. Confirmed.
Both survive. The verdict stands.
## VERDICT: Minor issues
Reviewed through the code cleanliness & maintainability lens. The new tools closely follow the existing `adapter` method patterns, comments are apt, and the test is thorough. Two small consistency nits, both verified by reading `internal/agent/tools.go`:
- **`internal/agent/tools.go:1053` — `share_garden`'s new-share success path omits `DisplayName` (and reports the input email, not the stored one), while every other return path in the same tool sets it.** The `ErrShareExists` branches (lines 1041, 1048) build `shareView{... DisplayName: existing.DisplayName}`, and `remove_share`/`list_shares` both populate it too. A model that shares a brand-new person back gets a `share` object shaped differently from the one it would get for a re-share or from `list_shares`. `AddShare` returns `*domain.GardenShare` (no `Email`/`DisplayName` — confirmed at `domain.go:314`), so the populated fields aren't free, but the divergence is still a leaky, surprising output shape for one tool. Low-churn fix: resolve via the existing `findShare` (or accept the gap and drop `DisplayName` everywhere for symmetry).
- **`internal/agent/tools.go:1102-1107` — an unknown `action` passed *without* `confirmed=true` produces a garbled refusal.** The guard does a map lookup `map[...]{"enable":…,"rotate":…,"disable":…}[action]`; for any action other than those three (e.g. `"share"`) the lookup returns `""`, so `errUnconfirmed("")` yields "not done — <empty> is outward-facing, so say exactly what you would do…". The clean "action must be get, enable, rotate or disable" message (line 1125) is only reachable on the `confirmed=true` path, because the `default:` case sits after the guard. Since the `default:` case already exists precisely because the enum isn't trusted, the unconfirmed branch should validate the action too — e.g. reject an unknown action before the confirm check, or fall through to the same "action must be…" error.
Everything else in scope is clean: `errUnconfirmed`, `shareView`, `linkView`, and `findShare` are well-named single-purpose helpers; the doc updates (CLAUDE.md, DESIGN.md, runtime.go prompt) all match convention.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Verified. Through the performance lens, this change is clean.
## VERDICT: No material issues found
The new sharing tools and `delete_planting` are agent-tool adapters over single-query service methods, invoked at most a handful of times per conversation turn — not a hot path.
Things I checked and found fine:
- **`shareGarden`'s conflict path** (`tools.go`) calls `findShare` → `svc.ListShares` a *second* time after `AddShare` returns `ErrShareExists`, then `UpdateShareRole`. That's an extra `ListSharesForGarden` query, but only on the already-shared branch, and share lists are per-garden and tiny (a handful of accounts). No meaningful cost. Not worth flagging.
- **`findShare`'s linear scan** over `ListShares` results (`tools.go`) — O(n) over a small per-garden list, no nested queries. Fine.
- **`listShares`** issues two store reads (`ListShares` + `GetPublicShareLink`) — bounded, one-shot. Fine.
- **`ListShares`/`AddShare`/`RemoveShare`** (`internal/service/shares.go`) are each a role check plus one store call; `ListSharesForGarden` is a single query, not an N+1 per recipient. Verified by reading `shares.go:19-71`.
- **`PublicShareURL`** (`internal/service/public.go:92`) is pure string concatenation. Fine.
- No new work inside loops, no unbounded growth, no missing pagination (share sets are inherently small), no blocking calls introduced.
Nothing in the performance lane is materially wrong here.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The test at `tools_test.go:1137` is decisive: it calls `public_link` with `action: "share"` and `confirmed: true`, and expects the refusal to contain `"get, enable, rotate or disable"` (the `switch` default message). For that test to pass, an off-enum `action` value must actually reach the function body — proving the `enum` tag is **advisory to the model, not enforced** by the framework. That confirms the draft's premise: an invalid action can arrive at `publicLink`.
Tracing the finding's scenario (`action="share"`, `confirmed=false`):
- `action` = `"share"` (non-empty, not `"get"`)
- `action != "get" && !args.Confirmed` → true → gate fires first
- `map[...]["share"]` → no such key → zero value `""`
- `errUnconfirmed("")` → `"...: not done — is outward-facing, so say exactly what you would do..."` — empty action name, wrong guidance
- The correct `switch default` message is never reached because the gate short-circuits.
The finding is confirmed against the real code. Impact is low (a misleading message to the model, not a crash), severity small, confidence high. The verdict stands.
## VERDICT: Minor issues
**Findings**
- `internal/agent/tools.go:1102-1108` — **Invalid `action` + unconfirmed yields a nonsensical refusal.** The confirmation gate runs *before* action validation. For a non-empty, non-`get` action that isn't in `{enable,rotate,disable}` (e.g. `"share"`), when `confirmed` is false the map lookup returns the zero value `""`, so `errUnconfirmed("")` produces the message *"not done — is outward-facing, so say exactly what you would do and ask the user first…"* — an empty action name and the wrong guidance. The clear error ("action must be get, enable, rotate or disable") only fires on the `switch` default, which is unreachable in this branch because the confirmation check short-circuits first. The `enum` tag is advisory to the model, not enforced by the framework (the PR's own test at `tools_test.go:1137` passes `action:"share"` through to the switch default with `confirmed=true`), so an off-enum value genuinely reaches this code; that unconfirmed unhappy path is untested. Impact is low (a confusing message to the model, not a crash); fix is to validate `action` against the allowed set before the confirmation check. Small.
</details>
</details>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've verified all the key security properties. Here's my assessment:
---
**No material issues found**
The security posture of this PR is sound. Here's what I checked:
**Authorization chain — verified solid**
- `listShares`, `shareGarden`, `removeShare`, `publicLink` all delegate to service methods (`ListShares`, `AddShare`, `UpdateShareRole`, `RemoveShare`, `GetPublicShareLink`, `EnablePublicShareLink`, `DisablePublicShareLink`) that call `requireGardenRole(roleOwner)` — checked in `internal/service/shares.go` and `internal/service/public.go`. A non-owner gets `ErrNotFound` (masked).
- `deletePlanting` delegates to `svc.DeletePlanting` which calls `objectForRole(ctx, actorID, pl.ObjectID, roleEditor)` before touching data.
- The actor identity (`a.actor`) is always sourced from the HTTP session (`actor := mustActor(c)` → `runner.Run(ctx, actor.ID, ...)` → `newToolbox(svc, actorID, today)`), never from tool arguments.
**Public link token — cryptographically secure**
- `newPublicToken()` calls `randToken(18)` which uses `crypto/rand` (verified at `internal/service/service.go:84`). 144 bits of entropy; not guessable.
**Role validation — double-enforced**
- The `enum:"viewer,editor"` struct tag is advisory for the LLM. The service's `isShareRole()` is the actual gate. Passing `"owner"` with `confirmed=true` fails at `svc.AddShare` with `ErrInvalidInput` — the test at `refused("share_garden", ..., "role": "owner", "confirmed": true)` covers this.
**`errUnconfirmed` with unknown `public_link` action — no bypass**
- If `action="share"` and `confirmed=true`, the map lookup yields `""` so the confirmation block is skipped; the switch's `default` case then returns `ErrInvalidInput`. No security gap.
**Email reflection in error messages — low-risk by design**
- User-supplied email appears in `errUnconfirmed(fmt.Sprintf("sharing this garden with %s as %s", email, role))` and `errUnconfirmed("removing " + email + " from this garden")`. This is reflected back to the same user's AI session. React escapes text content by default; no XSS path. The data never crosses an authorization boundary.
**Acknowledged design limitation: `confirmed=true` can be spoofed by the model**
- Explicitly documented in CLAUDE.md ("Neither is a guarantee, and a new outward-facing tool gets the same pair"). The defense is two-layered (prompt + schema) but not cryptographic. Any real damage still requires the service's owner check to pass — spoofing the flag doesn't grant access the actor doesn't already have.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The code confirms the finding exactly as described. Lines 1102-1107: the confirmed guard fires before the switch, and an unknown action causes the map lookup to return `""`, producing a garbled `errUnconfirmed("")` message. The `default` branch at line 1124-1125 is only reachable once the confirmed guard is already satisfied.
**Minor issues**
- **`internal/agent/tools.go:1102`** — Unknown action + `confirmed=false` produces a garbled error. The map lookup at line 1103 has keys only for `"enable"`, `"rotate"`, and `"disable"`; any other action value (including a misspelled one) causes a missing-key lookup that returns `""`, so `errUnconfirmed("")` yields `"not done — is outward-facing, …"` with an empty action description. The `default` branch at line 1124 that would return the clear `"action must be get, enable, rotate or disable"` message is inside the `switch` at line 1113, which is only entered after the confirmed guard is already satisfied — so an unknown action with `confirmed=false` can never reach it. Fix: validate the action against the known set before the confirmed check and return `ErrInvalidInput` immediately for unknown values; then let the confirmed gate run only for the known non-get actions.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
The logic is confirmed. With an unrecognized action (e.g. `"foo"`) and `confirmed=false`:
- `action != "get"` is true and `!args.Confirmed` is true → enters the guard
- `map[string]string{...}["foo"]` returns `""` (Go zero-value for a missing key)
- `errUnconfirmed("")` produces the garbled message with a blank action name
- The `default:` branch with the correct error is never reached because the switch is after this guard
The finding is genuine and confirmed.
---
**Minor issues**
- **`internal/agent/tools.go:1102–1108`** — in `publicLink`, the `!args.Confirmed` guard runs before action validation. For an unrecognized action (e.g. `"foo"`) with `confirmed=false`, the map lookup `map[string]string{...}[action]` returns `""` (Go zero value), and `errUnconfirmed("")` produces the grammatically broken message `"not done — is outward-facing, so say exactly what you would do and ask the user first..."` — the action name is blank. The `default:` branch that gives the correct `"action must be get, enable, rotate or disable"` error is only reachable when `confirmed=true` or `action=="get"`. Fix: move action validation (an early-return for unrecognized values) before the `confirmed` check, or add a fallback to the description map.
The test covers the invalid-action case only with `confirmed=true`, so the garbled message on `confirmed=false` is not caught. In practice the LLM is guided by the `enum` tag, but the guard is what breaks when it doesn't listen.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
I traced every query path in the new tools:
- **`listShares`** issues two separate `requireGardenRole` checks — one inside `ListShares`, one inside `GetPublicShareLink` — fetching the garden row twice for the same ownership assertion. At SQLite scale with these dataset sizes this is a micro-optimisation, not a material concern.
- **`shareGarden` on `ErrShareExists`** calls `findShare` → `ListShares` to resolve the email to a user ID, even though `AddShare` already did `GetUserByEmail` internally. The role-change path therefore executes up to six round-trips where three or four would do. There is no loop and no realistic scale at which garden shares grow large enough for this to be perceptible — `LIMIT 1000` on a table that typically holds single-digit rows per garden makes this a non-issue.
- **`removeShare`** similarly calls `findShare` (one `requireGardenRole` + one JOIN) then `RemoveShare` (another `requireGardenRole`): two ownership checks for one delete. Same analysis applies.
None of these are in loops, none operate on unbounded data, and the extra SQLite queries are sub-millisecond. The thresholds for flagging ("impact you can justify, not micro-optimisations") are not met here.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed. An unknown `action` (not in `{get, enable, rotate, disable}`) combined with `confirmed=false` hits the map lookup at line 1103–1107 which returns `""`, passing an empty string to `errUnconfirmed` — producing a blank where the action description should appear. The `default` branch that gives the proper "must be get, enable, rotate or disable" error is only reached after the confirmation guard, so it can't correct the bad message in this path.
---
**Verdict: Minor issues**
**`publicLink`: unknown `action` with `confirmed=false` produces a blank error message**
`internal/agent/tools.go:1102–1108`
```go
if action != "get" && !args.Confirmed {
return nil, errUnconfirmed(map[string]string{
"enable": "turning the public link on, so anyone with the link can see this garden",
"rotate": "rotating the public link, so the old link stops working",
"disable": "turning the public link off, so the link stops working",
}[action]) // unknown key → ""
}
```
If the model supplies an action outside `{get, enable, rotate, disable}` **and** `confirmed` is false, the map lookup returns `""` and the error reads:
> `invalid input: not done — is outward-facing, so say exactly what you would do and ask the user first…`
— blank where the action description should be. With `confirmed: true` the `default` branch correctly rejects the unknown action with a clear message. The schema enum is advisory and a model can pass anything; the empty-string path is reachable. The fix is to validate the action before the confirmation guard, returning the clear "must be get, enable, rotate or disable" error for any unrecognised value regardless of `confirmed`.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
- All sharing/public-link operations route through `Service.{ListShares,AddShare,UpdateShareRole,RemoveShare,GetPublicShareLink,EnablePublicShareLink,DisablePublicShareLink}`, each of which enforces owner-only via `requireGardenRole(..., roleOwner)` — so the adapter can't bypass authz even when a model passes `confirmed=true` (verified in `internal/service/shares.go:19-79` and `internal/service/public.go:54-108`). A non-owner's `list_shares` is rejected at the service layer (the test confirms this), which is the correct behavior for an authz gate.
- The token is never returned bare to the model; `linkView` only emits the assembled address `PublicShareURL(token)` = `cfg.BaseURL + /g/<token>` (or a relative path when `BaseURL` unset) — and only when enabled. No token leakage via a separate field. `BaseURL` is normalized at config load (`TrimRight` of trailing `/`, `internal/config/config.go:108`), and `PublicShareLink.Token` is `omitempty`, so a disabled link carries no token (verified `internal/service/public.go:12-22,90-100`).
- `shareGarden`'s `role` is lowered and validated against `isShareRole` (viewer/editor) inside `AddShare`/`UpdateShareRole`; an "owner" role is rejected with `ErrInvalidInput` (verified `internal/service/shares.go:34-36,57-59`; the test exercises the `owner` refusal). No role-injection/escalation path.
- `delete_planting` delegates to `Service.DeletePlanting` and inherits its permission checks; no new untrusted-input sink introduced here (signature is just a `plantingId int64`).
- Email/role inputs are trimmed and used as parameters to store lookups (`GetUserByEmail`, `CreateShare`), not interpolated into SQL; no injection surface in the adapter.
No security-relevant issues found within this lens.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: max steps reached without a final answer (max 14)
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`internal/agent/tools.go:1041` — `shareView` is built inline five times with all four fields.** `listShares` (line 997) constructs it in a loop; `shareGarden` builds it three different ways across its branches (lines 1041, 1048, 1053); `removeShare` builds it once (line 1090). Four of these repeat `UserID/Email/DisplayName/Role` field-by-field from a `domain.ShareWithUser`-shaped source. A tiny helper `toShareView(s domain.ShareWithUser) shareView` would remove the copy-paste and make the `Email`-vs-`existing.Email` / `updated.Role`-mixing in the `ErrShareExists` branch (lines 1048–1049) easier to follow at a glance.
- **`internal/agent/tools.go:1008` — `linkView` returns `map[string]any` while `shareView` is a typed struct.** Both are "the view the tools report," and `listShares` returns them side by side in the same map (line 1003). The link gets an untyped map (so callers/tests reach in with `publicLink["enabled"]`, see tools_test.go:1085–1088), the share gets a typed struct. Inconsistent for conceptually sibling output types. A small `linkView` struct (`Enabled bool` / `URL string \`json:"url,omitempty"\``) would match `shareView` and keep the `omitempty` rule the code now hand-implements with the `if link.Enabled` guard.
- **`internal/agent/tools.go:1103` — the per-action refusal map is rebuilt on every `publicLink` call, and the action vocabulary is re-spelled by hand in the `default` error.** A package-level `var publicLinkActions = map[string]string{...}` would (a) stop rebuilding the map per call and (b) make the action list one place, shared with the `default` error message at line 1125 which currently re-spells "get, enable, rotate or disable" by hand. The `enum` tag makes `errUnconfirmed("")` with an empty key unreachable today, so this is purely a readability nit.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the performance lens, the new tool handlers are all single-request, bounded paths:
- `listShares` — one `ListShares` (capped at `maxSharesListed=1000` in `internal/store/shares.go:45`) plus one `GetPublicShareLink`. No pagination concern at household scale, and the store already hard-limits the query.
- `removeShare` / `shareGarden`'s `ErrShareExists` path — call `findShare`, which re-runs `ListShares` (the full join) just to resolve an email to a `UserID`. This is one extra round-trip per outward-facing action, not in a loop, and bounded by the same 1000 cap. It's a mildly avoidable query (the service's `AddShare` already did `GetUserByEmail` and could in principle surface the existing share instead of forcing the adapter to re-scan the list), but the impact is negligible and it is not an N+1. Noted as trivial, not a finding.
- `deletePlanting` — `GetPlanting` + `objectForRole` + `DeletePlanting` + `record`; no hot-loop or allocation concern.
- `publicLink` — single service call per action; `linkView` allocates one small map. Fine.
I verified `ListSharesForGarden`'s `LIMIT`, the service signatures, and the `DeletePlanting`/`RemovePlanting` implementations by reading `internal/store/shares.go`, `internal/service/shares.go`, `internal/service/public.go`, and `internal/service/plantings.go`.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Config loading normalizes `BaseURL` with `strings.TrimRight(..., "/")`, so the trailing-slash finding is invalid. The first finding about the empty action name stands.
**Verdict: Minor issues**
- `internal/agent/tools.go:1102-1108` — `publicLink` looks up the refusal text via a `map[string]string` indexed by `action`. For an action outside `{get,enable,rotate,disable}` (the `enum` is a schema hint, not a hard gate — the test itself passes `"share"`), with `confirmed=false`, the lookup returns the zero string `""`, so `errUnconfirmed("")` produces a malformed refusal: `not done — is outward-facing, so say exactly what you would do...` — a double space and no action named, defeating the point of naming the action. The `default` branch that would give a proper "action must be get, enable, rotate or disable" error is only reached *after* the confirmation check, so it never fires for an unconfirmed bad action. Suggested fix: validate the action against the known set *before* the confirmation check, or fall back to a generic "that action" string when the map lookup is absent.
</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>
**Verdict: No material issues found**
I traced the new sharing tools and `delete_planting` through the service layer and verified:
- **Authz is enforced at the service layer**: `share_garden`, `remove_share`, and `public_link` (all mutating actions) delegate to `AddShare`, `RemoveShare`, `EnablePublicShareLink`, `DisablePublicShareLink`, which all require `roleOwner` via `requireGardenRole`. `list_shares` is read-only and also owner-gated. The agent adapter's `findShare` calls `ListShares`, so an owner check happens before `RemoveShare` is ever reached.
- **`confirmed=true` gating works as designed**: each outward-facing tool returns a precise refusal naming the action when `confirmed` is missing/false. This is the intended defense-in-depth (prompt + schema-level boolean), acknowledged in the PR description as not a cryptographic guarantee.
- **No injection vectors**: `email` and `role` inputs are trimmed/lower-cased and passed to parameterized store queries (`GetUserByEmail`, `CreateShare`). Error messages that embed the email/role are returned to the LLM, not rendered as raw HTML.
- **`publicLink` map lookup is safe**: a missing key in the action-description map returns the zero-value empty string (Go maps do not panic on missing keys), so an unknown action with `confirmed=false` yields a harmless generic refusal; an unknown action with `confirmed=true` hits the explicit `default` case in the `switch`.
- **`delete_planting` is consistent with existing destructive tools**: it checks `roleEditor` via `objectForRole`, is recorded in the history for undo, and is correctly classified as non-outward-facing (it doesn't change who can see the garden).
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Minor issues found
- **`PublicShareURL` trusts `cfg.BaseURL` without trimming a trailing slash, risking a double-slash URL.**
`internal/service/public.go:94-99` concatenates `s.cfg.BaseURL + "/g/" + token`. The config loader does `strings.TrimRight(..., "/")`, but nothing prevents a test or future code path from setting `cfg.BaseURL` directly to a value ending in `/`. The result would be `https://example.com//g/<token>`, a malformed URL that the model would hand to the user.
**Fix:** Defensively trim before concatenation: `strings.TrimRight(s.cfg.BaseURL, "/") + path`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Minor issues
- **`internal/agent/tools.go:1102-1108`** — `publicLink` allocates a 3-entry `map[string]string` on every outward-facing call just to index into it once. A small switch or inline `if action == "enable" { … }` would avoid the allocation, be easier to read, and prevent an invalid action from silently producing an empty description (`map["foo"]` → `""`) instead of the validation error the `switch default` handles later.
Suggested fix: replace the map literal with a switch that assigns the description string, then pass it to `errUnconfirmed`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
The sharing tool wrappers added in this PR are bounded and do not introduce measurable regressions:
- `listShares` fetches at most `maxSharesListed = 1000` shares (already enforced in the store layer) and one public-link state — bounded, no pagination missing.
- `findShare` does a linear scan over that bounded slice to resolve email→share. While a direct store lookup would be tighter, 1000 iterations is a hard cap and the tool is invoked one call at a time by the agent loop, so no N+1 or quadratic blow-up.
- `shareGarden` on `ErrShareExists` pays one extra `ListShares` round-trip before updating the role, but this is an edge path (re-sharing with a different role), not a hot loop.
- `deletePlanting` issues the same number of store calls as sibling mutation tools (`GetPlanting`, `objectForRole`, `DeletePlanting`, `record`) — no new alloc or copy path stands out.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found.**
I reviewed the diff through the error-handling and edge-case lens. The outward-facing sharing tools (`share_garden`, `remove_share`, `public_link`) correctly refuse destructive actions without `confirmed=true`, return named errors so the model can ask precisely, and validate inputs before proceeding. No errors are swallowed, no nil pointers are dereferenced, and there are no missing cleanup paths. The `delete_planting` wrapper passes through to the service layer which records the change for undo, and the test verifies the round-trip. All edge cases I could identify (empty inputs, unknown actions, already-disabled links, already-shared users, case-insensitive email matching, non-owner access) are handled or gracefully returned as errors.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- public_link checks the action before the confirmation gate, so an unknown
action is told so instead of being asked to confirm nothing in particular
(the 4/4 finding).
- linkView is a struct like shareView; toShareView builds the five share
results, and a fresh share is read back so it carries the person's name
like every other path.
- PublicShareURL trims a trailing slash off a hand-built base URL.
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit 07f33e62db into main2026-08-23 06:22:11 +00:00
steve
deleted branch feat/agent-sharing-tools2026-08-23 06:22:12 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Tier 3 of the agent-tool round (#129, #130): the outward-facing set, and the one destructive tool that was still missing.
What's new
list_shares,share_garden,remove_share,public_linkwrap the sharing service (ListShares/AddShare/UpdateShareRole/RemoveShare,Get/Enable/DisablePublicShareLink). They change who can see a garden beyond the screen, so they are gated twice:confirmed=true, which their descriptions allow only after a yes in the conversation. The refusal names the action, so the question the model then asks is precise. This is a schema-level reminder, not a guarantee (a model could pass it anyway), which is why the prompt rule stays too.share_gardenchanges the role of an existing share instead of failing onErrShareExists;remove_sharetakes the emaillist_sharesreports (the model has the address, not the id); an unknown email explains that the person must sign in once first (v1 has no invitation emails).public_linkreturns the address, never a bare token —PANSY_BASE_URL + /g/<token>through the newService.PublicShareURL, or the site-relative path when the base URL isn't configured.delete_planting— the hard delete for a plop that was never really planted (a misplacement, a duplicate), as opposed toremove_planting's "it came out". Recorded in the history, so undoable.CLAUDE.md gets the convention ("outward-facing tools ask first, and refuse without confirmed=true" — keep both halves); DESIGN.md roadmap note and UI step labels updated.
Tests
TestSharingToolsAskFirst(agent): each outward-facing action refused with the action named until confirmed; share → role change → no-op note; unknown email; a viewer can't list shares; remove by email, unknown email explained; public link get/enable (idempotent)/rotate/disable and a bad action; delete_planting gone from the season view, in the history, and restored byundo_change.🤖 Generated with Claude Code
🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 06:17:33Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -934,0 +1005,4 @@// linkView is a public link as the tools report it: on/off, and the address// when on. The token itself is only ever shown as part of that address.func (a *adapter) linkView(link *service.PublicShareLink) map[string]any {🟡 linkView returns untyped map[string]any while shareView is a typed struct — inconsistent representation of sibling view types
maintainability · flagged by 1 model
internal/agent/tools.go:1008—linkViewreturnsmap[string]anywhileshareViewis a typed struct. Both are "the view the tools report," andlistSharesreturns them side by side in the same map (line 1003). The link gets an untyped map (so callers/tests reach in withpublicLink["enabled"], see tools_test.go:1085–1088), the share gets a typed struct. Inconsistent for conceptually sibling output types. A smalllinkViewstruct (Enabled bool/URL string \json:"url,omitempty"\…🪰 Gadfly · advisory
@@ -934,0 +1038,4 @@return nil, ferr}if existing.Role == role {return map[string]any{"share": shareView{UserID: existing.UserID, Email: existing.Email, DisplayName: existing.DisplayName, Role: existing.Role},🟡 shareView is built inline five times; a toShareView helper would remove the copy-paste
maintainability · flagged by 1 model
internal/agent/tools.go:1041—shareViewis built inline five times with all four fields.listShares(line 997) constructs it in a loop;shareGardenbuilds it three different ways across its branches (lines 1041, 1048, 1053);removeSharebuilds it once (line 1090). Four of these repeatUserID/Email/DisplayName/Rolefield-by-field from adomain.ShareWithUser-shaped source. A tiny helpertoShareView(s domain.ShareWithUser) shareViewwould remove the copy-paste and make the `E…🪰 Gadfly · advisory
@@ -934,0 +1050,4 @@case err != nil:return nil, err}return map[string]any{"share": shareView{UserID: share.UserID, Email: email, Role: share.Role}}, nil🟡 share_garden new-share success path omits DisplayName, inconsistent with its other return paths and list_shares/remove_share
maintainability · flagged by 1 model
internal/agent/tools.go:1053—share_garden's new-share success path omitsDisplayName(and reports the input email, not the stored one), while every other return path in the same tool sets it. TheErrShareExistsbranches (lines 1041, 1048) buildshareView{... DisplayName: existing.DisplayName}, andremove_share/list_sharesboth populate it too. A model that shares a brand-new person back gets ashareobject shaped differently from the one it would get for a re-share or fro…🪰 Gadfly · advisory
@@ -934,0 +1099,4 @@if action == "" {action = "get"}if action != "get" && !args.Confirmed {🟠 Unknown publicLink action with confirmed=false produces empty-description errUnconfirmed; clear validation error is unreachable from that path
correctness, error-handling, maintainability · flagged by 4 models
internal/agent/tools.go:1102— Unknown action +confirmed=falseproduces a garbled error. The map lookup at line 1103 has keys only for"enable","rotate", and"disable"; any other action value (including a misspelled one) causes a missing-key lookup that returns"", soerrUnconfirmed("")yields"not done — is outward-facing, …"with an empty action description. Thedefaultbranch at line 1124 that would return the clear"action must be get, enable, rotate or disable"…🪰 Gadfly · advisory
@@ -90,0 +93,4 @@// Exists so the assistant can hand the gardener a link rather than a token.func (s *Service) PublicShareURL(token string) string {path := "/g/" + tokenif s.cfg != nil && s.cfg.BaseURL != "" {🟡 PublicShareURL does not trim trailing slash from BaseURL before concatenation
correctness · flagged by 1 model
🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 5 findings (1 with multi-model agreement)
internal/agent/tools.go:11024 single-model findings (lower confidence)
internal/agent/tools.go:1008internal/agent/tools.go:1041internal/agent/tools.go:1053internal/service/public.go:96Per-model detail
claude-code/opus (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've verified the security-relevant paths against the checked-out code. My findings:
VERDICT: No material issues found
This PR is a set of thin agent-tool adapters over already-existing, permission-gated service methods. I traced every new tool to its service call and every service call to its authorization check:
Authorization — all enforced at the service seam (verified):
listShares→Service.ListSharesrequiresroleOwner(shares.go:20); the test that "a viewer can't list shares" confirms this end-to-end.shareGarden→Service.AddShare/UpdateShareRole, bothroleOwner+isShareRolerole validation (shares.go:31,54,34,57). Arole:"owner"request is rejected asErrInvalidInput— no privilege escalation to owner via a share row.removeShare→Service.RemoveShareroutes throughrequireGardenRole(roleViewer)then owner-or-self (shares.go:69-75), so a non-participant gets maskedErrNotFound.publicLink→Get/Enable/DisablePublicShareLink, allroleOwner(public.go:55,70,105).deletePlanting→Service.DeletePlantingrequiresroleEditoron the parent object viaobjectForRole(plantings.go:289).Every adapter passes
a.actor, which is the authenticated session user threaded throughRunner.Run(actorID, …)(runtime.go:93,131) — the agent can only act with the caller's own permissions.gardenIdis a tool argument, but each service call re-checks ownership of that garden, so a cross-user access is impossible; within-user scope matches every other existing tool.Token / secret handling (verified):
newPublicTokenusescrypto/rand(service.go:100), notmath/rand.linkViewonly surfacesurl(which embeds the token) when the link is enabled, and only to the owner (GetPublicShareLinkis owner-gated). Emitting the token to the owner is the intended purpose; no leak to a lesser role or anonymous path.PublicShareURLjust prefixesPANSY_BASE_URL— no secret material added to logs/backups.Input validation: email/role are trimmed; email lookup is by exact account match (
GetUserByEmail),findSharecompares case-insensitively — no injection sink (parameterized store), no path/command/template surface. Theconfirmed=truegate is a model-behavior guardrail, not an access-control boundary, and the real boundary (owner-only) holds regardless of it.One defense-in-depth observation, not a defect in this diff: garden notes flow into the system prompt, so prompt-injection is a general concern for the agent — but it can't escalate sharing here, because the share tools are owner-only and a chat about a garden shared to you (where you'd read someone else's notes) fails the owner check.
Nothing in the security lens is materially wrong.
🎯 Correctness — Minor issues
I've confirmed the finding against the actual source. The code at
internal/agent/tools.go:1098-1126matches the draft's description exactly: for an invalid action (e.g."foo") sent withoutconfirmed,action != "get"is true, so the map lookupmap[...]{...}["foo"]returns""anderrUnconfirmed("")produces a refusal with an empty action name. Thedefault:case that says "action must be get, enable, rotate or disable" is only reachable once past the confirm gate. The finding holds.VERDICT: Minor issues
I reviewed this PR through the Correctness lens, verifying each new tool against the actual service layer, domain types, routes, and revert path (not just the diff).
One minor logic gap:
internal/agent/tools.go:1102(publicLink): the unconfirmed branch (action != "get" && !args.Confirmed) does a map lookupmap[...]{...}[action]for the refusal text. For an invalid action (e.g."foo") sent withoutconfirmed, the lookup returns"", so the refusal reads "not done — ⎵ is outward-facing…" with an empty action name. The proper "action must be get, enable, rotate or disable" validation in thedefault:switch case is only reachable whenconfirmed=true. Impact is small (a confusingly-worded refusal, still a refusal) and requires the model to ignore theenum, but the two validation orderings don't agree. A cheap fix: validate the action against the allowed set before the confirm check.Nothing blocking in my lens.
🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code:
tools.go:1053): The new-share success return isshareView{UserID: share.UserID, Email: email, Role: share.Role}— noDisplayName, and it echoes the inputemail. The three other return sites (1041, 1048, 1090) all populateDisplayName.AddSharereturns*domain.GardenShare(domain.go:314), which has neitherEmailnorDisplayName, so the divergence is real. Confirmed.tools.go:1102-1107): The guardaction != "get" && !args.Confirmeddoes a map lookup that returns""for any unknown action, feedingerrUnconfirmed(""). The clean "action must be get, enable, rotate or disable" (line 1125) is only reachable through thedefault:case on the confirmed path. Confirmed.Both survive. The verdict stands.
VERDICT: Minor issues
Reviewed through the code cleanliness & maintainability lens. The new tools closely follow the existing
adaptermethod patterns, comments are apt, and the test is thorough. Two small consistency nits, both verified by readinginternal/agent/tools.go:internal/agent/tools.go:1053—share_garden's new-share success path omitsDisplayName(and reports the input email, not the stored one), while every other return path in the same tool sets it. TheErrShareExistsbranches (lines 1041, 1048) buildshareView{... DisplayName: existing.DisplayName}, andremove_share/list_sharesboth populate it too. A model that shares a brand-new person back gets ashareobject shaped differently from the one it would get for a re-share or fromlist_shares.AddSharereturns*domain.GardenShare(noEmail/DisplayName— confirmed atdomain.go:314), so the populated fields aren't free, but the divergence is still a leaky, surprising output shape for one tool. Low-churn fix: resolve via the existingfindShare(or accept the gap and dropDisplayNameeverywhere for symmetry).internal/agent/tools.go:1102-1107— an unknownactionpassed withoutconfirmed=trueproduces a garbled refusal. The guard does a map lookupmap[...]{"enable":…,"rotate":…,"disable":…}[action]; for any action other than those three (e.g."share") the lookup returns"", soerrUnconfirmed("")yields "not done — is outward-facing, so say exactly what you would do…". The clean "action must be get, enable, rotate or disable" message (line 1125) is only reachable on theconfirmed=truepath, because thedefault:case sits after the guard. Since thedefault:case already exists precisely because the enum isn't trusted, the unconfirmed branch should validate the action too — e.g. reject an unknown action before the confirm check, or fall through to the same "action must be…" error.Everything else in scope is clean:
errUnconfirmed,shareView,linkView, andfindShareare well-named single-purpose helpers; the doc updates (CLAUDE.md, DESIGN.md, runtime.go prompt) all match convention.⚡ Performance — No material issues found
Verified. Through the performance lens, this change is clean.
VERDICT: No material issues found
The new sharing tools and
delete_plantingare agent-tool adapters over single-query service methods, invoked at most a handful of times per conversation turn — not a hot path.Things I checked and found fine:
shareGarden's conflict path (tools.go) callsfindShare→svc.ListSharesa second time afterAddSharereturnsErrShareExists, thenUpdateShareRole. That's an extraListSharesForGardenquery, but only on the already-shared branch, and share lists are per-garden and tiny (a handful of accounts). No meaningful cost. Not worth flagging.findShare's linear scan overListSharesresults (tools.go) — O(n) over a small per-garden list, no nested queries. Fine.listSharesissues two store reads (ListShares+GetPublicShareLink) — bounded, one-shot. Fine.ListShares/AddShare/RemoveShare(internal/service/shares.go) are each a role check plus one store call;ListSharesForGardenis a single query, not an N+1 per recipient. Verified by readingshares.go:19-71.PublicShareURL(internal/service/public.go:92) is pure string concatenation. Fine.Nothing in the performance lane is materially wrong here.
🧯 Error handling & edge cases — Minor issues
The test at
tools_test.go:1137is decisive: it callspublic_linkwithaction: "share"andconfirmed: true, and expects the refusal to contain"get, enable, rotate or disable"(theswitchdefault message). For that test to pass, an off-enumactionvalue must actually reach the function body — proving theenumtag is advisory to the model, not enforced by the framework. That confirms the draft's premise: an invalid action can arrive atpublicLink.Tracing the finding's scenario (
action="share",confirmed=false):action="share"(non-empty, not"get")action != "get" && !args.Confirmed→ true → gate fires firstmap[...]["share"]→ no such key → zero value""errUnconfirmed("")→"...: not done — is outward-facing, so say exactly what you would do..."— empty action name, wrong guidanceswitch defaultmessage is never reached because the gate short-circuits.The finding is confirmed against the real code. Impact is low (a misleading message to the model, not a crash), severity small, confidence high. The verdict stands.
VERDICT: Minor issues
Findings
internal/agent/tools.go:1102-1108— Invalidaction+ unconfirmed yields a nonsensical refusal. The confirmation gate runs before action validation. For a non-empty, non-getaction that isn't in{enable,rotate,disable}(e.g."share"), whenconfirmedis false the map lookup returns the zero value"", soerrUnconfirmed("")produces the message "not done — is outward-facing, so say exactly what you would do and ask the user first…" — an empty action name and the wrong guidance. The clear error ("action must be get, enable, rotate or disable") only fires on theswitchdefault, which is unreachable in this branch because the confirmation check short-circuits first. Theenumtag is advisory to the model, not enforced by the framework (the PR's own test attools_test.go:1137passesaction:"share"through to the switch default withconfirmed=true), so an off-enum value genuinely reaches this code; that unconfirmed unhappy path is untested. Impact is low (a confusing message to the model, not a crash); fix is to validateactionagainst the allowed set before the confirmation check. Small.claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've verified all the key security properties. Here's my assessment:
No material issues found
The security posture of this PR is sound. Here's what I checked:
Authorization chain — verified solid
listShares,shareGarden,removeShare,publicLinkall delegate to service methods (ListShares,AddShare,UpdateShareRole,RemoveShare,GetPublicShareLink,EnablePublicShareLink,DisablePublicShareLink) that callrequireGardenRole(roleOwner)— checked ininternal/service/shares.goandinternal/service/public.go. A non-owner getsErrNotFound(masked).deletePlantingdelegates tosvc.DeletePlantingwhich callsobjectForRole(ctx, actorID, pl.ObjectID, roleEditor)before touching data.a.actor) is always sourced from the HTTP session (actor := mustActor(c)→runner.Run(ctx, actor.ID, ...)→newToolbox(svc, actorID, today)), never from tool arguments.Public link token — cryptographically secure
newPublicToken()callsrandToken(18)which usescrypto/rand(verified atinternal/service/service.go:84). 144 bits of entropy; not guessable.Role validation — double-enforced
enum:"viewer,editor"struct tag is advisory for the LLM. The service'sisShareRole()is the actual gate. Passing"owner"withconfirmed=truefails atsvc.AddSharewithErrInvalidInput— the test atrefused("share_garden", ..., "role": "owner", "confirmed": true)covers this.errUnconfirmedwith unknownpublic_linkaction — no bypassaction="share"andconfirmed=true, the map lookup yields""so the confirmation block is skipped; the switch'sdefaultcase then returnsErrInvalidInput. No security gap.Email reflection in error messages — low-risk by design
errUnconfirmed(fmt.Sprintf("sharing this garden with %s as %s", email, role))anderrUnconfirmed("removing " + email + " from this garden"). This is reflected back to the same user's AI session. React escapes text content by default; no XSS path. The data never crosses an authorization boundary.Acknowledged design limitation:
confirmed=truecan be spoofed by the model🎯 Correctness — Minor issues
The code confirms the finding exactly as described. Lines 1102-1107: the confirmed guard fires before the switch, and an unknown action causes the map lookup to return
"", producing a garblederrUnconfirmed("")message. Thedefaultbranch at line 1124-1125 is only reachable once the confirmed guard is already satisfied.Minor issues
internal/agent/tools.go:1102— Unknown action +confirmed=falseproduces a garbled error. The map lookup at line 1103 has keys only for"enable","rotate", and"disable"; any other action value (including a misspelled one) causes a missing-key lookup that returns"", soerrUnconfirmed("")yields"not done — is outward-facing, …"with an empty action description. Thedefaultbranch at line 1124 that would return the clear"action must be get, enable, rotate or disable"message is inside theswitchat line 1113, which is only entered after the confirmed guard is already satisfied — so an unknown action withconfirmed=falsecan never reach it. Fix: validate the action against the known set before the confirmed check and returnErrInvalidInputimmediately for unknown values; then let the confirmed gate run only for the known non-get actions.🧹 Code cleanliness & maintainability — Minor issues
The logic is confirmed. With an unrecognized action (e.g.
"foo") andconfirmed=false:action != "get"is true and!args.Confirmedis true → enters the guardmap[string]string{...}["foo"]returns""(Go zero-value for a missing key)errUnconfirmed("")produces the garbled message with a blank action namedefault:branch with the correct error is never reached because the switch is after this guardThe finding is genuine and confirmed.
Minor issues
internal/agent/tools.go:1102–1108— inpublicLink, the!args.Confirmedguard runs before action validation. For an unrecognized action (e.g."foo") withconfirmed=false, the map lookupmap[string]string{...}[action]returns""(Go zero value), anderrUnconfirmed("")produces the grammatically broken message"not done — is outward-facing, so say exactly what you would do and ask the user first..."— the action name is blank. Thedefault:branch that gives the correct"action must be get, enable, rotate or disable"error is only reachable whenconfirmed=trueoraction=="get". Fix: move action validation (an early-return for unrecognized values) before theconfirmedcheck, or add a fallback to the description map.The test covers the invalid-action case only with
confirmed=true, so the garbled message onconfirmed=falseis not caught. In practice the LLM is guided by theenumtag, but the guard is what breaks when it doesn't listen.⚡ Performance — No material issues found
No material issues found
I traced every query path in the new tools:
listSharesissues two separaterequireGardenRolechecks — one insideListShares, one insideGetPublicShareLink— fetching the garden row twice for the same ownership assertion. At SQLite scale with these dataset sizes this is a micro-optimisation, not a material concern.shareGardenonErrShareExistscallsfindShare→ListSharesto resolve the email to a user ID, even thoughAddSharealready didGetUserByEmailinternally. The role-change path therefore executes up to six round-trips where three or four would do. There is no loop and no realistic scale at which garden shares grow large enough for this to be perceptible —LIMIT 1000on a table that typically holds single-digit rows per garden makes this a non-issue.removeSharesimilarly callsfindShare(onerequireGardenRole+ one JOIN) thenRemoveShare(anotherrequireGardenRole): two ownership checks for one delete. Same analysis applies.None of these are in loops, none operate on unbounded data, and the extra SQLite queries are sub-millisecond. The thresholds for flagging ("impact you can justify, not micro-optimisations") are not met here.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed. An unknown
action(not in{get, enable, rotate, disable}) combined withconfirmed=falsehits the map lookup at line 1103–1107 which returns"", passing an empty string toerrUnconfirmed— producing a blank where the action description should appear. Thedefaultbranch that gives the proper "must be get, enable, rotate or disable" error is only reached after the confirmation guard, so it can't correct the bad message in this path.Verdict: Minor issues
publicLink: unknownactionwithconfirmed=falseproduces a blank error messageinternal/agent/tools.go:1102–1108If the model supplies an action outside
{get, enable, rotate, disable}andconfirmedis false, the map lookup returns""and the error reads:— blank where the action description should be. With
confirmed: truethedefaultbranch correctly rejects the unknown action with a clear message. The schema enum is advisory and a model can pass anything; the empty-string path is reachable. The fix is to validate the action before the confirmation guard, returning the clear "must be get, enable, rotate or disable" error for any unrecognised value regardless ofconfirmed.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Service.{ListShares,AddShare,UpdateShareRole,RemoveShare,GetPublicShareLink,EnablePublicShareLink,DisablePublicShareLink}, each of which enforces owner-only viarequireGardenRole(..., roleOwner)— so the adapter can't bypass authz even when a model passesconfirmed=true(verified ininternal/service/shares.go:19-79andinternal/service/public.go:54-108). A non-owner'slist_sharesis rejected at the service layer (the test confirms this), which is the correct behavior for an authz gate.linkViewonly emits the assembled addressPublicShareURL(token)=cfg.BaseURL + /g/<token>(or a relative path whenBaseURLunset) — and only when enabled. No token leakage via a separate field.BaseURLis normalized at config load (TrimRightof trailing/,internal/config/config.go:108), andPublicShareLink.Tokenisomitempty, so a disabled link carries no token (verifiedinternal/service/public.go:12-22,90-100).shareGarden'sroleis lowered and validated againstisShareRole(viewer/editor) insideAddShare/UpdateShareRole; an "owner" role is rejected withErrInvalidInput(verifiedinternal/service/shares.go:34-36,57-59; the test exercises theownerrefusal). No role-injection/escalation path.delete_plantingdelegates toService.DeletePlantingand inherits its permission checks; no new untrusted-input sink introduced here (signature is just aplantingId int64).GetUserByEmail,CreateShare), not interpolated into SQL; no injection surface in the adapter.No security-relevant issues found within this lens.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: max steps reached without a final answer (max 14)
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
internal/agent/tools.go:1041—shareViewis built inline five times with all four fields.listShares(line 997) constructs it in a loop;shareGardenbuilds it three different ways across its branches (lines 1041, 1048, 1053);removeSharebuilds it once (line 1090). Four of these repeatUserID/Email/DisplayName/Rolefield-by-field from adomain.ShareWithUser-shaped source. A tiny helpertoShareView(s domain.ShareWithUser) shareViewwould remove the copy-paste and make theEmail-vs-existing.Email/updated.Role-mixing in theErrShareExistsbranch (lines 1048–1049) easier to follow at a glance.internal/agent/tools.go:1008—linkViewreturnsmap[string]anywhileshareViewis a typed struct. Both are "the view the tools report," andlistSharesreturns them side by side in the same map (line 1003). The link gets an untyped map (so callers/tests reach in withpublicLink["enabled"], see tools_test.go:1085–1088), the share gets a typed struct. Inconsistent for conceptually sibling output types. A smalllinkViewstruct (Enabled bool/URL string \json:"url,omitempty"`) would matchshareViewand keep theomitemptyrule the code now hand-implements with theif link.Enabled` guard.internal/agent/tools.go:1103— the per-action refusal map is rebuilt on everypublicLinkcall, and the action vocabulary is re-spelled by hand in thedefaulterror. A package-levelvar publicLinkActions = map[string]string{...}would (a) stop rebuilding the map per call and (b) make the action list one place, shared with thedefaulterror message at line 1125 which currently re-spells "get, enable, rotate or disable" by hand. Theenumtag makeserrUnconfirmed("")with an empty key unreachable today, so this is purely a readability nit.⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens, the new tool handlers are all single-request, bounded paths:
listShares— oneListShares(capped atmaxSharesListed=1000ininternal/store/shares.go:45) plus oneGetPublicShareLink. No pagination concern at household scale, and the store already hard-limits the query.removeShare/shareGarden'sErrShareExistspath — callfindShare, which re-runsListShares(the full join) just to resolve an email to aUserID. This is one extra round-trip per outward-facing action, not in a loop, and bounded by the same 1000 cap. It's a mildly avoidable query (the service'sAddSharealready didGetUserByEmailand could in principle surface the existing share instead of forcing the adapter to re-scan the list), but the impact is negligible and it is not an N+1. Noted as trivial, not a finding.deletePlanting—GetPlanting+objectForRole+DeletePlanting+record; no hot-loop or allocation concern.publicLink— single service call per action;linkViewallocates one small map. Fine.I verified
ListSharesForGarden'sLIMIT, the service signatures, and theDeletePlanting/RemovePlantingimplementations by readinginternal/store/shares.go,internal/service/shares.go,internal/service/public.go, andinternal/service/plantings.go.🧯 Error handling & edge cases — Minor issues
Config loading normalizes
BaseURLwithstrings.TrimRight(..., "/"), so the trailing-slash finding is invalid. The first finding about the empty action name stands.Verdict: Minor issues
internal/agent/tools.go:1102-1108—publicLinklooks up the refusal text via amap[string]stringindexed byaction. For an action outside{get,enable,rotate,disable}(theenumis a schema hint, not a hard gate — the test itself passes"share"), withconfirmed=false, the lookup returns the zero string"", soerrUnconfirmed("")produces a malformed refusal:not done — is outward-facing, so say exactly what you would do...— a double space and no action named, defeating the point of naming the action. Thedefaultbranch that would give a proper "action must be get, enable, rotate or disable" error is only reached after the confirmation check, so it never fires for an unconfirmed bad action. Suggested fix: validate the action against the known set before the confirmation check, or fall back to a generic "that action" string when the map lookup is absent.kimi-k2.6: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
I traced the new sharing tools and
delete_plantingthrough the service layer and verified:share_garden,remove_share, andpublic_link(all mutating actions) delegate toAddShare,RemoveShare,EnablePublicShareLink,DisablePublicShareLink, which all requireroleOwnerviarequireGardenRole.list_sharesis read-only and also owner-gated. The agent adapter'sfindSharecallsListShares, so an owner check happens beforeRemoveShareis ever reached.confirmed=truegating works as designed: each outward-facing tool returns a precise refusal naming the action whenconfirmedis missing/false. This is the intended defense-in-depth (prompt + schema-level boolean), acknowledged in the PR description as not a cryptographic guarantee.emailandroleinputs are trimmed/lower-cased and passed to parameterized store queries (GetUserByEmail,CreateShare). Error messages that embed the email/role are returned to the LLM, not rendered as raw HTML.publicLinkmap lookup is safe: a missing key in the action-description map returns the zero-value empty string (Go maps do not panic on missing keys), so an unknown action withconfirmed=falseyields a harmless generic refusal; an unknown action withconfirmed=truehits the explicitdefaultcase in theswitch.delete_plantingis consistent with existing destructive tools: it checksroleEditorviaobjectForRole, is recorded in the history for undo, and is correctly classified as non-outward-facing (it doesn't change who can see the garden).🎯 Correctness — Minor issues
Minor issues found
PublicShareURLtrustscfg.BaseURLwithout trimming a trailing slash, risking a double-slash URL.internal/service/public.go:94-99concatenatess.cfg.BaseURL + "/g/" + token. The config loader doesstrings.TrimRight(..., "/"), but nothing prevents a test or future code path from settingcfg.BaseURLdirectly to a value ending in/. The result would behttps://example.com//g/<token>, a malformed URL that the model would hand to the user.Fix: Defensively trim before concatenation:
strings.TrimRight(s.cfg.BaseURL, "/") + path.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/agent/tools.go:1102-1108—publicLinkallocates a 3-entrymap[string]stringon every outward-facing call just to index into it once. A small switch or inlineif action == "enable" { … }would avoid the allocation, be easier to read, and prevent an invalid action from silently producing an empty description (map["foo"]→"") instead of the validation error theswitch defaulthandles later.Suggested fix: replace the map literal with a switch that assigns the description string, then pass it to
errUnconfirmed.⚡ Performance — No material issues found
No material issues found.
The sharing tool wrappers added in this PR are bounded and do not introduce measurable regressions:
listSharesfetches at mostmaxSharesListed = 1000shares (already enforced in the store layer) and one public-link state — bounded, no pagination missing.findSharedoes a linear scan over that bounded slice to resolve email→share. While a direct store lookup would be tighter, 1000 iterations is a hard cap and the tool is invoked one call at a time by the agent loop, so no N+1 or quadratic blow-up.shareGardenonErrShareExistspays one extraListSharesround-trip before updating the role, but this is an edge path (re-sharing with a different role), not a hot loop.deletePlantingissues the same number of store calls as sibling mutation tools (GetPlanting,objectForRole,DeletePlanting,record) — no new alloc or copy path stands out.🧯 Error handling & edge cases — No material issues found
No material issues found.
I reviewed the diff through the error-handling and edge-case lens. The outward-facing sharing tools (
share_garden,remove_share,public_link) correctly refuse destructive actions withoutconfirmed=true, return named errors so the model can ask precisely, and validate inputs before proceeding. No errors are swallowed, no nil pointers are dereferenced, and there are no missing cleanup paths. Thedelete_plantingwrapper passes through to the service layer which records the change for undo, and the test verifies the round-trip. All edge cases I could identify (empty inputs, unknown actions, already-disabled links, already-shared users, case-insensitive email matching, non-owner access) are handled or gracefully returned as errors.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.