diff --git a/internal/commands/cards.go b/internal/commands/cards.go index bf11d15c..f727e466 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -437,16 +437,19 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { } // Convert content through rich text pipeline + var mentionNotice string if content != "" { content = richtext.MarkdownToHTML(content) content, err = resolveLocalImages(cmd, app, content) if err != nil { return err } - content, err = resolveMentions(cmd.Context(), app.Names, content) - if err != nil { - return err + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, content) + if mentionErr != nil { + return mentionErr } + content = mentionResult.HTML + mentionNotice = unresolvedMentionWarning(mentionResult.Unresolved) } // Build request @@ -504,10 +507,14 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { Description: "List cards", }) - return app.OK(card, + respOpts := []output.ResponseOption{ output.WithSummary(fmt.Sprintf("Created card #%d", card.ID)), output.WithBreadcrumbs(breadcrumbs...), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(card, respOpts...) }, } @@ -560,17 +567,19 @@ You can pass either a card ID or a Basecamp URL: if title != "" { req.Title = title } + var mentionNotice string if content != "" { html := richtext.MarkdownToHTML(content) html, err = resolveLocalImages(cmd, app, html) if err != nil { return err } - html, err = resolveMentions(cmd.Context(), app.Names, html) - if err != nil { - return err + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, html) + if mentionErr != nil { + return mentionErr } - req.Content = html + req.Content = mentionResult.HTML + mentionNotice = unresolvedMentionWarning(mentionResult.Unresolved) } if due != "" { req.DueOn = dateparse.Parse(due) @@ -588,7 +597,7 @@ You can pass either a card ID or a Basecamp URL: return convertSDKError(err) } - return app.OK(card, + respOpts := []output.ResponseOption{ output.WithSummary(fmt.Sprintf("Updated card #%s", cardIDStr)), output.WithBreadcrumbs( output.Breadcrumb{ @@ -597,7 +606,11 @@ You can pass either a card ID or a Basecamp URL: Description: "View card", }, ), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(card, respOpts...) }, } @@ -974,16 +987,19 @@ func NewCardCmd() *cobra.Command { } // Convert content through rich text pipeline + var mentionNotice string if content != "" { content = richtext.MarkdownToHTML(content) content, err = resolveLocalImages(cmd, app, content) if err != nil { return err } - content, err = resolveMentions(cmd.Context(), app.Names, content) - if err != nil { - return err + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, content) + if mentionErr != nil { + return mentionErr } + content = mentionResult.HTML + mentionNotice = unresolvedMentionWarning(mentionResult.Unresolved) } // Build request @@ -1040,10 +1056,14 @@ func NewCardCmd() *cobra.Command { Description: "List cards", }) - return app.OK(card, + respOpts := []output.ResponseOption{ output.WithSummary(fmt.Sprintf("Created card #%d", card.ID)), output.WithBreadcrumbs(cardBreadcrumbs...), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(card, respOpts...) }, } diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 7b8f063a..1a5fe921 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -363,22 +363,25 @@ func runChatPost(cmd *cobra.Command, app *appctx.App, chatID, project, content, } // Resolve @mentions — skip if user explicitly set a non-HTML content type. - // HTML-escape plain text first so characters like < > & are safe. + // When contentType is unset, convert Markdown to HTML first so the mention + // resolver operates on HTML input. + var mentionNotice string if contentType == "" || contentType == "text/html" { mentionInput := content if contentType == "" { mentionInput = richtext.MarkdownToHTML(content) } - resolved, resolveErr := resolveMentions(cmd.Context(), app.Names, mentionInput) + result, resolveErr := resolveMentions(cmd.Context(), app.Names, mentionInput) if resolveErr != nil { return resolveErr } - if resolved != mentionInput { - content = resolved + if result.HTML != mentionInput || len(result.Unresolved) > 0 { + content = result.HTML if contentType == "" { contentType = "text/html" } } + mentionNotice = unresolvedMentionWarning(result.Unresolved) } // Post message using SDK @@ -423,11 +426,15 @@ func runChatPost(cmd *cobra.Command, app *appctx.App, chatID, project, content, ) } - return app.OK(line, + respOpts := []output.ResponseOption{ output.WithSummary(summary), output.WithEntity("chat_line"), output.WithBreadcrumbs(breadcrumbs...), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(line, respOpts...) } func newChatUploadCmd(project, chatID *string) *cobra.Command { diff --git a/internal/commands/chat_test.go b/internal/commands/chat_test.go index b05f3a02..af658975 100644 --- a/internal/commands/chat_test.go +++ b/internal/commands/chat_test.go @@ -774,6 +774,155 @@ func TestChatPostPlainTextOptOut(t *testing.T) { "@mention should be left as literal text") } +// mockChatMultiMentionTransport has multiple people but not all — simulating a +// message with valid and invalid @mentions. +type mockChatMultiMentionTransport struct { + capturedBody []byte +} + +func (t *mockChatMultiMentionTransport) 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"}, + {"id": 42001, "name": "John Doe", "email_address": "john@example.com", "attachable_sgid": "sgid-john"} + ]` + 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") +} + +// TestChatPostMixedValidInvalidMentions verifies that a message with both +// valid and invalid @mentions is posted with valid ones resolved and invalid +// ones left as plain text, plus a notice in the response. +func TestChatPostMixedValidInvalidMentions(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockChatMultiMentionTransport{} + app, buf := newTestAppWithTransport(t, transport) + + cmd := NewChatCmd() + err := executeChatCommand(cmd, app, "post", "Hey @Jane.Smith and @Bobby, check this", + "--room", "789", "--in", "123") + require.NoError(t, err, "post with mixed mentions should succeed") + require.NotEmpty(t, transport.capturedBody) + + // The message should have been posted + var requestBody map[string]any + err = json.Unmarshal(transport.capturedBody, &requestBody) + require.NoError(t, err) + + content, ok := requestBody["content"].(string) + require.True(t, ok) + + // Valid mention should be resolved + assert.Contains(t, content, "bc-attachment", + "valid mention should be resolved to bc-attachment") + assert.Contains(t, content, "sgid-jane", + "Jane's SGID should be in the content") + + // Invalid mention should be left as plain text + assert.Contains(t, content, "@Bobby", + "unresolved mention should remain as plain text") + + // Response should include a notice about unresolved mentions + var envelope map[string]any + err = json.Unmarshal(buf.Bytes(), &envelope) + require.NoError(t, err) + notice, _ := envelope["notice"].(string) + assert.Contains(t, notice, "@Bobby", + "notice should mention the unresolved @Bobby") +} + +// TestChatPostAllInvalidMentionsStillPosts verifies that when all @mentions +// are invalid, the message is still posted as HTML with mentions as plain text. +// This catches a regression where all-unresolved mentions would silently fall +// back to plain-text delivery, dropping any markdown-to-HTML conversion. +func TestChatPostAllInvalidMentionsStillPosts(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockChatMultiMentionTransport{} + app, _ := newTestAppWithTransport(t, transport) + + cmd := NewChatCmd() + err := executeChatCommand(cmd, app, "post", "Hey @Nobody and @Ghost", + "--room", "789", "--in", "123") + require.NoError(t, err, "post with all invalid mentions should succeed") + require.NotEmpty(t, transport.capturedBody, "message should still be posted") + + var requestBody map[string]any + err = json.Unmarshal(transport.capturedBody, &requestBody) + require.NoError(t, err) + + // Content type must be promoted to text/html even when all mentions are + // unresolved, because the markdown-to-HTML conversion already happened. + assert.Equal(t, "text/html", requestBody["content_type"], + "content_type should be text/html even with all unresolved mentions") +} + +// TestChatPostAgentModeWarningOnStderr verifies that in quiet/agent mode, +// the unresolved mention warning appears on stderr while stdout contains +// data-only JSON (no envelope). +func TestChatPostAgentModeWarningOnStderr(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockChatMultiMentionTransport{} + app, _ := newTestAppWithTransport(t, transport) + + // Override output to FormatQuiet with separate stdout/stderr buffers + var stdout, stderr bytes.Buffer + app.Output = output.New(output.Options{ + Format: output.FormatQuiet, + Writer: &stdout, + ErrWriter: &stderr, + }) + + cmd := NewChatCmd() + err := executeChatCommand(cmd, app, "post", "Hey @Jane.Smith and @Bobby, check this", + "--room", "789", "--in", "123") + require.NoError(t, err) + + // stdout should contain data-only JSON (no envelope wrapper) + var data map[string]any + require.NoError(t, json.Unmarshal(stdout.Bytes(), &data)) + _, hasOK := data["ok"] + assert.False(t, hasOK, "quiet mode should not include envelope ok field") + + // stderr should contain the diagnostic warning + assert.Contains(t, stderr.String(), "notice: Unresolved mentions left as text: @Bobby") +} + // TestChatDeleteReturnsDeletedPayload verifies that delete returns {"deleted": true, "id": "..."}. func TestChatDeleteReturnsDeletedPayload(t *testing.T) { t.Setenv("BASECAMP_NO_KEYRING", "1") diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 73aa1ecf..7b7a33b2 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -233,10 +233,11 @@ You can pass either a comment ID or a Basecamp URL: } // Resolve @mentions - html, err = resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) if err != nil { return err } + html = mentionResult.HTML req := &basecamp.UpdateCommentRequest{ Content: html, @@ -247,7 +248,7 @@ You can pass either a comment ID or a Basecamp URL: return convertSDKError(err) } - return app.OK(comment, + respOpts := []output.ResponseOption{ output.WithEntity("comment"), output.WithSummary(fmt.Sprintf("Updated comment #%s", commentIDStr)), output.WithBreadcrumbs( @@ -257,7 +258,11 @@ You can pass either a comment ID or a Basecamp URL: Description: "View comment", }, ), - ) + } + if notice := unresolvedMentionWarning(mentionResult.Unresolved); notice != "" { + respOpts = append(respOpts, output.WithDiagnostic(notice)) + } + return app.OK(comment, respOpts...) }, } @@ -352,10 +357,12 @@ Content supports Markdown and @mentions (@Name or @First.Last): } // Resolve @mentions (e.g., @John, @John.Doe → clickable mention tags) - html, err = resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) if err != nil { return err } + html = mentionResult.HTML + mentionNotice := unresolvedMentionWarning(mentionResult.Unresolved) // Upload explicit --attach files and embed if len(attachFiles) > 0 { @@ -421,7 +428,7 @@ Content supports Markdown and @mentions (@Name or @First.Last): // Single comment: return the comment object directly if len(commented) == 1 && len(failed) == 0 && lastComment != nil { - return app.OK(lastComment, + respOpts := []output.ResponseOption{ output.WithEntity("comment"), output.WithSummary(fmt.Sprintf("Commented on #%s", commented[0])), output.WithBreadcrumbs( @@ -436,7 +443,11 @@ Content supports Markdown and @mentions (@Name or @First.Last): Description: "Update comment", }, ), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(lastComment, respOpts...) } // Batch: build result map @@ -453,9 +464,13 @@ Content supports Markdown and @mentions (@Name or @First.Last): summary = fmt.Sprintf("Added %d comment(s) to: %s", len(commented), strings.Join(commented, ", ")) } - return app.OK(result, + batchOpts := []output.ResponseOption{ output.WithSummary(summary), - ) + } + if mentionNotice != "" { + batchOpts = append(batchOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(result, batchOpts...) }, } diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 420e625a..37b5ebc5 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -3,6 +3,7 @@ package commands import ( "context" "encoding/json" + "errors" "fmt" "os" "strconv" @@ -478,11 +479,24 @@ func applySubscribeFlags(ctx context.Context, resolver *names.Resolver, subscrib // // 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) { +// +// 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) { return richtext.ResolveMentions(html, func(name string) (string, string, error) { person, err := resolver.ResolvePersonByName(ctx, name) 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 + // failures (e.g. 404 from the pingable endpoint) carry HTTPStatus + // and must still hard-fail. + var cliErr *output.Error + if errors.As(err, &cliErr) && cliErr.HTTPStatus == 0 && + (cliErr.Code == output.CodeNotFound || cliErr.Code == output.CodeAmbiguous) { + return "", "", fmt.Errorf("%w: %w", richtext.ErrMentionSkip, err) + } return "", "", err } if person.AttachableSGID == "" { @@ -507,6 +521,14 @@ func resolveMentions(ctx context.Context, resolver *names.Resolver, html string) ) } +// unresolvedMentionWarning formats a warning string for unresolved mentions. +func unresolvedMentionWarning(unresolved []string) string { + if len(unresolved) == 0 { + return "" + } + return "Unresolved mentions left as text: " + strings.Join(unresolved, ", ") +} + // projectFlagChanged reports whether the user explicitly passed --project or // its --in alias on the command line. func projectFlagChanged(cmd *cobra.Command) bool { diff --git a/internal/commands/messages.go b/internal/commands/messages.go index f84b8236..192db1b1 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -323,10 +323,12 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command } // Resolve @mentions - html, err = resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) if err != nil { return err } + html = mentionResult.HTML + mentionNotice := unresolvedMentionWarning(mentionResult.Unresolved) // Upload explicit --attach files and embed if len(attachFiles) > 0 { @@ -355,7 +357,7 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command return convertSDKError(err) } - return app.OK(message, + respOpts := []output.ResponseOption{ output.WithSummary(fmt.Sprintf("Posted message #%d", message.ID)), output.WithEntity("message"), output.WithBreadcrumbs( @@ -370,7 +372,11 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command Description: "List messages", }, ), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(message, respOpts...) }, } @@ -426,10 +432,11 @@ You can pass either a message ID or a Basecamp URL: } // Resolve @mentions - html, err = resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) if err != nil { return err } + html = mentionResult.HTML req := &basecamp.UpdateMessageRequest{ Subject: title, @@ -441,7 +448,7 @@ You can pass either a message ID or a Basecamp URL: return convertSDKError(err) } - return app.OK(message, + respOpts := []output.ResponseOption{ output.WithSummary(fmt.Sprintf("Updated message #%s", messageIDStr)), output.WithEntity("message"), output.WithBreadcrumbs( @@ -451,7 +458,11 @@ You can pass either a message ID or a Basecamp URL: Description: "View message", }, ), - ) + } + if notice := unresolvedMentionWarning(mentionResult.Unresolved); notice != "" { + respOpts = append(respOpts, output.WithDiagnostic(notice)) + } + return app.OK(message, respOpts...) }, } @@ -722,10 +733,12 @@ Content supports Markdown and @mentions (@Name or @First.Last): } // Resolve @mentions - html, err = resolveMentions(cmd.Context(), app.Names, html) + mentionResult, err := resolveMentions(cmd.Context(), app.Names, html) if err != nil { return err } + html = mentionResult.HTML + mentionNotice := unresolvedMentionWarning(mentionResult.Unresolved) // Upload explicit --attach files and embed if len(attachFiles) > 0 { @@ -752,7 +765,7 @@ Content supports Markdown and @mentions (@Name or @First.Last): return convertSDKError(err) } - return app.OK(message, + respOpts := []output.ResponseOption{ output.WithSummary(fmt.Sprintf("Posted message #%d", message.ID)), output.WithEntity("message"), output.WithBreadcrumbs( @@ -767,7 +780,11 @@ Content supports Markdown and @mentions (@Name or @First.Last): Description: "List messages", }, ), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(message, respOpts...) }, } diff --git a/internal/commands/messages_test.go b/internal/commands/messages_test.go index f587568d..6dc344b9 100644 --- a/internal/commands/messages_test.go +++ b/internal/commands/messages_test.go @@ -533,3 +533,63 @@ func TestMessagesCreateDefaultOmitsSubscriptions(t *testing.T) { _, ok := body["subscriptions"] assert.False(t, ok, "expected subscriptions to be omitted when neither flag is set") } + +// mockMessageListTransport handles the resolution chain and returns a truncated +// messages list (fewer messages than TotalCount) to exercise the truncation notice path. +type mockMessageListTransport struct{} + +func (mockMessageListTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + if req.Method != "GET" { + return nil, errors.New("unexpected method: " + req.Method) + } + + 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": "message_board", "id": 777, "enabled": true}]}` + case strings.Contains(req.URL.Path, "/messages.json"): + // Return 2 messages but signal 50 total via header + header.Set("X-Total-Count", "50") + body = `[{"id": 1, "subject": "Msg 1"}, {"id": 2, "subject": "Msg 2"}]` + default: + body = `{}` + } + + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(body)), + Header: header, + }, nil +} + +// TestMessagesListAgentModeTruncationSilent verifies that truncation notices +// do not leak to stderr in quiet/agent mode. Only diagnostic warnings +// (e.g. unresolved mentions) should appear on stderr. +func TestMessagesListAgentModeTruncationSilent(t *testing.T) { + transport := mockMessageListTransport{} + app, _ := setupMessagesMockApp(t, transport) + + // Override output to FormatQuiet with separate stdout/stderr buffers + var stdout, stderr bytes.Buffer + app.Output = output.New(output.Options{ + Format: output.FormatQuiet, + Writer: &stdout, + ErrWriter: &stderr, + }) + + cmd := NewMessagesCmd() + err := executeMessagesCommand(cmd, app, "list", "--in", "123") + require.NoError(t, err) + + // stdout should contain data + assert.NotEmpty(t, stdout.String()) + + // stderr must be empty — truncation notice should NOT leak + assert.Empty(t, stderr.String(), + "truncation notices should not appear on stderr in quiet mode") +} diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index e128ad44..4806ce59 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -465,16 +465,19 @@ func runScheduleCreate(cmd *cobra.Command, app *appctx.App, project, scheduleID, scheduleIDInt, _ := strconv.ParseInt(scheduleID, 10, 64) // Convert description through rich text pipeline + var mentionNotice string if description != "" { description = richtext.MarkdownToHTML(description) description, err = resolveLocalImages(cmd, app, description) if err != nil { return err } - description, err = resolveMentions(cmd.Context(), app.Names, description) - if err != nil { - return err + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, description) + if mentionErr != nil { + return mentionErr } + description = mentionResult.HTML + mentionNotice = unresolvedMentionWarning(mentionResult.Unresolved) } // Build request @@ -508,7 +511,7 @@ func runScheduleCreate(cmd *cobra.Command, app *appctx.App, project, scheduleID, resultSummary := fmt.Sprintf("Created schedule entry #%d: %s", entry.ID, summary) - return app.OK(entry, + respOpts := []output.ResponseOption{ output.WithSummary(resultSummary), output.WithBreadcrumbs( output.Breadcrumb{ @@ -522,7 +525,11 @@ func runScheduleCreate(cmd *cobra.Command, app *appctx.App, project, scheduleID, Description: "View all entries", }, ), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(entry, respOpts...) } func newScheduleUpdateCmd(project *string) *cobra.Command { @@ -593,17 +600,19 @@ You can pass either an entry ID or a Basecamp URL: req.EndsAt = endsAt hasChanges = true } + var mentionNotice string if description != "" { html := richtext.MarkdownToHTML(description) html, err = resolveLocalImages(cmd, app, html) if err != nil { return err } - html, err = resolveMentions(cmd.Context(), app.Names, html) - if err != nil { - return err + mentionResult, mentionErr := resolveMentions(cmd.Context(), app.Names, html) + if mentionErr != nil { + return mentionErr } - req.Description = html + req.Description = mentionResult.HTML + mentionNotice = unresolvedMentionWarning(mentionResult.Unresolved) hasChanges = true } if cmd.Flags().Changed("all-day") { @@ -639,7 +648,7 @@ You can pass either an entry ID or a Basecamp URL: resultSummary := fmt.Sprintf("Updated schedule entry #%s", entryID) - return app.OK(entry, + respOpts := []output.ResponseOption{ output.WithSummary(resultSummary), output.WithBreadcrumbs( output.Breadcrumb{ @@ -648,7 +657,11 @@ You can pass either an entry ID or a Basecamp URL: Description: "View entry", }, ), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(entry, respOpts...) }, } diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 8bb97373..3f09890d 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1192,6 +1192,7 @@ Examples: // Convert comment through rich text pipeline commentHTML := comment + var mentionNotice string if comment != "" { commentHTML = richtext.MarkdownToHTML(comment) var pipelineErr error @@ -1199,10 +1200,12 @@ Examples: if pipelineErr != nil { return pipelineErr } - commentHTML, pipelineErr = resolveMentions(cmd.Context(), app.Names, commentHTML) + mentionResult, pipelineErr := resolveMentions(cmd.Context(), app.Names, commentHTML) if pipelineErr != nil { return pipelineErr } + commentHTML = mentionResult.HTML + mentionNotice = unresolvedMentionWarning(mentionResult.Unresolved) } // Execute actions @@ -1245,7 +1248,7 @@ Examples: summary += fmt.Sprintf(", completed %d", len(result.Completed)) } - return app.OK(result, + respOpts := []output.ResponseOption{ output.WithSummary(summary), output.WithBreadcrumbs( output.Breadcrumb{ @@ -1254,7 +1257,11 @@ Examples: Description: "List todos", }, ), - ) + } + if mentionNotice != "" { + respOpts = append(respOpts, output.WithDiagnostic(mentionNotice)) + } + return app.OK(result, respOpts...) }, } diff --git a/internal/output/envelope.go b/internal/output/envelope.go index f3e2e924..b426c864 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -30,16 +30,17 @@ func TruncationNoticeWithTotal(count, totalCount int) string { // Response is the success envelope for JSON output. type Response struct { - OK bool `json:"ok"` - Data any `json:"data,omitempty"` - Summary string `json:"summary,omitempty"` - Notice string `json:"notice,omitempty"` // Informational message (e.g., truncation warning) - Breadcrumbs []Breadcrumb `json:"breadcrumbs,omitempty"` - Context map[string]any `json:"context,omitempty"` - Meta map[string]any `json:"meta,omitempty"` - Entity string `json:"-"` // Schema hint for presenter (not serialized) - DisplayData any `json:"-"` // Alternate data for styled/markdown rendering (not serialized) - presenterOpts []presenter.PresentOption // Display options for presenter (not serialized) + OK bool `json:"ok"` + Data any `json:"data,omitempty"` + Summary string `json:"summary,omitempty"` + Notice string `json:"notice,omitempty"` // Informational message (e.g., truncation warning) + Breadcrumbs []Breadcrumb `json:"breadcrumbs,omitempty"` + Context map[string]any `json:"context,omitempty"` + Meta map[string]any `json:"meta,omitempty"` + Entity string `json:"-"` // Schema hint for presenter (not serialized) + DisplayData any `json:"-"` // Alternate data for styled/markdown rendering (not serialized) + presenterOpts []presenter.PresentOption // Display options for presenter (not serialized) + noticeDiagnostic bool // when true, emit Notice to stderr in quiet mode } // Breadcrumb is a suggested follow-up action. @@ -73,10 +74,11 @@ const ( // Options controls output behavior. type Options struct { - Format Format - Writer io.Writer - Verbose bool - JQFilter string // jq expression to apply to JSON output (built-in via gojq) + Format Format + Writer io.Writer + ErrWriter io.Writer // Diagnostic output (notices in quiet mode); defaults to os.Stderr. + Verbose bool + JQFilter string // jq expression to apply to JSON output (built-in via gojq) } // DefaultOptions returns options for standard output. @@ -100,6 +102,9 @@ func New(opts Options) *Writer { if opts.Writer == nil { opts.Writer = os.Stdout } + if opts.ErrWriter == nil { + opts.ErrWriter = os.Stderr + } w := &Writer{opts: opts} if opts.JQFilter != "" { q, err := gojq.Parse(opts.JQFilter) @@ -184,6 +189,17 @@ func WithErrorStats(metrics *observability.SessionMetrics) ErrorResponseOption { } func (w *Writer) write(v any) error { + // In quiet mode (--agent/--quiet), surface diagnostic notices on stderr so + // automation consumers can detect degraded operations (e.g. unresolved + // mentions). Only notices marked as diagnostic emit here — informational + // notices like truncation warnings stay silent. This runs before the --jq + // early-return so that --agent --jq still emits the diagnostic. + if w.opts.Format == FormatQuiet { + if resp, ok := v.(*Response); ok && resp.noticeDiagnostic && resp.Notice != "" { + fmt.Fprintf(w.opts.ErrWriter, "notice: %s\n", resp.Notice) + } + } + // --jq flag: serialize to JSON, apply the jq filter, print results if w.opts.JQFilter != "" { return w.writeJQ(v) @@ -202,7 +218,6 @@ func (w *Writer) write(v any) error { switch format { case FormatQuiet: - // Extract just the data field for quiet mode if resp, ok := v.(*Response); ok { return w.writeQuiet(resp.Data) } @@ -413,7 +428,15 @@ func WithSummary(s string) ResponseOption { // WithNotice adds an informational notice to the response. // Use this for non-error messages like truncation warnings. func WithNotice(s string) ResponseOption { - return func(r *Response) { r.Notice = s } + return func(r *Response) { r.Notice = s; r.noticeDiagnostic = false } +} + +// WithDiagnostic sets a notice that is also emitted to stderr in quiet mode. +// Use this for degraded-operation warnings (e.g. unresolved mentions) that +// automation consumers need to detect. Truncation and other informational +// notices should use WithNotice instead. +func WithDiagnostic(s string) ResponseOption { + return func(r *Response) { r.Notice = s; r.noticeDiagnostic = true } } // WithBreadcrumbs adds breadcrumbs to the response. diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 28eae9a1..7e8fc0e5 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -443,6 +443,79 @@ func TestWriterQuietFormat(t *testing.T) { assert.False(t, exists, "Quiet format should not include envelope ok field") } +func TestWriterQuietFormatDiagnosticOnStderr(t *testing.T) { + var stdout, stderr bytes.Buffer + w := New(Options{ + Format: FormatQuiet, + Writer: &stdout, + ErrWriter: &stderr, + }) + + data := map[string]string{"id": "123"} + err := w.OK(data, WithDiagnostic("Unresolved mentions left as text: @Bobby")) + require.NoError(t, err) + + // Data on stdout should not contain the notice + var decoded map[string]string + require.NoError(t, json.Unmarshal(stdout.Bytes(), &decoded)) + assert.Equal(t, "123", decoded["id"]) + _, hasNotice := decoded["notice"] + assert.False(t, hasNotice, "notice should not appear in quiet stdout") + + // Diagnostic notice should appear on stderr + assert.Contains(t, stderr.String(), "Unresolved mentions left as text: @Bobby") +} + +func TestWriterQuietFormatNoNoticeOnStderr(t *testing.T) { + var stdout, stderr bytes.Buffer + w := New(Options{ + Format: FormatQuiet, + Writer: &stdout, + ErrWriter: &stderr, + }) + + data := map[string]string{"id": "123"} + err := w.OK(data, WithSummary("all good")) + require.NoError(t, err) + + assert.Empty(t, stderr.String(), "stderr should be empty when there is no notice") +} + +func TestWriterQuietJQDiagnosticOnStderr(t *testing.T) { + var stdout, stderr bytes.Buffer + w := New(Options{ + Format: FormatQuiet, + Writer: &stdout, + ErrWriter: &stderr, + JQFilter: ".id", + }) + + data := map[string]any{"id": "123"} + err := w.OK(data, WithDiagnostic("Unresolved mentions left as text: @Bobby")) + require.NoError(t, err) + + // jq output on stdout should contain the extracted value + assert.Contains(t, stdout.String(), "123") + + // Diagnostic notice should still appear on stderr despite --jq + assert.Contains(t, stderr.String(), "Unresolved mentions left as text: @Bobby") +} + +func TestWriterQuietFormatNoticeDoesNotWriteStderr(t *testing.T) { + var stdout, stderr bytes.Buffer + w := New(Options{ + Format: FormatQuiet, + Writer: &stdout, + ErrWriter: &stderr, + }) + + data := map[string]string{"id": "123"} + err := w.OK(data, WithNotice("Showing 25 of 100 results")) + require.NoError(t, err) + + assert.Empty(t, stderr.String(), "plain WithNotice should not write to stderr") +} + func TestWriterQuietFormatString(t *testing.T) { var buf bytes.Buffer w := New(Options{ diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index c75e6dde..224ba7d7 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -3,8 +3,10 @@ package richtext import ( + "errors" "fmt" "regexp" + "slices" "strconv" "strings" "sync" @@ -700,6 +702,17 @@ type MentionLookupFunc func(name string) (sgid, displayName string, err error) // Used by the person:ID mention syntax. type PersonByIDFunc func(id string) (sgid, canonicalName string, err error) +// ErrMentionSkip is a sentinel error that lookup functions can return to indicate +// that a fuzzy @Name mention should be left as plain text instead of failing the +// entire operation. Use this for recoverable errors like not-found or ambiguous. +var ErrMentionSkip = errors.New("mention skip") + +// MentionResult holds the resolved HTML and any mentions that could not be resolved. +type MentionResult struct { + HTML string + Unresolved []string +} + // MentionToHTML builds a mention tag. func MentionToHTML(sgid, name string) string { return `@Name and @@ -817,14 +831,17 @@ func resolveSGIDMentions(html string) string { } // resolveNameMentions processes fuzzy @Name and @First.Last patterns. -func resolveNameMentions(html string, lookup MentionLookupFunc) (string, error) { +// When a lookup returns ErrMentionSkip (wrapped or direct), the mention is left +// as plain text and its name is collected in the unresolved slice. +func resolveNameMentions(html string, lookup MentionLookupFunc) (string, []string, error) { matches := reMentionInput.FindAllStringSubmatchIndex(html, -1) if len(matches) == 0 { - return html, nil + return html, nil, nil } result := html htmlLower := strings.ToLower(html) + var unresolved []string for i := len(matches) - 1; i >= 0; i-- { m := matches[i] mentionStart, mentionEnd := m[4], m[5] @@ -855,14 +872,19 @@ func resolveNameMentions(html string, lookup MentionLookupFunc) (string, error) sgid, displayName, err := lookup(name) if err != nil { - return "", fmt.Errorf("failed to resolve mention %s: %w", mention, err) + if errors.Is(err, ErrMentionSkip) { + unresolved = append(unresolved, mention) + continue + } + return "", nil, fmt.Errorf("failed to resolve mention %s: %w", mention, err) } tag := MentionToHTML(sgid, displayName) result = result[:mentionStart] + tag + result[mentionEnd:] } - return result, nil + slices.Reverse(unresolved) + return result, unresolved, nil } // isInsideHTMLTag checks if position pos is inside an HTML tag (between < and >). diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index d00adcd4..55d0121b 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -968,8 +968,8 @@ func TestResolveMentions(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if result != tt.expected { - t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result, tt.expected) + if result.HTML != tt.expected { + t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result.HTML, tt.expected) } }) } @@ -1034,8 +1034,8 @@ func TestResolveMentions_MentionSGID(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if result != tt.expected { - t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result, tt.expected) + if result.HTML != tt.expected { + t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result.HTML, tt.expected) } }) } @@ -1093,8 +1093,8 @@ func TestResolveMentions_PersonID(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if result != tt.expected { - t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result, tt.expected) + if result.HTML != tt.expected { + t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result.HTML, tt.expected) } }) } @@ -1134,8 +1134,8 @@ func TestResolveMentions_SGIDInline(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if result != tt.expected { - t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result, tt.expected) + if result.HTML != tt.expected { + t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result.HTML, tt.expected) } }) } @@ -1156,8 +1156,8 @@ func TestResolveMentions_Mixed(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if result != expected { - t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result, expected) + if result.HTML != expected { + t.Errorf("ResolveMentions() =\n %q\nwant:\n %q", result.HTML, expected) } }) } @@ -1169,3 +1169,89 @@ func TestResolveMentions_PersonSchemeNilLookup(t *testing.T) { t.Error("expected error for person: scheme with nil lookupByID") } } + +func TestResolveMentions_ErrMentionSkip(t *testing.T) { + lookup := func(name string) (string, string, error) { + people := map[string][2]string{ + "John": {"sgid-john", "John Doe"}, + "John Doe": {"sgid-john", "John Doe"}, + "Beth": {"sgid-beth", "Beth Smith"}, + } + if p, ok := people[name]; ok { + return p[0], p[1], nil + } + return "", "", fmt.Errorf("%w: not found: %s", ErrMentionSkip, name) + } + + t.Run("mixed valid and invalid mentions", func(t *testing.T) { + input := `

