From 608ef7c58e90f3a60b791c0bb9149601488017bb Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sun, 23 Aug 2026 02:21:04 -0400 Subject: [PATCH] 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 }