Repository navigation
fix(gchat): keep literal asterisks and tildes in incoming messages - #1114
Manishram-ai wants to merge 2 commits into
Conversation
The Google Chat adapter still converted incoming bold and strikethrough with the regexes that vercel#957 removed from the Slack adapter, so `2 * 3` became `2 ** 3` and code such as `rm -rf *` was rewritten. Use the same marker matching as Slack's applyEmphasis, and skip inline code and fenced code blocks. 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. |
There was a problem hiding this comment.
Thanks for porting this. The fix is correct and I'd like to merge it once the new masking and fence-pairing logic has tests.
What I checked on 1dbf3e6f
- The 4 new tests: 3 fail on
mainand all 4 pass on this branch, as the description says. @chat-adapter/gchat: full suite passes (312 tests), plustsc --noEmit, Biome on the changed files, and the package build.- Ran ~45 inputs through
parseIncomingonmainand on this branch. Nothing got worse. These all got better:2 * 3,`rm -rf *`, fenced and```inline```code,*bold with `*` inside*(onmainthis came out as*bold with ** inside),*a* `*` *b*, and*a *b*. - Timing on 16k-char worst cases (
*aaaa…, runs of backticks, repeated```a, mixed*`~runs):toStandardMarkdownstays at a few ms or less, small next to parse time. So doing this before the budget check is fine. - Mutation testing on
markdown.ts, running the full gchat suite against each mutant. Two mutants that break the new logic still pass the whole suite (the line-break rule has no test either; see the comment on line 88):- Masking code with
""instead of"x".repeat(...)passes every test, even though it moves marker indices and drops bold after inline code. - Making
findCodeFenceEndstop before the closing fence passes every test, even though it pairs fences across separate code blocks.
- Masking code with
Requested change: add tests for those two paths (inline comments below include tests I checked: they pass here and catch both mutants). Everything else is non-blocking.
| cursor += isFence ? CODE_FENCE.length : 1; | ||
| continue; | ||
| } | ||
| masked += text.slice(start, cursor) + "x".repeat(end - cursor); |
There was a problem hiding this comment.
Needs a test. Lines 71-80 only work because this mask keeps masked the same length as text, so a match index in masked is also the marker's index in text. Nothing tests that. I swapped "x".repeat(end - cursor) for "" and all 312 gchat tests still passed, while run `npm i` then *restart* stopped producing bold and *bold with `*` inside* became **bold with `*` inside*. The existing tests only cover code with no emphasis next to it, and emphasis with no code. The test in the comment on markdown.test.ts covers this.
|
|
||
| function findCodeFenceEnd(text: string, index: number): number { | ||
| const close = text.indexOf(CODE_FENCE, index + CODE_FENCE.length); | ||
| return close === -1 ? -1 : close + CODE_FENCE.length; |
There was a problem hiding this comment.
Needs a test. If this returns close instead of close + CODE_FENCE.length, the closing fence gets read as a new opening fence and pairs with the next block's opening fence. Any emphasis between the two blocks then gets masked (for the input ```\na\n```\n*b*\n```\nc\n``` the *b* is left as is). With only one code block in the test, that mutant passes the full suite. A test with two blocks and emphasis between them catches it (see the markdown.test.ts comment).
| return close === -1 ? -1 : close + CODE_FENCE.length; | ||
| } | ||
|
|
||
| // Inline code spans never cross line breaks. |
There was a problem hiding this comment.
Non-blocking question: is this how Google Chat behaves, and is it the result we want after parseMarkdown? CommonMark code spans can cross line breaks. So `foo\n*bar*` skips the mask here, becomes `foo\n**bar**`, and parses to inlineCode "foo\n**bar**", which is the asterisk doubling inside code this PR is meant to stop. main and the Slack adapter do the same, so it's not a regression. Two options: let the mask cross newlines, matching the parser and leaving the code verbatim, or keep this rule and escape the stray backticks. Either way, a test that pins the chosen behavior would help.
| type: "root", | ||
| children: [{ type: "code", value: "x = *a*" }], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Requested: tests for emphasis next to inline code and for multiple code blocks. Without them, the two mutants described on markdown.ts lines 65 and 85 pass the full suite. Inserting this after this test passes on 1dbf3e6f, passes Biome, and fails against both mutants:
it("keeps bold that wraps or follows inline code", () => {
expect(converter.toAst("*bold with `*` inside*")).toMatchObject({
children: [
{
type: "paragraph",
children: [
{
type: "strong",
children: [
{ type: "text", value: "bold with " },
{ type: "inlineCode", value: "*" },
{ type: "text", value: " inside" },
],
},
],
},
],
});
expect(converter.toAst("run `npm i` then *restart*")).toMatchObject({
children: [
{
type: "paragraph",
children: [
{ type: "text", value: "run " },
{ type: "inlineCode", value: "npm i" },
{ type: "text", value: " then " },
{
type: "strong",
children: [{ type: "text", value: "restart" }],
},
],
},
],
});
});
it("pairs each code block's own fences", () => {
expect(
converter.toAst("```\na *1*\n```\n*bold*\n```\nb *2*\n```")
).toMatchObject({
children: [
{ type: "code", value: "a *1*" },
{
type: "paragraph",
children: [
{ type: "strong", children: [{ type: "text", value: "bold" }] },
],
},
{ type: "code", value: "b *2*" },
],
});
});| "@chat-adapter/gchat": patch | ||
| --- | ||
|
|
||
| Incoming Google Chat messages keep literal asterisks, tildes, and code as written. Bold and strikethrough now only apply when the markers wrap text with no space just inside them, so `2 * 3` and `~10` stay as typed, and inline code and code blocks keep their asterisks. |
There was a problem hiding this comment.
Non-blocking: tildes in URLs still get rewritten. "https://example.com/~alice and https://example.com/~bob" becomes ~~alice and https://example.com/~~bob on both main and this branch, because both ends have a non-space character inside them. That's fine to leave for a follow-up (Slack avoids it by masking <...> link tokens, and this change doesn't mask bare URLs). Consider narrowing the wording here so it doesn't promise that all literal tildes are kept.
Add tests for bold that wraps or follows inline code, and for two code blocks with bold between them. Both fail if the code mask changes the text length or a code block's closing fence is read as a new opening one. Narrow the changeset so it doesn't promise every tilde is kept. Signed-off-by: Manish Ram <manishrm@uw.edu>
|
Added both tests in 43444ea. They pass here and fail against the two mutants you described (masking with I left inline code across line breaks and tildes in bare URLs as they are, since |
|
@roboeve the two tests you asked for are in 43444ea, and both fail against the mutants you described. Could you take another look? |
Summary
The Google Chat adapter still converts incoming bold and strikethrough with the regexes that #957 removed from the Slack adapter. They match any
*...*or~...~pair on a line, including pairs with spaces inside and pairs inside code, so on main:2 * 3 and 4 * 5, ~10 to ~20comes through as2 ** 3 and 4 ** 5, ~~10 to ~~20inmessage.text`rm -rf *`becomes the inline coderm -rf **x = *a*inside a code block becomesx = **a**This switches
toStandardMarkdownto the same approach as Slack'sapplyEmphasis: a marker pair only counts when a non-space character sits right inside it, and inline code is skipped. The Slack adapter takes code fences out before this step and the Google Chat adapter doesn't, so this version skips fenced code blocks too.*bold*and~strike~still parse as before.Test plan
markdown.test.ts: literal*and~, bold next to a literal*,*in inline code, and*in a code block. Three of them fail on main; all 4 pass with the change.@chat-adapter/gchattests: 313 passed.pnpm validate: knip, check, typecheck, tests and build all pass. In the first full run one Linear adapter test timed out under load (5.04s against a 5s limit); it passed when I reran the tests.Checklist
git commit -s)pnpm validatepasses