Skip to content

fix(core): include full filePath alongside fileName in edit/write-file diff results #8607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JonathanOrr wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(core): include full filePath alongside fileName in edit/write-file diff results #8607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26796d3
fix(core): include full filePath alongside fileName in edit/write-fil…
JonathanOrr Aug 5, 2026
98d208e
chore: nudge CI — re-trigger automated review after PR body template fix
JonathanOrr Aug 6, 2026
a0e28cc
fix(core): populate filePath in the remaining FileDiff producers and …
JonathanOrr Aug 6, 2026
983ff5e
Merge remote-tracking branch 'origin/main' into fix/edit-write-file-l…
wenshao Aug 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,10 +611,17 @@ function extractLocations(rawInput, toolCallResult) {
if (toolCallResult && typeof toolCallResult === 'object') {
const display = toolCallResult.resultDisplay;
if (display && typeof display === 'object') {
if (typeof display.fileName === 'string' && display.fileName) {
// Prefer filePath (full path) over fileName (basename-only) when both are present.
const displayPath =
typeof display.filePath === 'string' && display.filePath
Comment on lines +615 to +616

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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)

? display.filePath
: typeof display.fileName === 'string' && display.fileName
? display.fileName
: undefined;
if (displayPath) {
// Avoid duplicates
if (!locations.some((loc) => loc.path === display.fileName)) {
locations.push({ path: display.fileName });
if (!locations.some((loc) => loc.path === displayPath)) {
locations.push({ path: displayPath });
}
}
}
Expand All @@ -630,7 +637,11 @@ function extractDiffContent(resultDisplay) {
return [
{
type: 'diff',
path: display.fileName,
// Prefer filePath (full path) over fileName (basename-only) when both are present.
path:
typeof display.filePath === 'string' && display.filePath
? display.filePath
: display.fileName,
oldText: display.originalContent ?? '',
newText: display.newContent,
},
Expand Down
107 changes: 107 additions & 0 deletions packages/acp-bridge/src/transcript-replay.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1275,6 +1275,113 @@ describe('createTranscriptReplayMachine', () => {
expect(machine.snapshot().pendingToolCalls).toEqual([]);
});

it('prefers filePath over the fileName basename when replaying an edit diff', () => {
const machine = createTranscriptReplayMachine();
updates(
machine,
record('assistant-1', 'assistant', {
message: {
role: 'model',
parts: [
{ functionCall: { name: 'edit_file', args: {}, id: 'call-1' } },
],
},
}),
);
const result = updates(
machine,
record('result-1', 'tool_result', {
message: {
role: 'user',
parts: [
{
functionResponse: {
name: 'edit_file',
response: { output: 'edited' },
},
},
],
},
toolCallResult: {
callId: 'call-1',
resultDisplay: {
fileDiff: '--- a\n+++ b\n',
fileName: 'Foo.kt',
filePath: '/workspace/app/src/main/java/com/example/Foo.kt',
originalContent: 'old',
newContent: 'new',
},
},
}),
);

expect(result[0]).toMatchObject({
sessionUpdate: 'tool_call_update',
toolCallId: 'call-1',
content: [
{
type: 'diff',
path: '/workspace/app/src/main/java/com/example/Foo.kt',
oldText: 'old',
newText: 'new',
},
],
});
});

it('falls back to the fileName basename when filePath is absent (pre-fix persisted sessions)', () => {
const machine = createTranscriptReplayMachine();
updates(
machine,
record('assistant-1', 'assistant', {
message: {
role: 'model',
parts: [
{ functionCall: { name: 'edit_file', args: {}, id: 'call-1' } },
],
},
}),
);
const result = updates(
machine,
record('result-1', 'tool_result', {
message: {
role: 'user',
parts: [
{
functionResponse: {
name: 'edit_file',
response: { output: 'edited' },
},
},
],
},
toolCallResult: {
callId: 'call-1',
resultDisplay: {
fileDiff: '--- a\n+++ b\n',
fileName: 'Foo.kt',
originalContent: 'old',
newContent: 'new',
},
},
}),
);

expect(result[0]).toMatchObject({
sessionUpdate: 'tool_call_update',
toolCallId: 'call-1',
content: [
{
type: 'diff',
path: 'Foo.kt',
oldText: 'old',
newText: 'new',
},
],
});
});

it('reports ambiguous same-name result correlation', () => {
const onDiagnostic = vi.fn();
const machine = createTranscriptReplayMachine({ onDiagnostic });
Expand Down
8 changes: 5 additions & 3 deletions packages/acp-bridge/src/transcript-replay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1458,9 +1458,11 @@ function extractDiffContent(resultDisplay: unknown): ToolCallContent | null {
return {
type: 'diff',
path:
typeof resultDisplay['fileName'] === 'string'
? resultDisplay['fileName']
: '',
typeof resultDisplay['filePath'] === 'string'
? resultDisplay['filePath']
: typeof resultDisplay['fileName'] === 'string'
? resultDisplay['fileName']
: '',
Comment on lines +1463 to +1465

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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)

oldText:
typeof resultDisplay['originalContent'] === 'string'
? resultDisplay['originalContent']
Expand Down
105 changes: 105 additions & 0 deletions packages/cli/src/ui/utils/export/normalize.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,111 @@ describe('normalizeSessionData', () => {
]);
});

it('exports the diff path from filePath rather than the fileName basename', () => {
const record: ChatRecord = {
uuid: 'tool-2',
parentUuid: null,
sessionId: 'session-1',
timestamp: '2025-01-01T00:00:00.000Z',
type: 'tool_result',
cwd: '',
version: '1.0.0',
message: {
role: 'user',
parts: [
{
functionResponse: {
id: 'call-2',
name: 'edit_file',
response: { output: 'ok' },
},
},
],
},
toolCallResult: {
callId: 'call-2',
resultDisplay: {
fileName: 'Foo.kt',
filePath: '/workspace/app/src/main/java/com/example/Foo.kt',
fileDiff: '--- Foo.kt\n+++ Foo.kt\n',
originalContent: 'old',
newContent: 'new',
},
},
};

const normalized = normalizeSessionData(
{
sessionId: 'session-1',
startTime: '2025-01-01T00:00:00.000Z',
messages: [],
},
[record],
config,
);

expect(normalized.messages[0].toolCall?.content).toEqual([
{
type: 'diff',
path: '/workspace/app/src/main/java/com/example/Foo.kt',
oldText: 'old',
newText: 'new',
},
]);
});

it('falls back to the fileName basename when filePath is absent (pre-fix persisted sessions)', () => {
const record: ChatRecord = {
uuid: 'tool-2b',
parentUuid: null,
sessionId: 'session-1',
timestamp: '2025-01-01T00:00:00.000Z',
type: 'tool_result',
cwd: '',
version: '1.0.0',
message: {
role: 'user',
parts: [
{
functionResponse: {
id: 'call-2b',
name: 'edit_file',
response: { output: 'ok' },
},
},
],
},
toolCallResult: {
callId: 'call-2b',
resultDisplay: {
fileName: 'Foo.kt',
fileDiff: '--- Foo.kt\n+++ Foo.kt\n',
originalContent: 'old',
newContent: 'new',
},
},
};

const normalized = normalizeSessionData(
{
sessionId: 'session-1',
startTime: '2025-01-01T00:00:00.000Z',
messages: [],
},
[record],
config,
);

expect(normalized.messages[0].toolCall?.content).toEqual([
{
type: 'diff',
path: 'Foo.kt',
oldText: 'old',
newText: 'new',
},
]);
});

it('accepts the minimal daemon export config shape', () => {
const minimalConfig: ExportConfig = {};
const record: ChatRecord = {
Expand Down
5 changes: 4 additions & 1 deletion packages/cli/src/ui/utils/export/normalize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,10 @@ function extractDiffContent(
return [
{
type: 'diff',
path: display['fileName'] as string,
path:
typeof display['filePath'] === 'string'
? display['filePath']
: (display['fileName'] as string),
Comment on lines +330 to +333

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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 "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)

oldText: (display['originalContent'] as string) ?? '',
newText: display['newContent'] as string,
},
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/core/coreToolScheduler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7673,6 +7673,7 @@ describe('CoreToolScheduler edit cancellation', () => {
'--- test.txt\n+++ test.txt\n@@ -1,1 +1,1 @@\n-old content\n+new content',
);
expect(cancelledCall.response.resultDisplay.fileName).toBe('test.txt');
expect(cancelledCall.response.resultDisplay.filePath).toBe('test.txt');
Comment on lines 7675 to +7676

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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)

});
});

Expand Down
1 change: 1 addition & 0 deletions packages/core/src/core/coreToolScheduler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1646,6 +1646,7 @@ export class CoreToolScheduler {
resultDisplay = {
fileDiff: waitingCall.confirmationDetails.fileDiff,
fileName: waitingCall.confirmationDetails.fileName,
filePath: waitingCall.confirmationDetails.filePath,
originalContent:
waitingCall.confirmationDetails.originalContent,
newContent: waitingCall.confirmationDetails.newContent,
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/tools/edit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,10 @@ describe('EditTool', () => {
expect(display.fileDiff).toMatch(initialContent);
expect(display.fileDiff).toMatch(newContent);
expect(display.fileName).toBe(testFile);
// `filePath` must carry the full path: UI consumers (e.g. the VSCode
// companion) cannot resolve a clickable location from `fileName`
// alone once the file is outside the workspace root.
expect(display.filePath).toBe(filePath);
expect(writeSpy).toHaveBeenCalledWith(
expect.objectContaining({ toolWriteOrigin: 'edit' }),
);
Expand Down
4 changes: 3 additions & 1 deletion packages/core/src/tools/edit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import * as fs from 'node:fs';
import * as path from 'node:path';
import type {
FileDiff,
ToolCallConfirmationDetails,
ToolEditConfirmationDetails,
ToolInvocation,
Expand Down Expand Up @@ -674,9 +675,10 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
'Current',
'Proposed',
);
const displayResult = {
const displayResult: FileDiff = {
fileDiff,
fileName,
filePath: this.params.file_path,
originalContent: editData.currentContent,
newContent: editData.newContent,
diffStat,
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/notebook-edit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,7 @@ class NotebookEditInvocation extends BaseToolInvocation<
const displayResult = {
fileDiff,
fileName,
filePath: this.params.notebook_path,

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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)

originalContent: prepared.originalContent,
newContent: prepared.updatedContent,
diffStat,
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/shell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1768,6 +1768,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
'Proposed',
),
fileName: edit.fileName,
filePath: edit.filePath,
Comment on lines 1770 to +1771

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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)

originalContent: edit.originalContent,
newContent: edit.newContent,
diffStat,
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/tools/tools.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,13 @@ export interface TaskListResultDisplay {
export interface FileDiff {
fileDiff: string;
fileName: string;
/**
* Full (project-relative or absolute) path to the edited file, as passed

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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.

Suggested change
* 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)

* to the tool. UI consumers must prefer this over `fileName` when
* resolving a clickable/openable location — `fileName` is a basename and
Comment on lines +818 to +819

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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:

  1. packages/cli/src/serve/live/live-task-service.ts:267 — emits fileChange task items with path: display.fileName to qwen serve clients; the local isFileDiff guard does not name filePath.
  2. packages/cli/src/ui/utils/export/collect.ts:170 — computes export stats as (args?.['file_path'] as string) || display.fileName; shell sed-edits (arg command) and notebook edits (arg notebook_path) carry no file_path arg, so stats degrade to basename and the writtenFilePaths Set collides distinct same-basename files (undercounting filesWritten).
  3. packages/cli/src/services/insight/generators/DataProcessor.ts:1123 — the insight generator does uniqueFiles.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 写下的约定相矛盾：

  1. packages/cli/src/serve/live/live-task-service.ts:267 —— 向 qwen serve 客户端发出的 fileChange 任务项使用 path: display.fileName；其本地 isFileDiff 守卫也没有包含 filePath。
  2. 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，writtenFilePaths Set 会把同名不同路径的文件合并（少计 filesWritten）。
  3. 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)

* cannot be used to locate files outside the workspace root.
*/
filePath?: string;
Comment on lines +818 to +822

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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)

originalContent: string | null;
newContent: string;
diffStat?: DiffStat;
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/tools/write-file.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ describe('WriteFileTool', () => {
expect(writtenContent).toBe(proposedContent);
const display = result.returnDisplay as FileDiff;
expect(display.fileName).toBe('execute_new_file.txt');
expect(display.filePath).toBe(filePath);
expect(display.fileDiff).toMatch(/--- execute_new_file.txt\tOriginal/);
expect(display.fileDiff).toMatch(/\+\+\+ execute_new_file.txt\tWritten/);
expect(display.fileDiff).toMatch(
Expand Down Expand Up @@ -857,6 +858,7 @@ describe('WriteFileTool', () => {
expect(writtenContent).toBe(proposedContent);
const display = result.returnDisplay as FileDiff;
expect(display.fileName).toBe('execute_existing_file.txt');
expect(display.filePath).toBe(filePath);
expect(display.fileDiff).toMatch(
initialContent.replace(/[.*+?^${}()|[\\]\\]/g, '\\$&'),
);
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/write-file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,7 @@ class WriteFileToolInvocation extends BaseToolInvocation<
const displayResult: FileDiff = {
fileDiff,
fileName,
filePath: file_path,

@qwen-code-ci-bot qwen-code-ci-bot Aug 6, 2026

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] 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)

originalContent,
newContent: content,
diffStat,
Expand Down
Loading