diff --git a/packages/cli/src/serve/routes/workspace-file-read.test.ts b/packages/cli/src/serve/routes/workspace-file-read.test.ts index 9c9824cdecb..7fccfbe7043 100644 --- a/packages/cli/src/serve/routes/workspace-file-read.test.ts +++ b/packages/cli/src/serve/routes/workspace-file-read.test.ts @@ -12,6 +12,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import request from 'supertest'; import { buildRecordArtifactReminder, + buildWorkspaceArtifactMetadata, Ignore, type Config, } from '@qwen-code/qwen-code-core'; @@ -601,30 +602,30 @@ 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 = '

Quarterly Chart

'; - /** 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( - { - isRecordArtifactEnabled: () => true, - getTargetDir: () => sessionCwd, - } as unknown as Config, - filePath, - ); + const config = { + isRecordArtifactEnabled: () => true, + getTargetDir: () => sessionCwd, + } as unknown as Config; + const reminder = buildRecordArtifactReminder(config, filePath); const match = /workspacePath "([^"]+)"/.exec(reminder ?? ''); if (!match?.[1]) { throw new Error(`no workspacePath in reminder: ${reminder ?? 'null'}`); } + const artifact = buildWorkspaceArtifactMetadata(config, filePath); + expect(artifact?.workspacePath).toBe(match[1]); return match[1]; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 6254a2b6920..ddaab28a003 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -199,7 +199,10 @@ export type { WriteFileTool, WriteFileToolParams } from './tools/write-file.js'; // Exported for the cross-package contract test in packages/cli (see the // function's own doc comment) — the daemon's file-read route must resolve the // workspacePath this produces. -export { buildRecordArtifactReminder } from './tools/write-file.js'; +export { + buildRecordArtifactReminder, + buildWorkspaceArtifactMetadata, +} from './tools/write-file.js'; export type { ArtifactTool, ArtifactToolParams, diff --git a/packages/core/src/tools/record-artifact.ts b/packages/core/src/tools/record-artifact.ts index 3e627e620ad..6a16f952b1f 100644 --- a/packages/core/src/tools/record-artifact.ts +++ b/packages/core/src/tools/record-artifact.ts @@ -28,10 +28,13 @@ export interface RecordArtifactParams { metadata?: Record; } -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 HTML, image, PDF, and notebook files it writes inside the workspace, so do not call record_artifact again for the same workspacePath; still call it for other formats such as Markdown, CSV, JSON, and plain text, and for files produced outside write_file. 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.`; +export const ARTIFACT_TITLE_MAX_LENGTH = 200; +export const ARTIFACT_WORKSPACE_PATH_MAX_LENGTH = 500; + class RecordArtifactInvocation extends BaseToolInvocation< RecordArtifactParams, ToolResult @@ -158,7 +161,12 @@ export class RecordArtifactTool extends BaseDeclarativeTool< params: RecordArtifactParams, ): string | null { params.title = (params.title ?? '').trim(); - const titleError = validateString(params.title, 'title', 200, true); + const titleError = validateString( + params.title, + 'title', + ARTIFACT_TITLE_MAX_LENGTH, + true, + ); if (titleError) { return titleError; } @@ -318,7 +326,7 @@ function isDisplayField(field: string): boolean { ); } -function hasControlCharacter( +export function hasControlCharacter( value: string, allowLineWhitespace = false, ): boolean { @@ -346,7 +354,7 @@ function hasControlCharacter( return false; } -function hasUnsafeDisplayPayload(value: string): boolean { +export function hasUnsafeDisplayPayload(value: string): boolean { return ( /<\s*\/?[a-z!]|&(?:#[0-9]+|#x[0-9a-f]+|[a-z][a-z0-9]+);|javascript\s*:|data\s*:\s*(?:text\/(?:html|javascript)|application\/javascript|image\/svg\+xml)/i.test( value, @@ -356,7 +364,12 @@ function hasUnsafeDisplayPayload(value: string): boolean { function validateWorkspacePath(value: string): string | null { const trimmed = value.trim(); - const stringError = validateString(trimmed, 'workspacePath', 500, true); + const stringError = validateString( + trimmed, + 'workspacePath', + ARTIFACT_WORKSPACE_PATH_MAX_LENGTH, + true, + ); if (stringError) { return stringError; } diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 4e67dcb55d3..6e3482dcc38 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -14,7 +14,11 @@ import { type Mocked, } from 'vitest'; import type { WriteFileToolParams } from './write-file.js'; -import { WriteFileTool } from './write-file.js'; +import { + WriteFileTool, + buildRecordArtifactReminder, + buildWorkspaceArtifactMetadata, +} from './write-file.js'; import { ToolErrorType } from './tool-error.js'; import type { FileDiff, ToolEditConfirmationDetails } from './tools.js'; import { ToolConfirmationOutcome } from './tools.js'; @@ -447,24 +451,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 = 'Weather'; const params = { file_path: filePath, - content: 'Weather', + 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 = { @@ -474,13 +488,129 @@ 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('sets application/x-ipynb+json mimeType for notebooks', async () => { + mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true); + const filePath = path.join(rootDir, 'notes', 'analysis.ipynb'); + const params = { + file_path: filePath, + content: '{"cells":[]}', + }; + + const result = await tool.build(params).execute(abortSignal); + + expect(result.artifacts?.[0]).toMatchObject({ + kind: 'notebook', + mimeType: 'application/x-ipynb+json', + }); }); - it('does not remind for ordinary source files', async () => { + 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: 'Weather', + }; + + const result = await tool.build(params).execute(abortSignal); + + expect(result.llmContent).not.toContain('automatically recorded'); + expect(result.artifacts).toBeUndefined(); + }); + + it('does not record artifacts whose filename contains unsafe markup', async () => { + mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true); + const filePath = path.join( + rootDir, + 'reports', + 'chart onerror=alert(1).html', + ); + const params = { + file_path: filePath, + content: 'XSS', + }; + + const result = await tool.build(params).execute(abortSignal); + + expect(result.llmContent).toContain('Successfully created'); + expect(result.llmContent).not.toContain('automatically recorded'); + expect(result.artifacts).toBeUndefined(); + }); + + it('does not record artifacts whose title exceeds 200 characters', async () => { + mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true); + const longName = 'a'.repeat(196) + '.html'; + const filePath = path.join(rootDir, 'reports', longName); + const params = { + file_path: filePath, + content: 'Long', + }; + + const result = await tool.build(params).execute(abortSignal); + + expect(result.llmContent).toContain('Successfully created'); + expect(result.llmContent).not.toContain('automatically recorded'); + expect(result.artifacts).toBeUndefined(); + }); + + it('does not record artifacts whose workspace path contains unsafe markup', async () => { + mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true); + const dir = path.join(rootDir, 'Q&A'); + fs.mkdirSync(dir, { recursive: true }); + const filePath = path.join(dir, 'summary.html'); + const params = { + file_path: filePath, + content: 'Summary', + }; + + const result = await tool.build(params).execute(abortSignal); + + expect(result.llmContent).toContain('Successfully created'); + expect(result.llmContent).not.toContain('automatically recorded'); + expect(result.artifacts).toBeUndefined(); + }); + + it('does not record ordinary source files as artifacts', async () => { mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true); const filePath = path.join(rootDir, 'src', 'index.ts'); const params = { @@ -490,10 +620,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 = { @@ -503,10 +634,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, @@ -526,10 +658,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; } @@ -1489,3 +1628,42 @@ describe('WriteFileTool', () => { }); }); }); + +describe('workspace artifact metadata guard', () => { + beforeEach(() => { + mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true); + }); + + // Pins the delegation: buildRecordArtifactReminder must agree with + // buildWorkspaceArtifactMetadata. If the reminder is reverted to compute the + // path independently (without the safety guard), it would still emit a hint + // for this markup-bearing filename while the artifact is correctly skipped, + // reintroducing the false "automatically recorded" claim. + it('keeps the reminder and the artifact in lockstep when the guard rejects', () => { + const rejected = path.resolve( + rootDir, + 'reports', + 'chart onerror=alert(1).html', + ); + expect(buildWorkspaceArtifactMetadata(mockConfig, rejected)).toBeNull(); + expect(buildRecordArtifactReminder(mockConfig, rejected)).toBeNull(); + }); + + it('skips artifacts whose workspace path exceeds the store limit', () => { + // A short filename buried in a deep directory: the workspace path blows + // past the 500-char store limit while the title stays well under its own, + // so this exercises the path-length clause on its own. + const deepDir = 'a'.repeat(510); + const filePath = path.resolve(rootDir, deepDir, 'x.html'); + expect(buildWorkspaceArtifactMetadata(mockConfig, filePath)).toBeNull(); + }); + + it('skips artifacts whose workspace path contains a control character', () => { + // The control character sits in a directory segment, not the basename: the + // title is path.basename(filePath), so a control character in the title + // would also appear in the path and could not prove the path-side check on + // its own. + const filePath = path.resolve(rootDir, 'reports\u000b', 'chart.html'); + expect(buildWorkspaceArtifactMetadata(mockConfig, filePath)).toBeNull(); + }); +}); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 63e6154af68..81237f15a9c 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -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, @@ -51,20 +53,31 @@ import { import { getLanguageFromFilePath } from '../utils/language-detection.js'; import { CommitAttributionService } from '../services/commitAttribution.js'; import { createDebugLogger } from '../utils/debugLogger.js'; +import { + ARTIFACT_TITLE_MAX_LENGTH, + ARTIFACT_WORKSPACE_PATH_MAX_LENGTH, + hasControlCharacter, + hasUnsafeDisplayPayload, +} from './record-artifact.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([ + ['.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 */ @@ -520,8 +533,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 @@ -561,12 +576,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) @@ -601,6 +619,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< return { llmContent: llmSuccessMessageParts.join(' '), returnDisplay: displayResult, + ...(artifact ? { artifacts: [artifact] } : {}), }; } catch (error) { // Capture detailed error information for debugging @@ -644,24 +663,74 @@ class WriteFileToolInvocation extends BaseToolInvocation< } /** - * Builds the `record_artifact` hint appended to a successful write. - * - * Exported because the string 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 - * 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. + * Kept for the cross-package contract test in `workspace-file-read.test.ts`: + * the daemon's `GET /file` route resolves the `workspacePath` this produces. + * Delegates to `buildWorkspaceArtifactMetadata` so the two agree by construction. */ export function buildRecordArtifactReminder( config: Config, filePath: string, +): string | null { + const artifact = buildWorkspaceArtifactMetadata(config, filePath); + return artifact ? formatRecordArtifactReminder(artifact.workspacePath) : null; +} + +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.` + ); +} + +export function buildWorkspaceArtifactMetadata( + config: Config, + filePath: string, + sizeBytes?: number, +): WorkspaceToolArtifact | null { + const workspacePath = getRecordArtifactWorkspacePath(config, filePath); + if (!workspacePath) { + return null; + } + const title = path.basename(filePath); + // The daemon store rejects titles and paths that are too long, carry control + // characters, or contain markup; skip the artifact rather than tell the model + // it was recorded when it will be dropped. + if ( + title.length > ARTIFACT_TITLE_MAX_LENGTH || + hasControlCharacter(title) || + hasUnsafeDisplayPayload(title) || + workspacePath.length > ARTIFACT_WORKSPACE_PATH_MAX_LENGTH || + hasControlCharacter(workspacePath) || + hasUnsafeDisplayPayload(workspacePath) + ) { + debugLogger.debug('workspace artifact skipped (safety checks)', { + path: filePath, + }); + return null; + } + return { + title, + kind: inferWorkspaceArtifactKind(filePath), + storage: 'workspace', + workspacePath, + mimeType: + getSpecificMimeType(filePath) ?? + (filePath.toLowerCase().endsWith('.ipynb') + ? 'application/x-ipynb+json' + : undefined), + 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())) { return null; } // The daemon's file-read route resolves workspacePath against the @@ -683,11 +752,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' ); }