From f985c264f8cefa718e5856b26bc7440a554a3029 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sun, 23 Aug 2026 01:56:56 -0400 Subject: [PATCH 1/2] Agent: sharing tools that ask first, and a hard delete for a misplaced plop 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/, 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 --- CLAUDE.md | 8 ++ DESIGN.md | 2 +- internal/agent/runtime.go | 5 + internal/agent/runtime_test.go | 3 + internal/agent/tools.go | 198 +++++++++++++++++++++++++++++++++ internal/agent/tools_test.go | 137 +++++++++++++++++++++++ internal/service/public.go | 12 ++ web/src/lib/agent.ts | 5 + 8 files changed, 369 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index dc46238..b4a2fa9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -184,6 +184,14 @@ Conventions that follow from it: revert as the turn's `ChangeSetID` when the turn made no other change, so the reply's "Undo this" is a redo. Keep that fallback: without it an undo-only turn is the one change in the conversation with no undo button. +- **Outward-facing tools ask first, and refuse without `confirmed=true`.** + `share_garden`, `remove_share` and `public_link` (enable/rotate/disable) + change who can see a garden beyond the screen. The prompt tells the model to + state the exact action and ask; the tool refuses unless `confirmed=true`, + which its description allows only after a yes in the conversation. Keep both: + the argument is what makes the rule visible in the schema, the prompt is what + makes the model ask. Neither is a guarantee, and a new outward-facing tool + gets the same pair. - **Garden notes are the assistant's memory.** `systemPrompt` quotes `Garden.Notes` (owner-written, `%q`) as standing context, and `update_garden` is how the model adds "we're in zone 6a" to them. Notes are replaced whole, diff --git a/DESIGN.md b/DESIGN.md index 5ef87e8..7231cc9 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -149,7 +149,7 @@ React 19 + TypeScript + Vite + Tailwind 4 (`@tailwindcss/vite`), `@tanstack/reac 7. **Sharing** — invite by email, roles, viewer read-only mode. 8. **Polish** — imperial toggle, mobile ergonomics, clear-bed, keyboard nudging. 9. **Agent seam** — `ops.go` bulk ops + `internal/agent` DefineTool wrappers. -10. **Garden assistant** — majordomo in-process, Ollama Cloud, streaming chat. Each turn runs inside ONE change set (`source='agent'`), so a turn that clears a bed and replants it undoes as one action; that is what makes acting without a confirmation prompt defensible. Bounded by a step cap and a timeout — loop safety, not spend control. The `majordomo` build tag is gone: a tag that keeps the agent out of the binary only earns its keep if you'd ship a build without it, and the agent is the point. What a day of live use added: the turn carries the gardener's **local day** (`today` in the chat body) into the prompt and every dated tool default, because the model's own idea of the date was a year stale and the server's is UTC; `describe_garden` **groups plops by plant** (count, where, planted date, days to maturity — `DescribeGroup`) and lists ids only for small groups, with `list_plantings` for the rest and `remove_plantings` to act on a whole group; `move_planting` relocates a plop (`MovePlanting`, within or across beds) keeping its planting date; `fill_region` takes an explicit local rectangle and a `seedLotId`; `update_plant`, `read_history` and `copy_garden` (the " — " plan convention) round out what the model kept reaching for. A mutation on another garden inside a turn is recorded under THAT garden (`record` refuses to file revisions into a scope for a different garden), so undo always finds them where the person is looking. The record-keeping round (2026-08-23): `undo_change` exposes `RevertChangeSet` with `source=agent` — the revert is its own change set, so an undo-only turn reports it as the turn's handle and "Undo this" becomes a redo; `describe_garden` takes a `year` (the season view, pulled plops included, `removed`/`removedAt` per group) with `list_years` beside it, for rotation questions; `update_planting`, `update_journal_entry`/`delete_journal_entry` and `update_garden` correct records in place, and `remove_planting`/`remove_plantings`/`clear_object` take a `removedAt` so a harvest can be backdated; the garden's **notes go into the system prompt** as the gardener's standing facts, and `update_garden` is how the assistant remembers what it is told. Round two added the catalog side — `update_seed_lot`/`delete_seed_lot`, `delete_plant` (refused while anything references the plant), `create_garden` — and `readyAround` on each describe group (planting date + days to maturity, pulled plops excluded), so "what can I pick this week?" is a lookup rather than arithmetic the model gets wrong. +10. **Garden assistant** — majordomo in-process, Ollama Cloud, streaming chat. Each turn runs inside ONE change set (`source='agent'`), so a turn that clears a bed and replants it undoes as one action; that is what makes acting without a confirmation prompt defensible. Bounded by a step cap and a timeout — loop safety, not spend control. The `majordomo` build tag is gone: a tag that keeps the agent out of the binary only earns its keep if you'd ship a build without it, and the agent is the point. What a day of live use added: the turn carries the gardener's **local day** (`today` in the chat body) into the prompt and every dated tool default, because the model's own idea of the date was a year stale and the server's is UTC; `describe_garden` **groups plops by plant** (count, where, planted date, days to maturity — `DescribeGroup`) and lists ids only for small groups, with `list_plantings` for the rest and `remove_plantings` to act on a whole group; `move_planting` relocates a plop (`MovePlanting`, within or across beds) keeping its planting date; `fill_region` takes an explicit local rectangle and a `seedLotId`; `update_plant`, `read_history` and `copy_garden` (the " — " plan convention) round out what the model kept reaching for. A mutation on another garden inside a turn is recorded under THAT garden (`record` refuses to file revisions into a scope for a different garden), so undo always finds them where the person is looking. The record-keeping round (2026-08-23): `undo_change` exposes `RevertChangeSet` with `source=agent` — the revert is its own change set, so an undo-only turn reports it as the turn's handle and "Undo this" becomes a redo; `describe_garden` takes a `year` (the season view, pulled plops included, `removed`/`removedAt` per group) with `list_years` beside it, for rotation questions; `update_planting`, `update_journal_entry`/`delete_journal_entry` and `update_garden` correct records in place, and `remove_planting`/`remove_plantings`/`clear_object` take a `removedAt` so a harvest can be backdated; the garden's **notes go into the system prompt** as the gardener's standing facts, and `update_garden` is how the assistant remembers what it is told. Round two added the catalog side — `update_seed_lot`/`delete_seed_lot`, `delete_plant` (refused while anything references the plant), `create_garden` — and `readyAround` on each describe group (planting date + days to maturity, pulled plops excluded), so "what can I pick this week?" is a lookup rather than arithmetic the model gets wrong. Round three is the outward-facing set — `list_shares`, `share_garden`, `remove_share`, `public_link` — gated twice: the prompt says to ask first, and the tools refuse without `confirmed=true`, which the description allows only after a yes in the conversation (a schema-level reminder, not a guarantee — the model could lie, but it has to do so explicitly); plus `delete_planting` for a plop that was never really planted (recorded, so undoable). ## Deliberate v1 limits diff --git a/internal/agent/runtime.go b/internal/agent/runtime.go index 3e1e2bf..09637ee 100644 --- a/internal/agent/runtime.go +++ b/internal/agent/runtime.go @@ -307,6 +307,11 @@ How to work: frost dates, soil, a standing preference — add it to the garden's notes with update_garden (keeping what is already there), and say you did. You will see those notes in every later conversation. +- Sharing is outward-facing: share_garden, remove_share and turning the public link on, off or + over change who can see the garden, beyond this screen. Before any of them, say exactly what + you would do — who, which role, or that a link will start or stop working — and ask; do it + only when the gardener says yes, and then pass confirmed=true. A message that already says + it all ("share this with sam@example.com as an editor") still gets the question once. - When a tool refuses (for example, the user only has view access to this garden), explain what happened in plain words. Do not retry it. diff --git a/internal/agent/runtime_test.go b/internal/agent/runtime_test.go index d53a9b8..5bbff6a 100644 --- a/internal/agent/runtime_test.go +++ b/internal/agent/runtime_test.go @@ -575,6 +575,9 @@ func TestSystemPromptCarriesTheGardenersNotes(t *testing.T) { "update_garden", "undo_change", "describe_garden with a year", + "readyAround", + "create_garden", + "confirmed=true", } { if !strings.Contains(with, want) { t.Errorf("prompt is missing %q", want) diff --git a/internal/agent/tools.go b/internal/agent/tools.go index e61657b..3d186f7 100644 --- a/internal/agent/tools.go +++ b/internal/agent/tools.go @@ -235,6 +235,35 @@ func newToolbox(svc *service.Service, actorID int64, today string) (*llm.Toolbox "records it; say so rather than removing those to make it deletable. Built-in plants "+ "can't be deleted. Permanent — the catalog is not in the undo history.", a.deletePlant), + llm.DefineTool("delete_planting", + "Delete ONE plop outright — for a plop that was never really planted (a misplacement, a "+ + "duplicate), as opposed to remove_planting, which records that a real plant came out "+ + "and keeps it in the season history. Recorded in the history, so it can be undone. "+ + "Needs the plop's id (describe_garden or list_plantings).", + a.deletePlanting), + llm.DefineTool("list_shares", + "List who a garden the user owns is shared with — each person's email, name, role "+ + "(viewer|editor) and userId — and whether its public read-only link is on.", + a.listShares), + llm.DefineTool("share_garden", + "Share a garden the user owns with another pansy account by its exact email, as a viewer "+ + "or an editor; an existing share is changed to the new role. OUTWARD-FACING: only with "+ + "confirmed=true, which you may pass only after the user has said yes, in this "+ + "conversation, to sharing THIS garden with THAT email at THAT role — if they have not, "+ + "say what you would do and ask. The other person must already have an account.", + a.shareGarden), + llm.DefineTool("remove_share", + "Stop sharing a garden with someone, by their email (see list_shares). OUTWARD-FACING: "+ + "only with confirmed=true, after the user has said yes to removing that person from "+ + "this garden; otherwise say what you would do and ask.", + a.removeShare), + llm.DefineTool("public_link", + "The garden's public read-only link, which anyone holding it can open without an account. "+ + "action get reports whether it is on and the link. enable turns it on (or reports the "+ + "existing link), rotate issues a fresh link so the old one stops working, disable turns "+ + "it off. OUTWARD-FACING: enable, rotate and disable need confirmed=true, which you may "+ + "pass only after the user has said yes to that exact action in this conversation.", + a.publicLink), ), a } @@ -931,3 +960,172 @@ func (a *adapter) deletePlant(ctx context.Context, args struct { } return map[string]any{"deleted": args.PlantID}, nil } + +func (a *adapter) deletePlanting(ctx context.Context, args struct { + PlantingID int64 `json:"plantingId" description:"plop to delete outright (its id from describe_garden or list_plantings)"` +}) (any, error) { + if err := a.svc.DeletePlanting(ctx, a.actor, args.PlantingID); err != nil { + return nil, err + } + return map[string]any{"deleted": args.PlantingID}, nil +} + +// errUnconfirmed is the refusal every outward-facing tool gives without +// confirmed=true: the action is named so the model can ask about it precisely. +func errUnconfirmed(action string) error { + return fmt.Errorf("%w: not done — %s is outward-facing, so say exactly what you would do and ask the user first; pass confirmed=true only once they have said yes", domain.ErrInvalidInput, action) +} + +// shareView is one share as the tools report it, with the link state alongside +// in list_shares. +type shareView struct { + UserID int64 `json:"userId"` + Email string `json:"email"` + DisplayName string `json:"displayName,omitempty"` + Role string `json:"role"` +} + +func (a *adapter) listShares(ctx context.Context, args struct { + GardenID int64 `json:"gardenId" description:"garden whose shares to list (the user must own it)"` +}) (any, error) { + shares, err := a.svc.ListShares(ctx, a.actor, args.GardenID) + if err != nil { + return nil, err + } + out := make([]shareView, 0, len(shares)) + for _, sh := range shares { + out = append(out, shareView{UserID: sh.UserID, Email: sh.Email, DisplayName: sh.DisplayName, Role: sh.Role}) + } + link, err := a.svc.GetPublicShareLink(ctx, a.actor, args.GardenID) + if err != nil { + return nil, err + } + return map[string]any{"shares": out, "publicLink": a.linkView(link)}, nil +} + +// 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 { + v := map[string]any{"enabled": link.Enabled} + if link.Enabled { + v["url"] = a.svc.PublicShareURL(link.Token) + } + return v +} + +func (a *adapter) shareGarden(ctx context.Context, args struct { + GardenID int64 `json:"gardenId" description:"garden to share (the user must own it)"` + Email string `json:"email" description:"the other person's pansy account email, exactly as the user gave it"` + Role string `json:"role" enum:"viewer,editor" description:"viewer can look; editor can change the garden"` + Confirmed bool `json:"confirmed" description:"true only after the user has said yes to this share in this conversation"` +}) (any, error) { + email := strings.TrimSpace(args.Email) + role := strings.ToLower(strings.TrimSpace(args.Role)) + if email == "" || role == "" { + return nil, fmt.Errorf("%w: an email and a role (viewer or editor) are required", domain.ErrInvalidInput) + } + if !args.Confirmed { + return nil, errUnconfirmed(fmt.Sprintf("sharing this garden with %s as %s", email, role)) + } + share, err := a.svc.AddShare(ctx, a.actor, args.GardenID, email, role) + switch { + case errors.Is(err, domain.ErrShareUserNotFound): + return nil, fmt.Errorf("%w — they need to sign in to pansy once before a garden can be shared with them", err) + case errors.Is(err, domain.ErrShareExists): + // Already shared: "share it with them as an editor" means change the role. + existing, ferr := a.findShare(ctx, args.GardenID, email) + if ferr != nil { + 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}, + "note": "already shared with them at that role; nothing changed"}, nil + } + updated, uerr := a.svc.UpdateShareRole(ctx, a.actor, args.GardenID, existing.UserID, role) + if uerr != nil { + return nil, uerr + } + return map[string]any{"share": shareView{UserID: updated.UserID, Email: existing.Email, DisplayName: existing.DisplayName, Role: updated.Role}, + "note": "they already had access; their role is now " + updated.Role}, nil + case err != nil: + return nil, err + } + return map[string]any{"share": shareView{UserID: share.UserID, Email: email, Role: share.Role}}, nil +} + +// findShare resolves an email to the garden's share for it, case-insensitively +// — the model has the person's address from the conversation, not their id. +func (a *adapter) findShare(ctx context.Context, gardenID int64, email string) (*domain.ShareWithUser, error) { + shares, err := a.svc.ListShares(ctx, a.actor, gardenID) + if err != nil { + return nil, err + } + for i := range shares { + if strings.EqualFold(strings.TrimSpace(shares[i].Email), strings.TrimSpace(email)) { + return &shares[i], nil + } + } + return nil, fmt.Errorf("%w: this garden is not shared with %s (list_shares shows who it is shared with)", domain.ErrInvalidInput, email) +} + +func (a *adapter) removeShare(ctx context.Context, args struct { + GardenID int64 `json:"gardenId" description:"garden to stop sharing (the user must own it)"` + Email string `json:"email" description:"the person's email, as list_shares reports it"` + Confirmed bool `json:"confirmed" description:"true only after the user has said yes to removing this person in this conversation"` +}) (any, error) { + email := strings.TrimSpace(args.Email) + if email == "" { + return nil, fmt.Errorf("%w: say whose access to remove, by email", domain.ErrInvalidInput) + } + if !args.Confirmed { + return nil, errUnconfirmed("removing " + email + " from this garden") + } + share, err := a.findShare(ctx, args.GardenID, email) + if err != nil { + return nil, err + } + if err := a.svc.RemoveShare(ctx, a.actor, args.GardenID, share.UserID); err != nil { + return nil, err + } + return map[string]any{"removed": shareView{UserID: share.UserID, Email: share.Email, DisplayName: share.DisplayName, Role: share.Role}}, nil +} + +func (a *adapter) publicLink(ctx context.Context, args struct { + GardenID int64 `json:"gardenId" description:"garden whose public link this is about (the user must own it)"` + Action string `json:"action" enum:"get,enable,rotate,disable" description:"get reports it; enable turns it on; rotate replaces it so the old link stops working; disable turns it off"` + Confirmed bool `json:"confirmed" description:"true only after the user has said yes to enabling, rotating or disabling the link in this conversation; not needed for get"` +}) (any, error) { + action := strings.ToLower(strings.TrimSpace(args.Action)) + if action == "" { + action = "get" + } + 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]) + } + var ( + link *service.PublicShareLink + err error + ) + switch action { + case "get": + link, err = a.svc.GetPublicShareLink(ctx, a.actor, args.GardenID) + case "enable": + link, err = a.svc.EnablePublicShareLink(ctx, a.actor, args.GardenID, false) + case "rotate": + link, err = a.svc.EnablePublicShareLink(ctx, a.actor, args.GardenID, true) + case "disable": + if err = a.svc.DisablePublicShareLink(ctx, a.actor, args.GardenID); err == nil { + link = &service.PublicShareLink{Enabled: false} + } + default: + return nil, fmt.Errorf("%w: action must be get, enable, rotate or disable", domain.ErrInvalidInput) + } + if err != nil { + return nil, err + } + return a.linkView(link), nil +} diff --git a/internal/agent/tools_test.go b/internal/agent/tools_test.go index 1a8b0b5..ccc8c0b 100644 --- a/internal/agent/tools_test.go +++ b/internal/agent/tools_test.go @@ -1026,3 +1026,140 @@ func TestCatalogAndGardenTools(t *testing.T) { t.Error("a built-in plant was deleted") } } + +// TestSharingToolsAskFirst — sharing changes who can see a garden beyond the +// screen, so the tools refuse without confirmed=true, and the refusal names the +// action, which is what the model then asks about. With it they work, and the +// existing-share, unknown-email and not-the-owner cases come back in words. +// delete_planting rides along: a hard delete that is still in the history. +func TestSharingToolsAskFirst(t *testing.T) { + ctx := context.Background() + svc, owner := newAgentTestService(t) + box := NewToolbox(svc, owner, "2026-08-23") + + call, mustCall := toolCaller(t, ctx, box) + refused := func(name string, args any, wantWords ...string) { + t.Helper() + res := call(name, args) + if !res.IsError { + t.Fatalf("%s %v succeeded, want a refusal", name, args) + } + for _, w := range wantWords { + if !strings.Contains(res.Content, w) { + t.Errorf("%s refusal = %q, want it to mention %q", name, res.Content, w) + } + } + } + + g, err := svc.CreateGarden(ctx, owner, service.GardenInput{Name: "Home", WidthCM: 1000, HeightCM: 1000}) + if err != nil { + t.Fatalf("garden: %v", err) + } + if _, err := svc.Register(ctx, service.RegisterInput{Email: "sam@example.com", DisplayName: "Sam", Password: "password123"}); err != nil { + t.Fatalf("register sam: %v", err) + } + + // --- share_garden: refused until confirmed, then grants, then changes the role. + refused("share_garden", map[string]any{"gardenId": g.ID, "email": "sam@example.com", "role": "editor"}, "ask the user first", "sam@example.com", "editor") + var shared struct { + Share shareView `json:"share"` + Note string `json:"note"` + } + mustCall("share_garden", map[string]any{"gardenId": g.ID, "email": "sam@example.com", "role": "editor", "confirmed": true}, &shared) + if shared.Share.Role != domain.RoleEditor || shared.Share.Email != "sam@example.com" { + t.Errorf("share = %+v, want sam as editor", shared) + } + mustCall("share_garden", map[string]any{"gardenId": g.ID, "email": "Sam@Example.com", "role": "viewer", "confirmed": true}, &shared) + if shared.Share.Role != domain.RoleViewer || !strings.Contains(shared.Note, "now viewer") { + t.Errorf("re-share as viewer = %+v, want the role changed and said so", shared) + } + mustCall("share_garden", map[string]any{"gardenId": g.ID, "email": "sam@example.com", "role": "viewer", "confirmed": true}, &shared) + if !strings.Contains(shared.Note, "nothing changed") { + t.Errorf("a no-op re-share = %+v, want a note that nothing changed", shared) + } + refused("share_garden", map[string]any{"gardenId": g.ID, "email": "nobody@example.com", "role": "viewer", "confirmed": true}, "no account", "sign in") + refused("share_garden", map[string]any{"gardenId": g.ID, "email": "sam@example.com", "role": "owner", "confirmed": true}) + + var listed struct { + Shares []shareView `json:"shares"` + PublicLink map[string]any `json:"publicLink"` + } + mustCall("list_shares", map[string]any{"gardenId": g.ID}, &listed) + if len(listed.Shares) != 1 || listed.Shares[0].Email != "sam@example.com" || listed.Shares[0].Role != domain.RoleViewer || listed.PublicLink["enabled"] != false { + t.Errorf("list_shares = %+v", listed) + } + // Not the owner: Sam can see the garden but can't manage its sharing. + sam, err := svc.Login(ctx, "sam@example.com", "password123") + if err != nil { + t.Fatalf("login sam: %v", err) + } + if r := NewToolbox(svc, sam.ID, "").Execute(ctx, llm.ToolCall{ID: "2", Name: "list_shares", Arguments: mustJSON(t, map[string]any{"gardenId": g.ID})}); !r.IsError { + t.Error("a viewer listed the garden's shares") + } + + // --- remove_share: refused until confirmed; by email; unknown email explained. + refused("remove_share", map[string]any{"gardenId": g.ID, "email": "sam@example.com"}, "ask the user first", "removing sam@example.com") + refused("remove_share", map[string]any{"gardenId": g.ID, "email": "nobody@example.com", "confirmed": true}, "not shared with") + mustCall("remove_share", map[string]any{"gardenId": g.ID, "email": "sam@example.com", "confirmed": true}, nil) + mustCall("list_shares", map[string]any{"gardenId": g.ID}, &listed) + if len(listed.Shares) != 0 { + t.Errorf("shares after remove = %+v, want none", listed.Shares) + } + + // --- public_link: get is free; enable/rotate/disable need a yes. + var link struct { + Enabled bool `json:"enabled"` + URL string `json:"url"` + } + mustCall("public_link", map[string]any{"gardenId": g.ID, "action": "get"}, &link) + if link.Enabled || link.URL != "" { + t.Errorf("fresh garden's link = %+v, want off with no url", link) + } + refused("public_link", map[string]any{"gardenId": g.ID, "action": "enable"}, "ask the user first", "anyone with the link") + mustCall("public_link", map[string]any{"gardenId": g.ID, "action": "enable", "confirmed": true}, &link) + if !link.Enabled || !strings.HasPrefix(link.URL, "/g/") { + t.Fatalf("enabled link = %+v, want on with a /g/ url", link) + } + first := link.URL + mustCall("public_link", map[string]any{"gardenId": g.ID, "action": "enable", "confirmed": true}, &link) + if link.URL != first { + t.Error("enabling an enabled link changed the url; that is what rotate is for") + } + mustCall("public_link", map[string]any{"gardenId": g.ID, "action": "rotate", "confirmed": true}, &link) + if !link.Enabled || link.URL == first { + t.Errorf("rotated link = %+v, want a different url", link) + } + refused("public_link", map[string]any{"gardenId": g.ID, "action": "disable"}, "stops working") + mustCall("public_link", map[string]any{"gardenId": g.ID, "action": "disable", "confirmed": true}, &link) + if link.Enabled { + t.Error("the link is still on after disable") + } + refused("public_link", map[string]any{"gardenId": g.ID, "action": "share", "confirmed": true}, "get, enable, rotate or disable") + + // --- delete_planting: gone from every view, but in the history — undoable. + bed, err := svc.CreateObject(ctx, owner, g.ID, service.ObjectInput{Kind: domain.KindBed, Name: "Bed", XCM: 500, YCM: 500, WidthCM: 200, HeightCM: 200}) + if err != nil { + t.Fatalf("bed: %v", err) + } + basil := mustPlant(t, svc, owner, "Basil", 25, "🌿") + var plop domain.Planting + mustCall("place_planting", map[string]any{"objectId": bed.ID, "plantId": basil.ID, "xCm": 0, "yCm": 0}, &plop) + mustCall("delete_planting", map[string]any{"plantingId": plop.ID}, nil) + var desc service.DescribeResult + mustCall("describe_garden", map[string]any{"gardenId": g.ID, "year": 2026}, &desc) + if len(desc.Objects[0].Plantings) != 0 { + t.Errorf("a deleted plop still shows in the season view: %+v", desc.Objects[0].Plantings) + } + var hist struct { + Entries []historyEntry `json:"entries"` + } + mustCall("read_history", map[string]any{"gardenId": g.ID, "limit": 1}, &hist) + if len(hist.Entries) != 1 || !strings.HasPrefix(hist.Entries[0].Summary, "Deleted a planting") { + t.Fatalf("history[0] = %+v, want the deletion", hist.Entries) + } + mustCall("undo_change", map[string]any{"changeSetId": hist.Entries[0].ID}, nil) + mustCall("describe_garden", map[string]any{"gardenId": g.ID}, &desc) + if len(desc.Objects[0].Plantings) != 1 || desc.Objects[0].Plantings[0].Each[0].ID != plop.ID { + t.Errorf("undoing the delete did not bring the plop back: %+v", desc.Objects[0].Plantings) + } +} diff --git a/internal/service/public.go b/internal/service/public.go index 393f326..bc609cc 100644 --- a/internal/service/public.go +++ b/internal/service/public.go @@ -87,6 +87,18 @@ func (s *Service) EnablePublicShareLink(ctx context.Context, actorID, gardenID i return linkState(token), nil } +// PublicShareURL is the address a public link opens at: absolute when the +// instance knows its base URL (PANSY_BASE_URL), else the site-relative path the +// editor uses, which a person can complete with the host they are looking at. +// 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 != "" { + return s.cfg.BaseURL + path + } + return path +} + // DisablePublicShareLink turns the public link off (clears the token). Owner // only; idempotent (disabling an already-disabled link is a no-op success). func (s *Service) DisablePublicShareLink(ctx context.Context, actorID, gardenID int64) error { diff --git a/web/src/lib/agent.ts b/web/src/lib/agent.ts index 0c5d07a..6d80e3a 100644 --- a/web/src/lib/agent.ts +++ b/web/src/lib/agent.ts @@ -109,6 +109,7 @@ const TOOL_LABELS: Record = { move_planting: 'Moving a plant', update_planting: 'Correcting a planting', remove_planting: 'Pulling a plant', + delete_planting: 'Deleting a planting', remove_plantings: 'Pulling plants', clear_object: 'Clearing a bed', find_plant: 'Looking up a plant', @@ -128,6 +129,10 @@ const TOOL_LABELS: Record = { copy_garden: 'Copying the garden', update_garden: 'Changing the garden', create_garden: 'Starting a garden', + list_shares: 'Checking who has access', + share_garden: 'Sharing the garden', + remove_share: 'Removing access', + public_link: 'Managing the public link', } export function describeStep(step: AgentStep): string { -- 2.54.0 From 608ef7c58e90f3a60b791c0bb9149601488017bb Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sun, 23 Aug 2026 02:21:04 -0400 Subject: [PATCH 2/2] Address #131 review: unknown link action is unknown, typed views, one share shape - 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 --- internal/agent/tools.go | 71 +++++++++++++++++++++++------------- internal/agent/tools_test.go | 13 ++++--- internal/service/public.go | 4 +- 3 files changed, 56 insertions(+), 32 deletions(-) diff --git a/internal/agent/tools.go b/internal/agent/tools.go index 3d186f7..2dfc9ce 100644 --- a/internal/agent/tools.go +++ b/internal/agent/tools.go @@ -993,26 +993,35 @@ func (a *adapter) listShares(ctx context.Context, args struct { return nil, err } out := make([]shareView, 0, len(shares)) - for _, sh := range shares { - out = append(out, shareView{UserID: sh.UserID, Email: sh.Email, DisplayName: sh.DisplayName, Role: sh.Role}) + for i := range shares { + out = append(out, toShareView(&shares[i])) } link, err := a.svc.GetPublicShareLink(ctx, a.actor, args.GardenID) if err != nil { return nil, err } - return map[string]any{"shares": out, "publicLink": a.linkView(link)}, nil + return map[string]any{"shares": out, "publicLink": a.linkOf(link)}, nil } // 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 { - v := map[string]any{"enabled": link.Enabled} +type linkView struct { + Enabled bool `json:"enabled"` + URL string `json:"url,omitempty"` +} + +func (a *adapter) linkOf(link *service.PublicShareLink) linkView { + v := linkView{Enabled: link.Enabled} if link.Enabled { - v["url"] = a.svc.PublicShareURL(link.Token) + v.URL = a.svc.PublicShareURL(link.Token) } return v } +func toShareView(sh *domain.ShareWithUser) shareView { + return shareView{UserID: sh.UserID, Email: sh.Email, DisplayName: sh.DisplayName, Role: sh.Role} +} + func (a *adapter) shareGarden(ctx context.Context, args struct { GardenID int64 `json:"gardenId" description:"garden to share (the user must own it)"` Email string `json:"email" description:"the other person's pansy account email, exactly as the user gave it"` @@ -1027,7 +1036,7 @@ func (a *adapter) shareGarden(ctx context.Context, args struct { if !args.Confirmed { return nil, errUnconfirmed(fmt.Sprintf("sharing this garden with %s as %s", email, role)) } - share, err := a.svc.AddShare(ctx, a.actor, args.GardenID, email, role) + _, err := a.svc.AddShare(ctx, a.actor, args.GardenID, email, role) switch { case errors.Is(err, domain.ErrShareUserNotFound): return nil, fmt.Errorf("%w — they need to sign in to pansy once before a garden can be shared with them", err) @@ -1038,19 +1047,22 @@ func (a *adapter) shareGarden(ctx context.Context, args struct { 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}, - "note": "already shared with them at that role; nothing changed"}, nil + return map[string]any{"share": toShareView(existing), "note": "already shared with them at that role; nothing changed"}, nil } - updated, uerr := a.svc.UpdateShareRole(ctx, a.actor, args.GardenID, existing.UserID, role) - if uerr != nil { - return nil, uerr + if _, err := a.svc.UpdateShareRole(ctx, a.actor, args.GardenID, existing.UserID, role); err != nil { + return nil, err } - return map[string]any{"share": shareView{UserID: updated.UserID, Email: existing.Email, DisplayName: existing.DisplayName, Role: updated.Role}, - "note": "they already had access; their role is now " + updated.Role}, nil + existing.Role = role + return map[string]any{"share": toShareView(existing), "note": "they already had access; their role is now " + role}, nil case err != nil: return nil, err } - return map[string]any{"share": shareView{UserID: share.UserID, Email: email, Role: share.Role}}, nil + // Read the share back so every path reports the same shape, name included. + created, err := a.findShare(ctx, args.GardenID, email) + if err != nil { + return nil, err + } + return map[string]any{"share": toShareView(created)}, nil } // findShare resolves an email to the garden's share for it, case-insensitively @@ -1087,7 +1099,7 @@ func (a *adapter) removeShare(ctx context.Context, args struct { if err := a.svc.RemoveShare(ctx, a.actor, args.GardenID, share.UserID); err != nil { return nil, err } - return map[string]any{"removed": shareView{UserID: share.UserID, Email: share.Email, DisplayName: share.DisplayName, Role: share.Role}}, nil + return map[string]any{"removed": toShareView(share)}, nil } func (a *adapter) publicLink(ctx context.Context, args struct { @@ -1099,12 +1111,21 @@ func (a *adapter) publicLink(ctx context.Context, args struct { if action == "" { action = "get" } - 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]) + // What each outward-facing action does, in the words the model should ask + // with. Checked before the confirmation gate, so an unknown action is told + // it is unknown rather than asked to confirm nothing in particular. + asks := map[string]string{ + "get": "", + "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", + } + ask, known := asks[action] + if !known { + return nil, fmt.Errorf("%w: action must be get, enable, rotate or disable", domain.ErrInvalidInput) + } + if ask != "" && !args.Confirmed { + return nil, errUnconfirmed(ask) } var ( link *service.PublicShareLink @@ -1117,15 +1138,13 @@ func (a *adapter) publicLink(ctx context.Context, args struct { link, err = a.svc.EnablePublicShareLink(ctx, a.actor, args.GardenID, false) case "rotate": link, err = a.svc.EnablePublicShareLink(ctx, a.actor, args.GardenID, true) - case "disable": + default: // disable if err = a.svc.DisablePublicShareLink(ctx, a.actor, args.GardenID); err == nil { link = &service.PublicShareLink{Enabled: false} } - default: - return nil, fmt.Errorf("%w: action must be get, enable, rotate or disable", domain.ErrInvalidInput) } if err != nil { return nil, err } - return a.linkView(link), nil + return a.linkOf(link), nil } diff --git a/internal/agent/tools_test.go b/internal/agent/tools_test.go index ccc8c0b..69be8b8 100644 --- a/internal/agent/tools_test.go +++ b/internal/agent/tools_test.go @@ -1066,8 +1066,8 @@ func TestSharingToolsAskFirst(t *testing.T) { Note string `json:"note"` } mustCall("share_garden", map[string]any{"gardenId": g.ID, "email": "sam@example.com", "role": "editor", "confirmed": true}, &shared) - if shared.Share.Role != domain.RoleEditor || shared.Share.Email != "sam@example.com" { - t.Errorf("share = %+v, want sam as editor", shared) + if shared.Share.Role != domain.RoleEditor || shared.Share.Email != "sam@example.com" || shared.Share.DisplayName != "Sam" { + t.Errorf("share = %+v, want Sam as editor, named", shared) } mustCall("share_garden", map[string]any{"gardenId": g.ID, "email": "Sam@Example.com", "role": "viewer", "confirmed": true}, &shared) if shared.Share.Role != domain.RoleViewer || !strings.Contains(shared.Note, "now viewer") { @@ -1081,11 +1081,11 @@ func TestSharingToolsAskFirst(t *testing.T) { refused("share_garden", map[string]any{"gardenId": g.ID, "email": "sam@example.com", "role": "owner", "confirmed": true}) var listed struct { - Shares []shareView `json:"shares"` - PublicLink map[string]any `json:"publicLink"` + Shares []shareView `json:"shares"` + PublicLink linkView `json:"publicLink"` } mustCall("list_shares", map[string]any{"gardenId": g.ID}, &listed) - if len(listed.Shares) != 1 || listed.Shares[0].Email != "sam@example.com" || listed.Shares[0].Role != domain.RoleViewer || listed.PublicLink["enabled"] != false { + if len(listed.Shares) != 1 || listed.Shares[0].Email != "sam@example.com" || listed.Shares[0].Role != domain.RoleViewer || listed.PublicLink.Enabled { t.Errorf("list_shares = %+v", listed) } // Not the owner: Sam can see the garden but can't manage its sharing. @@ -1135,6 +1135,9 @@ func TestSharingToolsAskFirst(t *testing.T) { t.Error("the link is still on after disable") } refused("public_link", map[string]any{"gardenId": g.ID, "action": "share", "confirmed": true}, "get, enable, rotate or disable") + // An unknown action is unknown whether or not it was confirmed — not a + // request to confirm nothing in particular. + refused("public_link", map[string]any{"gardenId": g.ID, "action": "share"}, "get, enable, rotate or disable") // --- delete_planting: gone from every view, but in the history — undoable. bed, err := svc.CreateObject(ctx, owner, g.ID, service.ObjectInput{Kind: domain.KindBed, Name: "Bed", XCM: 500, YCM: 500, WidthCM: 200, HeightCM: 200}) diff --git a/internal/service/public.go b/internal/service/public.go index bc609cc..132b165 100644 --- a/internal/service/public.go +++ b/internal/service/public.go @@ -94,7 +94,9 @@ func (s *Service) EnablePublicShareLink(ctx context.Context, actorID, gardenID i func (s *Service) PublicShareURL(token string) string { path := "/g/" + token if s.cfg != nil && s.cfg.BaseURL != "" { - return s.cfg.BaseURL + path + // config trims the trailing slash already; a Config built by hand + // (tests, an embedder) may not have. + return strings.TrimRight(s.cfg.BaseURL, "/") + path } return path } -- 2.54.0