Add @mention resolution for comments and descriptions#112
Add @mention resolution for comments and descriptions#112waynemsmith wants to merge 4 commits intobasecamp:masterfrom
Conversation
Resolve @firstname patterns in comment and card description text to ActionText mention HTML before sending to the API. This triggers Fizzy notifications for mentioned users, matching the behavior of the web UI. - Add GetHTML() to client for fetching HTML endpoints - Fetch mentionable users from /prompts/users endpoint - Parse <lexxy-prompt-item> elements for sgid and name data - Replace @firstname with <action-text-attachment> mention HTML - Cache user list per CLI invocation (sync.Once) - Graceful degradation: on fetch error, text is sent unchanged - Skip email patterns (user@example.com) - Warn on ambiguous or unresolved mentions - Update SKILL.md with @mention documentation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/mentions.go">
<violation number="1" location="internal/commands/mentions.go:38">
P2: Mention parsing is ASCII-only, so valid internationalized or hyphenated first names cannot be resolved from `@...` text.</violation>
<violation number="2" location="internal/commands/mentions.go:41">
P2: Mention-user HTML parsing is overly strict (attribute-order dependent), so minor markup changes can make parsing return zero users and silently disable mention resolution.</violation>
<violation number="3" location="internal/commands/mentions.go:169">
P2: Avatar lookup is not scoped to the current prompt-item block, so users can receive another item’s image source.</violation>
</file>
<file name="internal/commands/mentions_test.go">
<violation number="1" location="internal/commands/mentions_test.go:146">
P2: Missing test coverage for ambiguous first-name mentions leaves an important mention-resolution branch unverified.</violation>
<violation number="2" location="internal/commands/mentions_test.go:156">
P2: The "no @ passthrough" test does not assert zero `GetHTML` calls, so regressions that add unnecessary `/prompts/users` fetches for plain text would not be caught.</violation>
</file>
<file name="internal/commands/comment.go">
<violation number="1" location="internal/commands/comment.go:152">
P2: Mention resolution is applied to raw markdown text using regex, so `@...` inside markdown contexts (code/links/URLs) can be transformed unintentionally before parsing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
internal/commands/comment.go
Outdated
| return err | ||
| } | ||
| body = markdownToHTML(string(content)) | ||
| body = markdownToHTML(resolveMentions(string(content), getClient())) |
There was a problem hiding this comment.
P2: Mention resolution is applied to raw markdown text using regex, so @... inside markdown contexts (code/links/URLs) can be transformed unintentionally before parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/commands/comment.go, line 152:
<comment>Mention resolution is applied to raw markdown text using regex, so `@...` inside markdown contexts (code/links/URLs) can be transformed unintentionally before parsing.</comment>
<file context>
@@ -149,9 +149,9 @@ var commentCreateCmd = &cobra.Command{
return err
}
- body = markdownToHTML(string(content))
+ body = markdownToHTML(resolveMentions(string(content), getClient()))
} else if commentCreateBody != "" {
- body = markdownToHTML(commentCreateBody)
</file context>
There was a problem hiding this comment.
Fixed in c0b727e. resolveMentions() now protects markdown code spans (`@name`) and fenced code blocks (```...```) by replacing them with placeholders before scanning for mentions, then restoring them after. Added three test cases: inline code, fenced block, and mixed (mention outside + code inside).
There was a problem hiding this comment.
Pull request overview
This PR adds automatic @FirstName mention resolution for comment bodies and card descriptions in the Fizzy CLI, converting plain-text mentions into ActionText mention attachments so that mentioned users receive notifications (matching the web UI).
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Introduces mention resolution logic (fetch + parse mentionable users; replace
@FirstNamewith ActionText mention HTML) with unit tests and caching. - Adds
GetHTML()to the API client/interface to fetch/prompts/usersas HTML. - Wires mention resolution into card/comment create & update flows and documents the feature in SKILL docs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/mentions.go |
Implements mention caching, user list fetch/parsing, and @FirstName → ActionText replacement. |
internal/commands/mentions_test.go |
Adds unit tests for HTML parsing, resolution behavior, caching, and error fallback. |
internal/client/client.go |
Adds Client.GetHTML() for HTML endpoints. |
internal/client/interface.go |
Extends client.API with GetHTML(). |
internal/commands/mock_client_test.go |
Updates mock client to support GetHTML() for tests. |
internal/commands/comment.go |
Resolves mentions before converting comment body markdown to HTML. |
internal/commands/card.go |
Resolves mentions before converting description markdown to HTML. |
skills/fizzy/SKILL.md |
Documents @FirstName mention usage for the skill. |
internal/skills/SKILL.md |
Mirrors @FirstName mention documentation for embedded skill docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func buildMentionHTML(u mentionUser) string { | ||
| return fmt.Sprintf( | ||
| `<action-text-attachment sgid="%s" content-type="application/vnd.actiontext.mention">`+ | ||
| `<img title="%s" src="%s" width="48" height="48">%s`+ | ||
| `</action-text-attachment>`, | ||
| u.SGID, u.FullName, u.AvatarSrc, u.FirstName, | ||
| ) |
There was a problem hiding this comment.
buildMentionHTML interpolates SGID, FullName, AvatarSrc, and FirstName directly into HTML attributes / text without escaping. User names can legally contain characters like quotes or '<', which can break the generated markup or introduce HTML injection in the rich-text body. Use proper HTML escaping for attribute values and inner text (e.g., html.EscapeString) before formatting the attachment HTML, and add a test covering a name containing quotes or '<' to prevent regressions.
There was a problem hiding this comment.
Fixed in c0b727e. All user-controlled values (SGID, FullName, AvatarSrc, FirstName) are now escaped with html.EscapeString() before interpolation. Added TestBuildMentionHTMLEscaping covering names with quotes and <script> tags.
internal/commands/mentions.go
Outdated
| var mentionRegex = regexp.MustCompile(`(?:^|[^a-zA-Z0-9_.])@([a-zA-Z][a-zA-Z0-9_]*)`) | ||
|
|
||
| // promptItemRegex parses <lexxy-prompt-item> tags from the /prompts/users HTML. | ||
| var promptItemRegex = regexp.MustCompile(`<lexxy-prompt-item\s+search="([^"]+)"\s+sgid="([^"]+)"[^>]*>`) |
There was a problem hiding this comment.
promptItemRegex assumes a very specific attribute order/spacing (search then sgid) and will fail to parse users if the HTML emitter changes attribute order or inserts additional attributes (which is allowed by HTML). That would silently disable mention resolution. Prefer parsing the HTML with an HTML tokenizer/parser and extracting search/sgid attributes from each regardless of order (and similarly find the avatar within that element).
| var promptItemRegex = regexp.MustCompile(`<lexxy-prompt-item\s+search="([^"]+)"\s+sgid="([^"]+)"[^>]*>`) | |
| // It is robust to attribute order and extra attributes, while capturing search and sgid. | |
| var promptItemRegex = regexp.MustCompile(`<lexxy-prompt-item(?=[^>]*\ssearch="([^"]+)")(?=[^>]*\ssgid="([^"]+)")[^>]*>`) |
There was a problem hiding this comment.
Fixed in c0b727e. Note: the suggested lookahead regex ((?=...)) is not supported by Go's RE2 engine — it panics at compile time. Instead, replaced the single regex with separate searchAttrRegex and sgidAttrRegex patterns applied independently to each matched tag. This handles any attribute order without requiring PCRE features.
| func resolveMentions(text string, c client.API) string { | ||
| if !strings.Contains(text, "@") { | ||
| return text | ||
| } | ||
|
|
||
| mentionOnce.Do(func() { | ||
| mentionUsers, mentionErr = fetchMentionUsers(c) | ||
| }) | ||
|
|
||
| if mentionErr != nil { | ||
| fmt.Fprintf(os.Stderr, "Warning: could not fetch mentionable users: %v\n", mentionErr) | ||
| return text | ||
| } | ||
|
|
||
| if len(mentionUsers) == 0 { | ||
| return text | ||
| } | ||
|
|
||
| // Find all @Name matches with positions | ||
| type mentionMatch struct { | ||
| start int // start of @Name (the @ character) | ||
| end int // end of @Name | ||
| name string | ||
| } | ||
|
|
||
| allMatches := mentionRegex.FindAllStringSubmatchIndex(text, -1) | ||
| var matches []mentionMatch |
There was a problem hiding this comment.
resolveMentions runs on the raw markdown input before markdownToHTML(), so it will also rewrite @Name occurrences inside markdown code spans / fenced code blocks (e.g., @Sarah), changing what the user intended to show as literal code into an escaped <action-text-attachment ...> snippet. Add logic to skip mention resolution inside markdown code (or move mention resolution to operate on the post-markdown HTML while ignoring /
content), and add a unit test for the backtick/code-fence case.There was a problem hiding this comment.
Fixed in c0b727e. Code spans and fenced code blocks are now replaced with null-byte placeholders before mention resolution runs, then restored afterward. This prevents @name inside backticks or code fences from being transformed. Three new test cases cover this.
internal/client/client.go
Outdated
| req.Header.Set("Authorization", "Bearer "+c.Token) | ||
| req.Header.Set("Accept", "text/html") | ||
| req.Header.Set("User-Agent", "fizzy-cli/1.0") |
There was a problem hiding this comment.
GetHTML duplicates header-setting logic rather than reusing c.setHeaders(req). This risks divergence if common headers change (e.g., User-Agent/auth handling) and makes the client harder to maintain. Consider calling c.setHeaders(req) and then overriding only the Accept header to text/html.
| req.Header.Set("Authorization", "Bearer "+c.Token) | |
| req.Header.Set("Accept", "text/html") | |
| req.Header.Set("User-Agent", "fizzy-cli/1.0") | |
| c.setHeaders(req) | |
| req.Header.Set("Accept", "text/html") |
There was a problem hiding this comment.
Fixed in c0b727e. GetHTML() now calls c.setHeaders(req) and only overrides the Accept header to text/html.
- Support Unicode letters and hyphens in @mention names (e.g. @josé, @Mary-Jane) - Parse <lexxy-prompt-item> attributes in any order (not position-dependent) - Scope avatar lookup to each prompt-item block (prevents cross-user mismatch) - HTML-escape all user-controlled values in buildMentionHTML (prevent injection) - Skip @mentions inside markdown code spans and fenced code blocks - Reuse client.setHeaders() in GetHTML instead of duplicating header logic - Add tests: ambiguous mentions, no-fetch passthrough, attribute order, avatar scoping, code block protection, HTML escaping Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/commands/comment.go
Outdated
| body = markdownToHTML(resolveMentions(string(content), getClient())) | ||
| } else if commentCreateBody != "" { | ||
| body = markdownToHTML(commentCreateBody) | ||
| body = markdownToHTML(resolveMentions(commentCreateBody, getClient())) | ||
| } else { |
There was a problem hiding this comment.
getClient() constructs a new API client each call. In this command it’s invoked separately for each resolveMentions(...) path, which can create multiple identical clients during a single execution. Consider creating the client once (e.g., apiClient := getClient()) and reuse it for mention resolution in both branches.
There was a problem hiding this comment.
Fixed in f84f9d7. Hoisted getClient() before the if/else branches in all 4 call sites (comment create/update, card create/update).
internal/commands/comment.go
Outdated
| body = markdownToHTML(resolveMentions(string(content), getClient())) | ||
| } else if commentUpdateBody != "" { | ||
| body = markdownToHTML(commentUpdateBody) | ||
| body = markdownToHTML(resolveMentions(commentUpdateBody, getClient())) | ||
| } |
There was a problem hiding this comment.
Same as comment create: getClient() is called separately in each branch here, which can instantiate multiple identical clients in a single run. Prefer allocating the client once and reusing it when calling resolveMentions.
There was a problem hiding this comment.
Fixed in f84f9d7 — same change applied here.
internal/commands/card.go
Outdated
| description = markdownToHTML(resolveMentions(string(descContent), getClient())) | ||
| } else if cardCreateDescription != "" { | ||
| description = markdownToHTML(cardCreateDescription) | ||
| description = markdownToHTML(resolveMentions(cardCreateDescription, getClient())) | ||
| } |
There was a problem hiding this comment.
getClient() creates a new API client each time it’s called; here it’s invoked separately for the file vs flag branches. Consider creating the client once in the command (before the conditional) and reusing it for resolveMentions(...) to avoid redundant client construction.
There was a problem hiding this comment.
Fixed in f84f9d7 — same change applied here.
internal/commands/card.go
Outdated
| description = markdownToHTML(resolveMentions(string(content), getClient())) | ||
| } else if cardUpdateDescription != "" { | ||
| description = markdownToHTML(cardUpdateDescription) | ||
| description = markdownToHTML(resolveMentions(cardUpdateDescription, getClient())) | ||
| } |
There was a problem hiding this comment.
Same as create: this code calls getClient() separately in each branch, which can create multiple identical clients per invocation. Prefer allocating once and reusing when calling resolveMentions.
There was a problem hiding this comment.
Fixed in f84f9d7 — same change applied here.
| func (m *MockClient) GetHTML(path string) (*client.APIResponse, error) { | ||
| m.GetHTMLCalls = append(m.GetHTMLCalls, MockCall{Path: path}) | ||
| if m.GetHTMLError != nil { | ||
| return nil, m.GetHTMLError | ||
| } | ||
| if m.GetHTMLResponse != nil { | ||
| return m.GetHTMLResponse, nil | ||
| } | ||
| return &client.APIResponse{StatusCode: 200, Body: []byte("")}, nil | ||
| } |
There was a problem hiding this comment.
MockClient.GetHTML doesn’t support the existing path-based response routing (OnGet / getPathResponses) that MockClient.Get provides, so tests can’t easily set different HTML responses per endpoint. Consider adding similar routing (or a dedicated OnGetHTML) for consistency and more flexible tests.
There was a problem hiding this comment.
Acknowledged. Only one HTML endpoint exists currently (/prompts/users), so path-based routing is not needed yet. Happy to add OnGetHTML if a second HTML endpoint is introduced.
search was injecting the default board from config into every query via defaultBoard(), which scoped results to a single board. This made --tag and --assignee filters appear broken when cards were on other boards. Search should be cross-board by default — only scope to a board when --board is explicitly passed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Create the API client once per command instead of calling getClient() in each if/else branch. Only one branch executes, but hoisting makes intent clearer and avoids redundant construction if the code evolves. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // codeBlockRegex matches fenced code blocks (``` ... ```). | ||
| var codeBlockRegex = regexp.MustCompile("(?s)```.*?```") | ||
|
|
||
| // codeSpanRegex matches inline code spans (` ... `). | ||
| var codeSpanRegex = regexp.MustCompile("`[^`]+`") |
There was a problem hiding this comment.
The code-span and fenced-code protection is incomplete for CommonMark/GFM: codeSpanRegex only matches single-backtick spans and codeBlockRegex only matches triple-backtick fences. Mentions inside code delimited by multiple backticks (e.g. @Name) or fenced blocks using tildes / longer backtick fences can still be rewritten, contrary to the intended “don’t resolve in code” behavior. Consider detecting code regions via the Goldmark AST (preferred, since markdownToHTML already uses Goldmark), or at least expanding the protection to handle variable-length backtick fences and ~~~ fences, and add unit tests covering those cases.
| // codeBlockRegex matches fenced code blocks (``` ... ```). | |
| var codeBlockRegex = regexp.MustCompile("(?s)```.*?```") | |
| // codeSpanRegex matches inline code spans (` ... `). | |
| var codeSpanRegex = regexp.MustCompile("`[^`]+`") | |
| // codeBlockRegex matches fenced code blocks with backticks or tildes, | |
| // including variable-length fences (e.g. ``` ... ```, ```` ... ````, ~~~ ... ~~~). | |
| // The same fence character and length must be used to open and close the block. | |
| var codeBlockRegex = regexp.MustCompile(`(?s)(` + "`" + `{3,}|~{3,}).*?\1`) | |
| // codeSpanRegex matches inline code spans delimited by one or more backticks | |
| // (e.g. `code`, ``code``, ```code```), preventing mentions inside them | |
| // from being resolved. | |
| var codeSpanRegex = regexp.MustCompile(`(` + "`" + `+)[^` + "`" + `]*?\1`) |
| // GetHTML performs a GET request expecting an HTML response. | ||
| // Unlike Get, it sets Accept: text/html and does not attempt JSON parsing. | ||
| func (c *Client) GetHTML(path string) (*APIResponse, error) { | ||
| requestURL := c.buildURL(path) |
There was a problem hiding this comment.
GetHTML() is a new request path with important behavior differences from Get() (different Accept header and no JSON parsing). Since internal/client has extensive request/response tests already, it would be good to add a focused unit test for GetHTML() to prevent regressions (e.g., assert the server sees Accept: text/html, the raw body is returned, and non-2xx responses are surfaced via errorFromResponse).
Summary
@FirstNamepatterns in comment and card description text to ActionText mention HTML before sending to the API@Sarah can you review?instead of constructing<action-text-attachment>HTML manuallyHow it works
@, the CLI fetches mentionable users from/prompts/users(HTML endpoint with<lexxy-prompt-item>elements containing sgids)@FirstNamepatterns are matched case-insensitively against the user list<action-text-attachment sgid="..." content-type="application/vnd.actiontext.mention">HTMLmarkdownToHTML()as beforeDesign decisions
markdownToHTML()stays pure.resolveMentions()runs before it at each call site:markdownToHTML(resolveMentions(text, getClient()))GetHTML()on the client — new method that setsAccept: text/htmland skips JSON parsing, keepingGet()unchangedsync.Onceand reuseduser@example.comis not treated as a mentionChanges
internal/commands/mentions.gointernal/commands/mentions_test.gointernal/client/client.goGetHTML()methodinternal/client/interface.goGetHTML()toAPIinterfaceinternal/commands/comment.goresolveMentions()at 4 call sitesinternal/commands/card.goresolveMentions()at 4 call sitesinternal/commands/mock_client_test.goGetHTMLto mockskills/fizzy/SKILL.mdinternal/skills/SKILL.mdTest plan
go test ./internal/commands/ -run TestParseMentionUsers— HTML parsinggo test ./internal/commands/ -run TestResolveMentions— 9 test cases (passthrough, single, case-insensitive, multiple, email, unresolved, start-of-text, after-newline, single-name user)go test ./internal/commands/ -run TestResolveMentionsAPIError— graceful degradationgo test ./internal/commands/ -run TestResolveMentionsCaching— sync.Once cachinggo test ./internal/commands/ -run TestMarkdown— existing markdown tests unaffectedgo test ./internal/client/— existing client tests unaffectedfizzy comment create --card N --body "Hey @Wayne, testing"— mention renders on Fizzy with notification🤖 Generated with Claude Code
Summary by cubic
Adds
@FirstNamemention resolution in CLI comments and card descriptions, converting mentions to ActionText HTML with Fizzy notifications. Also fixes search to be cross-board by default and hoistsgetClient()above branches for clarity.New Features
@FirstNamebeforemarkdownToHTML(); wired into comment/card create and update./prompts/usersviaGetHTML(); parse<lexxy-prompt-item>in any order; avatar lookup scoped per item.skills/fizzy/SKILL.mdandinternal/skills/SKILL.md.Bug Fixes
--boardis set, fixing--tagand--assigneeacross boards.getClient()before conditional branches in comment/card commands.Written for commit f84f9d7. Summary will update on new commits.