Skip to content

fix(web-shell): preserve separators in tool description paths - #11977

Merged
ytahdn merged 1 commit into
QwenLM:mainfrom
ytahdn:codex/fix-web-shell-tool-path-separators
Sep 16, 2026
Merged

ytahdn merged 1 commit into
QwenLM:mainfrom
ytahdn:codex/fix-web-shell-tool-path-separators

Conversation

@ytahdn

@ytahdn ytahdn commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Preserves directory separators in Web Shell tool summaries by matching embedded absolute paths only at a token boundary. Relative paths, shortened paths, glob patterns, and URLs retain their original text; supported absolute paths still use the existing display shortening.

Why it's needed

A relative title such as packages/web-shell/client/components/MessageList.dom.test.tsx (lines 277-298) was displayed as packagesMessageList.dom.test.tsx (lines 277-298). The formatter treated the suffix beginning at the first slash as an absolute path and replaced it with its basename, joining the remaining directory prefix directly to the filename.

Reviewer Test Plan

How to verify

  1. In Web Shell, ask the agent to read lines 161–200 of packages/web-shell/client/adapters/messageTypes.ts and lines 277–298 of packages/web-shell/client/components/MessageList.dom.test.tsx.
  2. Expand the completed tool group. Both summaries should retain slash-separated paths and their line ranges.
  3. Expand a read result. File contents should remain accessible. Relative write/search titles, glob patterns, and URLs should retain their separators; absolute paths should retain the existing workspace-relative/basename display behavior.

Local validation: 172 targeted formatter and tool-row tests passed, including eight regression cases that failed before the fix. Full build, bundle, typecheck, targeted ESLint, and Prettier checks passed.

Evidence (Before & After)

Before After
Before: directory prefix joins the filename After: file summaries retain slash-separated paths

Before: global qwen 0.22.0, real daemon and file tools. The second file was read successfully but its summary lost the directory separators. The first request used a nonexistent types/messageTypes.ts path and failed; that row demonstrates only the display defect.

After: local bundle at ef69048fd67a5d919685cee488b0d11f9871a27b, real daemon and file tools. The first request was corrected to adapters/messageTypes.ts; both reads succeeded, summaries retained separators and line ranges, and expanding the second result showed the requested TSX content. No mock daemon was used. Screenshots were visually inspected. The baseline used the in-app browser; verification used Chrome because the in-app browser became unavailable.

After: expanded read result remains accessible

Images are stored on a separate asset branch in ytahdn/qwen-code, linked by immutable commit, and are absent from the implementation branch.

Tested on

OS Status
🍏 macOS ✅ Tested
🪟 Windows ⚠️ Not tested
🐧 Linux ⚠️ Not tested

Environment (optional)

macOS, Node.js 22.14.0; isolated runtime directories for the global baseline and local node dist/cli.js serve --port 0 --web --safe-mode verification.

Risk & Scope

  • Main risk or tradeoff: embedded absolute-path shortening now requires a token boundary; focused tests cover whitespace, quotes, parentheses, and Windows drive prefixes.
  • Not validated / out of scope: native Windows/Linux browser runs and existing formatting behavior outside this separator defect.
  • Breaking changes / migration notes: none; tool arguments and filesystem operations are unchanged.

Linked Issues

Reported and reproduced from a user screenshot; no linked GitHub issue.

中文说明

此 PR 的改动

仅在标记边界处匹配标题中嵌入的绝对路径,保留 Web Shell 工具摘要中的目录分隔符。相对路径、已缩写路径、通配符和 URL 保持原文;受支持的绝对路径继续沿用现有的显示缩短规则。

为什么需要

类似 packages/web-shell/client/components/MessageList.dom.test.tsx (lines 277-298) 的相对路径标题,此前会显示成 packagesMessageList.dom.test.tsx (lines 277-298)。格式化逻辑将第一个斜杠开始的后缀误认为绝对路径,并替换为文件名,导致剩余目录前缀直接与文件名粘连。

