Skip to content

fix(core): auto-record artifact file writes #7914

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

Merged
chiga0 merged 10 commits into from
Jul 29, 2026
Merged

fix(core): auto-record artifact file writes #7914

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b9ae027
fix(core): auto-record write_file artifacts
qinqi-ali Jul 28, 2026
866fe07
test(core): cover all inferWorkspaceArtifactKind kind groups (#7914)
qwen-code-ci-bot Jul 28, 2026
8a65624
test(core): cover .htm extension in artifact kind inference (#7914)
qwen-code Jul 28, 2026
d31994d
fix(core): clarify artifact registration guidance
qinqi-ali Jul 28, 2026
ac37767
fix(core): keep artifact extension kinds in sync
qinqi-ali Jul 28, 2026
16af7b8
fix(core): type workspace write artifacts
qinqi-ali Jul 28, 2026
2674aa4
fix(core): guard artifact title safety and fill ipynb mimeType (#7914)
Jul 28, 2026
f6afd48
fix(core): align artifact guard with store rules and fix Windows test…
Jul 29, 2026
a96c93b
Merge branch 'main' into fix/artifact-auto-record-write-file
wenshao Jul 29, 2026
9379f5b
fix(core): share artifact length limits and pin guard clauses (#7914)
Jul 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
20 changes: 10 additions & 10 deletions packages/cli/src/serve/routes/workspace-file-read.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,18 +557,18 @@ describe('capability advertisement', () => {
});
});

// The `record_artifact` hint that `write_file` appends to a successful write
// names a `workspacePath`, and the model hands that exact string back to this
// route. Producer and consumer live in different packages, each with its own
// notion of what the path is relative to — `write_file` starts from the session
// cwd, the route resolves against the bound workspace root. They agree for an
// ordinary session and drifted apart for a worktree one, where every artifact
// preview 404'd. Neither side's unit tests could catch that: both were
// internally consistent. These pin the round trip instead, over real HTTP.
describe('record_artifact workspacePath contract (write_file ⇄ GET /file)', () => {
// The artifact metadata that `write_file` returns for successful writes names a
// `workspacePath`, and this route later resolves that exact string. Producer and
// consumer live in different packages, each with its own notion of what the path
// is relative to — `write_file` starts from the session cwd, the route resolves
// against the bound workspace root. They agree for an ordinary session and
// drifted apart for a worktree one, where every artifact preview 404'd. Neither
// side's unit tests could catch that: both were internally consistent. These pin
// the round trip instead, over real HTTP.
describe('artifact workspacePath contract (write_file ⇄ GET /file)', () => {
const ARTIFACT = '<!doctype html><h1>Quarterly Chart</h1>';

/** The workspacePath the model is told to send, from the real producer. */
/** The workspacePath emitted by the real producer. */
function emittedWorkspacePath(sessionCwd: string, filePath: string): string {
const reminder = buildRecordArtifactReminder(
{
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/tools/record-artifact.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export interface RecordArtifactParams {
metadata?: Record<string, string | number | boolean | null>;
}

const DESCRIPTION = `Registers a session artifact so clients can show it in an artifacts panel. Use it after creating a useful file, URL, image, report, notebook, or other intermediate result that the user may want to open later.
const DESCRIPTION = `Registers a session artifact so clients can show it in an artifacts panel. Use it after creating a useful file, URL, image, report, notebook, or other intermediate result that the user may want to open later, unless the producing tool already returned artifact metadata. For example, write_file automatically records artifact-like workspace files, so do not call record_artifact again for the same workspacePath.
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Resolved

This tool only records metadata. It does not publish, upload, read, write, or verify the referenced resource. Provide exactly one locator: workspacePath, managedId, or url. Use the Artifact tool, not record_artifact, for published interactive HTML artifacts.`;

Expand Down
91 changes: 79 additions & 12 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 @@ -447,24 +447,34 @@ describe('WriteFileTool', () => {
);
});

it('reminds the model to record artifact-like workspace files', async () => {
it('records artifact-like workspace files in the tool result', async () => {
mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true);
const filePath = path.join(rootDir, 'reports', 'weather.html');
const content = '<!doctype html><html><body>Weather</body></html>';
const params = {
file_path: filePath,
content: '<!doctype html><html><body>Weather</body></html>',
content,
};

const result = await tool.build(params).execute(abortSignal);

expect(result.llmContent).toContain('record_artifact');
expect(result.llmContent).toContain('automatically recorded');
expect(result.llmContent).toContain(
'workspacePath "reports/weather.html"',
);
expect(result.artifacts).toBeUndefined();
expect(result.artifacts).toEqual([
{
title: 'weather.html',
kind: 'html',
storage: 'workspace',
workspacePath: 'reports/weather.html',
mimeType: 'text/html',
sizeBytes: Buffer.byteLength(content),
},
]);
});

it('reminds for case-insensitive artifact extensions', async () => {
it('records case-insensitive artifact extensions', async () => {
mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true);
const filePath = path.join(rootDir, 'reports', 'dashboard.HTML');
const params = {
Expand All @@ -474,13 +484,61 @@ describe('WriteFileTool', () => {

const result = await tool.build(params).execute(abortSignal);

expect(result.llmContent).toContain('record_artifact');
expect(result.llmContent).toContain('automatically recorded');
expect(result.llmContent).toContain(
'workspacePath "reports/dashboard.HTML"',
);
expect(result.artifacts?.[0]).toMatchObject({
title: 'dashboard.HTML',
kind: 'html',
storage: 'workspace',
workspacePath: 'reports/dashboard.HTML',
mimeType: 'text/html',
});
});

it.each([
['page.htm', 'html'],
['notebook.ipynb', 'notebook'],
['paper.pdf', 'pdf'],
['photo.png', 'image'],
['photo.jpeg', 'image'],
['photo.jpg', 'image'],
['diagram.svg', 'image'],
['photo.webp', 'image'],
])('infers artifact kind for %s as %s', async (fileName, expectedKind) => {
mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true);
const filePath = path.join(rootDir, 'reports', fileName);
const params = {
file_path: filePath,
content: 'artifact content',
};

const result = await tool.build(params).execute(abortSignal);

expect(result.artifacts?.[0]).toMatchObject({
title: fileName,
kind: expectedKind,
storage: 'workspace',
workspacePath: `reports/${fileName}`,
});
});

it('does not record artifact-like files when artifact recording is disabled', async () => {
mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(false);
const filePath = path.join(rootDir, 'reports', 'weather.html');
const params = {
file_path: filePath,
content: '<!doctype html><html><body>Weather</body></html>',
};

const result = await tool.build(params).execute(abortSignal);

expect(result.llmContent).not.toContain('automatically recorded');
expect(result.artifacts).toBeUndefined();
});

it('does not remind for ordinary source files', async () => {
it('does not record ordinary source files as artifacts', async () => {
mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true);
const filePath = path.join(rootDir, 'src', 'index.ts');
const params = {
Expand All @@ -490,10 +548,11 @@ describe('WriteFileTool', () => {

const result = await tool.build(params).execute(abortSignal);

expect(result.llmContent).not.toContain('record_artifact');
expect(result.llmContent).not.toContain('automatically recorded');
expect(result.artifacts).toBeUndefined();
});

it('does not remind for files outside the workspace', async () => {
it('does not record files outside the workspace as artifacts', async () => {
mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true);
const filePath = path.join(tempDir, 'outside.html');
const params = {
Expand All @@ -503,10 +562,11 @@ describe('WriteFileTool', () => {

const result = await tool.build(params).execute(abortSignal);

expect(result.llmContent).not.toContain('record_artifact');
expect(result.llmContent).not.toContain('automatically recorded');
expect(result.artifacts).toBeUndefined();
});

it('suggests workspace-root-relative path inside a worktree', async () => {
it('records workspace-root-relative path inside a worktree', async () => {
mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true);
const worktreeDir = path.join(
rootDir,
Expand All @@ -526,10 +586,17 @@ describe('WriteFileTool', () => {

const result = await tool.build(params).execute(abortSignal);

expect(result.llmContent).toContain('record_artifact');
expect(result.llmContent).toContain('automatically recorded');
expect(result.llmContent).toContain(
'workspacePath ".qwen/worktrees/my-feature/report.html"',
);
expect(result.artifacts?.[0]).toMatchObject({
title: 'report.html',
kind: 'html',
storage: 'workspace',
workspacePath: '.qwen/worktrees/my-feature/report.html',
mimeType: 'text/html',
});
} finally {
mockConfigInternal.getTargetDir = originalGetTargetDir;
}
Expand Down
95 changes: 74 additions & 21 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 @@ -12,6 +12,8 @@ import { isAnyAutoMemPath, isTeamAutoMemPath } from '../memory/paths.js';
import { checkTeamMemorySecrets } from '../memory/team-memory-secret-guard.js';
import type {
FileDiff,
ToolArtifact,
ToolArtifactKind,
ToolCallConfirmationDetails,
ToolEditConfirmationDetails,
ToolInvocation,
Expand Down Expand Up @@ -53,18 +55,23 @@ import { CommitAttributionService } from '../services/commitAttribution.js';
import { createDebugLogger } from '../utils/debugLogger.js';

const debugLogger = createDebugLogger('WRITE_FILE');
const ARTIFACT_LIKE_EXTENSIONS = new Set([
'.htm',
'.html',
'.ipynb',
'.jpeg',
'.jpg',
'.pdf',
'.png',
'.svg',
'.webp',
const ARTIFACT_KIND_BY_EXTENSION = new Map<string, ToolArtifactKind>([

@wenshao wenshao Jul 29, 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] Auto-recording changes the rate at which the store's 200-artifact cap is reached, and eviction gives no protection to the artifacts the user actually cares about.

maxArtifacts defaults to 200 and is never overridden (bridge.ts constructs the store with only sessionId/workspaceCwd/persistence), SOURCE_RESERVATIONS.tool = 100, and selectEvictionCandidate (sessionArtifacts.ts:1934) ranks on status === 'missing' → source over-reservation → !clientRetained → oldest createdAt. It never consults storage or retention, and clientRetained is only ever true for source === 'client' — so a published artifact with a live URL is fully evictable. Auto-recorded writes land in source: 'tool', the same bucket as record_artifact and Artifact-publish entries.

Reaching 100 tool artifacts used to require 100 deliberate registration calls. It now requires 100 .html/.svg/.ipynb file writes.

Measured on the real store: seed one published artifact and one record_artifact artifact, then ingest 220 auto-recorded writes → stored=200 publishedSurvived=false recordArtifactSurvived=false. Evictions carry durableTombstoneRequired and are persisted, and rebuild applies removed, so the loss is permanent rather than live-list only. The only trace is a stderr line naming an opaque artifactId.

Two things keep this from being worse, and are why I've filed it as a Suggestion: write_file writes model-generated text, so the image/PDF extensions here are practically unreachable, and rewrites of the same path merge by identity — you need >200 distinct artifact-like paths in one session. An SVG icon set, a generated docs tree, or a long session that accumulates across resumes will get there.

Worth either narrowing this map to the motivating kinds (.html, .htm, .ipynb, .pdf) and leaving bulk .svg to explicit registration, or giving auto-emitted artifacts a lower-priority eviction class so they cannot displace explicitly published ones.

— claude-opus-5[1m] via Claude Code /review

@qwen-code-dev-bot qwen-code-dev-bot Jul 29, 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.

Declined. Eviction policy lives in packages/acp-bridge/src/sessionArtifacts.ts. Narrowing the extension map or adding a lower-priority eviction class are both out of scope for this PR — worth a follow-up issue.

中文说明

拒绝。淘汰策略位于 packages/acp-bridge/src/sessionArtifacts.ts。缩窄扩展名映射或添加低优先级淘汰类别均超出本 PR 范围，建议单独开 issue 跟进。

['.htm', 'html'],
['.html', 'html'],
['.ipynb', 'notebook'],
['.jpeg', 'image'],
['.jpg', 'image'],
['.pdf', 'pdf'],
['.png', 'image'],
['.svg', 'image'],
['.webp', 'image'],
]);

type WorkspaceToolArtifact = ToolArtifact & {
storage: 'workspace';
workspacePath: string;
};

/**
* Parameters for the WriteFile tool
*/
Expand Down Expand Up @@ -520,8 +527,10 @@ class WriteFileToolInvocation extends BaseToolInvocation<
// pre-write placeholder. Best-effort: a stat failure here does
// not undo the successful write — the next Read will re-stat
// and either see fresh content or treat the entry as stale.
let postWriteSizeBytes: number | undefined;
try {
const postWriteStats = fs.statSync(file_path);
postWriteSizeBytes = postWriteStats.size;
this.config.getFileReadCache().recordWrite(file_path, postWriteStats);
} catch {
// Non-fatal: leaving a stale entry is preferable to failing
Expand Down Expand Up @@ -561,12 +570,15 @@ class WriteFileToolInvocation extends BaseToolInvocation<
`User modified the \`content\` to be: ${content}`,
);
}
const artifactReminder = buildRecordArtifactReminder(
const artifact = buildWorkspaceArtifactMetadata(
this.config,
file_path,
postWriteSizeBytes,
);
if (artifactReminder) {
llmSuccessMessageParts.push(artifactReminder);
if (artifact) {
llmSuccessMessageParts.push(
formatRecordArtifactReminder(artifact.workspacePath),
);
}

// Log file operation for telemetry (without diff_stat to avoid double-counting)
Expand Down Expand Up @@ -601,6 +613,7 @@ class WriteFileToolInvocation extends BaseToolInvocation<
return {
llmContent: llmSuccessMessageParts.join(' '),
returnDisplay: displayResult,
...(artifact ? { artifacts: [artifact] } : {}),
};
} catch (error) {
// Capture detailed error information for debugging
Expand Down Expand Up @@ -644,24 +657,62 @@ class WriteFileToolInvocation extends BaseToolInvocation<
}

/**
* Builds the `record_artifact` hint appended to a successful write.
* Builds the artifact-recording note appended to a successful write.
*
* Exported because the string it produces is a CONTRACT with the daemon's
* Exported because the workspacePath it produces is a CONTRACT with the daemon's
* `GET /file` route: the `workspacePath` computed here is later resolved by
* `resolveWithinWorkspace` against the bound workspace root. The two sides live
* in different packages and drifted apart once already (a worktree session
* emitted a worktree-relative path that the route resolved against the
* emitted a session-cwd-relative path that the route resolved against the
* workspace root, so every artifact preview 404'd). `workspace-file-read.test.ts`
* pins the round-trip; keep this exported so it can keep doing so.
*/
export function buildRecordArtifactReminder(
config: Config,
filePath: string,
): string | null {
const workspacePath = getRecordArtifactWorkspacePath(config, filePath);
if (!workspacePath) {
return null;
}
return formatRecordArtifactReminder(workspacePath);
}
Comment thread
qwen-code-dev-bot marked this conversation as resolved.
Resolved

function formatRecordArtifactReminder(workspacePath: string): string {
return (
`This file was automatically recorded as a workspace artifact with ` +
`workspacePath "${workspacePath}". No extra artifact registration step ` +
`is needed.`
);
}
Comment on lines +678 to +684

@wenshao wenshao Jul 29, 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 is an unconditional statement of completed fact, but only the daemon/ACP surface actually consumes ToolResult.artifacts — on the others the sentence is false and the model will relay it to the user verbatim.

  • Subagents. write_file is not in EXCLUDED_TOOLS_FOR_SUBAGENTS, so a subagent writing report.html gets both the artifacts array and this sentence. But AgentToolResultEvent has no artifacts field, and SubAgentTracker calls toolCallEmitter.emitResult({...}) without one, so _meta.artifacts is never stamped and extractSessionUpdateArtifacts sees nothing. There is no action=dropped line either — nothing ever reaches the store, so the drop is completely silent. Recovery is closed as well: record_artifact is deliberately excluded from subagents ("V1 session artifacts are owned by the parent daemon session"), and the new record_artifact description tells the parent not to re-record the path.
  • TUI and headless --prompt. isRecordArtifactEnabled() is true on both (it only excludes SDK mode), but the sole reader of ToolCallResponseInfo.artifacts is packages/cli/src/acp-integration/session/Session.ts. There is no artifacts consumer under packages/cli/src/ui.

The previous wording was a conditional imperative — "If this file is a reusable user-facing artifact, call record_artifact…" — which was harmless where nothing consumed it. Asserting a completed action is not.

Worth either gating the sentence on a session that has an artifact consumer, or phrasing it as a capability rather than a finished action, e.g. "recorded for clients that display an artifacts panel".

— claude-opus-5[1m] via Claude Code /review

@qwen-code-dev-bot qwen-code-dev-bot Jul 29, 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.

Declined. Gating the sentence on a session that has an artifact consumer would require plumbing surface awareness into the write path; softening the wording would churn ~6 test assertions for a non-blocking preference. The maintainer's round-4 verification confirmed comfort with merging as-is.

中文说明

拒绝。按会话类型门控需要在写入路径中引入场景感知；软化措辞会导致约 6 个测试断言的变动，且属于非阻断偏好。维护者第 4 轮验证已确认可按现状合并。


function buildWorkspaceArtifactMetadata(
config: Config,
filePath: string,
sizeBytes?: number,
): WorkspaceToolArtifact | null {
const workspacePath = getRecordArtifactWorkspacePath(config, filePath);
if (!workspacePath) {
return null;
}
return {
title: path.basename(filePath),
kind: inferWorkspaceArtifactKind(filePath),
storage: 'workspace',
workspacePath,
mimeType: getSpecificMimeType(filePath),
sizeBytes,
};
}

function getRecordArtifactWorkspacePath(
config: Config,
filePath: string,
): string | null {
if (!config.isRecordArtifactEnabled()) {
return null;
}
if (!ARTIFACT_LIKE_EXTENSIONS.has(path.extname(filePath).toLowerCase())) {
if (!ARTIFACT_KIND_BY_EXTENSION.has(path.extname(filePath).toLowerCase())) {
Comment on lines +726 to +733

@wenshao wenshao Jul 29, 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 helper anchors the emitted path at config.getTargetDir() and special-cases exactly one divergence — the .qwen/worktrees/<slug> shape. But getTargetDir() is mutable, and the store it feeds is not.

POST /session/:id/cd → bridge.changeSessionCwd → config.relocateWorkingDirectory(..., { skipArtifactMigration: true }) moves targetDir to any trusted absolute directory. changeSessionCwd only publishes a session_cwd_changed event — it never rebuilds entry.artifacts, and SessionArtifactStore is constructed once with workspaceCwd (bridge.ts:3843-3847), which is also GET /file's resolution root. So after a cd into a subdirectory, the emitted workspacePath is relative to the new cwd while both consumers resolve it against the original root.

Reproduced with this PR's own workspace-file-read.test.ts harness, session cwd = <workspace>/packages/core:

emitted  workspacePath = "report.html"
GET /file             -> 200, "<!doctype html><h1>UNRELATED ROOT FILE</h1>"   // wrong file
GET /file (weather)   -> 404 path_not_found

That 200-with-the-wrong-file is verbatim what the contract test's comment calls "worse than a 404".

To be clear about attribution: this arithmetic is not introduced here — the pre-existing helper computed the same path. What changes is the consequence. Before, it was a hint the model could ignore; now emission is unconditional and write_file asserts to the model that the file "was automatically recorded … No extra artifact registration step is needed". A defect that used to be probabilistic becomes deterministic for every artifact-like write in a relocated session.

Worth anchoring at the session's bound workspace root rather than getTargetDir() plus a path-shape regex — config.storage.getProjectRoot() is that value on the ACP path, and using it would let the .qwen/worktrees regex be deleted rather than extended. (Check the CLI --worktree startup path first, where Config is constructed with targetDir already inside the worktree.) A smaller alternative: return null — no artifact, no claim — when getTargetDir() is neither the bound root nor <root>/.qwen/worktrees/<slug>.

— claude-opus-5[1m] via Claude Code /review

@qwen-code-dev-bot qwen-code-dev-bot Jul 29, 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.

Declined. The reviewer confirmed this path arithmetic is pre-existing, not introduced by this PR. Anchoring at the bound workspace root and auditing the CLI --worktree startup path is a separate concern best handled in its own change.

中文说明

拒绝。评审者确认此路径计算是既有逻辑，非本 PR 引入。锚定到绑定的工作区根目录并审计 CLI --worktree 启动路径属于独立关注点，最好单独处理。

return null;
}
// The daemon's file-read route resolves workspacePath against the
Expand All @@ -683,11 +734,13 @@ export function buildRecordArtifactReminder(
) {
return null;
}
const workspacePath = relativePath.split(path.sep).join('/');
return relativePath.split(path.sep).join('/');
}

function inferWorkspaceArtifactKind(filePath: string): ToolArtifactKind {
return (
`If this file is a reusable user-facing artifact, call ` +
`record_artifact with workspacePath "${workspacePath}" before telling ` +
`the user it is available in the artifacts panel.`
ARTIFACT_KIND_BY_EXTENSION.get(path.extname(filePath).toLowerCase()) ??
'file'
);
}

Expand Down
Loading