Add deterministic mention syntax and harden fuzzy resolution

Introduce three deterministic mention paths that bypass fuzzy name
matching, giving agents and pipelines unambiguous control:

- [@name](mention:SGID) — zero API calls, caller-trusted SGID
- [@name](person:ID) — one API call, resolves against pingable set
- @sgid:VALUE — inline SGID embed for composability

ResolveMentions is restructured into three sequential passes (anchor →
@sgid → fuzzy @name), each producing <bc-attachment> tags that
subsequent passes skip.

Also hardens the existing fuzzy @name path with review fixes from #288:
- ResolvePersonByName resolves against pingable set only
- Expanded prefix regex to match after (, [, ", '
- Trailing-character bailout for hyphens and word-internal apostrophes
- Case-insensitive isInsideBcAttachment guard
- Chat auto-promotes text/plain → text/html when mentions resolve
- Fix double-escaping of HTML entities in mention: display text

Author: jeremy

internal/commands/chat_test.go

@@ -671,6 +671,108 @@ func TestChatPostViaSubcommandWithChatFlag(t *testing.T) {
 		"content should be passed through subcommand path")
 }
 
+// mockChatMentionTransport handles resolver API calls for mentions and captures POST body.
+type mockChatMentionTransport struct {
+	capturedBody []byte
+}
+
+func (t *mockChatMentionTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	header := make(http.Header)
+	header.Set("Content-Type", "application/json")
+
+	if req.Method == "GET" {
+		var body string
+		switch {
+		case strings.Contains(req.URL.Path, "/projects.json"):
+			body = `[{"id": 123, "name": "Test Project"}]`
+		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"):
+			body = `[{"id": 42000, "name": "Jane Smith", "email_address": "jane@example.com", "attachable_sgid": "sgid-jane"}]`
+		default:
+			body = `{}`
+		}
+		return &http.Response{
+			StatusCode: 200,
+			Body:       io.NopCloser(strings.NewReader(body)),
+			Header:     header,
+		}, nil
+	}
+
+	if req.Method == "POST" {
+		if req.Body != nil {
+			body, _ := io.ReadAll(req.Body)
+			t.capturedBody = body
+			req.Body.Close()
+		}
+		mockResp := `{"id": 999, "content": "Test", "created_at": "2024-01-01T00:00:00Z"}`
+		return &http.Response{
+			StatusCode: 201,
+			Body:       io.NopCloser(strings.NewReader(mockResp)),
+			Header:     header,
+		}, nil
+	}
+
+	return nil, errors.New("unexpected request")
+}
+
+// TestChatPostMentionPromotesToHTML verifies that a chat post with @Name
+// auto-promotes content type to text/html when mentions are resolved.
+func TestChatPostMentionPromotesToHTML(t *testing.T) {
+	t.Setenv("BASECAMP_NO_KEYRING", "1")
+
+	transport := &mockChatMentionTransport{}
+	app, _ := newTestAppWithTransport(t, transport)
+
+	cmd := NewChatCmd()
+	err := executeChatCommand(cmd, app, "post", "Hey @Jane.Smith, check this")
+	require.NoError(t, err)
+	require.NotEmpty(t, transport.capturedBody)
+
+	var requestBody map[string]any
+	err = json.Unmarshal(transport.capturedBody, &requestBody)
+	require.NoError(t, err)
+
+	// Content type should be promoted to text/html when mentions are present
+	assert.Equal(t, "text/html", requestBody["content_type"],
+		"content_type should be promoted to text/html when mentions are resolved")
+
+	content, ok := requestBody["content"].(string)
+	require.True(t, ok)
+	assert.Contains(t, content, "bc-attachment",
+		"content should contain bc-attachment mention tag")
+}
+
+// TestChatPostPlainTextOptOut verifies that --content-type text/plain
+// bypasses mention resolution and sends content as-is.
+func TestChatPostPlainTextOptOut(t *testing.T) {
+	t.Setenv("BASECAMP_NO_KEYRING", "1")
+
+	transport := &mockChatMentionTransport{}
+	app, _ := newTestAppWithTransport(t, transport)
+
+	cmd := NewChatCmd()
+	err := executeChatCommand(cmd, app, "post", "Hey @Jane.Smith", "--content-type", "text/plain")
+	require.NoError(t, err)
+	require.NotEmpty(t, transport.capturedBody)
+
+	var requestBody map[string]any
+	err = json.Unmarshal(transport.capturedBody, &requestBody)
+	require.NoError(t, err)
+
+	// Content type should remain text/plain
+	assert.Equal(t, "text/plain", requestBody["content_type"],
+		"content_type should remain text/plain when explicitly set")
+
+	content, ok := requestBody["content"].(string)
+	require.True(t, ok)
+	// Mentions should NOT be resolved — raw text preserved
+	assert.NotContains(t, content, "bc-attachment",
+		"content should not contain bc-attachment when content-type is text/plain")
+	assert.Contains(t, content, "@Jane.Smith",
+		"@mention should be left as literal text")
+}
+
 // TestChatDeleteReturnsDeletedPayload verifies that delete returns {"deleted": true, "id": "..."}.
 func TestChatDeleteReturnsDeletedPayload(t *testing.T) {
 	t.Setenv("BASECAMP_NO_KEYRING", "1")

internal/commands/helpers.go

@@ -470,18 +470,39 @@ func applySubscribeFlags(ctx context.Context, resolver *names.Resolver, subscrib
 	return nil, nil
 }
 
-// resolveMentions scans HTML for @Name and @First.Last mentions and replaces
-// them with Basecamp mention attachment tags. Silently returns unchanged HTML
-// if no mentions are found. Errors if a mentioned name cannot be resolved.
+// 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
+//
+// Also supports @sgid:VALUE inline syntax for pipeline composability.
+// Silently returns unchanged HTML if no mentions are found.
 func resolveMentions(ctx context.Context, resolver *names.Resolver, html string) (string, error) {
-	return richtext.ResolveMentions(html, func(name string) (string, string, error) {
-		person, err := resolver.ResolvePersonByName(ctx, name)
-		if err != nil {
-			return "", "", err
-		}
-		if person.AttachableSGID == "" {
-			return "", "", fmt.Errorf("person %q has no attachable SGID", person.Name)
-		}
-		return person.AttachableSGID, person.Name, nil
-	})
+	return richtext.ResolveMentions(html,
+		func(name string) (string, string, error) {
+			person, err := resolver.ResolvePersonByName(ctx, name)
+			if err != nil {
+				return "", "", err
+			}
+			if person.AttachableSGID == "" {
+				return "", "", fmt.Errorf("person %q has no attachable SGID", person.Name)
+			}
+			return person.AttachableSGID, person.Name, nil
+		},
+		func(id string) (string, string, error) {
+			personID, err := strconv.ParseInt(id, 10, 64)
+			if err != nil {
+				return "", "", output.ErrUsage(fmt.Sprintf("invalid person ID %q — must be numeric", id))
+			}
+			person, err := resolver.ResolvePersonByID(ctx, personID)
+			if err != nil {
+				return "", "", err
+			}
+			if person.AttachableSGID == "" {
+				return "", "", fmt.Errorf("person %q has no attachable SGID", person.Name)
+			}
+			return person.AttachableSGID, person.Name, nil
+		},
+	)
 }

internal/names/resolver.go

@@ -248,16 +248,14 @@ func (r *Resolver) ResolvePerson(ctx context.Context, input string) (string, str
 }
 
 // ResolvePersonByName resolves a person name to a full Person record including AttachableSGID.
-// Uses the same resolution logic as ResolvePerson (exact > case-insensitive > partial > pingable).
+// Resolves against the pingable set only — mentions require the target to be pingable.
 func (r *Resolver) ResolvePersonByName(ctx context.Context, input string) (*Person, error) {
-	// Fetch people for name resolution
-	people, err := r.getPeople(ctx)
+	pingable, err := r.getPingable(ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	// Try name resolution
-	match, matches := resolve(input, people, func(p Person) (int64, string) {
+	match, matches := resolve(input, pingable, func(p Person) (int64, string) {
 		return p.ID, p.Name
 	})
 
@@ -273,33 +271,31 @@ func (r *Resolver) ResolvePersonByName(ctx context.Context, input string) (*Pers
 		return nil, output.ErrAmbiguous("person", names)
 	}
 
-	// Fallback: try pingable people
-	pingable, _ := r.getPingable(ctx)
-	if len(pingable) > 0 {
-		pingMatch, pingMatches := resolve(input, pingable, func(p Person) (int64, string) {
-			return p.ID, p.Name
-		})
-		if pingMatch != nil {
-			return pingMatch, nil
-		}
-		if len(pingMatches) > 1 {
-			pingNames := make([]string, len(pingMatches))
-			for i, m := range pingMatches {
-				pingNames[i] = m.Name
-			}
-			return nil, output.ErrAmbiguous("person", pingNames)
-		}
-	}
-
-	// Not found
-	allPeople := deduplicatePeople(people, pingable)
-	suggestions := suggest(input, allPeople, func(p Person) string { return p.Name })
+	suggestions := suggest(input, pingable, 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)
 }
 
+// ResolvePersonByID looks up a person by numeric ID in the pingable set.
+// Returns the Person record including AttachableSGID.
+// Returns not-found if the person is not in the pingable set.
+func (r *Resolver) ResolvePersonByID(ctx context.Context, id int64) (*Person, error) {
+	pingable, err := r.getPingable(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	for i := range pingable {
+		if pingable[i].ID == id {
+			return &pingable[i], nil
+		}
+	}
+
+	return nil, output.ErrNotFound("Person", strconv.FormatInt(id, 10))
+}
+
 // deduplicatePeople merges two person lists, removing duplicates by ID.
 func deduplicatePeople(people, pingable []Person) []Person {
 	seen := make(map[int64]struct{}, len(people))

internal/names/resolver_test.go

@@ -1011,12 +1011,12 @@ func TestGetTodolistsPartialFetchFailsHard(t *testing.T) {
 
 func TestResolvePersonByName(t *testing.T) {
 	r := newMockResolver()
-	r.setPeople([]Person{
+	// ResolvePersonByName resolves against pingable set only
+	r.setPingable([]Person{
 		{ID: 1, AttachableSGID: "sgid-john", Name: "John Doe", Email: "john@example.com"},
 		{ID: 2, AttachableSGID: "sgid-jane", Name: "Jane Smith", Email: "jane@example.com"},
 		{ID: 3, AttachableSGID: "sgid-igor", Name: "Igor Logachev", Email: "igor@example.com"},
 	})
-	r.setPingable([]Person{}) // empty pingable cache to avoid SDK call
 
 	ctx := context.Background()
 
@@ -1048,8 +1048,9 @@ func TestResolvePersonByName(t *testing.T) {
 	}
 }
 
-func TestResolvePersonByNamePingableFallback(t *testing.T) {
+func TestResolvePersonByNamePingableOnly(t *testing.T) {
 	r := newMockResolver()
+	// Person exists in people list but NOT in pingable — should not be found
 	r.setPeople([]Person{
 		{ID: 1, AttachableSGID: "sgid-john", Name: "John Doe"},
 	})
@@ -1059,9 +1060,43 @@ func TestResolvePersonByNamePingableFallback(t *testing.T) {
 
 	ctx := context.Background()
 
-	// Should find in pingable
-	person, err := r.ResolvePersonByName(ctx, "External")
+	// Person in people but not pingable → not found
+	_, err := r.ResolvePersonByName(ctx, "John Doe")
+	require.Error(t, err)
+
+	var outErr *output.Error
+	require.True(t, errors.As(err, &outErr))
+	assert.Equal(t, output.CodeNotFound, outErr.Code)
+
+	// Person in pingable → found
+	person, err := r.ResolvePersonByName(ctx, "External Client")
 	require.NoError(t, err)
 	assert.Equal(t, "External Client", person.Name)
 	assert.Equal(t, "sgid-client", person.AttachableSGID)
 }
+
+func TestResolvePersonByID(t *testing.T) {
+	r := newMockResolver()
+	r.setPingable([]Person{
+		{ID: 42000, AttachableSGID: "sgid-jane", Name: "Jane Smith"},
+		{ID: 42001, AttachableSGID: "sgid-bob", Name: "Bob Jones"},
+	})
+
+	ctx := context.Background()
+
+	t.Run("found", func(t *testing.T) {
+		person, err := r.ResolvePersonByID(ctx, 42000)
+		require.NoError(t, err)
+		assert.Equal(t, "Jane Smith", person.Name)
+		assert.Equal(t, "sgid-jane", person.AttachableSGID)
+	})
+
+	t.Run("not found", func(t *testing.T) {
+		_, err := r.ResolvePersonByID(ctx, 99999)
+		require.Error(t, err)
+
+		var outErr *output.Error
+		require.True(t, errors.As(err, &outErr))
+		assert.Equal(t, output.CodeNotFound, outErr.Code)
+	})
+}