fix(core): include full filePath alongside fileName in edit/write-file diff results - #8607
fix(core): include full filePath alongside fileName in edit/write-file diff results#8607JonathanOrr wants to merge 4 commits into
Conversation
…e diff results EditTool and WriteFileTool build their post-execution ToolResultDisplay (FileDiff) with only `fileName` (a basename via path.basename), never the full path. The ACP diff-content builders (acp-bridge's live tool-call update path and cli's session export/replay path) then use that basename as the location for the resulting `diff` content block's `path`. Editor integrations (e.g. the VSCode IDE companion) resolve a relative `path` against the workspace root, so any edited/written file outside the project root renders a file-link that opens `<workspace-root>/<basename>` instead of the real nested file, always failing with "file not found" for anything not literally at the top level. The pre-execution confirmation objects (ToolEditConfirmationDetails) already carry both `fileName` and `filePath` correctly — this only affects the result recorded after execute() runs, which is what gets persisted to session history and replayed later. Fix: add `filePath` to `FileDiff` and populate it in both tools' execute() results, and prefer it over `fileName` in both ACP diff-content extractors.
github-actions
Bot
added
the
review/self-reported
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @JonathanOrr — and for the thorough root-cause analysis in the linked issue.
Before review can proceed, the PR body needs to follow the pull request template. The current body uses free-form headings (Summary / Why / Test plan) and is missing the required sections:
## What this PR does## Why it's needed## Reviewer Test Plan— including### How to verify,### Evidence (Before & After), and the### Tested onOS table## Risk & Scope## Linked Issues- A Chinese translation of the body inside a
<details>block
The content you've written is good — most of it maps directly onto the template sections, so this is a restructure rather than a rewrite. The Tested on table is the genuinely new part: which OSes did you verify on locally?
Once the body follows the template, push a new commit or re-trigger with @qwen-code /triage and the gate will run again.
中文说明
感谢提交 PR，@JonathanOrr——也感谢你在关联 issue 中详尽的根因分析。
在进入审查之前，PR 正文需要遵循 PR 模板。当前正文使用了自由格式的标题（Summary / Why / Test plan），缺少以下必需章节：
## What this PR does## Why it's needed## Reviewer Test Plan——包含### How to verify、### Evidence (Before & After)和### Tested on操作系统表格## Risk & Scope## Linked Issues- 在
<details>块中提供正文的中文翻译
你写的内容质量很好——大部分可以直接对应到模板章节，所以这只是重新组织结构，而不是重写。真正需要补充的是 Tested on 表格：你在本地验证了哪些操作系统？
正文遵循模板后，推送新 commit 或使用 @qwen-code /triage 重新触发，门禁会再次运行。
— Qwen Code · qwen3.8-max
|
@qwen-code /triage |
No code change. The PR body was restructured to match the repo's PR template (see the qwen-code-ci-bot review), and the /triage comment retrigger doesn't apply here since it requires the commenter to have write access to this repo — pushing a commit is the permission-agnostic path to re-run the automated review gate.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Test Plan (not a blocker): src/transcript-replay.test.ts — no such file or directory; src/ui/utils/export/normalize.test.ts — no such file or directory; src/tools/edit.test.ts — no such file or directory; src/tools/write-file.test.ts — no such file or directory; some_dir/test.md — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。 Test Plan（非阻断）：src/transcript-replay.test.ts — no such file or directory; src/ui/utils/export/normalize.test.ts — no such file or directory; src/tools/edit.test.ts — no such file or directory; src/tools/write-file.test.ts — no such file or directory; some_dir/test.md — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| * to the tool. UI consumers must prefer this over `fileName` when | ||
| * resolving a clickable/openable location — `fileName` is a basename and | ||
| * cannot be used to locate files outside the workspace root. | ||
| */ | ||
| filePath?: string; |
There was a problem hiding this comment.
[Suggestion] The new filePath field is populated by only 2 of the 4 in-tree FileDiff result-display producers. The shell tool's sed-edit display (makeSedEditDisplay in packages/core/src/tools/shell.ts, returned as returnDisplay) and the notebook-edit displayResult (packages/core/src/tools/notebook-edit.ts) build the identical display shape without it, even though both hold the full path (edit.filePath — already set on the adjacent sed-edit confirmation-details path — and this.params.notebook_path). Both displays pass the extractors' 'fileName' in display && 'newContent' in display guard, so they fall through to the fileName fallback. — Failure scenario: a sed -i edit or notebook cell edit is recorded in a session; when that session is replayed through the ACP bridge or exported, both extractors fall back to the basename, so the companion diff link is again unresolvable for files outside the workspace root — the issue 8606 symptom survives for these two tool kinds.
// shell.ts — makeSedEditDisplay return object
filePath: edit.filePath,
// notebook-edit.ts — displayResult
filePath: this.params.notebook_path,
中文说明
新增的 filePath 字段只被 4 个 FileDiff 结果展示生产者中的 2 个填充。shell 工具的 sed 编辑展示（packages/core/src/tools/shell.ts 中的 makeSedEditDisplay，作为 returnDisplay 返回）与 notebook-edit 的 displayResult（packages/core/src/tools/notebook-edit.ts）构建了相同形状的展示对象却没有填充该字段，尽管两者都持有完整路径（edit.filePath——相邻的 sed 编辑确认信息路径中已经设置——以及 this.params.notebook_path）。这两个展示都能通过提取器的 'fileName' in display && 'newContent' in display 形状守卫，因此会走 fileName 回退分支。——失败场景：通过 sed -i 命令或 notebook 单元格编辑修改的文件被记录到会话中；当该会话经 ACP 桥回放或被导出时，两个提取器都会回退到 basename，companion 中的 diff 链接对工作区根目录之外的文件再次无法解析——issue 8606 的症状在这两种工具类型上仍然存在。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const displayResult: FileDiff = { | ||
| fileDiff, | ||
| fileName, | ||
| filePath: file_path, |
There was a problem hiding this comment.
[Suggestion] The cancelled-edit path drops this field. When the user cancels an edit at the confirmation prompt, the cancelled branch in packages/core/src/core/coreToolScheduler.ts (~L1551-1568) reconstructs a FileDiff-shaped resultDisplay from the confirmation details copying only fileDiff, fileName, originalContent, newContent — discarding confirmationDetails.filePath, which is a required field on ToolEditConfirmationDetails and is populated by every edit-type confirmation builder. The preserved display is persisted into the session record like any tool result. — Failure scenario: user cancels an edit of app/src/main/java/com/example/Foo.kt; the recorded display has no filePath, so replaying or exporting that session falls back to the basename — reproducing the issue 8606 broken link for every cancelled edit (cancelled sed-edits and notebook edits also flow through this one branch).
// coreToolScheduler.ts, cancelled branch — add alongside the copied fields:
filePath: waitingCall.confirmationDetails.filePath,
中文说明
取消编辑的路径会丢弃这个字段。当用户在确认提示处取消编辑时，packages/core/src/core/coreToolScheduler.ts（约 L1551-1568）的 cancelled 分支会从确认信息重建一个 FileDiff 形状的 resultDisplay，但只复制了 fileDiff、fileName、originalContent、newContent——丢弃了 confirmationDetails.filePath，而它是 ToolEditConfirmationDetails 上的必填字段，且所有编辑类确认构建器都会填充它。被保留的展示会像普通工具结果一样持久化到会话记录中。——失败场景：用户取消一次对 app/src/main/java/com/example/Foo.kt 的编辑；记录下来的展示没有 filePath，因此回放或导出该会话时会回退到 basename——使每一次被取消的编辑都复现 issue 8606 的失效链接（被取消的 sed 编辑和 notebook 编辑同样经过这个分支）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| : typeof resultDisplay['fileName'] === 'string' | ||
| ? resultDisplay['fileName'] | ||
| : '', |
There was a problem hiding this comment.
[Suggestion] The fileName fallback branch — the backward-compat path for old persisted sessions, which the PR description explicitly scopes in — has no test in either extractor. All new tests supply filePath; neither test file (before or after this PR) exercises the fileName-only shape, which is the shape of every session persisted before this fix and of live producers that still don't set filePath. The sibling fallback in packages/cli/src/ui/utils/export/normalize.ts has the same gap. — Failure scenario: a future change that collapses this ternary (e.g. treating filePath as always present) survives the suite; old-session replays silently get path: '' instead of the basename, reintroducing the broken-link class for all pre-fix session data.
// one fileName-only record per extractor, e.g.:
resultDisplay: {
fileDiff: '--- a\n+++ b\n',
fileName: 'Foo.kt',
originalContent: 'old',
newContent: 'new',
},
// → expect path: 'Foo.kt'
中文说明
fileName 回退分支——即面向旧的已持久化会话的向后兼容路径，PR 描述中明确将其纳入修复范围——在两个提取器中都没有测试。所有新增测试都提供了 filePath；两个测试文件（无论本 PR 之前还是之后）都没有覆盖仅有 fileName 的形状，而这正是本次修复之前所有已持久化会话的形状，也是目前仍不设置 filePath 的在线生产者的形状。packages/cli/src/ui/utils/export/normalize.ts 中的姊妹回退分支存在同样的缺口。——失败场景：未来某个折叠该三元表达式的改动（例如认为 filePath 总是存在）不会被测试套件拦截；旧会话回放会悄悄得到 path: '' 而非 basename，使失效链接问题在所有修复前的会话数据上复现。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| path: | ||
| typeof display['filePath'] === 'string' | ||
| ? display['filePath'] | ||
| : (display['fileName'] as string), |
There was a problem hiding this comment.
[Suggestion] This "prefer filePath, fall back to fileName" resolution is hand-inlined into two near-identical extractDiffContent twins (here and in packages/acp-bridge/src/transcript-replay.ts) that already diverge in guard style: the acp-bridge copy uses typeof … === 'string' guards on both fields with a '' terminal fallback, while this copy uses an unguarded (display['fileName'] as string) cast. A third untyped copy (integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js) still emits basename-only paths. Both packages depend on @qwen-code/qwen-code-core, so a shared resolver is reachable (as a small Node-free core subpath module, since transcript-replay.ts deliberately avoids barrel imports to keep the browser bundle Node-free). — Concrete cost: any future change to the resolution rule must touch three files; missing one silently re-creates the issue 8606 symptom in that consumer — the concurrent-runner copy is already out of date at this commit.
// e.g. in a narrow Node-free core subpath module:
export function resolveFileDiffPath(display) { … }
// called from both extractors instead of the inlined ternaries
中文说明
这个"优先 filePath、回退 fileName"的解析逻辑被手工内联到了两个几乎相同的 extractDiffContent 孪生函数中（此处与 packages/acp-bridge/src/transcript-replay.ts），且两者的守卫风格已经出现分歧：acp-bridge 的副本对两个字段都使用 typeof … === 'string' 守卫并以 '' 作为最终回退，而此处的副本使用无守卫的 (display['fileName'] as string) 强转。第三个未类型化的副本（integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js）仍然只输出 basename 路径。两个包都依赖 @qwen-code/qwen-code-core，因此可以提取一个共享解析器（作为一个小的无 Node 依赖的 core 子路径模块，因为 transcript-replay.ts 为避免浏览器产物引入 Node 依赖而刻意不使用 barrel 导入）。——具体代价：未来任何对解析规则的修改都必须同时改三个文件；漏掉其中一个就会在该消费方悄悄复现 issue 8606 的症状——concurrent-runner 的副本在本次提交时就已经过时。
— qwen3.8-max via Qwen Code /review (v0.21.6)
…fix the cancelled-edit path Addresses review feedback on QwenLM#8607: the filePath fix only covered EditTool/WriteFileTool's post-execution display, leaving three more code paths reproducing the same broken-link symptom from QwenLM#8606. - shell.ts's makeSedEditDisplay (sed -i edits) and notebook-edit.ts's displayResult both build the identical FileDiff shape without filePath, even though both already have the full path in scope (edit.filePath, this.params.notebook_path) — same one-line fix as the original PR. - coreToolScheduler.ts's cancelled-edit branch reconstructs a FileDiff from confirmationDetails but only copied fileDiff/fileName/originalContent/ newContent, dropping the filePath already present on ToolEditConfirmationDetails. Every cancelled edit hit the basename fallback. - Adds a fileName-only fallback test to both extractDiffContent copies (acp-bridge and cli/export/normalize) — previously zero coverage existed for the shape every pre-fix session, and both still-broken producers above, actually produce. - Applies the same minimal filePath-preferred resolution to the third, untyped extractDiffContent copy in integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js, which had no filePath handling at all. Not included: extracting the three near-identical "prefer filePath, fall back to fileName" resolutions into a shared resolver, per the review's suggestion — real cleanup, but a separate scope of change (would need a new Node-free core subpath module, given transcript-replay.ts's deliberate avoidance of barrel imports for bundle size) from finishing this bug fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all four points from the last review in a follow-up commit:
Held off on extracting the three resolutions into a shared resolver — agree it's real duplication, but pulling it into a Node-free core subpath module (given All affected packages ( |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: reverse audit — stopped before round 4 by the review time budget. Not reviewed: verification and reverse audit — each ran and opened its brief, but neither was launched with the prompt the CLI built — the launches were written by hand, so the posted findings cannot be counted as verified, and what the agents were actually asked is not what this skill certifies. Test Plan (not a blocker): src/transcript-replay.test.ts — no such file or directory; src/ui/utils/export/normalize.test.ts — no such file or directory; src/tools/edit.test.ts — no such file or directory; src/tools/write-file.test.ts — no such file or directory; some_dir/test.md — no such file or directory.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：反向审计——评审时间预算不足，未能开始第 4 轮。 未审查：验证与反向审计——两者都运行并打开了各自的 brief，但都不是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，发布的发现不能算作已验证，agent 实际被要求做的也不是本 skill 所认证的内容。 Test Plan（非阻断）：src/transcript-replay.test.ts — no such file or directory; src/ui/utils/export/normalize.test.ts — no such file or directory; src/tools/edit.test.ts — no such file or directory; src/tools/write-file.test.ts — no such file or directory; some_dir/test.md — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| * to the tool. UI consumers must prefer this over `fileName` when | ||
| * resolving a clickable/openable location — `fileName` is a basename and |
There was a problem hiding this comment.
[Suggestion] R2-1: Three in-tree FileDiff path consumers still resolve basename-first and never read the new filePath field, contradicting the contract this JSDoc writes:
packages/cli/src/serve/live/live-task-service.ts:267— emitsfileChangetask items withpath: display.fileNametoqwen serveclients; the localisFileDiffguard does not namefilePath.packages/cli/src/ui/utils/export/collect.ts:170— computes export stats as(args?.['file_path'] as string) || display.fileName; shell sed-edits (argcommand) and notebook edits (argnotebook_path) carry nofile_patharg, so stats degrade to basename and thewrittenFilePathsSet collides distinct same-basename files (undercountingfilesWritten).packages/cli/src/services/insight/generators/DataProcessor.ts:1123— the insight generator doesuniqueFiles.add(diff.fileName)for its "files touched" metric.
Failure scenario: (1) A session editing a nested file emits an unresolvable basename to live clients while replay/export of the same session shows the full path — same data, different fidelity per view. (2) Exporting a session where sed-edits touched a/index.ts and b/index.ts records one writtenFilePaths entry (index.ts) instead of two. (3) Probe run: two records with the same basename but different full paths yield totalFiles = 1 in the insight metric; preferring filePath flips it to 2.
Suggested fix — prefer filePath when it is a non-empty string at all three sites:
// live-task-service.ts (extend the local isFileDiff guard accordingly)
path: display.filePath ?? display.fileName,
// collect.ts
filePath = (typeof display.filePath === 'string' && display.filePath)
|| (args?.['file_path'] as string)
|| display.fileName;
// DataProcessor.ts
uniqueFiles.add(typeof diff.filePath === 'string' && diff.filePath ? diff.filePath : diff.fileName);Deferrable to a follow-up if you want to keep this PR scoped to the two ACP extractors.
中文说明
树内还有三个 FileDiff 路径消费方仍然优先使用 basename、从不读取新的 filePath 字段，与这段 JSDoc 写下的约定相矛盾：
packages/cli/src/serve/live/live-task-service.ts:267—— 向qwen serve客户端发出的fileChange任务项使用path: display.fileName；其本地isFileDiff守卫也没有包含filePath。packages/cli/src/ui/utils/export/collect.ts:170—— 导出统计按(args?.['file_path'] as string) || display.fileName计算；shell sed 编辑（参数为command）和 notebook 编辑（参数为notebook_path）都没有file_path参数，因此统计退化为 basename，writtenFilePathsSet 会把同名不同路径的文件合并（少计filesWritten）。packages/cli/src/services/insight/generators/DataProcessor.ts:1123—— insight 生成器用uniqueFiles.add(diff.fileName)统计"触及文件数"指标。
失败场景：(1) 编辑嵌套文件的会话在实时视图中向客户端发出无法解析的 basename，而同一会话的回放/导出却显示完整路径——同样的数据，不同视图保真度不同。(2) 导出一个用 sed 编辑过 a/index.ts 和 b/index.ts 的会话时，writtenFilePaths 只记录一条（index.ts）而不是两条。(3) 实测：两条 basename 相同但完整路径不同的记录在 insight 指标中得到 totalFiles = 1；改为优先使用 filePath 后为 2。
建议修复——三处都在 filePath 为非空字符串时优先使用它（见上方代码块）。如果希望本 PR 只聚焦两个 ACP 提取器，可以推迟到后续 PR。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const displayResult = { | ||
| fileDiff, | ||
| fileName, | ||
| filePath: this.params.notebook_path, |
There was a problem hiding this comment.
[Suggestion] R2-2: The new filePath populate for notebook edits has no test coverage — none of the 23 execute() tests in notebook-edit.test.ts assert anything about returnDisplay, while the sibling edit/write-file/cancelled-edit sites all gained filePath assertions in this PR.
Failure scenario: a future refactor that drops or renames this line regresses notebook-edit ACP/export diff paths to the basename while the whole suite stays green — re-introducing, scoped to notebook edits, the exact bug class this PR fixes. Verified by probe: deleting this line leaves all 31 tests green.
Suggested fix — add one assertion in an existing success-path test (the temp-dir path is absolute and distinct from the basename):
expect((result.returnDisplay as FileDiff).filePath).toBe(filePath);中文说明
notebook 编辑新增的 filePath 填充没有测试覆盖——notebook-edit.test.ts 的 23 个 execute() 测试都没有对 returnDisplay 做任何断言，而兄弟位置（edit/write-file/取消编辑路径）在本 PR 中都加上了 filePath 断言。
失败场景：未来某次重构删除或改名这一行时，notebook 编辑的 ACP/导出 diff 路径会退回 basename，而整个测试套件仍是绿的——在 notebook 编辑范围内重新引入本 PR 所修复的这类 bug。已通过探针验证：删除这一行后全部 31 个测试仍然通过。
建议修复——在一个现有的成功路径测试中加入上方代码块中的断言（temp 目录路径是绝对路径且与 basename 不同）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| fileName: edit.fileName, | ||
| filePath: edit.filePath, |
There was a problem hiding this comment.
[Suggestion] R2-3: makeSedEditDisplay's new filePath has no test: this display is returned as returnDisplay on the successful sed-edit path (shell.ts:1968-1970), but shell.test.ts's only filePath assertion (line 533) targets the confirmation-details object — a different construction site — and the sed success-path test asserts only llmContent/trackEdit/writeTextFile.
Failure scenario: if this line is dropped or changed to edit.fileName, applied sed edits surface to ACP/VSCode consumers with a basename-only diff path — the edited file can't be opened from the diff — and no test fails.
Suggested fix — add to the 'applies a qualifying sed -i command' test:
expect((result.returnDisplay as FileDiff).filePath).toBe(expectedSedFilePath);(expectedSedFilePath is a resolved absolute path, distinct from the basename, so the assertion kills the substitution mutant.)
中文说明
makeSedEditDisplay 新增的 filePath 没有测试：该展示在 sed 编辑成功路径上作为 returnDisplay 返回（shell.ts:1968-1970），但 shell.test.ts 中唯一的 filePath 断言（第 533 行）针对的是确认详情对象——另一个构造点——而 sed 成功路径测试只断言了 llmContent/trackEdit/writeTextFile。
失败场景：如果这一行被删除或改为 edit.fileName，已应用的 sed 编辑会以仅含 basename 的 diff 路径呈现给 ACP/VSCode 消费方——编辑过的文件无法从 diff 打开——且没有任何测试失败。
建议修复——在 'applies a qualifying sed -i command' 测试中加入上方代码块中的断言（expectedSedFilePath 是解析后的绝对路径，与 basename 不同，因此该断言能杀死替换变异）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| expect(cancelledCall.response.resultDisplay.fileName).toBe('test.txt'); | ||
| expect(cancelledCall.response.resultDisplay.filePath).toBe('test.txt'); |
There was a problem hiding this comment.
[Suggestion] R2-4: This new assertion cannot discriminate the field's source: the mock confirmation details set fileName and filePath to the same 'test.txt' (~L6804-6805), so the one-line mutation filePath: waitingCall.confirmationDetails.fileName in coreToolScheduler.ts:1565 passes this test. Production confirmation details carry filePath = params.file_path (absolute) vs fileName = basename, so the mutant is observable — a weak test, not an equivalent mutant.
Failure scenario: if the cancelled-edit path is refactored to source filePath from fileName, cancelled edits ship a basename-only path to ACP consumers (losing the clickable location this PR adds) and this test stays green. Verified by probe: the mutant survives as written; distinct mock values catch it.
Suggested fix — give the mock distinct values and assert the distinct literal:
// mock confirmation details (~L6804): filePath: '/workspace/test.txt'
expect(cancelledCall.response.resultDisplay.filePath).toBe('/workspace/test.txt');中文说明
这条新断言无法区分字段的来源：mock 确认详情把 fileName 和 filePath 设成了相同的 'test.txt'（约 L6804-6805），因此 coreToolScheduler.ts:1565 处的单行变异 filePath: waitingCall.confirmationDetails.fileName 也能通过该测试。生产环境的确认详情携带 filePath = params.file_path（绝对路径）与 fileName = basename，两者不同，所以该变异是可观测的——这是弱测试，而非等价变异。
失败场景：如果取消编辑路径被重构为从 fileName 取 filePath，被取消的编辑会向 ACP 消费方发出仅含 basename 的路径（丢失本 PR 新增的可点击位置），而该测试仍是绿的。已通过探针验证：变异在当前写法下存活；使用不同的 mock 值即可捕获。
建议修复——给 mock 设置不同的值并断言该不同的字面量（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const displayPath = | ||
| typeof display.filePath === 'string' && display.filePath |
There was a problem hiding this comment.
[Suggestion] R2-5: The new filePath-over-fileName preference logic added to this exporter is unreachable by every test command: the file is a plain .js helper outside all npm workspaces, integration-tests/vitest.config.ts collects only **/*.test.ts, it is invoked only by the manual runner.py harness, and no workflow or root npm script references concurrent-runner.
Failure scenario: if the displayPath selection regresses (fallback order flipped, dedup comparing the wrong field), no unit or integration suite fails; the generated HTML report silently shows basename-only or wrongly deduplicated file locations — the exact bug this PR fixes elsewhere, reappearing here ungated.
Suggested fix: add a colocated test the integration vitest root collects (e.g. export-html-from-chatrecord-jsonl.test.js importing extractLocations/extractDiffContent and asserting filePath is preferred when present and fileName is the fallback), or extract the two pure functions into a workspace module with unit tests.
中文说明
新增到这个导出器的"优先 filePath、回退 fileName"逻辑对任何测试命令都不可达：该文件是位于所有 npm workspace 之外的纯 .js 辅助脚本，integration-tests/vitest.config.ts 只收集 **/*.test.ts，它仅由手动运行的 runner.py 调用，且没有任何 workflow 或根 npm 脚本引用 concurrent-runner。
失败场景：如果 displayPath 选择逻辑退化（回退顺序颠倒、去重比较了错误的字段），没有任何单元或集成测试会失败；生成的 HTML 报告会悄悄显示仅含 basename 或被错误去重的文件位置——本 PR 在别处修复的 bug 在这里不受任何门禁保护地复现。
建议修复：添加一个集成 vitest 根能收集的同目录测试（例如 export-html-from-chatrecord-jsonl.test.js，导入 extractLocations/extractDiffContent，断言存在 filePath 时优先使用、回退到 fileName），或把这两个纯函数提取到某个 workspace 模块中并配单元测试。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| fileDiff: string; | ||
| fileName: string; | ||
| /** | ||
| * Full (project-relative or absolute) path to the edited file, as passed |
There was a problem hiding this comment.
[Suggestion] R2-6: The JSDoc contract written here misstates the field's invariant: it says "Full (project-relative or absolute) path to the edited file, as passed to the tool", but every producer guarantees an absolute path — edit.ts/write-file.ts/notebook-edit.ts reject non-absolute paths in validateToolParamValues (e.g. edit.ts:819), and shell sed-edits resolve relative targets via resolveSedFilePath (shell.ts:1692) before the display is built, so the value is explicitly not "as passed".
Failure scenario: a future maintainer adding an edit-producing tool reads this doc and emits a project-relative filePath; the ACP extractors pass it through verbatim as the diff path, and the persisted display carries no session cwd to resolve it against — ACP clients cannot open the file, re-introducing the exact bug this PR fixes.
| * Full (project-relative or absolute) path to the edited file, as passed | |
| * Absolute path to the edited file. Producers must resolve/normalize the |
(then continue the JSDoc: producers resolve the path before setting this; consumers may rely on it being absolute)
中文说明
此处写下的 JSDoc 约定与该字段的实际不变量不符：它写的是"编辑文件的完整（项目相对或绝对）路径，按传入工具时的原样"，但每个生产者都保证路径是绝对路径——edit.ts/write-file.ts/notebook-edit.ts 在 validateToolParamValues 中拒绝非绝对路径（如 edit.ts:819），shell sed 编辑在构建展示对象之前就会通过 resolveSedFilePath（shell.ts:1692）解析相对目标，因此该值明确不是"按传入时的原样"。
失败场景：未来某位维护者新增一个会产生编辑结果的工具，读到这段文档后输出了项目相对的 filePath；ACP 提取器会把它原样作为 diff 的 path 传出，而持久化的展示对象中又没有会话 cwd 可供解析——ACP 客户端无法打开该文件，从而重新引入本 PR 所修复的 bug。
建议修复——把不变量改为"编辑文件的绝对路径。生产者必须在设置该字段前解析/规范化路径"（见上方 suggestion 块及补充说明）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
…ink-missing-directory Resolve packages/core/src/tools/edit.test.ts: keep both this branch's display.filePath assertion and main's toolWriteOrigin assertion in the same test.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-1 three FileDiff consumers still basename-first — already reported (comment 3728965255)
- R2-2 notebook-edit filePath populate untested — already reported (comment 3728965265)
- R2-3 sed-edit display filePath untested — already reported (comment 3728965276)
- R2-4 cancelled-edit assertion cannot discriminate field source — already reported (comment 3728965281)
- R2-5 exporter filePath logic unreachable by any test — already reported (comment 3728965286)
- R2-6 JSDoc misstates filePath invariant — already reported (comment 3728965291)
Test Plan (not a blocker): src/transcript-replay.test.ts — no such file or directory; src/ui/utils/export/normalize.test.ts — no such file or directory; src/tools/edit.test.ts — no such file or directory; src/tools/write-file.test.ts — no such file or directory; some_dir/test.md — no such file or directory.
中文说明
已审查——无阻断问题。
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：src/transcript-replay.test.ts — no such file or directory; src/ui/utils/export/normalize.test.ts — no such file or directory; src/tools/edit.test.ts — no such file or directory; src/tools/write-file.test.ts — no such file or directory; some_dir/test.md — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.3)
5 participants
What this PR does
Adds an optional
filePathfield to theFileDifftype (packages/core/src/tools/tools.ts) and populates it with the full path inEditTool.execute()andWriteFileTool.execute()(packages/core/src/tools/edit.ts,packages/core/src/tools/write-file.ts). Updates the two ACP diff-content extractors —packages/acp-bridge/src/transcript-replay.ts(live tool-call updates) andpackages/cli/src/ui/utils/export/normalize.ts(session export/replay) — to preferfilePathoverfileNamewhen building adiffcontent block'spath, falling back tofileNamewhenfilePathisn't present (old persisted session data).Why it's needed
The VSCode IDE companion (and any other ACP-based editor integration) renders a clickable file-link for completed Edit/WriteFile tool calls, sourced from the
diffcontent block'spath. Thatpathwas being built fromfileName, which is only a basename (path.basename(file_path)) — it carries no directory information. The editor extension correctly resolves a relativepathagainst the workspace root, so a file likeapp/src/main/java/com/example/Foo.ktproduces a link to<workspace-root>/Foo.kt— nonexistent for any file not literally at the top level of the project, so clicking it always fails with "file not found". This reproduces on every edit/write in the VSCode companion, both live and when reopening past sessions. Full repro and root-cause trace are in #8606.The pre-execution confirmation object (
ToolEditConfirmationDetails, shown in the "Allow this edit?" prompt) already carries bothfileNameandfilePathcorrectly — the omission is specific to the post-execution result that gets shown in chat and persisted to session history.NotebookEditTool's equivalent display object already includesfilePath, andpackages/cli/src/ui/utils/export/collect.tsalready has a comment/workaround acknowledgingfileName"may be basename" — so this looks like an oversight scoped toedit_file/write_file, not an intentional shape.Reviewer Test Plan
How to verify
some_dir/test.md).<workspace-root>/test.md. After this PR: the correct nested file opens.Also covered by unit tests — see Evidence below for before/after test runs proving each new test fails without the fix and passes with it.
Evidence (Before & After)
Not a TUI/visual change, so no screenshots — evidence is test output and a real repro against the shipped extension.
Before (fix reverted, regression tests fail):
(Same failure shape reproduced for the
packages/cli/src/ui/utils/export/normalize.test.tsregression test with the corresponding source change reverted.)After (fix applied):
I also verified the real-world symptom directly: I applied this same logic change to the
dist/qwen-clichunks bundled inside the installedqwenlm.qwen-code-vscode-ide-companionVSCode extension (0.21.6, matches this PR's base) and reloaded the window. Before: clicking a nested file's edit-link opened<workspace-root>/<basename>and errored "file not found". After: it opens the real file. (Root-cause investigation and this reproduction are detailed in #8606.)Tested on
core/acp-bridge/cliunit suites, and manual repro against the installed extension)Environment (optional)
Node v22.23.1,
npm run typecheck/npm run lint/npx prettier --checkon the changed files,vitest runforpackages/core,packages/acp-bridge, andpackages/cli(export/normalize suite).Risk & Scope
filePathis a new optional field, both consumers keep afileNamefallback, so behavior for any caller that doesn't set it is unchanged..vsixfrom this exact PR branch in VSCode — the manual repro was verified by applying the equivalent logic change directly to the installed extension's bundled JS, not a build of this PR. Doesn't retroactively fix file-links in already-persisted session history (~/.qwen/projects/**/chats/*.jsonl) recorded before this fix, since those records only ever capturedfileName.filePathis additive and optional.Linked Issues
Fixes #8606
中文说明
这个 PR 做了什么
在
FileDiff类型（packages/core/src/tools/tools.ts）中新增了一个可选字段filePath，并在EditTool.execute()和WriteFileTool.execute()（packages/core/src/tools/edit.ts、packages/core/src/tools/write-file.ts）中用完整路径填充它。同时更新了两处 ACP diff 内容提取逻辑——packages/acp-bridge/src/transcript-replay.ts（实时工具调用更新）和packages/cli/src/ui/utils/export/normalize.ts（会话导出/回放）——在构建diff内容块的path时优先使用filePath，仅当filePath不存在时（旧的已持久化会话数据）才回退到fileName。为什么需要这个改动
VSCode IDE companion（以及任何基于 ACP 的编辑器集成）在渲染已完成的 Edit/WriteFile 工具调用时，会显示一个可点击的文件链接，其来源是
diff内容块的path。而这个path此前是由fileName构建的，fileName只是一个文件名（通过path.basename(file_path)得到），不包含任何目录信息。编辑器插件会正确地将相对path解析为相对于工作区根目录的路径，因此像app/src/main/java/com/example/Foo.kt这样的文件会生成指向<workspace-root>/Foo.kt的链接——对于任何不在项目顶层的文件来说，这个路径并不存在，所以点击链接总是报错“文件未找到”。这个问题在 VSCode companion 中的每一次编辑/写入都会复现，无论是当前会话中的实时操作，还是重新打开历史会话。完整的复现步骤和根因排查见 #8606。执行前的确认对象（
ToolEditConfirmationDetails，即“是否允许此次编辑？”提示中展示的对象）已经正确地同时携带了fileName和filePath——这个遗漏只发生在执行后的结果上，而这个结果正是显示在聊天记录中并被持久化到会话历史里的内容。NotebookEditTool对应的展示对象已经包含了filePath，并且packages/cli/src/ui/utils/export/collect.ts中已经有一段注释/变通处理，承认fileName“可能只是文件名（basename）”——所以这看起来是edit_file/write_file特有的疏漏，而不是有意为之的设计。审阅者测试计划
如何验证
some_dir/test.md）。<workspace-root>/test.md。在此 PR 之后：会正确打开嵌套路径下的文件。单元测试也覆盖了这一点——见下方“证据”部分，展示了每个新增测试在没有此修复时会失败、在有此修复后会通过。
证据（修复前 / 修复后）
这不是一个 TUI/视觉层面的改动，所以没有截图——证据是测试输出，以及针对已发布插件的真实复现。
修复前（还原修复后，回归测试失败）：
（在还原
packages/cli/src/ui/utils/export/normalize.test.ts对应源码改动后，也复现了同样形式的失败。）修复后：
我还直接验证了真实场景中的问题：我把同样的逻辑改动应用到了本机已安装的
qwenlm.qwen-code-vscode-ide-companionVSCode 插件（0.21.6，与本 PR 的基线版本一致）内置的dist/qwen-cli代码块中，然后重新加载了窗口。修复前：点击嵌套文件的编辑链接会打开<workspace-root>/<文件名>并报“文件未找到”。修复后：会正确打开真实文件。（根因排查和这个复现过程详见 #8606。）测试环境
core/acp-bridge/cli全量单元测试套件，以及针对已安装插件的手动复现）运行环境（可选）
Node v22.23.1，对改动文件运行了
npm run typecheck/npm run lint/npx prettier --check，并对packages/core、packages/acp-bridge、packages/cli（export/normalize 套件）运行了vitest run。风险与范围
filePath是新增的可选字段，两处消费方都保留了fileName的回退逻辑，因此任何未设置该字段的调用方行为不变。.vsix进行验证——手动复现是通过将等价的逻辑改动直接应用到已安装插件的打包 JS 中完成的，而不是基于本 PR 构建的产物。此修复不会回溯修复已经持久化的历史会话记录中的文件链接（~/.qwen/projects/**/chats/*.jsonl），因为那些记录当初只捕获了fileName。filePath是新增的可选字段。关联 Issue
Fixes #8606