From 8dd214b4accc8385b26600a304d26abb199b84d6 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 29 Sep 2026 18:50:18 -0700 Subject: [PATCH 01/11] Mention agents by name within the command's project Fuzzy @Name mentions resolve only against the pingable set, and agents are never pingable by design, so an @mention meant for an agent always posted as plain text. When the pingable set has no exact answer and the command has a project in scope (a project it resolved, the item URL's bucket, or --in), the agents on that project's people list are candidates too. A name equal to an agent's wins over a person whose name merely contains it; people otherwise keep precedence, ambiguity is still reported, and a person who is not pingable is still not mentionable. [@Name](person:ID) for an agent's ID now resolves through a person lookup instead of failing the command. The unresolved-mention notice says how an agent can be reached. --- internal/commands/cards.go | 6 +- internal/commands/chat.go | 4 +- internal/commands/chat_test.go | 3 + internal/commands/comment.go | 6 +- internal/commands/comment_thread_test.go | 2 +- internal/commands/helpers.go | 75 ++++++- internal/commands/mention_agents_test.go | 159 ++++++++++++++ internal/commands/messages.go | 6 +- internal/commands/schedule.go | 4 +- internal/commands/todos.go | 2 +- internal/names/mention.go | 184 +++++++++++++++++ internal/names/mention_test.go | 253 +++++++++++++++++++++++ internal/names/resolver.go | 4 + skills/basecamp/SKILL.md | 7 +- 14 files changed, 692 insertions(+), 23 deletions(-) create mode 100644 internal/commands/mention_agents_test.go create mode 100644 internal/names/mention.go create mode 100644 internal/names/mention_test.go diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 4877b6e76..23e52bd29 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -982,7 +982,7 @@ Use - as the body argument to read the body from stdin: if err != nil { return err } - mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, content) + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, mentionScope(app, resolvedProjectID), content) if mentionErr != nil { return mentionErr } @@ -1101,7 +1101,7 @@ You can pass either a card ID or a Basecamp URL: } // Extract ID from URL if provided - cardIDStr := extractID(args[0]) + cardIDStr, urlProjectID := extractWithProject(args[0]) cardID, err := strconv.ParseInt(cardIDStr, 10, 64) if err != nil { @@ -1157,7 +1157,7 @@ You can pass either a card ID or a Basecamp URL: if err != nil { return err } - mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, html) + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, mentionScope(app, urlProjectID, projectFlagValue(cmd), app.Flags.Project), html) if mentionErr != nil { return mentionErr } diff --git a/internal/commands/chat.go b/internal/commands/chat.go index b6cd655f6..c2c9cebda 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -425,7 +425,7 @@ func runChatPost(cmd *cobra.Command, app *appctx.App, chatID, project, content, if contentType == "" { mentionInput = richtext.MarkdownToHTML(content) } - result, resolveErr := resolveMentions(cmd.Context(), app.Names, mentionInput) + result, resolveErr := resolveMentions(cmd.Context(), app.Names, mentionScope(app, resolvedProjectID, project, app.Flags.Project), mentionInput) if resolveErr != nil { return resolveErr } @@ -1025,7 +1025,7 @@ edit to rich text.`, if ct == "" { messageContent = richtext.MarkdownToHTML(messageContent) } - result, resolveErr := resolveMentions(cmd.Context(), app.Names, messageContent) + result, resolveErr := resolveMentions(cmd.Context(), app.Names, mentionScope(app, urlProjectID, *project, app.Flags.Project), messageContent) if resolveErr != nil { return resolveErr } diff --git a/internal/commands/chat_test.go b/internal/commands/chat_test.go index a7bdef17a..10c80b2fe 100644 --- a/internal/commands/chat_test.go +++ b/internal/commands/chat_test.go @@ -961,6 +961,9 @@ func (t *mockChatMultiMentionTransport) RoundTrip(req *http.Request) (*http.Resp switch { case strings.Contains(req.URL.Path, "/projects.json"): body = `[{"id": 123, "name": "Test Project"}]` + case strings.HasSuffix(req.URL.Path, "/projects/123/people.json"): + // The project's people, where an unpingable agent would be found. + body = `[{"id": 42000, "name": "Jane Smith", "attachable_sgid": "sgid-jane", "personable_type": "User"}]` case strings.Contains(req.URL.Path, "/projects/"): body = `{"id": 123, "dock": [{"name": "chat", "id": 789, "enabled": true}]}` case strings.Contains(req.URL.Path, "/circles/people.json") || strings.Contains(req.URL.Path, "/people/pingable.json"): diff --git a/internal/commands/comment.go b/internal/commands/comment.go index bbc51ac83..ec0386b62 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -1051,7 +1051,7 @@ as backslash-n.`, // Extract comment ID from URL if provided // Uses extractCommentWithProject to prefer CommentID from URL fragments - commentIDStr, _ := extractCommentWithProject(args[0]) + commentIDStr, urlProjectID := extractCommentWithProject(args[0]) commentID, err := strconv.ParseInt(commentIDStr, 10, 64) if err != nil { @@ -1084,7 +1084,7 @@ as backslash-n.`, } // Resolve @mentions - mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, mentionScope(app, urlProjectID, projectFlagValue(cmd), app.Flags.Project), html) if err != nil { return err } @@ -1231,7 +1231,7 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: } // Resolve @mentions (e.g., @John, @John.Doe → clickable mention tags) - mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, mentionScope(app, sharedURLProject(recordingArg), projectFlagValue(cmd), app.Flags.Project), html) if err != nil { return err } diff --git a/internal/commands/comment_thread_test.go b/internal/commands/comment_thread_test.go index bc8a2ed3e..090b45975 100644 --- a/internal/commands/comment_thread_test.go +++ b/internal/commands/comment_thread_test.go @@ -229,7 +229,7 @@ func TestFocusMentionRoundTripsThroughResolveMentions(t *testing.T) { // embedded SGID must resolve to a mention attachment with zero // lookups — even when the display label came from a hostile name. html := richtext.MarkdownToHTML(syntax) - result, err := resolveMentions(context.Background(), nil, html) + result, err := resolveMentions(context.Background(), nil, nil, html) require.NoError(t, err) assert.Contains(t, result.HTML, "BAh7CEkiCG") assert.Contains(t, result.HTML, "application/vnd.basecamp.mention") diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index b8f6f0886..a2d83948b 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -632,19 +632,26 @@ func applySubscribeFlags(ctx context.Context, resolver *names.Resolver, subscrib // resolveMentions scans HTML for mention syntax and replaces matches with // Basecamp mention attachment tags. Supports three syntaxes: // - [@Name](mention:SGID) — zero API calls (SGID embedded directly) -// - [@Name](person:ID) — one API call (ID→SGID via pingable set) -// - @Name / @First.Last — fuzzy name resolution via pingable set +// - [@Name](person:ID) — one API call (ID→SGID via pingable set; an agent's +// ID costs one more, a person lookup) +// - @Name / @First.Last — fuzzy name resolution via pingable set, then the +// agents on the project in scope // // Also supports @sgid:VALUE inline syntax for pipeline composability. // Silently returns unchanged HTML if no mentions are found. // +// Agents cannot be pinged, so the pingable set never holds them. scope names +// the project whose agents a fuzzy @Name may reach; it is consulted only when +// the pingable set has no exact answer, and may be nil when the command has +// no project in scope. +// // Fuzzy @Name mentions that cannot be resolved (not found or ambiguous) are // left as plain text; their names are returned in the Unresolved slice. // Deterministic syntaxes (mention:SGID, person:ID) still hard-fail on error. -func resolveMentions(ctx context.Context, resolver *names.Resolver, html string) (richtext.MentionResult, error) { +func resolveMentions(ctx context.Context, resolver *names.Resolver, scope names.ProjectScope, html string) (richtext.MentionResult, error) { return richtext.ResolveMentions(html, func(name string) (string, string, error) { - person, err := resolver.ResolvePersonByName(ctx, name) + person, err := resolver.ResolveMentionByName(ctx, name, scope) if err != nil { // Downgrade resolver-level not-found and ambiguous to skip — leave // as plain text. Only match errors without an HTTP status; API-level @@ -667,7 +674,7 @@ func resolveMentions(ctx context.Context, resolver *names.Resolver, html string) if err != nil { return "", "", output.ErrUsage(fmt.Sprintf("invalid person ID %q — must be numeric", id)) } - person, err := resolver.ResolvePersonByID(ctx, personID) + person, err := resolver.ResolveMentionByID(ctx, personID) if err != nil { return "", "", err } @@ -679,12 +686,68 @@ func resolveMentions(ctx context.Context, resolver *names.Resolver, html string) ) } +// mentionScope returns the project a fuzzy @mention may draw agents from: the +// first non-blank candidate, in the caller's order of precedence — typically +// a project the command already resolved, a URL's bucket, then --in. A +// numeric ID is used as-is; a name is resolved only when the scope is +// consulted. Returns nil when every candidate is blank. +func mentionScope(app *appctx.App, candidates ...string) names.ProjectScope { + for _, candidate := range candidates { + candidate = strings.TrimSpace(candidate) + if candidate == "" { + continue + } + if scope := names.ParseProjectScope(candidate); scope != nil { + return scope + } + return func(ctx context.Context) (int64, error) { + id, _, err := app.Names.ResolveProject(ctx, candidate) + if err != nil { + return 0, err + } + return strconv.ParseInt(id, 10, 64) + } + } + return nil +} + +// sharedURLProject returns the bucket named by every URL in a comma-separated +// target list, or "" when the list holds no URL or its URLs disagree. +func sharedURLProject(targets string) string { + shared := "" + for part := range strings.SplitSeq(targets, ",") { + _, projectID := extractWithProject(strings.TrimSpace(part)) + if projectID == "" { + continue + } + if shared != "" && shared != projectID { + return "" + } + shared = projectID + } + return shared +} + // unresolvedMentionWarning formats a warning string for unresolved mentions. +// The hint names the one reach a fuzzy @Name has for agents, since a +// mention meant for an agent is the miss a reader cannot otherwise explain. func unresolvedMentionWarning(unresolved []string) string { if len(unresolved) == 0 { return "" } - return "Unresolved mentions left as text: " + strings.Join(unresolved, ", ") + return "Unresolved mentions left as text: " + strings.Join(unresolved, ", ") + + " (agents match by name only on the project from --in or a URL; [@Name](person:ID) needs no project)" +} + +// projectFlagValue returns the --project/--in value visible to cmd, including +// one a parent command group defines, or "" when neither flag exists. +func projectFlagValue(cmd *cobra.Command) string { + for _, name := range []string{"project", "in"} { + if f := cmd.Flags().Lookup(name); f != nil && f.Value.String() != "" { + return f.Value.String() + } + } + return "" } // projectFlagChanged reports whether the user explicitly passed --project or diff --git a/internal/commands/mention_agents_test.go b/internal/commands/mention_agents_test.go new file mode 100644 index 000000000..e8c39878d --- /dev/null +++ b/internal/commands/mention_agents_test.go @@ -0,0 +1,159 @@ +package commands + +import ( + "bytes" + "encoding/json" + "io" + "net/http" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// agentMentionTransport fakes a project whose people include an agent. The +// agent is absent from the pingable set, as it is in Basecamp: an agent can +// never be in a Ping. +type agentMentionTransport struct { + mu sync.Mutex + gets map[string]int + posted []byte +} + +func (t *agentMentionTransport) getCount(suffix string) int { + t.mu.Lock() + defer t.mu.Unlock() + n := 0 + for path, c := range t.gets { + if strings.HasSuffix(path, suffix) { + n += c + } + } + return n +} + +func (t *agentMentionTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + respond := func(status int, body string) (*http.Response, error) { + return &http.Response{StatusCode: status, Body: io.NopCloser(strings.NewReader(body)), Header: header}, nil + } + + path := req.URL.Path + if req.Method == http.MethodGet { + t.mu.Lock() + if t.gets == nil { + t.gets = map[string]int{} + } + t.gets[path]++ + t.mu.Unlock() + + jane := `{"id": 42000, "name": "Jane Smith", "attachable_sgid": "sgid-jane", "personable_type": "User"}` + quincy := `{"id": 7, "name": "Quincy", "attachable_sgid": "sgid-quincy", "personable_type": "Agent"}` + switch { + case strings.HasSuffix(path, "/circles/people.json"): + return respond(200, "["+jane+"]") + case strings.HasSuffix(path, "/projects/123/people.json"): + return respond(200, "["+jane+","+quincy+"]") + case strings.HasSuffix(path, "/people/7"), strings.HasSuffix(path, "/people/7.json"): + return respond(200, quincy) + case strings.HasSuffix(path, "/projects.json"): + return respond(200, `[{"id": 123, "name": "Test Project"}]`) + default: + return respond(404, `{"error": "not found"}`) + } + } + + if req.Body != nil { + body, _ := io.ReadAll(req.Body) + req.Body.Close() + t.mu.Lock() + t.posted = body + t.mu.Unlock() + } + return respond(201, `{"id": 999, "content": "ok", "created_at": "2024-01-01T00:00:00Z"}`) +} + +func (t *agentMentionTransport) postedContent(tb testing.TB) string { + tb.Helper() + require.NotEmpty(tb, t.posted, "nothing was posted") + var body map[string]any + require.NoError(tb, json.Unmarshal(t.posted, &body)) + content, _ := body["content"].(string) + return content +} + +func noticeOf(tb testing.TB, buf *bytes.Buffer) string { + tb.Helper() + var envelope map[string]any + require.NoError(tb, json.Unmarshal(buf.Bytes(), &envelope)) + notice, _ := envelope["notice"].(string) + return notice +} + +const agentCardURL = "https://3.basecamp.com/99999/buckets/123/card_tables/cards/789" + +func TestCommentsCreateMentionsAgentFromURLProject(t *testing.T) { + transport := &agentMentionTransport{} + app, buf := newTestAppWithTransport(t, transport) + + err := executeChatCommand(NewCommentsCmd(), app, "create", agentCardURL, "Hey @Quincy, and @Jane.Smith") + require.NoError(t, err) + + content := transport.postedContent(t) + assert.Contains(t, content, `sgid="sgid-quincy"`) + assert.Contains(t, content, `sgid="sgid-jane"`) + assert.Empty(t, noticeOf(t, buf)) + assert.Equal(t, 1, transport.getCount("/projects/123/people.json")) +} + +func TestCommentsCreateMentionsAgentFromInFlag(t *testing.T) { + transport := &agentMentionTransport{} + app, _ := newTestAppWithTransport(t, transport) + + err := executeChatCommand(NewCommentsCmd(), app, "create", "789", "Hey @Quincy and @quincy", "--in", "123") + require.NoError(t, err) + + content := transport.postedContent(t) + assert.Equal(t, 2, strings.Count(content, `sgid="sgid-quincy"`)) + assert.Equal(t, 1, transport.getCount("/projects/123/people.json"), "one project fetch per run") +} + +func TestCommentsCreateAgentMissWithoutProjectIsNotSilent(t *testing.T) { + transport := &agentMentionTransport{} + app, buf := newTestAppWithTransport(t, transport) + + // A bare ID names no project, and the configured default project is not + // evidence of where the recording lives. + err := executeChatCommand(NewCommentsCmd(), app, "create", "789", "Hey @Quincy") + require.NoError(t, err) + + assert.NotContains(t, transport.postedContent(t), "sgid-quincy") + notice := noticeOf(t, buf) + assert.Contains(t, notice, "@Quincy") + assert.Contains(t, notice, "--in", "the miss should say how to reach an agent") + assert.Zero(t, transport.getCount("/projects/123/people.json")) +} + +func TestCommentsCreateMentionsAgentByPersonID(t *testing.T) { + transport := &agentMentionTransport{} + app, _ := newTestAppWithTransport(t, transport) + + err := executeChatCommand(NewCommentsCmd(), app, "create", "789", "Hey [@Quincy](person:7)") + require.NoError(t, err) + + assert.Contains(t, transport.postedContent(t), `sgid="sgid-quincy"`) +} + +func TestChatPostMentionsAgentInRoomProject(t *testing.T) { + transport := &agentMentionTransport{} + app, buf := newTestAppWithTransport(t, transport) + + err := executeChatCommand(NewChatCmd(), app, "post", "@Quincy please look", "--room", "789", "--in", "123") + require.NoError(t, err) + + assert.Contains(t, transport.postedContent(t), `sgid="sgid-quincy"`) + assert.Empty(t, noticeOf(t, buf)) +} diff --git a/internal/commands/messages.go b/internal/commands/messages.go index f221d2512..dd4e9ec90 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -544,7 +544,7 @@ Use - as the body argument to read the body from stdin: } // Resolve @mentions - mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, mentionScope(app, resolvedProjectID), html) if err != nil { return err } @@ -640,7 +640,7 @@ You can pass either a message ID or a Basecamp URL: } // Extract ID from URL if provided - messageIDStr := extractID(args[0]) + messageIDStr, urlProjectID := extractWithProject(args[0]) messageID, err := strconv.ParseInt(messageIDStr, 10, 64) if err != nil { @@ -672,7 +672,7 @@ You can pass either a message ID or a Basecamp URL: } // Resolve @mentions - mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, mentionScope(app, urlProjectID, projectFlagValue(cmd), app.Flags.Project), html) if err != nil { return err } diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index b0787de86..71f256eb5 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -537,7 +537,7 @@ func runScheduleCreate(cmd *cobra.Command, app *appctx.App, project, scheduleID, if err != nil { return err } - mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, description) + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, mentionScope(app, resolvedProjectID), description) if mentionErr != nil { return mentionErr } @@ -723,7 +723,7 @@ You can pass either an entry ID or a Basecamp URL: if err != nil { return err } - mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, html) + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, mentionScope(app, resolvedProjectID), html) if mentionErr != nil { return mentionErr } diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 560930639..080d4e2cd 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -2010,7 +2010,7 @@ Examples: if pipelineErr != nil { return pipelineErr } - mentionResult, pipelineErr := resolveMentions(cmd.Context(), app.Names, commentHTML) + mentionResult, pipelineErr := resolveMentions(cmd.Context(), app.Names, mentionScope(app, project), commentHTML) if pipelineErr != nil { return pipelineErr } diff --git a/internal/names/mention.go b/internal/names/mention.go new file mode 100644 index 000000000..dd69216f3 --- /dev/null +++ b/internal/names/mention.go @@ -0,0 +1,184 @@ +package names + +import ( + "context" + "errors" + "strconv" + "strings" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// ProjectScope yields the project whose agents an @mention may reach. The +// resolver calls it only when the pingable set cannot answer on its own, so +// a mention of a pingable person never pays for resolving the project. A nil +// scope means no project is in scope. +type ProjectScope func(context.Context) (int64, error) + +// ResolveMentionByName resolves a fuzzy @Name mention. +// +// People resolve against the pingable set, exactly as ResolvePersonByName +// does. Agents cannot be pinged by design, so they are never in that set; +// when scope names a project, the agents on that project are candidates too. +// Only agents are drawn from the project: a person who is not pingable stays +// unmentionable. +// +// A name that equals an agent's (ignoring case) wins over a person whose name +// merely contains it, so "@Quincy" reaches the agent Quincy rather than a +// pingable "Quincy Jones". Otherwise people keep precedence, and an agent is +// matched by partial name only when no person matches at all. +func (r *Resolver) ResolveMentionByName(ctx context.Context, input string, scope ProjectScope) (*Person, error) { + pingable, err := r.getPingable(ctx) + if err != nil { + return nil, err + } + + match, matches := resolve(input, pingable, personIDName) + if match != nil && strings.EqualFold(match.Name, input) { + return match, nil + } + if len(matches) > 1 && strings.EqualFold(matches[0].Name, input) { + return nil, ambiguousPeople(matches) + } + + agents, err := r.scopedAgents(ctx, scope) + if err != nil { + // Only fail when agents were the sole remaining answer: a mention the + // pingable set already matches loses just the exact-agent preference. + if match == nil && len(matches) == 0 { + return nil, err + } + agents = nil + } + + agent, agentMatches := resolve(input, agents, personIDName) + switch { + case agent != nil && strings.EqualFold(agent.Name, input): + return agent, nil + case len(agentMatches) > 1 && strings.EqualFold(agentMatches[0].Name, input): + return nil, ambiguousPeople(agentMatches) + case match != nil: + return match, nil + case len(matches) > 1: + return nil, ambiguousPeople(matches) + case agent != nil: + return agent, nil + case len(agentMatches) > 1: + return nil, ambiguousPeople(agentMatches) + } + + candidates := append(append([]Person{}, pingable...), agents...) + suggestions := suggest(input, candidates, func(p Person) string { return p.Name }) + if len(suggestions) > 0 { + return nil, output.ErrNotFoundHint("Person", input, "Did you mean: "+strings.Join(suggestions, ", ")) + } + return nil, output.ErrNotFound("Person", input) +} + +func (r *Resolver) scopedAgents(ctx context.Context, scope ProjectScope) ([]Person, error) { + if scope == nil { + return nil, nil + } + projectID, err := scope(ctx) + if err != nil { + return nil, err + } + return r.getProjectAgents(ctx, projectID) +} + +// ResolveMentionByID resolves a [@Name](person:ID) mention. A pingable person +// resolves from the cached pingable set. Any other ID is looked up directly +// and accepted only if it is an agent, which is the one kind of principal the +// pingable set leaves out by design. +func (r *Resolver) ResolveMentionByID(ctx context.Context, id int64) (*Person, error) { + person, err := r.ResolvePersonByID(ctx, id) + var cliErr *output.Error + if err == nil || !errors.As(err, &cliErr) || cliErr.Code != output.CodeNotFound || cliErr.HTTPStatus != 0 { + return person, err + } + notFound := err + + p, err := r.forAccount().People().Get(ctx, id) + if err != nil { + converted := convertSDKError(err) + var e *output.Error + if errors.As(converted, &e) && e.HTTPStatus == 404 { + return nil, notFound + } + return nil, converted + } + if p.PersonableType != personableAgent { + return nil, notFound + } + return &Person{ + ID: p.ID, + AttachableSGID: p.AttachableSGID, + Name: p.Name, + Email: p.EmailAddress, + PersonableType: p.PersonableType, + }, nil +} + +const personableAgent = "Agent" + +func personIDName(p Person) (int64, string) { return p.ID, p.Name } + +func ambiguousPeople(matches []Person) error { + names := make([]string, len(matches)) + for i, m := range matches { + names[i] = m.Name + } + return output.ErrAmbiguous("person", names) +} + +// getProjectAgents returns the agents on a project, fetched once per project +// per run. The project's people list is the one directory that includes +// agents together with the attachable SGID a mention needs. +func (r *Resolver) getProjectAgents(ctx context.Context, projectID int64) ([]Person, error) { + r.mu.RLock() + agents, ok := r.agents[projectID] + r.mu.RUnlock() + if ok { + return agents, nil + } + + r.mu.Lock() + defer r.mu.Unlock() + if agents, ok := r.agents[projectID]; ok { + return agents, nil + } + + result, err := r.forAccount().People().ListProjectPeople(ctx, projectID, nil) + if err != nil { + return nil, convertSDKError(err) + } + + agents = []Person{} + for _, p := range result.People { + if p.PersonableType == personableAgent { + agents = append(agents, Person{ + ID: p.ID, + AttachableSGID: p.AttachableSGID, + Name: p.Name, + Email: p.EmailAddress, + PersonableType: p.PersonableType, + }) + } + } + + if r.agents == nil { + r.agents = make(map[int64][]Person) + } + r.agents[projectID] = agents + return agents, nil +} + +// ParseProjectScope returns a scope for a project given by numeric ID, or nil +// when the value is empty or not an ID. +func ParseProjectScope(projectID string) ProjectScope { + id, err := strconv.ParseInt(projectID, 10, 64) + if err != nil || id <= 0 { + return nil + } + return func(context.Context) (int64, error) { return id, nil } +} diff --git a/internal/names/mention_test.go b/internal/names/mention_test.go new file mode 100644 index 000000000..c79b18984 --- /dev/null +++ b/internal/names/mention_test.go @@ -0,0 +1,253 @@ +package names + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// mentionServer fakes the three endpoints mention resolution reads: the +// pingable set (people only — agents cannot be pinged), a project's people +// (people and agents alike), and a single person. It counts requests by path +// so tests can hold the resolver to one fetch per list per run. +type mentionServer struct { + mu sync.Mutex + calls map[string]int + pingable []map[string]any + project map[string][]map[string]any + people map[string]map[string]any +} + +func (s *mentionServer) count(path string) int { + s.mu.Lock() + defer s.mu.Unlock() + return s.calls[path] +} + +func (s *mentionServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + s.calls[r.URL.Path]++ + s.mu.Unlock() + + w.Header().Set("Content-Type", "application/json") + path := strings.TrimPrefix(r.URL.Path, "/99999") + switch { + case path == "/circles/people.json": + _ = json.NewEncoder(w).Encode(s.pingable) + case strings.HasPrefix(path, "/projects/") && strings.HasSuffix(path, "/people.json"): + id := strings.TrimSuffix(strings.TrimPrefix(path, "/projects/"), "/people.json") + people, ok := s.project[id] + if !ok { + http.NotFound(w, r) + return + } + _ = json.NewEncoder(w).Encode(people) + case strings.HasPrefix(path, "/people/"): + id := strings.TrimSuffix(strings.TrimPrefix(path, "/people/"), ".json") + person, ok := s.people[id] + if !ok { + http.NotFound(w, r) + return + } + _ = json.NewEncoder(w).Encode(person) + default: + http.NotFound(w, r) + } +} + +func person(id int64, name, sgid, personable string) map[string]any { + return map[string]any{"id": id, "name": name, "attachable_sgid": sgid, "personable_type": personable} +} + +func newMentionFixture(t *testing.T) (*Resolver, *mentionServer) { + t.Helper() + jane := person(1, "Jane Smith", "sgid-jane", "User") + quincy := person(7, "Quincy", "sgid-quincy", "Agent") + shy := person(2, "Shy Human", "sgid-shy", "User") + s := &mentionServer{ + calls: map[string]int{}, + pingable: []map[string]any{jane}, + project: map[string][]map[string]any{ + "123": {jane, quincy, shy}, + }, + people: map[string]map[string]any{"1": jane, "2": shy, "7": quincy}, + } + server := httptest.NewServer(s) + t.Cleanup(server.Close) + + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: server.URL}, testTokenProvider{}, basecamp.WithMaxRetries(1)) + return NewResolver(sdkClient, nil, "99999"), s +} + +func inProject(id int64) ProjectScope { + return func(context.Context) (int64, error) { return id, nil } +} + +const projectPeoplePath = "/99999/projects/123/people.json" + +func TestResolveMentionByNameFindsAgentInProject(t *testing.T) { + r, s := newMentionFixture(t) + ctx := context.Background() + + p, err := r.ResolveMentionByName(ctx, "Quincy", inProject(123)) + require.NoError(t, err) + assert.Equal(t, "sgid-quincy", p.AttachableSGID) + + p, err = r.ResolveMentionByName(ctx, "quincy", inProject(123)) + require.NoError(t, err) + assert.Equal(t, "sgid-quincy", p.AttachableSGID) + + assert.Equal(t, 1, s.count(projectPeoplePath), "project people fetched once per run") + assert.Equal(t, 1, s.count("/99999/circles/people.json"), "pingable fetched once per run") +} + +func TestResolveMentionByNamePingableHitCostsNothingExtra(t *testing.T) { + r, s := newMentionFixture(t) + + p, err := r.ResolveMentionByName(context.Background(), "Jane Smith", inProject(123)) + require.NoError(t, err) + assert.Equal(t, "sgid-jane", p.AttachableSGID) + assert.Zero(t, s.count(projectPeoplePath)) +} + +func TestResolveMentionByNameScopeIsLazy(t *testing.T) { + r, _ := newMentionFixture(t) + called := false + scope := func(context.Context) (int64, error) { called = true; return 123, nil } + + _, err := r.ResolveMentionByName(context.Background(), "Jane Smith", scope) + require.NoError(t, err) + assert.False(t, called, "an exact pingable hit must not resolve the project") +} + +func TestResolveMentionByNameLeavesPeopleSemanticsAlone(t *testing.T) { + r, _ := newMentionFixture(t) + + // A person who is in the project but not pingable stays unmentionable: + // only agents are unpingable by design. + _, err := r.ResolveMentionByName(context.Background(), "Shy Human", inProject(123)) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "got %v", err) + assert.Equal(t, output.CodeNotFound, outErr.Code) +} + +func TestResolveMentionByNameWithoutScopeMisses(t *testing.T) { + r, s := newMentionFixture(t) + + _, err := r.ResolveMentionByName(context.Background(), "Quincy", nil) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "got %v", err) + assert.Equal(t, output.CodeNotFound, outErr.Code) + assert.Zero(t, s.count(projectPeoplePath)) +} + +func TestResolveMentionByNamePrefersExactAgentOverPartialPerson(t *testing.T) { + r, s := newMentionFixture(t) + s.pingable = append(s.pingable, person(3, "Quincy Jones", "sgid-qj", "User")) + + p, err := r.ResolveMentionByName(context.Background(), "Quincy", inProject(123)) + require.NoError(t, err) + assert.Equal(t, "sgid-quincy", p.AttachableSGID) + + // A partial name still reaches the person, as before. + p, err = r.ResolveMentionByName(context.Background(), "Quincy J", inProject(123)) + require.NoError(t, err) + assert.Equal(t, "sgid-qj", p.AttachableSGID) +} + +func TestResolveMentionByNameAgentAmbiguity(t *testing.T) { + r, s := newMentionFixture(t) + s.project["123"] = append(s.project["123"], + person(8, "Build Bot", "sgid-bb", "Agent"), + person(9, "Deploy Bot", "sgid-db", "Agent")) + + _, err := r.ResolveMentionByName(context.Background(), "Bot", inProject(123)) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "got %v", err) + assert.Equal(t, output.CodeAmbiguous, outErr.Code) + + p, err := r.ResolveMentionByName(context.Background(), "Build", inProject(123)) + require.NoError(t, err) + assert.Equal(t, "sgid-bb", p.AttachableSGID) +} + +func TestResolveMentionByNameProjectFetchFailureIsHard(t *testing.T) { + r, _ := newMentionFixture(t) + + _, err := r.ResolveMentionByName(context.Background(), "Quincy", inProject(404)) + require.Error(t, err) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "got %v", err) + assert.NotZero(t, outErr.HTTPStatus, "an API failure must carry its status so callers hard-fail") +} + +func TestResolveMentionByIDFindsAgent(t *testing.T) { + r, s := newMentionFixture(t) + ctx := context.Background() + + p, err := r.ResolveMentionByID(ctx, 7) + require.NoError(t, err) + assert.Equal(t, "sgid-quincy", p.AttachableSGID) + + p, err = r.ResolveMentionByID(ctx, 1) + require.NoError(t, err) + assert.Equal(t, "sgid-jane", p.AttachableSGID) + assert.Zero(t, s.count("/99999/people/1.json"), "a pingable hit needs no person lookup") +} + +func TestResolveMentionByIDLeavesPeopleSemanticsAlone(t *testing.T) { + r, _ := newMentionFixture(t) + + _, err := r.ResolveMentionByID(context.Background(), 2) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "got %v", err) + assert.Equal(t, output.CodeNotFound, outErr.Code) + + _, err = r.ResolveMentionByID(context.Background(), 404) + require.True(t, errors.As(err, &outErr), "got %v", err) + assert.Equal(t, output.CodeNotFound, outErr.Code) +} + +func TestResolveMentionByNameKeepsPingableMatchWhenAgentLookupFails(t *testing.T) { + r, _ := newMentionFixture(t) + + // "Jane" partially matches a pingable person; the project cannot be + // read, which costs only the exact-agent preference. + p, err := r.ResolveMentionByName(context.Background(), "Jane", inProject(404)) + require.NoError(t, err) + assert.Equal(t, "sgid-jane", p.AttachableSGID) +} + +func TestResolveMentionByIDPingableFailureIsHard(t *testing.T) { + r, s := newMentionFixture(t) + s.pingable = nil + // The pingable set fails while the person lookup would succeed: the + // failure must surface rather than be read as "not pingable". + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if strings.HasSuffix(req.URL.Path, "/circles/people.json") { + http.Error(w, `{"error":"boom"}`, http.StatusInternalServerError) + return + } + s.ServeHTTP(w, req) + })) + t.Cleanup(server.Close) + r.sdk = basecamp.NewClient(&basecamp.Config{BaseURL: server.URL}, testTokenProvider{}, basecamp.WithMaxRetries(1)) + + p, err := r.ResolveMentionByID(context.Background(), 7) + require.Error(t, err, "resolved %+v despite the pingable failure", p) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "got %v", err) + assert.Equal(t, http.StatusInternalServerError, outErr.HTTPStatus) +} diff --git a/internal/names/resolver.go b/internal/names/resolver.go index f58bc9017..832962a7e 100644 --- a/internal/names/resolver.go +++ b/internal/names/resolver.go @@ -36,6 +36,7 @@ type Resolver struct { projects []Project people []Person pingable []Person // cached /people/pingable.json + agents map[int64][]Person // agents on a project, keyed by project ID todolists map[string][]Todolist // keyed by project ID me *Person // cached /my/profile.json result } @@ -69,6 +70,7 @@ func NewResolver(sdkClient *basecamp.Client, authMgr *auth.Manager, accountID st auth: authMgr, accountID: accountID, todolists: make(map[string][]Todolist), + agents: make(map[int64][]Person), } } @@ -83,6 +85,7 @@ func (r *Resolver) SetAccountID(accountID string) { r.projects = nil r.people = nil r.pingable = nil + r.agents = make(map[int64][]Person) r.me = nil r.todolists = make(map[string][]Todolist) } @@ -367,6 +370,7 @@ func (r *Resolver) ClearCache() { r.projects = nil r.people = nil r.pingable = nil + r.agents = make(map[int64][]Person) r.me = nil r.todolists = make(map[string][]Todolist) } diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 337fbe1af..64612b5e2 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -93,9 +93,9 @@ Full CLI coverage: 195 tracked in-scope endpoints across todos, cards, messages, 4. **Check context** via `.basecamp/config.json` before assuming project 5. **Content fields accept Markdown, and most accept @mentions** — the CLI converts these rich-text fields from Markdown to HTML: message bodies, document bodies, comment content, todo descriptions, card bodies, schedule entry descriptions, upload descriptions, check-in answers and notes. Two rich-text fields are sent as written, so give them HTML: todolist descriptions and gauge needle descriptions. Chat is different again: `chat post` sends plain text unless you pass `--content-type text/html` or the line carries a mention. Use Markdown formatting (lists, bold, links, code blocks, tables) for rich content. @mentions resolve in message bodies, comment content, card bodies, schedule descriptions and chat lines — not in todo descriptions, documents, uploads, check-ins or notes. Four mention syntaxes are available (prefer deterministic for agents): - **`[@Name](mention:SGID)`** — zero API calls, embeds SGID directly (preferred for agents) - - **`[@Name](person:ID)`** — one API call, resolves person ID to SGID via pingable set + - **`[@Name](person:ID)`** — one API call, resolves person ID to SGID via pingable set (an agent's ID takes one more lookup and needs no project) - **`@sgid:VALUE`** — inline SGID embed for pipeline composability - - **`@Name` / `@First.Last`** — fuzzy name resolution (may be ambiguous) + - **`@Name` / `@First.Last`** — fuzzy name resolution (may be ambiguous). People match from the pingable set; agents are never pingable, so they match from the people on the command's project (from `--in` or the item URL) — with no project in scope, an agent's name is left as text Raw HTML is also accepted, but it is all-or-nothing per field: a tag the CLI detects as HTML (`

`, `