Agent: sharing tools that ask first, and a hard delete for a misplaced plop #131

Merged
steve merged 2 commits from feat/agent-sharing-tools into main 2026-08-23 06:22:11 +00:00
Owner

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

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)
steve added 1 commit 2026-08-23 06:13:13 +00:00
Agent: sharing tools that ask first, and a hard delete for a misplaced plop
Build image / build-and-push (push) Successful in 10s
Gadfly review (reusable) / review (pull_request) Successful in 4m18s
Adversarial Review (Gadfly) / review (pull_request) Successful in 4m18s
f985c264f8
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]>

🪰 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

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

🪰 Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 5 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -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:1008linkView 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>
@@ -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:1041shareView 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>
@@ -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:1053share_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>
@@ -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=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>
@@ -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/" + token
if s.cfg != nil && s.cfg.BaseURL != "" {

🟡 PublicShareURL does not trim trailing slash from BaseURL before concatenation

correctness · flagged by 1 model

🪰 Gadfly · advisory

🟡 **PublicShareURL does not trim trailing slash from BaseURL before concatenation** _correctness · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>

🪰 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
4 single-model findings (lower confidence)
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
Per-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):

  • listSharesService.ListShares requires roleOwner (shares.go:20); the test that "a viewer can't list shares" confirms this end-to-end.
  • shareGardenService.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.
  • removeShareService.RemoveShare routes through requireGardenRole(roleViewer) then owner-or-self (shares.go:69-75), so a non-participant gets masked ErrNotFound.
  • publicLinkGet/Enable/DisablePublicShareLink, all roleOwner (public.go:55,70,105).
  • deletePlantingService.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 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.

🧹 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:1053share_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 — 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 findSharesvc.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):

  • 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-1108Invalid 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.
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, 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 findShareListShares 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

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.

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

  • 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:1041shareView 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:1008linkView 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.
  • deletePlantingGetPlanting + 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-1108publicLink 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.
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_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-1108publicLink 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>
steve added 1 commit 2026-08-23 06:21:06 +00:00
- 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 main 2026-08-23 06:22:11 +00:00
steve deleted branch feat/agent-sharing-tools 2026-08-23 06:22:12 +00:00
Sign in to join this conversation.