Repository navigation
fix(twilio,messenger): send literal text and bare links as written - #1115
Manishram-ai wants to merge 3 commits into
Conversation
SMS and Messenger show Markdown syntax as typed, so the backslash escapes that stringifyMarkdown adds to literal `_`, `*`, `#`, and `~` appear in the message, hard breaks arrive as a trailing backslash, and bare URLs and email addresses arrive wrapped in angle brackets. Serialize text nodes and breaks as written, and send a link whose text is its own address as that address. Signed-off-by: Manish Ram <manishrm@uw.edu>
|
@Manishram-ai is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
bensabic
left a comment
There was a problem hiding this comment.
Thanks for this. The approach is right: reusing the handlers option from #1039 and unwrapping GFM bare-address links fixes the escape and <url> noise in SMS and Messenger. I checked it out locally and ran the following.
Validation
- Twilio 173/173 and Messenger 201/201 unit tests pass.
pnpm check,pnpm knip, and typecheck for both packages are clean. Integration tests pass (63 files / 1426 tests), including the Messenger replays. - I confirmed your claim: running the new
markdown.test.tscases againstmain's converters fails 10 cases in each adapter.
Smoke test: postMessage({ markdown }) through each adapter with mocked HTTP. The Twilio Body and the Messenger Send API message.text both come out literal (snake_case_name, 3 * 4, #1, a plain newline for the hard break, and bare URL/www./email unchanged), and [the docs](…) and **bold** stay the same.
Mutation testing (11 hand mutations per converter): removing either handler, the single-child check, any of the three address forms, the www. guard, or the replacement itself makes tests fail. Two mutations survive:
- Removing
structuredClonein both converters.walkAstchanges the tree in place (packages/chat/src/markdown.ts:597-598), so this is a real coverage gap. It matters most in Messenger, where the clone is new in this PR. - Removing the
child?.type !== "text"guard in Twilio. Messenger only catches it because its vitest config runs typecheck. At runtime,[**https://x.com**](https://x.com)would then throw onvalue.startsWith.
Requested changes (details inline)
- The
www.check is case-sensitive, but GFM autolinksWWW.example.comandWww.example.com, which still go out as[WWW.example.com](http://WWW.example.com). I verified thatvalue.toLowerCase().startsWith("www.")fixes it, passes Biome, and keeps[example.com](http://example.com)as a link. handlersandbareLinkTextare duplicated across the two adapters. Both already depend on@chat-adapter/shared, and WhatsApp has the same bug today (Track <https://example.com/x>,[www.example.com](http://www.example.com),<help@example.com>). Please move them into shared so the three plain-text-ish adapters can't drift.- Add tests for the two surviving mutations:
fromAstdoesn't change the AST it's given, and a link whose only child isn't text is left alone. - Messenger
AGENTS.md(Format conversion, lines 131-138, and the directory comment at line 33) still describes a plain-text converter withlabel (url). Since this PR edits that converter and the description already points out the mismatch, please correct the doc so agents working here aren't misled.
Non-blocking
- Messenger doesn't flatten tables the way Twilio does, so with escapes gone, a cell containing
x\|ynow goes out as| x|y | z |, which reads as an extra column. Messenger could use the sametableToAsciitreatment, here or in a follow-up. - Image alt text is still escaped (
→), because thetexthandler doesn't coverimage.alt. That's minor, but the changeset says literal text is sent as written. - Link labels lose their escapes too (
[the \] docs](…)→[the ] docs](…)). That's fine since neither platform renders Markdown, but it's worth knowing.
| if ( | ||
| node.url === value || | ||
| node.url === `mailto:${value}` || | ||
| (value.startsWith("www.") && node.url === `http://${value}`) |
There was a problem hiding this comment.
GFM matches the www. prefix case-insensitively, so WWW.example.com / Www.Example.com still parse as links with url: "http://WWW.example.com". This check misses them, and they go out as [WWW.example.com](http://WWW.example.com). I verified locally:
| (value.startsWith("www.") && node.url === `http://${value}`) | |
| (value.toLowerCase().startsWith("www.") && node.url === `http://${value}`) |
With this change, Twilio's suite passes, biome check is clean (an inline /^www\./i trips useTopLevelRegex), Go to WWW.example.com or Www.Example.com goes out as written, and [example.com](http://example.com) stays a link. Please add WWW.example.com to the bare-address it.each table in both adapters.
| if ( | ||
| node.url === value || | ||
| node.url === `mailto:${value}` || | ||
| (value.startsWith("www.") && node.url === `http://${value}`) |
There was a problem hiding this comment.
Same case-sensitivity bug as Twilio: WWW.example.com is still sent as [WWW.example.com](http://WWW.example.com). Needs the same toLowerCase() fix (or, better, one shared implementation; see the next comment).
| * text is the address. Returns that address, so it can be sent as written | ||
| * instead of as `<url>` or `[www.example.com](http://www.example.com)`. | ||
| */ | ||
| function bareLinkText(node: Content): string | undefined { |
There was a problem hiding this comment.
handlers and bareLinkText are byte-for-byte the same in Messenger. Both adapters already depend on @chat-adapter/shared, and WhatsApp has the same bare-address bug today (renderPostable({ markdown: "Track https://example.com/x" }) → Track <https://example.com/x>). Could this move to packages/adapter-shared (e.g. an unwrapBareLinks(ast) helper plus a literalTextHandlers export) so the fix lives in one place and the www. case fix above only has to be made once?
| export class MessengerFormatConverter extends BaseFormatConverter { | ||
| fromAst(ast: Root): string { | ||
| return stringifyMarkdown(ast).trim(); | ||
| const transformed = walkAst(structuredClone(ast), (node: Content) => { |
There was a problem hiding this comment.
This structuredClone is load-bearing: walkAst rewrites node.children in place (packages/chat/src/markdown.ts:597-598), so without it, postMessage({ ast }) would turn the caller's link nodes into text nodes. But when I removed it as a mutation, all 201 tests still passed, and the same is true for Twilio. Please add a test that the input AST is unchanged after fromAst, e.g.
const ast = root([paragraph([link("https://example.com", [text("https://example.com")])])]);
const before = structuredClone(ast);
converter.fromAst(ast);
expect(ast).toEqual(before);| expect(converter.renderPostable({ markdown })).toBe(expected); | ||
| }); | ||
|
|
||
| it("keeps links with their own label as Markdown links", () => { |
There was a problem hiding this comment.
Mutation gap: when I removed the child?.type !== "text" guard in bareLinkText, Twilio's suite still passed (Twilio's vitest doesn't typecheck, unlike Messenger's). At runtime, [**https://example.com**](https://example.com) would then throw on value.startsWith. Please add a case where the link's only child isn't text (strong or inlineCode) and assert it stays a Markdown link.
Twilio and Messenger had the same text and break handlers and the same bare link check. Move them to @chat-adapter/shared as literalTextHandlers and unwrapBareLinks, so both adapters use one copy. GFM matches the www. prefix in any case, so unwrapBareLinks also unwraps addresses such as WWW.example.com. Add tests that fromAst leaves the AST it's given unchanged and that a link whose only child isn't text stays a Markdown link. Signed-off-by: Manish Ram <manishrm@uw.edu>
AGENTS.md described a plain-text converter that strips formatting and writes links as `label (url)`. Describe the Markdown output, the shared literal text helpers, and the GFM parsing instead. Also say that the adapter truncates text over the Send API's 2000-character limit, since it doesn't split it into several messages. Signed-off-by: Manish Ram <manishrm@uw.edu>
|
Thanks for going through this so carefully. I pushed two commits:
I left WhatsApp out to keep this PR to Twilio and Messenger, but it can use the same helpers in a follow-up. I marked the shared changeset as a patch. Let me know if you'd rather have a minor. |
Summary
Twilio and Messenger send the output of
stringifyMarkdownas is, and neither platform renders Markdown, so the serializer's escapes and link syntax show up in messages:Use snake_case_nameUse snake\_case\_nameUse snake_case_namePrice: 3 * 4 = 12Price: 3 \* 4 = 12Price: 3 * 4 = 12#1 priority\#1 priority#1 priorityline oneand a hard breakline one\and a newlineline oneand a newlineTrack it at https://example.com/trackTrack it at <https://example.com/track>Track it at https://example.com/trackEmail help@example.comEmail <help@example.com>Email help@example.comGo to www.example.comGo to [www.example.com](http://www.example.com)Go to www.example.comGo to WWW.example.comGo to [WWW.example.com](http://WWW.example.com)Go to WWW.example.comBoth converters now use two new helpers from
@chat-adapter/shared.literalTextHandlersgoes to thehandlersoption ofstringifyMarkdown, the option #1039 added for WhatsApp, so text nodes are written as they are and hard breaks become plain newlines. GFM parses a bare URL,www.address (in any case), or email address as a link whose text is the address, sounwrapBareLinksreplaces that kind of link with its text in a copy of the AST before serializing. The input AST isn't changed.Links with their own label, such as
[the docs](https://example.com/docs), still come out as Markdown links, and so do bold, lists, and headings. The Twilio tests expect**hello**to be sent as**hello**, so this PR leaves that behavior alone.Messenger's
AGENTS.mddescribed a plain-text converter that strips formatting and writes links aslabel (url), but the converter has passed Markdown through like Twilio's since the adapter was added. This PR updates that section to describe the converter as it works now, and fixes the note about long messages, which the adapter truncates to 2000 characters instead of splitting. Neither adapter's docs page covers text formatting, so there's no docs change.Test plan
markdown.test.tscases in both adapters cover literal text, hard breaks, and bare URLs,www.addresses, and email addresses. The first 10 new cases in each adapter fail onmainand pass with this change.literal-text.test.tscases in@chat-adapter/sharedtest the helpers directly, includingWWW.example.comandWww.Example.com.fromAstdoesn't change the AST it's given, in both adapters.structuredClone, the text-child check, ortoLowerCase()from the shared helper makes tests fail in shared, Twilio, and Messenger. Removing either handler, the single-child check, thewww.check, or any of the three address forms also makes at least one test fail.pnpm validatepasses, including the shared (206), Twilio (177), and Messenger (205) test suites, except for the Nuxt example build. That build fails on my machine withENAMETOOLONGin Nitro's output folder with or without this change, and CI doesn't build it.Checklist
git commit -s)pnpm validatepasses