From 2eee9b7186beb975c7d47bf5d67996cfdcd112c4 Mon Sep 17 00:00:00 2001 From: Long Jin Date: Tue, 13 Oct 2026 09:17:35 +0000 Subject: [PATCH] fix(cli): recognize markdown fences only on matching lines (#13331) * fix(cli): recognize markdown fences only on matching lines Fixes #13309 * perf(cli): avoid repeated markdown fence scans * fix(cli): guard fence scans and strengthen regressions --- eslint.legacy-filenames.mjs | 1 - .../cli/src/ui/hooks/use-llm-stream.test.tsx | 6 +- packages/cli/src/ui/hooks/use-llm-stream.ts | 2 +- .../cli/src/ui/utils/MarkdownDisplay.test.tsx | 32 ++++ packages/cli/src/ui/utils/MarkdownDisplay.tsx | 2 +- ...ies.test.ts => markdown-utilities.test.ts} | 170 +++++++++++++++++- ...downUtilities.ts => markdown-utilities.ts} | 96 ++++++---- 7 files changed, 263 insertions(+), 46 deletions(-) rename packages/cli/src/ui/utils/{markdownUtilities.test.ts => markdown-utilities.test.ts} (66%) rename packages/cli/src/ui/utils/{markdownUtilities.ts => markdown-utilities.ts} (81%) diff --git a/eslint.legacy-filenames.mjs b/eslint.legacy-filenames.mjs index 419b8bac23b..2a09b7b82c4 100644 --- a/eslint.legacy-filenames.mjs +++ b/eslint.legacy-filenames.mjs @@ -199,7 +199,6 @@ export const legacyFilenames = [ 'LspConfigLoader', 'LspConnectionFactory', 'LspResponseNormalizer', - 'markdownUtilities', 'matcherGrouping', 'mcpCommand', 'McpPromptLoader', diff --git a/packages/cli/src/ui/hooks/use-llm-stream.test.tsx b/packages/cli/src/ui/hooks/use-llm-stream.test.tsx index 30eaa7a6cd5..5a066934292 100644 --- a/packages/cli/src/ui/hooks/use-llm-stream.test.tsx +++ b/packages/cli/src/ui/hooks/use-llm-stream.test.tsx @@ -51,7 +51,7 @@ import type { UseHistoryManagerReturn } from './useHistoryManager.js'; import type { HistoryItem, SlashCommandProcessorResult } from '../types.js'; import { MessageType, StreamingState, ToolCallStatus } from '../types.js'; import type { LoadedSettings } from '../../config/settings.js'; -import { findLastSafeSplitPoint } from '../utils/markdownUtilities.js'; +import { findLastSafeSplitPoint } from '../utils/markdown-utilities.js'; import { MAX_INLINE_IMAGE_ENCODED_LENGTH, MAX_INLINE_IMAGES_PER_ITEM, @@ -178,9 +178,9 @@ vi.mock('./shellCommandProcessor.js', () => ({ vi.mock('./atCommandProcessor.js'); -vi.mock('../utils/markdownUtilities.js', async (importOriginal) => { +vi.mock('../utils/markdown-utilities.js', async (importOriginal) => { const actual = - await importOriginal(); + await importOriginal(); return { ...actual, // Only the split-point chooser is mocked so tests can drive commit diff --git a/packages/cli/src/ui/hooks/use-llm-stream.ts b/packages/cli/src/ui/hooks/use-llm-stream.ts index 76cdeabc3e2..732d9963d9a 100644 --- a/packages/cli/src/ui/hooks/use-llm-stream.ts +++ b/packages/cli/src/ui/hooks/use-llm-stream.ts @@ -110,7 +110,7 @@ import { findLastSafeSplitPoint, splitFencedMarkdown, getEnclosingFenceInfo, -} from '../utils/markdownUtilities.js'; +} from '../utils/markdown-utilities.js'; import { fitPendingSlice } from '../utils/pending-rendered-height.js'; import { useStateAndRef } from './useStateAndRef.js'; import { normalizePartList } from '../../utils/normalize-part-list.js'; diff --git a/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx b/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx index c18c9a90b7d..618c8ebea50 100644 --- a/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx +++ b/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx @@ -13,6 +13,7 @@ import { renderMermaidVisual } from './mermaidVisualRenderer.js'; import { RenderModeProvider } from '../contexts/RenderModeContext.js'; import { getScreenBuffer } from '../selection/screen-buffer.js'; import { getSelectedText } from '../selection/selection-text.js'; +import { splitFencedMarkdown } from './markdown-utilities.js'; function copiedFrame(stdout: NodeJS.WriteStream): string { const frame = getScreenBuffer(stdout)!.frame!; @@ -115,6 +116,37 @@ describe('', () => { expect(lastFrame()).toMatchSnapshot(); }); + it.each([ + ['~~~', '```'], + ['```', '~~~'], + ])( + 'renders %s code across an inner %s line after a streaming split', + (outer, inner) => { + const text = + `${outer}md\nHere:\n${inner}\nstill inside\n${outer}\nAfter.`.replace( + /\n/g, + eol, + ); + const { after } = splitFencedMarkdown(text, text.indexOf('still')); + for (const renderedText of [text, after]) { + for (const isPending of [false, true]) { + const { lastFrame, unmount } = renderWithProviders( + , + ); + const frame = stripAnsi(lastFrame() ?? ''); + expect(frame).toMatch(/3\s+still inside/); + expect(frame).toContain('After.'); + expect(frame).not.toContain('qwen-code'); + unmount(); + } + } + }, + ); + it('clips a long pending message to availableTerminalHeight', () => { // A long streaming message must NOT render all its lines: the live // (non-) frame would exceed the terminal height and ink would diff --git a/packages/cli/src/ui/utils/MarkdownDisplay.tsx b/packages/cli/src/ui/utils/MarkdownDisplay.tsx index 857139cd187..a1abba1194e 100644 --- a/packages/cli/src/ui/utils/MarkdownDisplay.tsx +++ b/packages/cli/src/ui/utils/MarkdownDisplay.tsx @@ -16,7 +16,7 @@ import { useSettings } from '../contexts/SettingsContext.js'; import { MermaidDiagram } from './MermaidDiagram.js'; import { renderInlineLatex } from './latexRenderer.js'; import { useRenderMode } from '../contexts/RenderModeContext.js'; -import { parseCodeFenceInfo } from './markdownUtilities.js'; +import { parseCodeFenceInfo } from './markdown-utilities.js'; import { fitPendingSlice, splitMarkdownTableRow, diff --git a/packages/cli/src/ui/utils/markdownUtilities.test.ts b/packages/cli/src/ui/utils/markdown-utilities.test.ts similarity index 66% rename from packages/cli/src/ui/utils/markdownUtilities.test.ts rename to packages/cli/src/ui/utils/markdown-utilities.test.ts index 6a2de4fc573..654560845b3 100644 --- a/packages/cli/src/ui/utils/markdownUtilities.test.ts +++ b/packages/cli/src/ui/utils/markdown-utilities.test.ts @@ -4,15 +4,16 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; +import { CODE_FENCE_RE } from './pending-rendered-height.js'; import { findLastSafeSplitPoint, splitFencedMarkdown, parseCodeFenceInfo, getEnclosingFenceInfo, -} from './markdownUtilities.js'; +} from './markdown-utilities.js'; -describe('markdownUtilities', () => { +describe('markdown-utilities', () => { describe('findLastSafeSplitPoint', () => { it('should split at the last double newline if not in a code block', () => { const content = 'paragraph1\n\nparagraph2\n\nparagraph3'; @@ -91,6 +92,57 @@ describe('markdownUtilities', () => { const content = '```ts\n' + 'a'.repeat(100); expect(findLastSafeSplitPoint(content, 40)).toBe(40); }); + + it('re-arms the closed-block walk for the single-newline fallback', () => { + const content = '```a\nx\n\ny\n```\n```b\nz\n```TAIL'; + expect(findLastSafeSplitPoint(content, 22)).toBe(14); + }); + + it.each(['```', '``````', '~~~'])( + 'skips the fence regex for a backtick after the full %s delimiter', + (delimiter) => { + const content = delimiter + ' '.repeat(20000) + '`\n\nafter'; + const scan = vi.spyOn(CODE_FENCE_RE, 'exec'); + try { + expect(findLastSafeSplitPoint(content, 16384)).toBe(16384); + expect(scan.mock.calls.length).toBe(0); + } finally { + scan.mockRestore(); + } + }, + ); + + it.each([200, 2000])( + 'scans fences once when rejecting %s internal blank lines', + (records) => { + const fenced = `\`\`\`csv\n${'x\n\n'.repeat(records)}\`\`\`\n`; + const content = fenced + 'a'.repeat(16500 - fenced.length); + const scan = vi.spyOn(CODE_FENCE_RE, 'exec'); + try { + expect(findLastSafeSplitPoint(content, 16384)).toBe(fenced.length); + expect(scan.mock.calls.length).toBeGreaterThan(0); + expect(scan.mock.calls.length).toBeLessThanOrEqual(4); + } finally { + scan.mockRestore(); + } + }, + ); + + it('visits each fence once across many closed blocks', () => { + const blocks = 1000; + const fenced = '```csv\nx\n\n```\n'.repeat(blocks); + const content = fenced + 'a'.repeat(100); + const scan = vi.spyOn(CODE_FENCE_RE, 'exec'); + try { + expect(findLastSafeSplitPoint(content, fenced.length + 10)).toBe( + fenced.length, + ); + expect(scan.mock.calls.length).toBeGreaterThan(0); + expect(scan.mock.calls.length).toBeLessThanOrEqual(blocks * 2); + } finally { + scan.mockRestore(); + } + }); }); describe('splitFencedMarkdown', () => { @@ -283,4 +335,116 @@ describe('markdownUtilities', () => { }); }); }); + + describe('fence recognition', () => { + it.each([ + ['~~~', '```'], + ['```', '~~~'], + ])('keeps %s open across an inner %s line', (outer, inner) => { + const opening = `${outer}md\nHere:\n${inner}\n`; + const pending = `${opening}still inside\n`; + const content = `${pending}${outer}\nAfter.\n`; + const splitPoint = content.indexOf('still'); + + for (const text of [pending, content]) { + expect(getEnclosingFenceInfo(text, splitPoint)).toEqual({ + lang: 'md', + startLine: 1, + }); + expect(findLastSafeSplitPoint(text, splitPoint)).toBe(splitPoint); + expect(splitFencedMarkdown(text, splitPoint)).toEqual({ + before: `${opening}${outer}\n`, + after: `${outer}md qwen-code:start-line=3\n${text.slice(splitPoint)}`, + }); + } + expect( + getEnclosingFenceInfo(content, content.indexOf('After')), + ).toBeNull(); + }); + + it.each(['~~~x', '~~~~~x'])( + 'advances past a same-character fence marker in the info string: %s', + (info) => { + const content = `~~~ ${info}\ncode\n~~~\nAfter`; + expect(getEnclosingFenceInfo(content, content.indexOf('code'))).toEqual( + { + lang: info, + startLine: 1, + }, + ); + expect( + getEnclosingFenceInfo(content, content.indexOf('After')), + ).toBeNull(); + }, + ); + + it('advances past a fence info string containing the other fence marker', () => { + const content = '``` ~~~\ncode\n```\nAfter'; + expect(getEnclosingFenceInfo(content, content.indexOf('code'))).toEqual({ + lang: '~~~', + startLine: 1, + }); + expect( + getEnclosingFenceInfo(content, content.indexOf('After')), + ).toBeNull(); + }); + + it.each([ + ['~~~', 'Use `~~~` as the fence marker.'], + ['```', 'Use ``` as the fence marker.'], + ])('keeps an inline %s marker in ordinary prose', (_marker, intro) => { + const content = `${intro}\nMore ordinary prose follows.`; + const splitPoint = content.indexOf('More'); + + expect(getEnclosingFenceInfo(content, splitPoint)).toBeNull(); + expect(findLastSafeSplitPoint(content, splitPoint + 8)).toBe(splitPoint); + expect(splitFencedMarkdown(content, splitPoint)).toEqual({ + before: `${intro}\n`, + after: 'More ordinary prose follows.', + }); + }); + + it.each(['```ts`invalid', '~~~ts`invalid'])( + 'rejects an opening fence with a backtick in its info string: %s', + (opening) => { + const content = `${opening}\nMore ordinary prose follows.`; + const splitPoint = content.indexOf('More'); + + expect(getEnclosingFenceInfo(content, splitPoint)).toBeNull(); + expect(findLastSafeSplitPoint(content, splitPoint + 8)).toBe( + splitPoint, + ); + expect(splitFencedMarkdown(content, splitPoint)).toEqual({ + before: `${opening}\n`, + after: 'More ordinary prose follows.', + }); + }, + ); + + it.each(['````', '~~~~'])( + 'recognizes an indented %s fence after ordinary prose', + (delimiter) => { + const intro = 'Use `~~~` as the fence marker.\n'; + const content = `${intro} ${delimiter}ts\nline1\nline2\n ${delimiter}\n\nAfter prose.`; + const splitPoint = content.indexOf('line2'); + const afterBlock = content.indexOf('After'); + + expect(getEnclosingFenceInfo(content, splitPoint)).toEqual({ + lang: 'ts', + startLine: 1, + }); + expect(findLastSafeSplitPoint(content, splitPoint)).toBe( + intro.length + 2, + ); + expect(splitFencedMarkdown(content, splitPoint)).toEqual({ + before: `${intro} ${delimiter}ts\nline1\n${delimiter}\n`, + after: `${delimiter}ts qwen-code:start-line=2\nline2\n ${delimiter}\n\nAfter prose.`, + }); + expect(getEnclosingFenceInfo(content, afterBlock)).toBeNull(); + expect(findLastSafeSplitPoint(content, afterBlock + 3)).toBe( + afterBlock, + ); + }, + ); + }); }); diff --git a/packages/cli/src/ui/utils/markdownUtilities.ts b/packages/cli/src/ui/utils/markdown-utilities.ts similarity index 81% rename from packages/cli/src/ui/utils/markdownUtilities.ts rename to packages/cli/src/ui/utils/markdown-utilities.ts index e8d96ff1d0c..afb86bfffc2 100644 --- a/packages/cli/src/ui/utils/markdownUtilities.ts +++ b/packages/cli/src/ui/utils/markdown-utilities.ts @@ -4,6 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { CODE_FENCE_RE } from './pending-rendered-height.js'; + /* **Background & Purpose:** @@ -40,9 +42,9 @@ so rendered history chunks do not break fenced code blocks unnecessarily. */ /** - * Finds the next fenced-code delimiter (a run of 3+ ``` or ~~~) at or after - * `from`, returning its index, fence character and the FULL run length. Both - * fence types are recognized. The run length matters: `indexOf('```')` matches + * Finds the next standalone fenced-code delimiter at or after `from`, using + * MarkdownDisplay's line-matching rules and returning its index, fence character + * and the FULL run length. The run length matters: `indexOf('```')` matches * only the first 3 chars of a longer run, so callers must advance past the whole * run (index + length) or a 6-backtick fence would be miscounted as two. */ @@ -50,21 +52,39 @@ const findNextFence = ( content: string, from: number, ): { index: number; char: '`' | '~'; length: number } | null => { - const backtick = content.indexOf('```', from); - const tilde = content.indexOf('~~~', from); - if (backtick === -1 && tilde === -1) return null; - let index: number; - let char: '`' | '~'; - if (tilde === -1 || (backtick !== -1 && backtick < tilde)) { - index = backtick; - char = '`'; - } else { - index = tilde; - char = '~'; + let lineStart = from === 0 ? 0 : content.lastIndexOf('\n', from - 1) + 1; + while (lineStart < content.length) { + const newlineIndex = content.indexOf('\n', lineStart); + const lineEnd = newlineIndex === -1 ? content.length : newlineIndex; + // This pre-filter must remain a superset of CODE_FENCE_RE. + let delimiterStart = lineStart; + while (content[delimiterStart] === ' ') delimiterStart++; + if ( + delimiterStart >= from && + (content.startsWith('```', delimiterStart) || + content.startsWith('~~~', delimiterStart)) + ) { + let delimiterEnd = delimiterStart + 3; + while (content[delimiterEnd] === content[delimiterStart]) delimiterEnd++; + if (content.slice(delimiterEnd, lineEnd).includes('`')) { + lineStart = lineEnd + 1; + continue; + } + const match = CODE_FENCE_RE.exec(content.slice(lineStart, lineEnd)); + if (!match) { + lineStart = lineEnd + 1; + continue; + } + const delimiter = match[1]!; + return { + index: delimiterStart, + char: delimiter[0] === '`' ? '`' : '~', + length: delimiter.length, + }; + } + lineStart = lineEnd + 1; } - let length = 0; - while (content[index + length] === char) length++; - return { index, char, length }; + return null; }; /** @@ -80,6 +100,7 @@ const findNextFence = ( const getEnclosingFence = ( content: string, indexToTest: number, + onClosed?: (startIndex: number, endIndex: number) => void, ): { startIndex: number; delimiter: string; infoString: string } | null => { let open: { char: '`' | '~'; len: number; index: number } | null = null; let searchPos = 0; @@ -89,6 +110,7 @@ const getEnclosingFence = ( if (!open) { open = { char: fence.char, len: fence.length, index: fence.index }; } else if (fence.char === open.char && fence.length >= open.len) { + onClosed?.(open.index, fence.index); open = null; } searchPos = fence.index + fence.length; @@ -103,20 +125,6 @@ const getEnclosingFence = ( }; }; -/** - * Checks if a given character index is inside a fenced code block (``` or ~~~). - * Shares its fence-matching rules with {@link getEnclosingFence}. - */ -const isIndexInsideCodeBlock = (content: string, index: number): boolean => - getEnclosingFence(content, index) !== null; - -/** - * Finds the starting index of the code block (``` or ~~~) that encloses the - * given index. Returns -1 if the index is not inside a code block. - */ -const findEnclosingCodeBlockStart = (content: string, index: number): number => - getEnclosingFence(content, index)?.startIndex ?? -1; - /** * When `index` sits inside an open fenced code block, returns that block's * language and gutter start line; otherwise null. Lets the streaming commit @@ -145,14 +153,27 @@ export const findLastSafeSplitPoint = ( return content.length; } - const enclosingBlockStart = findEnclosingCodeBlockStart(content, searchEnd); - if (enclosingBlockStart !== -1) { + const closedBlocks: Array<{ start: number; end: number }> = []; + const enclosingFence = getEnclosingFence(content, searchEnd, (start, end) => { + closedBlocks.push({ start, end }); + }); + if (enclosingFence) { // The end of the content is contained in a code block. Split right before. - return hasLengthCap && enclosingBlockStart === 0 + return hasLengthCap && enclosingFence.startIndex === 0 ? searchEnd - : enclosingBlockStart; + : enclosingFence.startIndex; } + // Candidates move backwards, so each closed block is visited at most once + // rather than re-scanning all fences for every rejected newline. + let blockIndex = closedBlocks.length - 1; + const isInsideClosedBlock = (index: number): boolean => { + while (blockIndex >= 0 && index <= closedBlocks[blockIndex]!.start) { + blockIndex--; + } + return blockIndex >= 0 && index <= closedBlocks[blockIndex]!.end; + }; + // Search for the last double newline (\n\n) not in a code block. let searchStartIndex = searchEnd; while (searchStartIndex >= 0) { @@ -165,7 +186,7 @@ export const findLastSafeSplitPoint = ( const potentialSplitPoint = dnlIndex + 2; if ( potentialSplitPoint <= searchEnd && - !isIndexInsideCodeBlock(content, potentialSplitPoint) + !isInsideClosedBlock(potentialSplitPoint) ) { return potentialSplitPoint; } @@ -176,6 +197,7 @@ export const findLastSafeSplitPoint = ( } if (hasLengthCap) { + blockIndex = closedBlocks.length - 1; searchStartIndex = searchEnd; while (searchStartIndex >= 0) { const nlIndex = content.lastIndexOf('\n', searchStartIndex); @@ -186,7 +208,7 @@ export const findLastSafeSplitPoint = ( const potentialSplitPoint = nlIndex + 1; if ( potentialSplitPoint <= searchEnd && - !isIndexInsideCodeBlock(content, potentialSplitPoint) + !isInsideClosedBlock(potentialSplitPoint) ) { return potentialSplitPoint; }