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..9804ca423 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, resolvedProjectID, *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..db0000a31 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, batchMentionScope(cmd, app, recordingArg), 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..2eb6f027d 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -632,19 +632,32 @@ 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) + // A project that cannot be resolved is the command's error, not a + // missed mention: never downgrade it to plain text. + var scopeErr *names.ScopeError + if errors.As(err, &scopeErr) { + return "", "", scopeErr.Err + } 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 +680,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 +692,128 @@ 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 +} + +// sameProjectID compares two project IDs as numbers, so "00123" and "123" +// name the same project. Non-numeric values compare as text. +func sameProjectID(a, b string) bool { + x, errA := strconv.ParseInt(a, 10, 64) + y, errB := strconv.ParseInt(b, 10, 64) + if errA != nil || errB != nil { + return a == b + } + return x == y +} + +// batchMentionScope returns the mention scope for a comments create target +// list. One resolved mention goes to every target, so the scope must be a +// project every target is known to be in; anything less leaves agents out of +// scope and a fuzzy agent mention stays text, with the unresolved notice. It +// never refuses the command: which targets are posted to is decided exactly +// as before, with a URL's bucket winning over --in. +// +// - Every target a URL in one bucket: that bucket. +// - Only bare IDs: --in, if given. +// - URLs in one bucket plus bare IDs: that bucket, if --in names it too +// (--in is what vouches for the bare IDs). +// - URLs in different buckets: none. +// +// Tokens that are neither a URL nor a number are skipped by the posting loop, +// so they are skipped here too. A named --in is resolved only if the scope is +// consulted. +func batchMentionScope(cmd *cobra.Command, app *appctx.App, targets string) names.ProjectScope { + urlProject, bare := "", false + for part := range strings.SplitSeq(targets, ",") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + recordingID, projectID := extractWithProject(part) + if _, err := strconv.ParseInt(recordingID, 10, 64); err != nil { + continue + } + switch { + case projectID == "": + bare = true + case urlProject != "" && !sameProjectID(urlProject, projectID): + return nil + default: + urlProject = projectID + } + } + + explicit := projectFlagValue(cmd) + if explicit == "" { + explicit = app.Flags.Project + } + switch { + case urlProject == "": + return mentionScope(app, explicit) + case !bare: + return mentionScope(app, urlProject) + case explicit == "": + return nil + } + + vouched := mentionScope(app, explicit) + if vouched == nil { + return nil + } + return func(ctx context.Context) (int64, error) { + id, err := vouched(ctx) + if err != nil { + return 0, err + } + if !sameProjectID(strconv.FormatInt(id, 10), urlProject) { + return 0, nil + } + return id, nil + } +} + // unresolvedMentionWarning formats a warning string for unresolved mentions. +// The hint names the one way a fuzzy @Name reaches an agent, 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 among the people on the command's project; name it with --in or a URL if the command has none; [@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..94da11629 --- /dev/null +++ b/internal/commands/mention_agents_test.go @@ -0,0 +1,240 @@ +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)) +} + +func TestCommentsCreateUnknownProjectInMentionScopeFails(t *testing.T) { + transport := &agentMentionTransport{} + app, _ := newTestAppWithTransport(t, transport) + + // A project that doesn't resolve is the command's error, not an + // unresolved mention to post as plain text. + err := executeChatCommand(NewCommentsCmd(), app, "create", "789", "Hey @Quincy", "--in", "No Such Project") + require.Error(t, err) + assert.Contains(t, err.Error(), "No Such Project") + assert.Empty(t, transport.posted) +} + +// batchScopeOf returns the project a comments create batch scopes agent +// mentions to, or 0 when it has none. +func batchScopeOf(t *testing.T, targets string, flags ...string) int64 { + t.Helper() + app, _ := newTestAppWithTransport(t, &agentMentionTransport{}) + cmd := NewCommentsCmd() + require.NoError(t, cmd.ParseFlags(flags)) + scope := batchMentionScope(cmd, app, targets) + if scope == nil { + return 0 + } + id, err := scope(t.Context()) + require.NoError(t, err) + return id +} + +func TestBatchMentionScope(t *testing.T) { + a := "https://3.basecamp.com/99999/buckets/123/todos/1" + b := "https://3.basecamp.com/99999/buckets/123/todos/2" + c := "https://3.basecamp.com/99999/buckets/456/todos/3" + + assert.Equal(t, int64(123), batchScopeOf(t, a)) + assert.Equal(t, int64(123), batchScopeOf(t, a+","+b)) + assert.Equal(t, int64(123), batchScopeOf(t, a, "--in", "456"), "a URL's bucket wins over --in, as for posting") + assert.Equal(t, int64(456), batchScopeOf(t, "1,2", "--in", "456"), "--in scopes a batch of bare IDs") + assert.Zero(t, batchScopeOf(t, "1,2")) + assert.Zero(t, batchScopeOf(t, a+",2"), "a bare ID's project is unknown") + assert.Equal(t, int64(123), batchScopeOf(t, a+",2", "--in", "123"), "--in naming the URL's bucket vouches for the bare ID") + assert.Equal(t, int64(123), batchScopeOf(t, a+",2", "--in", "00123"), "IDs compare as numbers") + assert.Zero(t, batchScopeOf(t, a+",2", "--in", "124"), "--in elsewhere cannot vouch for the bare ID") + assert.Zero(t, batchScopeOf(t, a+","+c), "URLs in different projects") + assert.Zero(t, batchScopeOf(t, a+","+c, "--in", "123"), "no single project holds every target") + assert.Equal(t, int64(123), batchScopeOf(t, a+",garbage"), "a token the posting loop skips does not unscope the batch") + assert.Equal(t, int64(123), batchScopeOf(t, a+",2", "--in", "Test Project"), "a named --in resolves when consulted") + assert.Zero(t, batchScopeOf(t, a+",2", "--in", " "), "a blank --in vouches for nothing") +} + +func TestCommentsCreateNamedInIsResolvedOnlyWhenAnAgentNeedsIt(t *testing.T) { + transport := &agentMentionTransport{} + app, _ := newTestAppWithTransport(t, transport) + + err := executeChatCommand(NewCommentsCmd(), app, "create", agentCardURL+",790", "Hey @Jane.Smith", "--in", "Test Project") + require.NoError(t, err) + assert.Contains(t, transport.postedContent(t), `sgid="sgid-jane"`) + assert.Zero(t, transport.getCount("/projects.json"), "an exact pingable match never resolves the project") +} + +func TestCommentsCreateConflictingInStillPosts(t *testing.T) { + c := "https://3.basecamp.com/99999/buckets/456/todos/3" + transport := &agentMentionTransport{} + app, buf := newTestAppWithTransport(t, transport) + + // The URL names where the comment goes, as it always has; --in cannot + // vouch for the bare ID, so the agent is left as text with the notice. + err := executeChatCommand(NewCommentsCmd(), app, "create", c+",790", "Hey @Quincy", "--in", "123") + require.NoError(t, err) + assert.NotContains(t, transport.postedContent(t), "sgid-quincy") + assert.Contains(t, noticeOf(t, buf), "@Quincy") +} + +func TestCommentsCreateAcceptsURLProjectMatchingIn(t *testing.T) { + transport := &agentMentionTransport{} + app, _ := newTestAppWithTransport(t, transport) + + err := executeChatCommand(NewCommentsCmd(), app, "create", agentCardURL+",790", "Hey @Quincy", "--in", "123") + require.NoError(t, err) + assert.Contains(t, transport.postedContent(t), `sgid="sgid-quincy"`) +} 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..7e8a342e8 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, urlProjectID, 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..f63429400 --- /dev/null +++ b/internal/names/mention.go @@ -0,0 +1,233 @@ +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, or one that yields 0, means no project is in scope. +type ProjectScope func(context.Context) (int64, error) + +// ScopeError reports that a mention's project scope could not be resolved. +// It is distinct from a person lookup miss, which callers may downgrade. +type ScopeError struct{ Err error } + +func (e *ScopeError) Error() string { return e.Err.Error() } +func (e *ScopeError) Unwrap() error { return e.Err } + +// 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 { + // A scope that cannot be resolved is always an error. A failed fetch + // of its people fails only when agents were the sole remaining + // answer: a mention the pingable set already matches loses just the + // exact-agent preference. + var scopeErr *ScopeError + if errors.As(err, &scopeErr) || (match == nil && len(matches) == 0) { + return nil, err + } + agents = nil + } + + // Agents with the same name, ignoring case, are ambiguous even when one + // matches exactly: resolve would pick the first, and the other is as + // much the agent meant. + var namesakes []Person + for _, a := range agents { + if strings.EqualFold(a.Name, input) { + namesakes = append(namesakes, a) + } + } + if len(namesakes) > 1 { + return nil, ambiguousPeople(namesakes) + } + + agent, agentMatches := resolve(input, agents, personIDName) + switch { + case len(namesakes) == 1: + return &namesakes[0], nil + 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, &ScopeError{Err: err} + } + if projectID == 0 { + return nil, nil + } + 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 + + r.mu.RLock() + cached, ok := r.agentsByID[id] + r.mu.RUnlock() + if ok { + return cached, nil + } + + 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 + } + agent := &Person{ + ID: p.ID, + AttachableSGID: p.AttachableSGID, + Name: p.Name, + Email: p.EmailAddress, + PersonableType: p.PersonableType, + } + + r.mu.Lock() + if r.agentsByID == nil { + r.agentsByID = make(map[int64]*Person) + } + r.agentsByID[id] = agent + r.mu.Unlock() + return agent, 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] + failed := r.agentErrs[projectID] + r.mu.RUnlock() + if ok || failed != nil { + return agents, failed + } + + r.mu.Lock() + defer r.mu.Unlock() + if agents, ok := r.agents[projectID]; ok { + return agents, nil + } + if failed := r.agentErrs[projectID]; failed != nil { + return nil, failed + } + + result, err := r.forAccount().People().ListProjectPeople(ctx, projectID, nil) + if err != nil { + // Remember the failure for the run: a mention the pingable set + // already matches tolerates it, and each such mention must not retry. + if r.agentErrs == nil { + r.agentErrs = make(map[int64]error) + } + r.agentErrs[projectID] = convertSDKError(err) + return nil, r.agentErrs[projectID] + } + + 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..61432aa44 --- /dev/null +++ b/internal/names/mention_test.go @@ -0,0 +1,308 @@ +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) +} + +func TestResolveMentionByNameRemembersAFailedAgentFetch(t *testing.T) { + r, s := newMentionFixture(t) + + for range 3 { + _, err := r.ResolveMentionByName(context.Background(), "Jane", inProject(404)) + require.NoError(t, err) + } + assert.Equal(t, 1, s.count("/99999/projects/404/people.json"), "a failed fetch is not retried within a run") +} + +func TestResolveMentionByNameReportsScopeErrorsAsSuch(t *testing.T) { + r, _ := newMentionFixture(t) + scopeErr := output.ErrNotFound("Project", "Nope") + scope := func(context.Context) (int64, error) { return 0, scopeErr } + + _, err := r.ResolveMentionByName(context.Background(), "Quincy", scope) + var se *ScopeError + require.True(t, errors.As(err, &se), "got %v", err) + assert.Same(t, scopeErr, se.Err) +} + +func TestResolveMentionByNameScopeErrorBeatsAPartialPersonMatch(t *testing.T) { + r, _ := newMentionFixture(t) + scope := func(context.Context) (int64, error) { return 0, output.ErrNotFound("Project", "Nope") } + + _, err := r.ResolveMentionByName(context.Background(), "Jane", scope) + var se *ScopeError + require.True(t, errors.As(err, &se), "got %v", err) +} + +func TestResolveMentionByNameSameNamedAgentsAreAmbiguous(t *testing.T) { + for _, second := range []string{"Quincy", "quincy"} { + t.Run(second, func(t *testing.T) { + r, s := newMentionFixture(t) + s.project["123"] = append(s.project["123"], person(8, second, "sgid-quincy-2", "Agent")) + + _, err := r.ResolveMentionByName(context.Background(), "Quincy", inProject(123)) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "got %v", err) + assert.Equal(t, output.CodeAmbiguous, outErr.Code) + }) + } +} + +func TestResolveMentionByIDLooksUpAnAgentOncePerRun(t *testing.T) { + r, s := newMentionFixture(t) + + for range 3 { + p, err := r.ResolveMentionByID(context.Background(), 7) + require.NoError(t, err) + assert.Equal(t, "sgid-quincy", p.AttachableSGID) + } + assert.Equal(t, 1, s.count("/99999/people/7")+s.count("/99999/people/7.json")) +} diff --git a/internal/names/resolver.go b/internal/names/resolver.go index f58bc9017..0130c4e6f 100644 --- a/internal/names/resolver.go +++ b/internal/names/resolver.go @@ -32,12 +32,15 @@ type Resolver struct { resolveMeFn func(context.Context) (int64, string, error) // Session-scoped cache - mu sync.RWMutex - projects []Project - people []Person - pingable []Person // cached /people/pingable.json - todolists map[string][]Todolist // keyed by project ID - me *Person // cached /my/profile.json result + mu sync.RWMutex + projects []Project + people []Person + pingable []Person // cached /people/pingable.json + agents map[int64][]Person // agents on a project, keyed by project ID + agentErrs map[int64]error // failed agent fetches, so a run does not retry them + agentsByID map[int64]*Person // agents looked up directly by person ID + todolists map[string][]Todolist // keyed by project ID + me *Person // cached /my/profile.json result } // Project represents a Basecamp project for name resolution. @@ -69,6 +72,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 +87,9 @@ func (r *Resolver) SetAccountID(accountID string) { r.projects = nil r.people = nil r.pingable = nil + r.agents = make(map[int64][]Person) + r.agentErrs = nil + r.agentsByID = nil r.me = nil r.todolists = make(map[string][]Todolist) } @@ -367,6 +374,9 @@ func (r *Resolver) ClearCache() { r.projects = nil r.people = nil r.pingable = nil + r.agents = make(map[int64][]Person) + r.agentErrs = nil + r.agentsByID = nil r.me = nil r.todolists = make(map[string][]Todolist) } diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 337fbe1af..d680dcc43 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 project the command works in: the project it already resolved (for example on `messages create`, `cards create`, or `chat post` without `--room`, including your configured default), else the item URL's project, else `--in`. 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 (`

`, `