fix(mention): canonicalize @mention labels server-side#65
Merged
Conversation
The mention markdown `[@Label](mention://type/UUID)` has two parts: the visible label and the routable UUID. Mention routing reads only the UUID, but the rendered comment shows only the label, so a label that disagrees with its UUID silently misroutes — the UI shows "@A" while agent B is triggered and replies. This already happened in the squad coordination flow (a leader's delegation paired one roster row's label with another row's UUID). Defend against it at write time, in the place every comment passes through: on `CreateComment` / `UpdateComment` and the agent task- completion comment path, rewrite each mention's label to the current name of the entity its UUID resolves to. - Agent / squad: if the UUID resolves in the workspace, replace the label with the entity's current name. If it doesn't resolve, keep the mention markdown as-is — `ParseMentions` will still find no live trigger target, and the existing top-level "mention only others" trigger-suppression keeps working. - Member: resolve via `GetMemberByUserAndWorkspace`, not `GetUser` — the latter is workspace-unscoped and would leak the name of any user in the system. If the user is not a member of this workspace, strip the mention entirely. - When canonicalization changes a label, log a warning with the original label, resolved entity name, and UUID. This gives a searchable signal for the prompt/UI bug that produced the mismatch in the first place. Care taken in the regex layer: - Issue-identifier expansion (`MUL-123` → markdown link) previously re-scanned inside mention labels, so an agent named "MUL-117 reviewer" would have its label rewritten into nested markdown. Split skip-region detection into `findSkipRegions` (code fences / inline code, shared with canonicalization) and `expandSkipRegions` (also covers existing link spans, used by issue expansion). - Canonical labels are markdown-escaped before being spliced back in, so a name containing `]` or `\` can't break the surrounding link.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
scanMentionAt's "continue on ] not followed by (" branch existed to
support labels with balanced inner brackets like [@david[TF]](...),
but the same branch fired when the ] belonged to a plain bracketed
phrase (e.g. [this]) with no inner [. The scanner kept walking past
whitespace and the next [ until it anchored on an unrelated real
mention's ](mention://...), producing a match whose span covered the
unrelated text in between.
In CanonicalizeMentions that span was replaced wholesale, silently
deleting user content: "check [this] out [@bot](mention://...) please"
was persisted as "check [@bot](mention://...) please".
Track unescaped [ depth opened after the leading [ at start. A ] at
depth > 0 is treated as closing an inner [ (the [@david[TF]] case
still parses). A ] at depth 0 not followed by ( now bails out
instead of continuing the scan, so the outer FindMentionMatches loop
advances past the plain bracketed text and finds the real mention
intact.
Regression tests cover both ParseMentions (positions invisible there,
so the existing test is a no-op safety pin) and CanonicalizeMentions
(where the data-loss actually shows up).
Tighten the backend label round-trip surface so the canonicalize defense holds against malformed input and pathological names: - scanMentionAt: keep strict depth-tracking with anchor only at depth 0. An earlier attempt to anchor on `](mention://` at depth > 0 (to support unescaped unpaired brackets in labels) created a new data-loss path for bracketed prefixes like `note [draft [@bob](mention://...)` — the outer `[draft ` text would get swallowed into the match. Producer-side escape is the right place to handle bracket-bearing names. - parseMentionAt: reject empty labels after stripping the optional `@`. Without this, `[](mention://all/all)` or `[@](mention://all/all)` route as @ALL — an invisible markdown link could silently broadcast to the whole workspace. - Add util.EscapeMentionLabel / util.UnescapeMentionLabel as the single source of truth for the escape contract. `\` is escaped FIRST so a name like `foo\[bar` emits `foo\\\[bar` (not `foo\\[bar`, which the scanner would consume as `\\` + raw `[`). Inverse unescapes brackets BEFORE collapsing `\\` so a legitimate `\[` pair is not split. - squad_briefing.formatMention now routes the name through EscapeMentionLabel before splicing into markdown. - mention.CanonicalizeMentions drops its local escapeMentionLabel and uses the shared util helper. - handler.logMentionLabelMismatch uses UnescapeMentionLabel for the equality check so a legitimately bracket- or backslash-bearing canonical name doesn't false-positive the warning. Test coverage: empty-label and bare-@ rejection (parse + canonicalize); non-@ bracketed prefix before mention preserved (no data loss); unescaped unpaired bracket inside label passes through deterministically (no character deletion); escape↔unescape inverse contract; full producer→ parser round-trip for plain / bracket / backslash / mixed names.
Mirror the backend's util.EscapeMentionLabel / UnescapeMentionLabel contract in the two frontend consumers so labels round-trip through the editor and trigger-summary path without losing or gaining backslashes: - mention-extension renderMarkdown (safeLabel): escape `\` FIRST, then `[` and `]`. Order matters — without leading `\` escape, a name like `foo\[bar` emits `foo\\[bar`, which the scanner consumes as `\\` + raw `[` and fails to round-trip. - mention-extension tokenize: unescape `\[`, `\]`, then `\\` (reverse of the producer). Also mirror util.parseMentionAt's empty-label guard — `[@](mention://all/all)` backtracks into the label group (label = `@`), and without rejection here the editor would tokenize it, then renderMarkdown would emit `[@@](mention://all/all)`, slipping past the backend's non-empty check and routing as @ALL on save. - strip-mention-markdown: same unescape order so trigger summaries for a name like `Ops\Bot` no longer show an extra backslash. Regression tests pin: label with literal `\`; label with `\` adjacent to `[` (4-char-into-7-source-char round-trip); `[](...)` and `[@](...)` rejected by the tokenizer.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CreateComment,UpdateComment, and the agent task-completion comment path).Behaviour by entity type
ParseMentionsstill sees it. This preserves the existing top-level "mention-only-others suppresses trigger" semantics — turning unresolvable agent mentions into plain text would break that invariant.GetMemberByUserAndWorkspace.GetUseris workspace-unscoped and would leak the display name of any user in the system, so it is deliberately not used. If the user isn't a member of this workspace, the mention is stripped to plain text.WARNlog with the original label, resolved name, and UUID — a searchable signal whenever the upstream prompt/UI produced a mismatch.Other care taken
MUL-123→ markdown link) previously re-scanned inside existing mention labels. An agent named "MUL-117 reviewer" would have its label rewritten into nested markdown.findSkipRegionsis split into a code-fence-only helper (shared with canonicalization) andexpandSkipRegions(also covers link spans, used by issue expansion).]or\\can't escape the surrounding link.Test plan
go test ./internal/mention/... ./internal/handler/... ./internal/util/...go build ./...🤖 Generated with Claude Code