审查者测试计划

如何验证

  1. 在 Web Shell 中,让 agent 读取 packages/web-shell/client/adapters/messageTypes.ts 的第 161–200 行,以及 packages/web-shell/client/components/MessageList.dom.test.tsx 的第 277–298 行。
  2. 展开已完成的工具组。两条摘要应保留斜杠分隔的路径和行号范围。
  3. 展开读取结果。文件内容应仍可查看。相对路径的写入/搜索标题、通配符和 URL 应保留分隔符;绝对路径应保持现有的工作区相对路径或文件名显示规则。

本地验证:172 个格式化及工具行相关测试通过,其中 8 个回归用例在修复前失败。全量 build、bundle、typecheck,以及定向 ESLint 和 Prettier 检查均通过。

证据(修复前后)

修复前 修复后
修复前:目录前缀与文件名粘连 修复后:文件摘要保留斜杠分隔的路径

修复前:全局 qwen 0.22.0,使用真实 daemon 和文件工具。第二个文件读取成功,但摘要丢失目录分隔符。第一条请求使用了不存在的 types/messageTypes.ts 路径,读取失败;该行仅用于展示显示问题。

修复后:提交 ef69048fd67a5d919685cee488b0d11f9871a27b 的本地 bundle,使用真实 daemon 和文件工具。第一条请求改为正确的 adapters/messageTypes.ts;两次读取均成功,摘要保留分隔符和行号范围,展开第二条结果可查看所请求的 TSX 内容。未使用模拟 daemon,截图已目视检查。基线使用应用内浏览器;验证时因应用内浏览器不可用而改用 Chrome。

修复后:展开读取结果仍可查看内容

截图存放于 ytahdn/qwen-code 的独立素材分支,通过固定 commit 链接引用,不进入实现分支。

测试平台

OS 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

macOS、Node.js 22.14.0;全局基线和本地 node dist/cli.js serve --port 0 --web --safe-mode 验证使用各自独立的运行时目录。

风险与范围

  • 主要风险或取舍:标题内嵌绝对路径的缩短现在要求匹配标记边界;定向测试覆盖空白、引号、括号和 Windows 盘符前缀。
  • 未验证或范围外:Windows/Linux 原生浏览器运行,以及本次分隔符问题之外的既有格式化行为。
  • 破坏性变更或迁移说明:无;工具参数和文件系统操作不变。

关联 Issue

来自用户截图报告并已复现,未关联 GitHub issue。

@ytahdn

ytahdn commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

E2E verification report / E2E 验证报告

Result: VERIFIED_FIXED at ef69048fd67a5d919685cee488b0d11f9871a27b on macOS. Real daemon/model/file reads; no mock daemon.

  • Before: global qwen 0.22.0 displayed the successfully read second file as packagesMessageList.dom.test.tsx (lines 277-298). The first request had an incorrect types/messageTypes.ts path and failed; it is evidence of the display defect only.
  • After: local node dist/cli.js serve --port 0 --web --safe-mode; the first request was corrected to adapters/messageTypes.ts. Both reads succeeded. Their summaries preserved all directory separators and line ranges; expanding the second result displayed the requested TSX content.
  • Evidence: before/after and expanded-result screenshots are embedded in the PR body, visually inspected, and stored separately on the fork's asset branch with immutable commit links. The baseline used the in-app browser; verification used Chrome because the in-app browser became unavailable.
  • Checks: 172 targeted formatter/tool-row tests passed, including eight cases that failed before the fix. Full build, bundle, typecheck, targeted ESLint, and Prettier passed. Windows/Linux browser verification was not performed.