Hey @John, @Beth, and @Bobby are you around

` + result, err := ResolveMentions(input, lookup, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expected := `

Hey ` + MentionToHTML("sgid-john", "John Doe") + `, ` + + MentionToHTML("sgid-beth", "Beth Smith") + `, and @Bobby are you around

` + if result.HTML != expected { + t.Errorf("HTML =\n %q\nwant:\n %q", result.HTML, expected) + } + if len(result.Unresolved) != 1 || result.Unresolved[0] != "@Bobby" { + t.Errorf("Unresolved = %v, want [@Bobby]", result.Unresolved) + } + }) + + t.Run("all invalid fuzzy mentions preserves input order", func(t *testing.T) { + input := `

Hey @Unknown and @Nobody

` + result, err := ResolveMentions(input, lookup, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if result.HTML != input { + t.Errorf("HTML should be unchanged, got:\n %q", result.HTML) + } + want := []string{"@Unknown", "@Nobody"} + if len(result.Unresolved) != len(want) { + t.Fatalf("Unresolved = %v, want %v", result.Unresolved, want) + } + for i, name := range want { + if result.Unresolved[i] != name { + t.Errorf("Unresolved[%d] = %q, want %q", i, result.Unresolved[i], name) + } + } + }) + + t.Run("all valid mentions returns empty unresolved", func(t *testing.T) { + input := `

Hey @John and @Beth

` + result, err := ResolveMentions(input, lookup, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(result.Unresolved) != 0 { + t.Errorf("Unresolved should be empty, got %v", result.Unresolved) + } + }) + + t.Run("non-skip error still fails", func(t *testing.T) { + failLookup := func(name string) (string, string, error) { + return "", "", fmt.Errorf("network timeout") + } + input := `

Hey @John

` + _, err := ResolveMentions(input, failLookup, nil) + if err == nil { + t.Error("expected error for non-skip failure") + } + }) + + t.Run("person ID scheme still hard fails", func(t *testing.T) { + lookupByID := func(id string) (string, string, error) { + return "", "", fmt.Errorf("not pingable") + } + input := `@Ghost` + _, err := ResolveMentions(input, lookup, lookupByID) + if err == nil { + t.Error("expected error for person:ID resolution failure") + } + }) +}