Skip to content

fix(cli): align inline math recognition - #7701

Merged
wenshao merged 3 commits into
QwenLM:mainfrom
CubeLander:fix/cli-inline-math-recognition
Jul 25, 2026
Merged

fix(cli): align inline math recognition#7701
wenshao merged 3 commits into
QwenLM:mainfrom
CubeLander:fix/cli-inline-math-recognition

Conversation

@CubeLander

@CubeLander CubeLander commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This PR gives the CLI one bounded inline-math recognition contract across prose rendering, table rendering, source-copy selection, and pending table measurement. It recognizes single-character expressions, keeps escaped dollars and exact backtick-delimited code spans literal, and preserves the existing currency and shell-variable safeguards.

The regression coverage exercises CJK-adjacent formulas, one- and multi-backtick code spans, table cells, source-copy ordering, the 1024-character bound, raw currency text, and rendered-width measurement.

Why it's needed

The existing lookahead requires at least two content characters, so valid expressions such as $x$ and $α$ remain raw. The four consumers also handle context independently: escaped dollars can become math, while source copy can select expressions that the renderer correctly leaves inside inline code.

Keeping those paths on the same recognizer prevents the displayed output, copied TeX source, table layout, and streaming bookkeeping from disagreeing. This fixes #7699.

Reviewer Test Plan

How to verify

Enable inline math and render Values $x$、$α$。 Literal \$xy$. Code: \$xy$`. Longer code: ``a `$zz$` b``. Price: $20 and $30.` The two single-character expressions should render without delimiters; the escaped expression, both code spans, and both prices should remain literal.

Run /copy inline-latex against assistant output containing those cases followed by $x$ and $\alpha$. Only x and \alpha should participate in inline-LaTeX numbering.

Render a table containing $x$, \$xy$, a multi-backtick code span, and $P(A|B)$. Single-character math should render, literal contexts should remain source, and the pipe inside recognized math should not create another column.

Evidence (Before & After)

Case Before After
$x$ / $α$ Left raw and unavailable to inline-LaTeX copy Rendered and copied consistently
\$xy$ Treated as math Preserved as escaped source
`$xy$` Renderer kept it literal, but source copy selected xy Literal and excluded from source copy
Multi-backtick code Shorter backtick runs could expose enclosed math Only an exact delimiter run closes the code span
$20 / $30 Prose Still prose

The pre-fix behavior was reproduced with the exact current-main recognizers. The post-fix component suite exercises the real renderer, copy command, table renderer, and pending-height paths.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux

Environment (optional)

macOS 26.5.2 with Node.js v26.4.0: 6 focused test files and 364 tests passed. npm run build, npm run typecheck, npm run lint, and npm run bundle passed; the final CLI package build, typecheck, and lint also passed after the last source refinement.

Ubuntu Linux x86_64 (kernel 7.0.0-28) with Node.js v22.23.1 and npm 10.9.8: a fresh clone of commit cce61724e completed npm ci (including the full build and bundle), then passed CLI typecheck, lint, and the same 364 focused tests.

Risk & Scope

  • Main risk or tradeoff: deliberately conservative recognition leaves escaped or malformed input as source rather than guessing that it is math.
  • Not validated / out of scope: Web Shell behavior, new delimiters, broader TeX rendering, model authoring instructions, and a local Windows run.
  • Breaking changes / migration notes: none; existing $...$ support remains opt-in and currency guards remain in place.

Linked Issues

Fixes #7699

中文说明

本 PR 的改动

本 PR 为 CLI 的普通文本渲染、表格渲染、源码复制选择和流式表格测量建立一个有长度边界的统一行内公式识别契约。它可以识别单字符公式,让转义美元符号和使用精确反引号长度界定的代码段保持字面形式,同时保留现有的货币和 shell 变量防护。

回归测试覆盖 CJK 相邻公式、单反引号与多反引号代码段、表格单元格、源码复制顺序、1024 字符上限、货币文本和渲染宽度测量。

为什么需要

现有前瞻至少要求两个内容字符,因此 $x$$α$ 等有效公式会保持原始源码。四个消费路径还会分别处理上下文:转义美元符号可能变成公式,而源码复制可能选中渲染器已经正确保留在行内代码中的表达式。

让这些路径使用同一个识别器,可以避免显示输出、复制的 TeX 源码、表格布局和流式状态计算彼此不一致。本 PR 修复 #7699

Reviewer 测试计划

如何验证

启用行内公式并渲染 Values $x$、$α$。 Literal \$xy$. Code: \$xy$`. Longer code: ``a `$zz$` b``. Price: $20 and $30.`。两个单字符公式应当去掉定界符后渲染;转义表达式、两个代码段和两处价格应保持字面形式。

对包含上述情况并在末尾带有 $x$$\alpha$ 的 assistant 输出运行 /copy inline-latex。只有 x\alpha 应参与行内 LaTeX 编号。

渲染一个包含 $x$\$xy$、多反引号代码段和 $P(A|B)$ 的表格。单字符公式应正常渲染,字面上下文应保留源码,已识别公式内的竖线不应产生新列。

证据(修复前后)

情况 修复前 修复后
$x$ / $α$ 保持源码且无法通过行内 LaTeX 命令复制 渲染与复制行为一致
\$xy$ 被当作公式 保留转义源码
`$xy$` 渲染器保留字面形式,但源码复制会选中 xy 保持字面形式且不参与源码复制
多反引号代码 较短的反引号 run 可能暴露内部公式 只有长度完全相同的定界符 run 才会关闭代码段
$20 / $30 普通文本 仍为普通文本

修复前行为通过当前 main 的原始识别器精确复现。修复后的 component suite 覆盖真实的渲染器、复制命令、表格渲染器和 pending-height 路径。

已测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux

环境(可选)

macOS 26.5.2,Node.js v26.4.0:6 个 focused 测试文件、364 个测试全部通过。npm run buildnpm run typechecknpm run lintnpm run bundle 全部通过;最后一次源码收紧后,CLI package 的 build、typecheck 和 lint 也再次通过。

Ubuntu Linux x86_64(kernel 7.0.0-28),Node.js v22.23.1、npm 10.9.8:从 commit cce61724e 全新 clone 后完成 npm ci(包括完整 build 与 bundle),随后通过 CLI typecheck、lint 和同一组 364 个 focused 测试。

风险与范围

  • 主要风险或取舍:识别规则刻意保持保守;对于转义或格式不完整的输入,优先保留源码,而不是猜测它是公式。
  • 未验证或范围外:Web Shell 行为、新定界符、更广泛的 TeX 渲染、模型 authoring instructions,以及本地 Windows 运行。
  • Breaking changes / migration notes:无;现有 $...$ 支持仍需显式启用,货币防护保持不变。

关联 Issue

Fixes #7699

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Jul 25, 2026
@CubeLander

CubeLander commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

E2E test report

Baseline reproduction

A read-only harness using the exact upstream/main recognizers reproduced the reported failures:

  • $x$ and $α$ were not recognized.
  • \$xy$ was treated as math.
  • `$xy$` remained literal in the renderer but was selected by /copy inline-latex.
  • Multi-backtick code could expose enclosed math to rendering and copy.
  • $20 and $30 remained prose.
  • Pending table splitting treated escaped \$a|b$ as math and incorrectly protected its pipe.

Post-fix verification

On macOS 26.5.2 with Node.js v26.4.0:

  • 6 focused CLI test files passed: 364/364 tests.
  • The exercised paths include the inline renderer, rendered-width measurement, /copy inline-latex, table rendering, and pending table splitting.
  • Single-character and CJK-adjacent formulas, escaped dollars, exact backtick-run boundaries, currency, shell variables, source-copy ordering, and the 1024-character bound are covered.
  • npm run build, npm run typecheck, npm run lint, and npm run bundle passed.
  • After the final code-span refinement, the CLI package build, typecheck, lint, and focused suite were rerun successfully.

On Ubuntu Linux x86_64 (kernel 7.0.0-28) with Node.js v22.23.1 and npm 10.9.8:

  • A fresh SSH-host clone verified the exact PR head, cce61724e.
  • npm ci completed successfully, including the repository's full build and bundle prepare steps.
  • CLI typecheck and lint passed.
  • The same 6 focused files passed: 364/364 tests.

Remaining gap

No fresh model/API-driven terminal capture was run for the rebased final diff because a global qwen binary is unavailable on the macOS workstation. The actual renderer, copy, table, and pending-height component paths are covered directly on macOS and Linux. Windows was not tested locally and is left to CI.

@CubeLander
CubeLander marked this pull request as ready for review July 25, 2026 03:54
Copilot AI review requested due to automatic review settings July 25, 2026 03:54
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on 6b98c18 and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— 6b98c18 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed bug with solid evidence. Issue #7699 documents the exact recognizer failures (single-char expressions rejected, escaped dollars mishandled, copy command ignoring code spans), with a per-consumer behavior table and a root-cause confirmation from a maintainer. The issue carries type/bug and welcome-pr labels. This is not theoretical hardening — the current-main regex provably requires two content characters before the closing delimiter.

