From 3d69b0f458edef4a4c68e0182f5c3c385979f5f8 Mon Sep 17 00:00:00 2001 From: Christoph Leiter Date: Fri, 28 Aug 2026 10:58:27 +0200 Subject: [PATCH] fix(app): render markdown verbatim instead of substituting characters (#3253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit markdown-it's typographer option rewrites characters after parsing, so a literal (c) came out as ©. Agents label steps and options (a), (b), (c) all the time, and an ordinary list rendering as (a), (b), © stops your eye every time. The same option turns ... into an ellipsis, --- into an em dash, and straight quotes into curly ones. Agent output is text people paste back into a shell or a file, so a curly quote in --name="my repo" looks correct on screen and fails when pasted. The markdown file preview had the same problem, showing characters that are not in the file on disk. Every in-app markdown surface now goes through one factory in utils/markdown-parser.ts, which leaves typographer off: agent chat, plan cards, pull request comment bodies and the file preview. The plan card was rendering through its library's built-in parser because it never passed a markdownit prop, and that default has typographer on. Link behaviour is unchanged. The assistant parser keeps its file:// permission and no other surface gains it, and linkify stays on for chat and the default renderer and off for plan cards, matching what each did before. Co-authored-by: Claude Opus 5 --- .../e2e/browser/plan-card-markdown.spec.ts | 37 +++++++++ .../app/src/components/markdown/renderer.tsx | 5 +- packages/app/src/components/plan-card.tsx | 8 +- packages/app/src/styles/markdown-styles.ts | 6 +- .../utils/assistant-markdown-parser.test.ts | 28 +++++++ .../src/utils/assistant-markdown-parser.ts | 11 ++- .../app/src/utils/markdown-parser.test.ts | 75 +++++++++++++++++++ packages/app/src/utils/markdown-parser.ts | 19 +++++ .../agent/providers/mock-load-test-agent.ts | 6 +- 9 files changed, 185 insertions(+), 10 deletions(-) create mode 100644 packages/app/e2e/browser/plan-card-markdown.spec.ts create mode 100644 packages/app/src/utils/assistant-markdown-parser.test.ts create mode 100644 packages/app/src/utils/markdown-parser.test.ts create mode 100644 packages/app/src/utils/markdown-parser.ts diff --git a/packages/app/e2e/browser/plan-card-markdown.spec.ts b/packages/app/e2e/browser/plan-card-markdown.spec.ts new file mode 100644 index 0000000000..b5d7cfe1e7 --- /dev/null +++ b/packages/app/e2e/browser/plan-card-markdown.spec.ts @@ -0,0 +1,37 @@ +import { expect, test } from "../support/fixtures"; +import { waitForPermissionPrompt } from "../support/helpers/permissions"; +import { openAgentRoute, seedMockAgentWorkspace } from "../support/helpers/mock-agent"; + +// The plan card has to pass its own markdown parser. Drop that prop and +// react-native-markdown-display silently supplies one with `typographer: true` +// (node_modules/react-native-markdown-display/src/index.js:139-141), which +// turns (c) into ©, curls straight quotes, and rewrites --- as an em dash. +// No unit test can catch that, because the defect lives in the JSX. +// +// Nothing here is provider-specific: the mock provider drives it, and the +// behavior is the same for every agent. +test.describe("Plan card markdown", () => { + test("renders plan text verbatim", async ({ page }) => { + test.setTimeout(180_000); + + const session = await seedMockAgentWorkspace({ + repoPrefix: "plan-card-markdown-", + title: "Plan card markdown e2e", + initialPrompt: "Emit synthetic plan approval.", + }); + + try { + await openAgentRoute(page, session); + await waitForPermissionPrompt(page, 120_000); + + const planCard = page.getByTestId("permission-plan-card"); + await expect(planCard).toContainText("(c)"); + await expect(planCard).toContainText('--name="my repo"'); + await expect(planCard).toContainText("---buzz"); + await expect(planCard).not.toContainText("©"); + await expect(planCard).not.toContainText("—buzz"); + } finally { + await session.cleanup(); + } + }); +}); diff --git a/packages/app/src/components/markdown/renderer.tsx b/packages/app/src/components/markdown/renderer.tsx index 61505326bc..db51607a8a 100644 --- a/packages/app/src/components/markdown/renderer.tsx +++ b/packages/app/src/components/markdown/renderer.tsx @@ -28,6 +28,7 @@ import { MarkdownParagraphView, MarkdownTextSpan } from "@/components/markdown-t import { MarkdownTableCellText } from "@/components/markdown-text-selection"; import { getMarkdownListMarker, getMarkdownListSpacing } from "@/utils/markdown-list"; import { markdownNodeContainsType } from "@/utils/markdown-ast"; +import { createMarkdownParser } from "@/utils/markdown-parser"; import { createCompactMarkdownStyles, createMarkdownStyles } from "@/styles/markdown-styles"; import type { Theme } from "@/styles/theme"; import { openExternalUrl } from "@/utils/open-external-url"; @@ -65,7 +66,9 @@ function compactMarkdownStyleMapping(theme: Theme): Partial; @@ -219,7 +225,7 @@ export function PlanCard({ {resolvedTitle} {description ? {description} : null} - + {text} {footer ? {footer} : null} diff --git a/packages/app/src/styles/markdown-styles.ts b/packages/app/src/styles/markdown-styles.ts index fa49aca69a..5d10624136 100644 --- a/packages/app/src/styles/markdown-styles.ts +++ b/packages/app/src/styles/markdown-styles.ts @@ -16,7 +16,11 @@ function contentHeadingLineHeight(contentSize: number, tier: keyof typeof FONT_S * * Usage: * const markdownStyles = useMemo(() => createMarkdownStyles(theme), [theme]); - * {content} + * {content} + * + * Always pass `markdownit` from `@/utils/markdown-parser`. Omit it and + * react-native-markdown-display builds its own parser with `typographer: true`, + * which rewrites a literal `(c)` as ©. */ export function createMarkdownStyles(theme: Theme) { return { diff --git a/packages/app/src/utils/assistant-markdown-parser.test.ts b/packages/app/src/utils/assistant-markdown-parser.test.ts new file mode 100644 index 0000000000..70d806a1fb --- /dev/null +++ b/packages/app/src/utils/assistant-markdown-parser.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from "vitest"; +import { createAssistantMarkdownParser } from "./assistant-markdown-parser"; + +describe("createAssistantMarkdownParser", () => { + it("renders agent text verbatim", () => { + const parser = createAssistantMarkdownParser(); + + // The reported bug, plus the substitutions that share its cause. + expect(parser.renderInline("(c) (C) (r) (tm) (p)")).toBe("(c) (C) (r) (tm) (p)"); + expect(parser.renderInline("wait for it...")).toBe("wait for it..."); + expect(parser.renderInline("a -- b")).toBe("a -- b"); + // Smart quotes are off too: a curled quote is not pasteable into a shell. + expect(parser.renderInline(`run --name="my repo"`)).toBe("run --name="my repo""); + expect(parser.renderInline("it's fine")).toBe("it's fine"); + }); + + it("allows file:// links, unlike every other parser", () => { + const parser = createAssistantMarkdownParser(); + + expect(parser.render("[open](file:///tmp/a.ts)")).toContain('href="file:///tmp/a.ts"'); + }); + + it("still rejects javascript: links", () => { + const parser = createAssistantMarkdownParser(); + + expect(parser.render("[x](javascript:alert(1))")).not.toContain("href"); + }); +}); diff --git a/packages/app/src/utils/assistant-markdown-parser.ts b/packages/app/src/utils/assistant-markdown-parser.ts index 4a8493ca8b..5e64fb629f 100644 --- a/packages/app/src/utils/assistant-markdown-parser.ts +++ b/packages/app/src/utils/assistant-markdown-parser.ts @@ -1,13 +1,12 @@ -import MarkdownIt from "markdown-it"; +import type MarkdownIt from "markdown-it"; +import { createMarkdownParser } from "@/utils/markdown-parser"; export function createAssistantMarkdownParser(): MarkdownIt { - const parser = new MarkdownIt({ - html: false, - linkify: true, - typographer: true, - }); + const parser = createMarkdownParser({ linkify: true }); const defaultValidateLink = parser.validateLink.bind(parser); + // Assistant messages are the only surface allowed to link into the + // filesystem. Every other parser keeps markdown-it's stricter default. parser.validateLink = (url: string) => url.trim().toLowerCase().startsWith("file://") || defaultValidateLink(url); diff --git a/packages/app/src/utils/markdown-parser.test.ts b/packages/app/src/utils/markdown-parser.test.ts new file mode 100644 index 0000000000..38783f9245 --- /dev/null +++ b/packages/app/src/utils/markdown-parser.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from "vitest"; +import { createMarkdownParser } from "./markdown-parser"; + +// Every string markdown-it's typographer would rewrite, with the character it +// would rewrite it to. Sourced from markdown-it/lib/rules_core/replacements.js +// and smartquotes.js. `--flag` is deliberately absent: the en-dash rules need +// whitespace or a word character on both sides, so a CLI flag after a space is +// never touched and asserting on it would prove nothing. +const REWRITTEN_BY_TYPOGRAPHER = [ + "(c)", + "(C)", + "(r)", + "(R)", + "(tm)", + "(TM)", + "(p)", + "(P)", + "+-", + "two dots .. here", + "wait for it...", + "what????", + "stop!!!!", + "hmm,, ok", + "a -- b", + "word--word", + "a --- b", + 'run --name="my repo"', + "it's fine", +]; + +describe("createMarkdownParser", () => { + it("renders every typographer-rewritten sequence verbatim", () => { + const parser = createMarkdownParser({ linkify: true }); + + for (const source of REWRITTEN_BY_TYPOGRAPHER) { + expect(parser.renderInline(source)).toBe(escapeHtml(source)); + } + }); + + it("would fail if typographer were switched back on", () => { + // Guards the assertion above: proves the fixture actually exercises the + // rules, so a future refactor can't quietly make the test vacuous. + const typographer = createMarkdownParser({ linkify: true }); + typographer.set({ typographer: true }); + + for (const source of REWRITTEN_BY_TYPOGRAPHER) { + expect(typographer.renderInline(source)).not.toBe(escapeHtml(source)); + } + }); + + it("rejects file:// links", () => { + const parser = createMarkdownParser({ linkify: true }); + + expect(parser.render("[open](file:///tmp/a.ts)")).not.toContain("href"); + }); + + it("rejects javascript: links", () => { + const parser = createMarkdownParser({ linkify: true }); + + expect(parser.render("[x](javascript:alert(1))")).not.toContain("href"); + }); + + it("linkifies bare URLs only when asked", () => { + expect(createMarkdownParser({ linkify: true }).render("see https://paseo.sh now")).toContain( + 'href="https://paseo.sh"', + ); + expect( + createMarkdownParser({ linkify: false }).render("see https://paseo.sh now"), + ).not.toContain("href"); + }); +}); + +function escapeHtml(value: string): string { + return value.replaceAll("&", "&").replaceAll("<", "<").replaceAll('"', """); +} diff --git a/packages/app/src/utils/markdown-parser.ts b/packages/app/src/utils/markdown-parser.ts new file mode 100644 index 0000000000..3a408f1f2c --- /dev/null +++ b/packages/app/src/utils/markdown-parser.ts @@ -0,0 +1,19 @@ +import MarkdownIt from "markdown-it"; + +/** + * The one place that decides how the app parses markdown. + * + * `typographer` stays off. It switches on two markdown-it core rules that + * rewrite characters the user may copy back into a shell or a file: + * `replacements` turns `(c)` into ©, `a -- b` into an en dash and `...` into + * an ellipsis, and `smartquotes` curls straight quotes so `--name="x"` stops + * being pasteable. Agent output, plans and file previews all have to render + * the text they were handed. + * + * `linkify` is a parameter rather than a shared default because the surfaces + * disagree today: chat and the default renderer linkify bare URLs, plan cards + * never have. Unifying that is a product decision on its own. + */ +export function createMarkdownParser({ linkify }: { linkify: boolean }): MarkdownIt { + return new MarkdownIt({ html: false, linkify }); +} diff --git a/packages/server/src/server/agent/providers/mock-load-test-agent.ts b/packages/server/src/server/agent/providers/mock-load-test-agent.ts index 1f1f35876c..315bc728c1 100644 --- a/packages/server/src/server/agent/providers/mock-load-test-agent.ts +++ b/packages/server/src/server/agent/providers/mock-load-test-agent.ts @@ -1283,7 +1283,11 @@ export class MockLoadTestAgentSession implements AgentSession { title: "Plan", description: "Review the proposed plan before implementation starts.", input: { - plan: "1. Add the README note.\n2. Keep the change scoped.\n3. Verify the diff.", + // The (c), the quoted flag and the --- are load-bearing: they trip + // three different markdown-it typographer rules, and + // plan-card-markdown.spec.ts asserts all three render verbatim. That + // is the only guard against the plan card's parser prop going missing. + plan: '1. Add the (c) README note.\n2. Run --name="my repo".\n3. Verify ---buzz in the diff.', }, actions: [ {