结果:VERIFIED_FIXED。在 macOS 上验证提交 ef69048fd67a5d919685cee488b0d11f9871a27b,使用真实 daemon、模型和文件读取,未使用模拟 daemon。

  • **修复前:**全局 qwen 0.22.0 将成功读取的第二个文件显示成 packagesMessageList.dom.test.tsx (lines 277-298)。第一条请求使用错误的 types/messageTypes.ts 路径而失败,仅作为显示缺陷的证据。
  • **修复后:**使用本地 node dist/cli.js serve --port 0 --web --safe-mode,将第一条请求更正为 adapters/messageTypes.ts。两次读取均成功,摘要保留全部目录分隔符和行号范围;展开第二条结果可查看所请求的 TSX 内容。
  • **证据:**PR 正文已嵌入修复前后和展开结果截图,均已目视检查;图片独立存放于 fork 的素材分支,使用固定 commit 链接。基线使用应用内浏览器,验证时因应用内浏览器不可用而改用 Chrome。
  • **检查:**172 个格式化及工具行测试通过,其中 8 个用例在修复前失败。全量 build、bundle、typecheck、定向 ESLint 和 Prettier 均通过。未进行 Windows/Linux 浏览器验证。

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — this is a real bug, and I could confirm it without running anything.

Template looks good ✓

Problem: observed, not theoretical. I traced the chain statically on the base commit: read-file.ts's getDescription() returns shortenPath(makeRelative(...)) plus a (lines N-M) suffix, so a longish workspace-relative path reaches the web-shell as something like packages/.../MessageList.dom.test.tsx (lines 277-298). formatDescriptionPaths then scans for embedded absolute paths with (?:[A-Za-z]:)?\/[^\s'")]+, which latches onto the / right after packages — mid-token — replaces that suffix with its basename, and glues the result to the leftover prefix: packagesMessageList.dom.test.tsx (lines 277-298). Your screenshots match that derivation exactly. The same trap also catches ~/… and URLs — https://example.com/docs/index.html used to render as httpindex.html — so the blast radius is wider than the single title you reported, which is a point in the PR's favour.

Direction: right layer. Core is producing a correct relative path; the defect is the web-shell formatter treating a mid-token / as the start of an absolute path. Fixing it in read-file.ts instead would have regressed CLI display for no reason. No CHANGELOG reference — this is an unreleased display defect, so there wouldn't be one.