Direction: aligned. This fixes a consistency bug in the existing inline-math feature (#3680), not a new direction. The maintainer's comment on #7699 explicitly invited a community PR with this exact fix direction (shared module, fixed lookahead, escaped-dollar guard, code-span awareness). CHANGELOG has no direct reference to inline math, but the area is an existing shipped feature with a confirmed regression.

Size: not applicable — no core module paths touched. All changes are in packages/cli/src/ui/ and integration-tests/.

Approach: the scope feels right. The PR does exactly what the issue and maintainer prescribed — extract one shared recognizer (inline-math.ts), fix the minimum-length lookahead, add (?<!\\) for escaped dollars, make the copy command code-span-aware, and update all four consumers. No unrelated changes, no drive-by refactors. The 259/45 line split across 11 files is reasonable for a fix that touches four consumers plus a new shared module and tests. The new commit (6b98c18) adds the missing test for the escaped closing-dollar guard that the maintainer flagged — a clean, focused addition.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的 bug,证据充分。Issue #7699 记录了识别器的具体故障(单字符表达式被拒绝、转义美元符号处理错误、copy 命令忽略代码段),附有各消费路径的行为对照表和维护者的根因确认。Issue 带有 type/bugwelcome-pr 标签。这不是理论性加固——当前 main 的正则确实要求闭合定界符前至少两个内容字符。

方向:对齐。这修复的是现有行内公式功能(#3680)的一致性 bug,不是新方向。维护者在 #7699 的评论中明确欢迎社区 PR,且修复方向完全一致(共享模块、修正前瞻、转义美元守卫、代码段感知)。

规模:不适用——未触及核心模块路径。所有改动在 packages/cli/src/ui/integration-tests/ 中。

方案:范围合理。PR 完全按照 issue 和维护者的建议执行——提取共享识别器(inline-math.ts)、修正最小长度前瞻、添加转义美元守卫、让 copy 命令感知代码段、更新全部四个消费者。无无关改动,无顺手重构。新提交(6b98c18)补充了维护者指出的转义闭合美元守卫测试——干净、聚焦的补充。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 6b98c18cedfe5898c103d246b6bdf33b2e4eddb1 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: given the issue's root-cause analysis, I would (1) create a shared inline-math.ts exporting the canonical regex source and two helpers — a findInlineMathExpressions(text) that skips code spans, and a sticky readInlineMathSpanAt(text, index) for table splitting; (2) fix the lookahead to allow single-char content; (3) add (?<!\\) to both opening and closing dollar guards; (4) replace the four copy-pasted recognizers with imports from the shared module; (5) add regression tests for each reported failure mode, including the escaped closing-dollar guard.

Comparison with the diff: the PR matches this proposal exactly. The implementation is clean and correct:

  • Regex fix — the new pattern uses [^$\n]{0,1023}[^$\s] instead of the old lookahead-then-capture that required at least two content characters. Single-char content now matches via the trailing [^$\s] alone, while the 1024-char bound and trailing-non-whitespace constraint are preserved. The (?<![\\\w$]) lookbehind correctly rejects escaped dollars, and (?<!\\) before the closing delimiter prevents a backslash-escaped dollar from acting as a closer.

  • Code-span pattern — the named-group backreference (inlineCodeFence) with boundary guards enforces exact backtick-run matching: a single-backtick delimiter cannot close a double-backtick span and vice versa. The old greedy pattern got this wrong, allowing shorter runs to prematurely close longer code spans.

  • Shared modulefindInlineMathExpressions pre-computes code-span ranges and filters math matches that fall inside them. readInlineMathSpanAt replaces the per-file sticky regex. Both are straightforward, no over-abstraction.

  • getPlainTextLength rewrite — the old version used sequential .replace() calls that could not handle multi-backtick code spans or the code/math interaction. The new version walks the combined regex and handles each match type explicitly. This is the right approach. Note: getPlainTextLength currently has no production caller (only its definition and test reference it; table column widths come from renderMarkdownToAnsi + getCachedStringWidth inside TableRenderer). The rewrite is a genuine improvement but delivers no user-visible fix today — wiring it up or removing it would be a reasonable follow-up, not a blocker.

  • All four consumers updatedInlineMarkdownRenderer.tsx, TableRenderer.tsx, copyCommand.ts, and pending-rendered-height.ts all import from the shared module. No leftover local regex copies.

  • New commit (6b98c18) — adds the test the maintainer flagged: the (?<!\\) guard before the closing $ is now covered by two assertions ($x\$ and $a b\$). This was the only untested guard in the mutation matrix (M2). Clean, focused addition.

No critical blockers or AGENTS.md violations found. The test coverage is thorough — single-char, CJK-adjacent, escaped dollars (opening and closing), single/multi-backtick code spans, currency, shell variables, bounds, table cells, and copy-command ordering.

CI Test Evidence

The PR's CI workflow (Qwen Code CI) is running on the reviewed commit. Ubuntu unit tests are still in progress; Windows and macOS test jobs were skipped (standard for fork PRs). Integration tests were also skipped.

Final CI results for 6b98c18 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Not verified: Windows and macOS behavior (CI jobs skipped for fork PRs). The author reports local testing on macOS and Linux. The maintainer has independently verified the previous head (cce61724e) end-to-end on macOS with real TUI, clipboard, and a 1,226-input cross-consumer differential. A maintainer can trigger the isolated @qwen-code /tmux job for real-scenario TUI verification of this head if desired.

中文说明

代码审查

独立方案: 根据 issue 的根因分析,我会 (1) 创建共享模块 inline-math.ts,导出规范正则源码和两个辅助函数;(2) 修正前瞻以允许单字符内容;(3) 在开头和闭合美元符号守卫中添加 (?<!\\);(4) 用共享模块替换四处复制的识别器;(5) 为每种报告的故障模式添加回归测试,包括转义闭合美元守卫。

与 diff 的对比: PR 完全匹配此方案。实现干净正确:

  • 正则修正 — 新模式用 [^$\n]{0,1023}[^$\s] 替换了旧的前瞻-捕获模式,允许单字符内容同时保留 1024 字符上限和尾部非空白约束。(?<![\\\w$]) 正确拒绝转义美元符号,(?<!\\) 防止转义闭合美元。
  • 代码段模式 — 命名组反向引用确保精确反引号长度匹配,旧模式无法正确处理。
  • 共享模块findInlineMathExpressions 预计算代码段范围并过滤其中的 math 匹配。无过度抽象。
  • getPlainTextLength 重写 — 旧版用连续 .replace() 无法处理多反引号代码段。新版遍历组合正则,显式处理每种匹配类型。注意:该函数目前没有生产调用方,重写是改进但今天不带来用户可见修复——后续接上或删除均合理,不构成阻塞。
  • 四个消费者全部更新 — 无残留的本地正则副本。
  • 新提交(6b98c18 — 补充了维护者指出的转义闭合美元守卫测试(M2 变异矩阵中唯一未覆盖的 guard)。干净、聚焦。

未发现关键阻塞问题或 AGENTS.md 违规。测试覆盖全面。

CI 测试证据

PR 的 CI 工作流正在审查提交上运行。Ubuntu 单元测试仍在进行中;Windows 和 macOS 测试作业被跳过(fork PR 的标准行为)。

未验证:Windows 和 macOS 行为(CI 作业因 fork PR 被跳过)。维护者已在 macOS 上对前一个 head(cce61724e)做了完整的端到端本地验证。维护者可触发隔离的 @qwen-code /tmux 作业进行真实场景 TUI 验证。

Qwen Code · qwen3.8-max-preview

Reviewed at 6b98c18cedfe5898c103d246b6bdf33b2e4eddb1 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — solid, well-scoped bugfix that does exactly what the issue and maintainer prescribed; the new commit closes the last untested guard. Only non-blocking notes are the skipped Windows/macOS CI (standard for fork PRs) and getPlainTextLength having no production caller yet.

This is what a good community bugfix looks like. The problem is real and well-documented (#7699, maintainer-confirmed, welcome-pr), the solution matches the prescribed direction precisely, and the implementation is clean — one shared module, four consumers updated, no leftover duplication, no scope creep. The regex work is correct: I traced single-char, escaped-dollar (both opening and closing), code-span, and currency cases through the new pattern and they all behave as intended. The getPlainTextLength rewrite is a genuine improvement over the old .replace() chain. Tests cover every reported failure mode plus edge cases the author added proactively (CJK adjacency, multi-backtick nesting, the 1024-char bound).

The new commit (6b98c18) directly addresses the maintainer's mutation-matrix finding (M2): the (?<!\\) guard before the closing $ now has two dedicated assertions. That was the only guard without test coverage, and it is now load-bearing in the test suite.

The maintainer's independent end-to-end verification of the previous head (real TUI, real clipboard, 1,226-input differential, 7-mutation matrix, performance profiling) gives high confidence that the behavioral change is correct and complete. The new commit is test-only, so that verification carries forward.

If I had to maintain this in six months, I would thank the author — the shared module means the next inline-math fix only needs to happen in one place instead of four.

Approval deferred until CI lands green on 6b98c18cedfe5898c103d246b6bdf33b2e4eddb1.

中文说明

置信度:4/5 — 扎实、范围合理的 bugfix,完全按照 issue 和维护者的建议执行;新提交补上了最后一个未测试的 guard。唯一的非阻塞备注是 Windows/macOS CI 被跳过(fork PR 的标准限制)以及 getPlainTextLength 目前没有生产调用方。

这是一个优秀的社区 bugfix 范例。问题真实且记录充分(#7699,维护者确认,welcome-pr),方案精确匹配规定方向,实现干净——一个共享模块,四个消费者更新,无残留重复,无范围蔓延。正则工作正确:我追踪了单字符、转义美元(开头和闭合)、代码段和货币用例,行为均符合预期。getPlainTextLength 重写是对旧 .replace() 链的实质性改进。测试覆盖了所有报告的故障模式及作者主动添加的边缘用例。

新提交(6b98c18)直接回应了维护者的变异矩阵发现(M2):闭合 $ 前的 (?<!\\) 守卫现在有两个专用断言。这是唯一没有测试覆盖的 guard,现在已在测试套件中生效。

维护者对前一个 head 的独立端到端验证(真实 TUI、真实剪贴板、1,226 条输入差分、7 项变异矩阵、性能分析)为行为变更的正确性和完整性提供了高度信心。新提交仅为测试,因此该验证继续有效。

审批已推迟,等待 CI 在 6b98c18cedfe5898c103d246b6bdf33b2e4eddb1 上全绿后执行。

Qwen Code · qwen3.8-max-preview

Reviewed at 6b98c18cedfe5898c103d246b6bdf33b2e4eddb1 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a shared, bounded inline-math recognizer for the CLI so that prose rendering, table rendering, /copy inline-latex, and pending table measurement agree on what counts as inline math (including single-character expressions) while keeping escaped dollars and backtick-delimited inline code literal. This addresses the inconsistency described in #7699.

Changes:

  • Added a centralized inline-math/inline-code recognition module and updated renderers to use its regex sources.
  • Unified table-splitting and streaming/pending-height math-span detection on the same span reader.
  • Updated unit + integration coverage to lock in behavior for single-char/CJK-adjacent math, escaped dollars, inline code spans (including multi-backtick), and table-cell parsing.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/cli/src/ui/utils/TableRenderer.tsx Swaps local inline-math/code patterns for shared recognizer sources in table rendering.
packages/cli/src/ui/utils/pending-rendered-height.ts Uses the shared inline-math span reader for table-row splitting during pending height estimation.
packages/cli/src/ui/utils/pending-rendered-height.test.ts Adds regression coverage to ensure escaped dollars don’t “hide” table separators.
packages/cli/src/ui/utils/MarkdownDisplay.test.tsx Verifies table math rendering stays consistent with escaped/code-span literal handling.
packages/cli/src/ui/utils/InlineMarkdownRenderer.tsx Replaces ad-hoc regexes with shared sources and normalizes math/code handling in plain-text measurement.
packages/cli/src/ui/utils/InlineMarkdownRenderer.test.tsx Adds tests for single-char/CJK-adjacent math, escaped dollars, code spans, and width measurement.
packages/cli/src/ui/utils/inline-math.ts New shared inline math + inline code span patterns and helper APIs.
packages/cli/src/ui/utils/inline-math.test.ts New tests defining the bounded recognition contract (length bound, escapes, code spans).
packages/cli/src/ui/commands/copyCommand.ts Switches /copy inline-latex selection to the shared inline-math finder (excluding code spans/escaped dollars).
packages/cli/src/ui/commands/copyCommand.test.ts Adds coverage for correct selection/numbering with single-char math and literal contexts.
integration-tests/terminal-capture/scenarios/markdown-rendering.ts Extends the markdown rendering scenario prompt to include single-char math and literal escaped/code cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +306 to 310
const cleanText = normalizedText
.replace(/\*\*(.*?)\*\*/g, '$1')
.replace(/\*(.*?)\*/g, '$1')
.replace(/_(.*?)_/g, '$1')
.replace(/~~(.*?)~~/g, '$1')

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

中文说明

已审查——无阻断问题。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。

— qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. Not reviewed: chunk 1, chunk 2 — no agent reported covering these; nobody read them. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Agent 1a: Line-by-line correctness, Agent 2: Security, Agent 3: Code quality, Agent 4: Performance & efficiency, Agent 5: Test coverage, Agent 6a: Undirected audit — attacker mindset, Agent 6b: Undirected audit — 3 AM oncall mindset, Agent 6c: Undirected audit — six-months-later maintainer, Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it. Not reviewed: reverse audit — its prompt was built, but no agent was launched with it — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries, and cannot be certified. Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified.

— qwen3.7-max via Qwen Code /review

Comment on lines +8 to 9
4. Inline math $x$ and $y = \\\\frac{-b \\\\pm \\\\sqrt{b^2 - 4ac}}{2a}$.
5. One display math block using $$ fences.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This scenario adds valuable inline-math content (items 4–5, 7), but no CI workflow or npm test command collects the terminal-capture scenarios — the only entry point is the manual capture:markdown-rendering script. If the rendering or /copy latex integration for math regresses, this scenario silently produces stale screenshots without any automated gate catching it.

Concrete cost: the three bugs this PR fixes (single-char $x$, escaped \$xy$, code-span `$xy$`) now have new scenario assertions that will never run in CI.

Consider either adding a CI step that runs capture:markdown-rendering (or the whole terminal-capture suite), or adding an assertion-based integration test for the math rendering + /copy latex flow in a vitest-collected file.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification — local real-run report

I rebuilt and re-verified this PR end-to-end on real hardware (no mocks of the code under test), comparing head cce61724e against its merge-base 451bcad99.

Verdict: the fix is real, correctly scoped, and I recommend merging. Two non-blocking notes are at the bottom.


Setup

Two isolated worktrees (PR head and merge-base), each with its own real node_modules and its own full npm run build. Contamination checks before trusting any A/B:

  • all 18 node_modules/@qwen-code/* entries realpath inside their own worktree (a first attempt borrowed a donor whose node_modules was a symlink back to another checkout — that arm was discarded and redone);
  • inlineCodeFence is present in the PR tree's dist/, absent in the base tree's.

Environment: macOS 15.7.7 (arm64), Node v22.23.1, npm 10.9.8.


1. The PR's own suite reproduces exactly

✓ src/ui/utils/inline-math.test.ts               (5 tests)
✓ src/ui/utils/pending-rendered-height.test.ts   (36 tests)
✓ src/ui/commands/copyCommand.test.ts            (38 tests)
✓ src/ui/utils/InlineMarkdownRenderer.test.tsx   (28 tests)
✓ src/ui/utils/TableRenderer.test.tsx            (71 tests)
✓ src/ui/utils/MarkdownDisplay.test.tsx          (186 tests)

Test Files  6 passed (6)      Tests  364 passed (364)

2. The new tests are load-bearing (A/B against base source)

I copied the PR's four modified test files verbatim onto the base source, leaving base's recognizers in place. 9 tests fail, and they cover all four consumers — so nothing here is a vacuous test:

Consumer Failing on base
InlineMarkdownRenderer single-char + CJK-adjacent · escaped dollars · multi-backtick code spans · longer backtick runs · width measurement
MarkdownDisplay (table) table math with escaped/code-span cells (Unix and Windows EOL)
copyCommand single-char copy, skipping escaped/code spans
pending-rendered-height escaped math consuming a table separator
Test Files  4 failed (4)    Tests  9 failed | 279 passed (288)

3. Real TUI end-to-end (built CLI in a pty)

I booted the built CLI in a pty against a mock model streaming the identical markdown sample to both trees, and captured the real terminal:

before/after in the real TUI

Four user-visible defects on main, all fixed:

main @ 451bcad99 PR @ cce61724e
$x$ / $α$ left raw rendered x / α
\$xy$ rendered as math → shown as \xy preserved as \$xy$
``a$zz$b`` inner math leaked out → `a zz b` literal a `$zz$` b
table cells same three defects inside the table correct; $P(A|B)$ renders and its pipe does not split the column

4. Real clipboard end-to-end

I then drove /copy inline-latex 1|2|3 in the live TUI and read the actual system clipboard back with pbpaste (the pre-existing clipboard was saved and restored):

clipboard before/after

This is the sharpest demonstration of the bug. On main the only three copyable "expressions" are an escaped dollar and two inline-code spans — none of which the renderer displays as math — while $x$ and $α$ are not copyable at all. On the PR the three are exactly the three the renderer draws as math, in document order.

5. Cross-consumer differential, 1,226 inputs

I drove all four consumers (real ink render, real copyCommand.action, getPlainTextLength, splitMarkdownTableRow) over 28 contract cases plus 1,200 deterministic pseudo-random strings over {$ \ a b Q space | 1 α}`, on both trees:

  • renderer↔copy disagreements: 23 → 9;
  • 46 inputs lose recognition, and 100% of them fall into the three intended buckets (20 escaped opening \$, 20 escaped closing \$, 6 code-span) — zero unintended losses;
  • 28 inputs gain recognition, all intended (single-char, code-span skip, etc.).

The 9 residual disagreements all need an unbalanced code span immediately adjacent to a bare $. Exhaustive search over every string of length ≤ 4 in {$, , a, b}gives one minimal repro, ``$$ ``: the renderer draws $$(code span + literal$) while /copy inline-latex 1yields `` . To check reachability I also ran 10 hand-written realistic sentences that mix $ `` code spans with $x$ math (`In shell `$` expands vars; in TeX $x$ is math.`, `Cost is `$5` but $x$ is math.`, …) — renderer and copy agree on all 10 on the PR, and on none of the math on base. I do not consider the residual a merge blocker.

6. Mutation matrix — 5 of 7 guards proven load-bearing

I reverted each guard in inline-math.ts individually and re-ran the 364 focused tests:

# Mutation Result
M1 drop \\ from the opening lookbehind 7 tests fail
M3 restore the 2-content-character minimum 10 tests fail
M4 revert the code-span pattern to `+.+?`+ 7 tests fail
M5 drop the code-range skip in findInlineMathExpressions 2 tests fail
M7 make readInlineMathSpanAt non-sticky (yg) 2 tests fail
M2 drop (?<!\\) before the closing $ 364/364 still pass ⚠️
M6 loosen the bound 1024 → 4096 364/364 still pass (equivalent mutant — the bound test derives its input from the exported constant, so it verifies the relationship, not the value; no action needed)

7. Performance — no complexity regression

getPlainTextLength timed on both trees. The adversarial shape is a`b `` repeated:

length base PR ratio growth per doubling
1,000 0.42 ms 0.85 ms 2.01×
2,000 1.27 ms 2.70 ms 2.12× 3.18×
4,000 4.54 ms 9.86 ms 2.17× 3.65×
8,000 17.0 ms 37.7 ms 2.21× 3.82×
16,000 65.3 ms 147 ms 2.25× 3.91×

Both trees are already ~O(n²) on this shape (base grows 3.84× per doubling, PR 3.91×) — the exact-run backtick matching adds a constant ~2.2×, not a new complexity class, so this is not a new ReDoS surface. On realistic prose there is no measurable change (139-char paragraph with math + code + prices: 82.6 µs → 79.5 µs), and even the worst 2,000-char adversarial line (2.70 ms) stays below the 6.9 ms both trees spend on an ordinary 2,000-char plain line.

8. CI-equivalent gates

  • npm run typecheck — exit 0
  • npm run lint — exit 0
  • prettier --check on all six changed source files — clean
  • full packages/cli suite: 700 files / 15,036 passed / 1 failed. The single failure is AuthDialog > drives API key provider steps from endpoint options metadata, which also fails on the base tree (base: 11 AuthDialog failures) — environmental, unrelated to this PR. File count and test count deltas (+1 file, +14 tests) match the PR exactly.
  • Merges cleanly with current origin/main (de1c31772, 10 commits ahead of this PR's base); none of those 10 commits touch any file this PR touches.

Non-blocking notes

a. (?<!\\) before the closing $ is behaviour-bearing but untested (M2). I confirmed it is not an equivalent mutant — this two-liner passes unmutated and fails with the guard removed (it starts recognising x\ as math):

it('rejects a formula whose closing dollar is escaped', () => {
  expect(findInlineMathExpressions(String.raw`A $x\$ B`)).toEqual([]);
  expect(findInlineMathExpressions(String.raw`Total $a b\$ end`)).toEqual([]);
});

Worth adding to inline-math.test.ts so the guard cannot be dropped silently later. Related, and purely informational: the same guard means a formula whose last content character is a backslash (e.g. a trailing TeX \\ line break, $a \\$) is now left as source. That matches the PR's stated conservative stance; I mention it only so it is a recorded decision.

b. getPlainTextLength currently has no production caller. git grep getPlainTextLength -- '*.ts' '*.tsx' returns only its own definition and InlineMarkdownRenderer.test.tsx; table column widths actually come from renderMarkdownToAnsi + getCachedStringWidth inside TableRenderer. The rewrite itself is a genuine improvement — I compared measured vs actually-drawn width over 12 probes and the PR agrees 12/12 where base agrees 8/12 (base under-measures **a`b`c** as 3 against a drawn width of 5, and `$xy$` as 2 against 4) — but since nothing calls it, it delivers no user-visible fix today. Either wiring it up or deleting it would be a reasonable follow-up; not something to hold this PR for.

中文说明

Maintainer 本地真实验证报告

我在本机对本 PR 做了完整的端到端复核(被测代码本身没有任何 mock),对比 head cce61724e 与其 merge-base 451bcad99

结论:问题真实存在,修复方向正确、范围得当,建议合并。 两条非阻断性说明见文末。

环境搭建

两个隔离 worktree(PR head 与 merge-base),各自拥有独立的真实 node_modules 并各自完整执行 npm run build。在信任任何 A/B 结果之前先做污染检查:

  • 全部 18 个 node_modules/@qwen-code/* 条目的 realpath 都落在各自 worktree 内部(第一次尝试借用的 donor,其 node_modules 是指回另一个 checkout 的符号链接,该组已作废并重做);
  • PR 树的 dist/ 中存在 inlineCodeFence,base 树中不存在。

环境:macOS 15.7.7(arm64)、Node v22.23.1、npm 10.9.8。

1. PR 自带测试完全复现

6 个测试文件、364 个测试全部通过,与 PR 描述一致。

2. 新增测试确实有效(对 base 源码做 A/B)

把 PR 的 4 个测试文件原样拷到 base 源码上、保留 base 的识别器,结果 9 个测试失败,且覆盖了全部四个消费路径,说明没有空转测试:

消费路径 在 base 上失败的用例
InlineMarkdownRenderer 单字符与 CJK 相邻 · 转义美元 · 多反引号代码段 · 更长反引号 run · 宽度测量
MarkdownDisplay(表格) 含转义/代码段单元格的表格公式(Unix Windows 换行各一)
copyCommand 单字符复制、跳过转义与代码段
pending-rendered-height 转义公式吞掉表格分隔符

3. 真实 TUI 端到端(pty 驱动已构建的 CLI)

用 pty 启动已构建的 CLI,接一个 mock model 向两棵树流式输出完全相同的 markdown,并截取真实终端画面(见上方第一张图)。main 上有四处用户可见缺陷,本 PR 全部修复:

main @ 451bcad99 PR @ cce61724e
$x$ / $α$ 保持源码 渲染为 x / α
\$xy$ 被当作公式 → 显示成 \xy 保留为 \$xy$
``a$zz$b`` 内部公式泄漏 → `a zz b` 保持字面 a `$zz$` b
表格单元格 表格内同样存在上述三处缺陷 正确;$P(A|B)$ 正常渲染且其竖线不会拆列

4. 真实剪贴板端到端

随后在实时 TUI 中执行 /copy inline-latex 1|2|3,再用 pbpaste 读回真实系统剪贴板(原剪贴板内容已备份并还原),见上方第二张图。

这是对该 bug 最直观的证明:在 main 上,仅有的三个可复制“表达式”分别是一个转义美元和两个行内代码段——它们都不是渲染器显示为公式的内容——而 $x$$α$ 根本无法被复制。在本 PR 上,这三个恰好就是渲染器按文档顺序显示为公式的那三个。

5. 跨消费路径差分:1,226 条输入

用四个真实消费路径(真实 ink 渲染、真实 copyCommand.actiongetPlainTextLengthsplitMarkdownTableRow)跑 28 条契约用例 + 1,200 条基于 {$ \ a b Q 空格 | 1 α}` 的确定性伪随机字符串,两棵树各跑一遍:

  • 渲染器↔复制的不一致数:23 → 9
  • 46 条输入的识别结果减少,且100% 落在三个预期分类内(20 条转义开头 \$、20 条转义结尾 \$、6 条代码段)——没有任何非预期的识别丢失
  • 28 条输入新增识别,全部符合预期。

残留的 9 条不一致都需要一个不配对的代码段紧邻裸 $。对 {$, , a, b}上长度 ≤ 4 的全部字符串做穷举,得到唯一最小复现 ``$$ ``:渲染器画出 $$(代码段 + 字面 $),而 /copy inline-latex 1得到 `` 。为评估可达性,我另外构造了 10 条把 $ `` 代码段与 $x$ 公式混排的真实语句,在 PR 上10 条全部一致。该残留不构成合并阻断。

6. 变异测试:7 个 guard 中 5 个被证明有效

逐个回退 inline-math.ts 中的 guard 并重跑 364 个测试:M1 开头 lookbehind 的 \\(7 个失败 ✅)、M3 恢复两字符下限(10 个失败 ✅)、M4 回退代码段模式(7 个失败 ✅)、M5 去掉代码段跳过逻辑(2 个失败 ✅)、M7 sticky yg(2 个失败 ✅)。M2 去掉闭合 $ 前的 (?<!\\) 时 364 个测试仍全绿 ⚠️。M6 把上限 1024 放宽到 4096 也全绿,但那是等价变异(上限测试用导出的常量构造输入,验证的是关系而非具体数值),无需处理。

7. 性能:没有复杂度回退

ab `` 重复构造的对抗输入测 getPlainTextLength`:1,000/2,000/4,000/8,000/16,000 字符下,base 为 0.42/1.27/4.54/17.0/65.3 ms,PR 为 0.85/2.70/9.86/37.7/147 ms。两棵树在该形状下本来就是约 O(n²)(base 每翻倍增长 3.84×,PR 3.91×),因此精确 run 长度匹配带来的是约 2.2× 的常数开销,而不是新的复杂度等级,不构成新的 ReDoS 面。真实文本没有可测量的变化(139 字符、含公式/代码/价格的段落:82.6 µs → 79.5 µs);即使最差的 2,000 字符对抗行(2.70 ms)也低于两棵树在普通 2,000 字符纯文本行上都要花的 6.9 ms。

8. 等价于 CI 的检查

  • npm run typecheck — 退出码 0
  • npm run lint — 退出码 0
  • 对全部 6 个改动源文件执行 prettier --check — 通过
  • packages/cli 全量测试:700 个文件 / 15,036 通过 / 1 失败。唯一失败项 AuthDialog > drives API key provider steps from endpoint options metadatabase 树上同样失败(base 共 11 个 AuthDialog 失败),属环境性问题,与本 PR 无关。文件数与用例数增量(+1 文件、+14 用例)与 PR 完全吻合。
  • 与当前 origin/mainde1c31772,比本 PR base 多 10 个提交)可干净合并;这 10 个提交没有触碰本 PR 涉及的任何文件。

非阻断性说明

a. 闭合 $ 前的 (?<!\\) 有行为作用但无测试覆盖(M2)。 我确认它不是等价变异——下面这两行在未变异时通过、去掉该 guard 后失败(会开始把 x\ 识别成公式):

it('rejects a formula whose closing dollar is escaped', () => {
  expect(findInlineMathExpressions(String.raw`A $x\$ B`)).toEqual([]);
  expect(findInlineMathExpressions(String.raw`Total $a b\$ end`)).toEqual([]);
});

建议补进 inline-math.test.ts,避免日后被静默移除。相关的纯信息性一点:同一个 guard 也意味着最后一个内容字符是反斜杠的公式(例如结尾带 TeX 换行 \\$a \\$)现在会保留为源码。这符合本 PR 声明的保守策略,此处仅作为一项已记录的决策说明。

b. getPlainTextLength 目前没有生产调用方。 git grep getPlainTextLength -- '*.ts' '*.tsx' 只返回它自身的定义和 InlineMarkdownRenderer.test.tsx;表格列宽实际来自 TableRenderer 内的 renderMarkdownToAnsi + getCachedStringWidth。这次重写本身确实是改进——我用 12 组探针对比“测量宽度 vs 实际绘制宽度”,PR 12/12 一致,base 只有 8/12(base 把 **a`b`c** 测成 3,实际绘制宽度是 5;把 `$xy$` 测成 2,实际是 4)——但由于没有调用方,它今天不会带来用户可见的修复。后续要么把它接上,要么删掉,都合理;不必为此阻塞本 PR。

Verified locally by the maintainer: two built worktrees, real pty TUI runs, real system clipboard, 1,226-input cross-consumer differential, and a 7-mutation guard matrix.

@wenshao

wenshao commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

The stage comments above were updated with the latest result. View workflow run.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

中文说明

⚠️ 已从批准降级为评论:CI still running。 已审查。

— qwen3.7-max via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 25, 2026
Merged via the queue into QwenLM:main with commit cb36659 Jul 25, 2026
62 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): align inline math recognition across render, copy, tables, and streaming

5 participants