Size: not applicable — packages/web-shell/client/** is not a core path. For the record: 6 production lines (+4/-2) across one source file, plus 31 test lines. No unrelated edits.

Approach: this is what I'd have written. Before reading the diff my own proposal was "require a token boundary before an embedded absolute path", which is exactly the (^|[\s'"(]) prefix group. I also considered a negative lookbehind and would have rejected it for what I assume is your reason too — a capture group is plain ES5 and carries no engine-support question at all, whereas an unsupported lookbehind is a SyntaxError when the regex literal is parsed, not a graceful degradation. Using a deliberate allowlist of boundary characters rather than "not preceded by a word character" is the right call as well: : has to stay outside the set or URLs break again.

Risk: no elevated risk signals — neither changed file matches the revert-correlated path list. Outside this test file, getToolDescription/getToolSummaryDescription have exactly one consumer (ToolGroup.tsx, display-only; the string is never parsed back into a path), so the surface is bounded.

One thing to think about, not a blocker: the boundary set is whitespace, ', ", (. A path wrapped in backticks or square brackets, or following a bare :, will now keep its full absolute form instead of shortening. That failure mode is benign — verbose, never corrupted — and widening the set would put URLs back at risk, so I'd leave it unless a real title shape turns up.

Moving on to code review. 🔍

中文说明

感谢贡献——这是一个真实存在的 bug,我不需要运行任何代码就能确认。

模板完整 ✓

**问题:**已观测到,不是理论性加固。我在 base commit 上静态追踪了整条链路:read-file.tsgetDescription() 返回 shortenPath(makeRelative(...)) 加上 (lines N-M) 后缀,因此一个较长的工作区相对路径传到 web-shell 时形如 packages/.../MessageList.dom.test.tsx (lines 277-298)formatDescriptionPaths(?:[A-Za-z]:)?\/[^\s'")]+ 扫描内嵌绝对路径,会在 packages 后面的那个 / 处——也就是 token 中间——命中,把该后缀替换成 basename,再与残留前缀粘连,得到 packagesMessageList.dom.test.tsx (lines 277-298)。你的截图与这个推导完全一致。同一个陷阱也会影响 ~/… 和 URL——https://example.com/docs/index.html 以前会显示成 httpindex.html——所以影响面比你报告的那一个标题更宽,这反而是本 PR 的加分项。

**方向:**修在了正确的层。core 产出的相对路径本身是对的,缺陷在于 web-shell 的格式化逻辑把 token 中间的 / 当成绝对路径的开头。改在 read-file.ts 反而会无谓地破坏 CLI 的显示。CHANGELOG 没有相关记录——这是尚未发布的显示缺陷,本来也不会有。

**规模:**不适用——packages/web-shell/client/** 不是核心路径。记录一下:一个源文件共 6 行生产代码(+4/-2),外加 31 行测试,没有夹带无关改动。

**方案:**和我自己的写法一致。在读 diff 之前我的独立提案就是"内嵌绝对路径必须出现在 token 边界处",也就是这个 (^|[\s'"(]) 前缀分组。我也考虑过负向后顾断言并会放弃它,理由想必与你相同——捕获组是纯 ES5,完全不涉及引擎支持问题;而不被支持的后顾断言会在解析正则字面量时直接抛 SyntaxError,并不是优雅降级。刻意采用边界字符白名单而不是"前面不是单词字符"同样是对的:: 必须留在集合之外,否则 URL 会重新出问题。

**风险:**无升级风险信号——两个改动文件都不在 revert 相关路径列表内。除本测试文件外,getToolDescription/getToolSummaryDescription 只有一个消费者(ToolGroup.tsx,仅用于显示;该字符串不会被反向解析成路径),所以影响面是有界的。

有一点可以想想,但不是阻塞项:边界集合是空白字符、'"(。用反引号或方括号包裹的路径,或者紧跟在裸 : 之后的路径,现在会保留完整绝对形式而不再缩短。这种失败模式是良性的——只是变长,不会损坏——而扩大集合会让 URL 重新受影响,所以除非出现真实的标题形态,我倾向于保持现状。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Code review

No blockers. What I actually checked, rather than assumed:

I hand-traced all twelve new expectations through getDescriptionFromTitleformatDescriptionPaths → the new regex → pathForDisplay, reading the base-commit source in a review worktree. All twelve hold, including the four that pin existing behaviour ('…', "…", (…), C:/…). The eight "preserves separators" cases are load-bearing, not decorative — they genuinely fail on the old regex. I confirmed the URL one independently: https://example.com/docs/index.html used to match at s:, because the optional [A-Za-z]: group happily eats the tail of https, then basename to index.html and render as httpindex.html. Same class of corruption as the ~/… case, just with more characters lost.

I also re-traced the pre-existing title tests that route through the same function, since adding a boundary requirement is exactly the kind of change that silently stops shortening something an older test depends on. ReadFile: README.md and ReadFile /workspace/project/README.md both short-circuit on the isAbsoluteLikePath(trimmed) early return and never reach the regex. WriteFile: Writing to /workspace/project/src/index.ts still finds its boundary at the space and still yields Writing to src/index.ts. The Shell: dataworks-infra … title contains no / at all. None of them change, which is why the existing suite shouldn't move.

Downstream surface is bounded and I named every consumer: outside this test file, getToolDescription / getToolSummaryDescription are called only from ToolGroup.tsx (three sites), purely to render a label. The description is never parsed back into a path or used for a file operation, so a display change can't desync anything. The getToolDescription in packages/core/src/agents/runtime/agent-core.ts is an unrelated method sharing a name, not a consumer.

Two mechanical things that could have bitten and don't: _match is fine under this repo's ESLint, because @typescript-eslint/no-unused-vars carries argsIgnorePattern: '^_' in every block that applies here, and annotating prefix / filePath as string type-checks against the ...args: any[] replacer signature. The regex stays linear — one zero-width-or-single-char alternation, one optional drive prefix, one greedy class, no nested quantifiers — so there's no ReDoS surface even though MAX_DESCRIPTION_LENGTH is applied only after formatting.

One non-blocking note. The C:/workspace/project/src/index.tsindex.ts case pins a lossy outcome that predates this PR: a Windows absolute path outside the workspace cwd collapses to a bare basename in pathForDisplay, dropping all directory context. Correct to pin as today's behaviour, but worth knowing that the test now has to be edited deliberately if anyone later improves pathForDisplay to keep a relative tail there. The boundary-set question from Stage 1 still stands as the only open design point; I won't repeat it.

Test evidence

This was an unattended CI run (GITHUB_EVENT_NAME=pull_request_target), so I built nothing and executed nothing from this PR — no checkout, no test run, no npm. Everything above is static: base-commit source read in a review worktree, plus the diff from the API. The only test evidence is the PR's own CI, quoted below. Not verified by me: any runtime or rendered behaviour. The before/after screenshots and the "172 targeted tests, eight failing before the fix" figures are the author's claim, not my measurement — I could confirm the eight would fail by tracing the old regex, which is not the same as watching them fail.

At the time of writing there is nothing red to excerpt. Three substantive checks are still running; two workflow runs with event == "pull_request" remain in flight.

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

Check Conclusion
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ 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,失败项排在最前。

The skipped macOS/Windows Test jobs are this repo's normal PR matrix, not a gap this PR opened, and for this specific change they wouldn't have added signal anyway: the formatter is pure string manipulation with no I/O and no platform API, and its one platform-specific branch — the [A-Za-z]: drive prefix — is pinned by a unit test that does run on the ubuntu job.

Sandboxed verification, if a maintainer wants it: @qwen-code /verify would settle the load-bearing part — that the eight regression cases actually fail on the base build and pass on this head. I traced that they should, but a trace is an argument, not an A/B measurement, and this PR's suite would still go green with the fix reverted if those eight cases were weaker than they look. The residual claim no test here covers is the rendered Web Shell row in a non-macOS browser: the author verified manually on macOS only and states Windows/Linux browser runs were not performed. The Capture web-shell visuals job above covers some of that ground on ubuntu. The author has write access, so both lanes are directly available.

中文说明

代码审查

没有阻塞项。以下是我实际核对过的内容,而不是想当然:

我在审查 worktree 中读 base commit 源码,手工推演了全部 12 条新断言的完整路径:getDescriptionFromTitleformatDescriptionPaths → 新正则 → pathForDisplay。12 条全部成立,包括 4 条用于固定既有行为的用例('…'"…"(…)C:/…)。8 条"保留分隔符"用例是真正有承载力的,不是装饰——它们在旧正则下确实会失败。URL 那条我独立验证过:https://example.com/docs/index.html 以前会在 s: 处命中,因为可选的 [A-Za-z]: 分组会吃掉 https 的尾部,随后取 basename 得到 index.html,最终显示成 httpindex.html。这与 ~/… 属于同一类损坏,只是丢掉的字符更多。

我也重新推演了走同一函数的既有标题用例,因为"增加边界要求"正是那种会悄悄让某个老断言不再被缩短的改动。ReadFile: README.mdReadFile /workspace/project/README.md 都在 isAbsoluteLikePath(trimmed) 提前返回处短路,根本到不了正则;WriteFile: Writing to /workspace/project/src/index.ts 仍在空格处找到边界,仍得到 Writing to src/index.tsShell: dataworks-infra … 标题里完全没有 /。三者都不变,这也是既有测试不应有变动的原因。

下游影响面是有界的,且我点名了每一个消费者:除本测试文件外,getToolDescription / getToolSummaryDescription 只被 ToolGroup.tsx 调用(三处),纯粹用于渲染标签。该描述不会被反向解析成路径,也不参与任何文件操作,所以显示层的改动不会让下游失同步。packages/core/src/agents/runtime/agent-core.ts 里的 getToolDescription 只是同名的无关方法,不是消费者。

两个本来可能出问题但没有的机械细节:_match 在本仓库 ESLint 下没问题,因为适用于此处的每个配置块都带了 argsIgnorePattern: '^_';把 prefix / filePath 标注为 string 也能通过 replacer 的 ...args: any[] 签名。正则保持线性——一个零宽或单字符的选择分支、一个可选盘符前缀、一个贪婪字符类,没有嵌套量词——因此即使 MAX_DESCRIPTION_LENGTH 是在格式化之后才生效,也不存在 ReDoS 面。

一条非阻塞提示。C:/workspace/project/src/index.tsindex.ts 这条固定的其实是一个早于本 PR 的有损行为:工作区 cwd 之外的 Windows 绝对路径在 pathForDisplay 里会塌缩成裸文件名,丢掉全部目录上下文。把它作为当前行为固定下来是对的,但要知道:如果以后有人改进 pathForDisplay、在那里保留相对尾部,就必须有意识地修改这条测试。Stage 1 提出的边界集合问题仍是唯一悬而未决的设计点,此处不再重复。

测试证据

本次是无人值守的 CI 运行(GITHUB_EVENT_NAME=pull_request_target),因此我没有构建、也没有执行本 PR 的任何内容——没有 checkout,没有跑测试,没有 npm。以上全部是静态结论:在审查 worktree 中读 base commit 源码,加上通过 API 取得的 diff。唯一的测试证据就是下面引用的 PR 自身 CI。**未经我验证:**任何运行时或渲染层行为。修复前后的截图以及"172 个定向测试、其中 8 个在修复前失败"这些数字是作者的陈述,不是我的实测——我能通过推演旧正则确认那 8 条应当失败,但这与亲眼看到它们失败不是一回事。

撰写时没有任何红色检查可供摘录。三项实质检查仍在运行;event == "pull_request" 的工作流还有 2 个未完成。

上表中 macOS/Windows 的 Test 作业被跳过,是本仓库 PR 矩阵的常态,不是本 PR 造成的缺口;对这个具体改动而言它们本来也提供不了额外信号:格式化逻辑是纯字符串处理,没有 I/O、不依赖平台 API,而其中唯一的平台相关分支——[A-Za-z]: 盘符前缀——已由一条确实会在 ubuntu 作业中运行的单测固定。

如果维护者希望做沙箱验证:@qwen-code /verify 可以解决最有承载力的那部分——即那 8 条回归用例在 base 构建上确实失败、在本 head 上确实通过。我推演过它们应当如此,但推演是论证,不是 A/B 实测;而且如果那 8 条用例没有看上去那么强,本 PR 的测试套件在把修复回滚后依然会全绿。此处没有任何测试覆盖的残余主张,是 Web Shell 行在非 macOS 浏览器中的实际渲染:作者仅在 macOS 上手工验证,并说明未做 Windows/Linux 浏览器验证。上表中的 Capture web-shell visuals 作业在 ubuntu 上覆盖了其中一部分。作者具备 write 权限,因此两条通道都可以直接使用。

Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head ef69048. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

No screenshot changes against the PR base.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — smallest possible fix for a defect I reproduced from source independently of the PR, with tests that genuinely fail without it.

Stepping back. My independent proposal before reading the diff was "require a token boundary before an embedded absolute path", and that is literally what this does — so there is no simpler path I found that it missed, which is the usual reason I hold a PR back. Four production lines, one test file, no drive-by edits, no formatting churn, nothing unrelated riding along. In six months the tests are what will make this maintainable: they spell out the boundary contract (whitespace, quotes, parens, start-of-string) and pin the ~/… and URL cases, which are exactly the ones that would silently regress if someone later "simplifies" the regex back.

I went looking for reasons to say no and did not find one. I re-traced the pre-existing title tests through the new regex specifically because a boundary requirement is the kind of change that quietly stops shortening something an older assertion depends on; none of them move. I named every consumer of the two exported functions and confirmed the string is display-only and never parsed back into a path, so nothing downstream can desync. The problem itself I confirmed without the author's help — read-file.ts's getDescription() emits shortenPath(makeRelative(...)) plus a line-range suffix, and the old regex basenames everything from the first mid-token /, so any read of a file more than ~80 characters deep rendered as a corrupted glue-together of prefix and filename. That is not a theoretical concern, it happens on ordinary use, and the same regex was mangling URLs into httpindex.html.

The two notes I left in Stage 2 are not defects in this diff and I do not want them read as hesitation: the boundary set omits backticks, brackets and bare colons by necessity, because admitting : would put URLs back at risk, and the failure mode of an omission is a longer string, never a corrupted one; the Windows drive case pins lossy behaviour that lives in pathForDisplay and predates this PR. If either ever matters, it matters as a follow-up, not as a change to this PR.

The one honest limit on my confidence is that this was a static review on an unattended CI run: I built nothing and ran nothing, so I have not watched the eight regression cases fail on base and pass on head. I traced that they must, and the author's reported numbers match my derivation exactly, but a trace is an argument rather than a measurement — Stage 2 names the lane that would convert it into one.

Approval is deferred until CI lands green on ef69048fd67a5d919685cee488b0d11f9871a27b. One workflow run with event == "pull_request" was still in flight when I wrote this (Test (ubuntu-latest, Node 22.x), Lint & Static, and Capture web-shell visuals had not reported), and nothing was red. Approving now would attest to a result that does not exist yet, so the finalize workflow will post the commit-pinned approval once every check on that SHA completes green, and will withhold it and flag the status comment if anything lands red or the head moves. The fork-refactor guardrail does not apply — this is a fix, and Stage 0 raised no escalation, since packages/web-shell/client/** is not a core path.

中文说明

Confidence: 5/5 —— 用尽可能小的改动修复了一个我独立于 PR、直接从源码复现出来的缺陷,且配套测试在没有这个修复时确实会失败。

退一步看整体。我在读 diff 之前的独立提案就是"内嵌绝对路径必须出现在 token 边界处",而这正是本 PR 做的事——所以我没有找到它遗漏的更简路径,而那通常是我压下一个 PR 的理由。4 行生产代码、一个测试文件,没有顺手改动,没有格式化噪声,没有任何无关内容夹带。半年后真正让这个改动可维护的是它的测试:这些测试把边界契约(空白字符、引号、括号、字符串开头)写清楚了,并固定住了 ~/… 和 URL 这两类用例——而一旦有人日后把正则"简化"回去,最先无声退化的正是它们。

我是带着"找出说不的理由"去看的,没有找到。我特意把既有的标题用例重新过了一遍新正则,因为"增加边界要求"正是那种会悄悄让某个老断言不再被缩短的改动;结果它们都不变。我点名了两个导出函数的每一个消费者,确认该字符串仅用于显示、绝不会被反向解析成路径,因此下游不可能失同步。问题本身我在没有作者帮助的情况下也确认了——read-file.tsgetDescription() 输出 shortenPath(makeRelative(...)) 加行号范围后缀,而旧正则会把从第一个 token 中间的 / 起的全部内容取 basename,于是任何路径深度超过约 80 字符的读取,都会显示成前缀与文件名粘连的损坏结果。这不是理论性问题,日常使用就会发生;同一个正则还会把 URL 弄成 httpindex.html

我在 Stage 2 留下的两条提示并不是本 diff 的缺陷,也不应被读成犹豫:边界集合不含反引号、方括号和裸冒号是必要的取舍,因为一旦纳入 :,URL 就会重新受影响,而"漏掉某个边界字符"的后果只是字符串变长,绝不会损坏;Windows 盘符那条固定的有损行为位于 pathForDisplay,早于本 PR 存在。如果这两点日后真的重要,也应当作为后续工作,而不是要求本 PR 改动。

对我这份信心唯一诚实的限制是:本次是无人值守 CI 运行下的静态审查——我没有构建、也没有运行任何东西,所以我没有亲眼看到那 8 条回归用例在 base 上失败、在 head 上通过。我推演出它们必然如此,作者报告的数字也与我的推导完全吻合,但推演是论证而不是实测——Stage 2 已点名可以把论证变成实测的那条通道。

**批准将推迟到 CI 在 ef69048fd67a5d919685cee488b0d11f9871a27b 上全绿之后。**我撰写本条时,还有 1 个 event == "pull_request" 的工作流在运行中(Test (ubuntu-latest, Node 22.x)Lint & StaticCapture web-shell visuals 尚未出结果),且没有任何红色检查。此刻批准等于为一个尚不存在的结果背书,因此 finalize 工作流会在该 SHA 上所有检查全绿后发出绑定到该 commit 的批准;如有任何检查变红或 head 发生移动,它会拒绝批准并在状态评论中标出。fork refactor 保护规则不适用于此——这是一个 fix;Stage 0 也没有触发升级,因为 packages/web-shell/client/** 不是核心路径。

Qwen Code · qwen3.8-max-2026-09-02

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

@chiga0 chiga0 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.

No blocking findings. Approval blockers: none.

Triage: Standard tier

Root cause (static trace)

The old regex (?:[A-Za-z]:)?\/[^\s'")+ had no start-boundary requirement. In packages/web-shell/.../MessageList.dom.test.tsx, the /web-shell/... suffix matched as an absolute path; pathForDisplay took its basename, and the remaining prefix packages concatenated directly to produce packagesMessageList.dom.test.tsx.

URL edge case independently verified: https://example.com/docs/index.html — old regex matched at position 4 (s: as drive prefix), basename to index.html, htt + index.html = httpindex.html. New regex requires a token boundary before s; p preceding it is not in [\s'"(], so no match. Preserved. Confirmed the bot's independent derivation.

New regex spot-checks

  • " prefix: "/workspace/project/src/a.ts" -> prefix=", path=/workspace/project/src/a.ts (stops at "), result "src/a.ts". Closing " untouched. OK.
  • ( prefix: (/workspace/project/src/a.ts) -> prefix=(, path stops at ), result (src/a.ts). OK.
  • ^ zero-width: /workspace/.../a.ts extra -> prefix="", result src/a.ts extra. OK.
  • ' is in both start boundary and stop set: '/path/file' -> prefix=', filePath=/path/file (stops at closing '), result 'basename'. OK.
  • Windows C:/... at start of string: isAbsoluteLikePath early-return handles it directly. Mid-string with space boundary also works.
  • Relative paths e.g. packages/web-shell/...: p before first / not in boundary set, no match, preserved. OK.

Test validity

All 8 "preserves separators" cases fail on the old regex (mid-token / matched) and pass on the new one. The 4 "normalizes embedded absolute path" cases verify boundary-detected paths are still shortened. Tests exercise both getToolDescription and getToolSummaryDescription through the full call chain.

Cross-check vs existing reviews

qwen-code-ci-bot stage 2/3 independently reproduced the root cause, hand-traced all 12 new expectations, confirmed no blockers, confidence 5/5. URL corruption case (httpindex.html) independently confirmed by me above.

Unreviewed dimensions

  • Windows / Linux browser runs not tested (PR acknowledges with warning)
  • Execution tier not run: pure display-layer regex change, no state, no I/O

Reviewed with AI assistance.

@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. ✅

@ytahdn
ytahdn added this pull request to the merge queue Sep 16, 2026
Merged via the queue into QwenLM:main with commit 1a980b5 Sep 16, 2026
65 of 66 checks passed
@ytahdn
ytahdn deleted the codex/fix-web-shell-tool-path-separators branch September 16, 2026 03:18
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.24.0.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants