diff --git a/packages/acp-bridge/src/sessionArtifacts.test.ts b/packages/acp-bridge/src/sessionArtifacts.test.ts index 690e2e0898d..ed9b8ab5d3d 100644 --- a/packages/acp-bridge/src/sessionArtifacts.test.ts +++ b/packages/acp-bridge/src/sessionArtifacts.test.ts @@ -899,6 +899,339 @@ describe('SessionArtifactStore', () => { } }); + it('updates workspace title and description when the same path is recorded again', async () => { + const store = new SessionArtifactStore({ + sessionId: 's2-workspace-rerecord-title', + workspaceCwd: workspace, + }); + await fs.mkdir(path.join(workspace, 'reports'), { recursive: true }); + await fs.writeFile(path.join(workspace, 'reports/dashboard.html'), 'hello'); + + const created = await store.upsertMany( + [ + { + title: 'Draft', + description: 'First pass', + workspacePath: 'reports/dashboard.html', + }, + ], + { strict: true }, + ); + const artifactId = created.changes[0]?.artifactId; + + const updated = await store.upsertMany( + [ + { + title: 'Final dashboard', + description: 'Ready for review', + workspacePath: 'reports/dashboard.html', + }, + ], + { strict: true }, + ); + + expect(updated.changes).toHaveLength(1); + expect(updated.changes[0]).toMatchObject({ + action: 'updated', + artifactId, + artifact: { + id: artifactId, + storage: 'workspace', + title: 'Final dashboard', + description: 'Ready for review', + workspacePath: 'reports/dashboard.html', + }, + }); + expect((await store.list()).artifacts).toMatchObject([ + { + id: artifactId, + title: 'Final dashboard', + description: 'Ready for review', + workspacePath: 'reports/dashboard.html', + }, + ]); + }); + + it('keeps a curated title when write_file re-records the same workspace path', async () => { + const store = new SessionArtifactStore({ + sessionId: 's2-workspace-rerecord-preserve-title', + workspaceCwd: workspace, + }); + await fs.mkdir(path.join(workspace, 'reports'), { recursive: true }); + await fs.writeFile(path.join(workspace, 'reports/sales.csv'), 'a,b\n'); + + const created = await store.upsertMany( + [ + { + title: 'Quarterly sales report', + description: 'Curated', + workspacePath: 'reports/sales.csv', + toolName: 'record_artifact', + }, + ], + { strict: true }, + ); + const artifactId = created.changes[0]?.artifactId; + + await store.upsertMany( + [ + { + title: 'sales.csv', + workspacePath: 'reports/sales.csv', + toolName: 'write_file', + }, + ], + { strict: true }, + ); + + expect((await store.list()).artifacts).toMatchObject([ + { + id: artifactId, + title: 'Quarterly sales report', + description: 'Curated', + toolName: 'record_artifact', + }, + ]); + }); + + it('keeps a curated title after write_file then record_artifact then write_file', async () => { + const store = new SessionArtifactStore({ + sessionId: 's2-workspace-rerecord-write-then-curate', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'report.html'), 'ok'); + + await store.upsertMany( + [ + { + title: 'report.html', + workspacePath: 'report.html', + toolName: 'write_file', + toolCallId: 'call-write', + }, + ], + { strict: true }, + ); + await store.upsertMany( + [ + { + title: 'Q3 Report', + workspacePath: 'report.html', + toolName: 'record_artifact', + toolCallId: 'call-record', + }, + ], + { strict: true }, + ); + await store.upsertMany( + [ + { + title: 'report.html', + workspacePath: 'report.html', + toolName: 'write_file', + }, + ], + { strict: true }, + ); + + expect((await store.list()).artifacts).toMatchObject([ + { + title: 'Q3 Report', + toolName: 'record_artifact', + toolCallId: 'call-record', + }, + ]); + }); + + it('keeps a curated title when a record_artifact hook re-records the same path', async () => { + const store = new SessionArtifactStore({ + sessionId: 's2-workspace-rerecord-record-hook', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'report.html'), 'ok'); + + await store.upsertMany( + [ + { + title: 'Q3 Report', + workspacePath: 'report.html', + source: 'tool', + toolName: 'record_artifact', + }, + ], + { strict: true }, + ); + await store.upsertMany( + [ + { + title: 'report.html', + workspacePath: 'report.html', + source: 'hook', + toolName: 'record_artifact', + hookEventName: 'PostToolUse', + }, + ], + { strict: true }, + ); + + expect((await store.list()).artifacts).toMatchObject([ + { + title: 'Q3 Report', + source: 'tool', + toolName: 'record_artifact', + }, + ]); + }); + + it('keeps a title curated without toolName when write_file later auto-records', async () => { + const store = new SessionArtifactStore({ + sessionId: 's2-workspace-rerecord-unattributed-title', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'notes.html'), 'ok'); + + await store.upsertMany( + [ + { + title: 'notes.html', + workspacePath: 'notes.html', + toolName: 'write_file', + }, + ], + { strict: true }, + ); + await store.upsertMany( + [ + { + title: 'Release notes', + workspacePath: 'notes.html', + }, + ], + { strict: true }, + ); + await store.upsertMany( + [ + { + title: 'notes.html', + workspacePath: 'notes.html', + toolName: 'write_file', + }, + ], + { strict: true }, + ); + + expect((await store.list()).artifacts).toMatchObject([ + { + title: 'Release notes', + }, + ]); + expect((await store.list()).artifacts[0]?.toolName).toBeUndefined(); + }); + + it('keeps a hook-curated title when write_file later auto-records the same path', async () => { + const store = new SessionArtifactStore({ + sessionId: 's2-workspace-rerecord-hook-title', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'sales.csv'), 'a,b\n'); + + await store.upsertMany( + [ + { + title: 'Quarterly sales report', + workspacePath: 'sales.csv', + source: 'hook', + toolName: 'write_file', + hookEventName: 'PostToolUse', + }, + ], + { strict: true }, + ); + await store.upsertMany( + [ + { + title: 'sales.csv', + workspacePath: 'sales.csv', + source: 'tool', + toolName: 'write_file', + }, + ], + { strict: true }, + ); + + expect((await store.list()).artifacts).toMatchObject([ + { + title: 'Quarterly sales report', + source: 'hook', + toolName: 'write_file', + }, + ]); + }); + + it('keeps the existing description when a re-record omits it', async () => { + const store = new SessionArtifactStore({ + sessionId: 's2-workspace-rerecord-keep-description', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'notes.html'), 'ok'); + + await store.upsertMany( + [ + { + title: 'Draft', + description: 'Keep me', + workspacePath: 'notes.html', + }, + ], + { strict: true }, + ); + + const updated = await store.upsertMany( + [ + { + title: 'Final notes', + workspacePath: 'notes.html', + }, + ], + { strict: true }, + ); + + expect(updated.changes[0]?.artifact).toMatchObject({ + title: 'Final notes', + description: 'Keep me', + }); + }); + + it('uses the later title when the same workspace path appears twice in one batch', async () => { + const store = new SessionArtifactStore({ + sessionId: 's2-workspace-batch-duplicate', + workspaceCwd: workspace, + }); + await fs.writeFile(path.join(workspace, 'dup.html'), 'ok'); + + const created = await store.upsertMany( + [ + { + title: 'First', + description: 'Old', + workspacePath: 'dup.html', + }, + { + title: 'Second', + description: 'New', + workspacePath: 'dup.html', + }, + ], + { strict: true }, + ); + + expect(created.changes).toHaveLength(1); + expect(created.changes[0]?.artifact).toMatchObject({ + title: 'Second', + description: 'New', + }); + }); + it('accepts trusted published file urls outside the workspace', async () => { const store = new SessionArtifactStore({ sessionId: 's2-published-file-url', diff --git a/packages/acp-bridge/src/sessionArtifacts.ts b/packages/acp-bridge/src/sessionArtifacts.ts index 69f900b6fcd..75309b5a6d8 100644 --- a/packages/acp-bridge/src/sessionArtifacts.ts +++ b/packages/acp-bridge/src/sessionArtifacts.ts @@ -1663,8 +1663,20 @@ function mergeBatchArtifact( delete merged.workspacePath; return merged; } + const refreshDisplay = + existing.storage === 'workspace' && + next.storage === 'workspace' && + shouldRefreshWorkspaceDisplay(next, existing); return { ...existing, + title: refreshDisplay ? next.title : existing.title, + description: refreshDisplay + ? (next.description ?? existing.description) + : existing.description, + toolName: refreshDisplay ? next.toolName : existing.toolName, + source: refreshDisplay ? next.source : existing.source, + hookEventName: refreshDisplay ? next.hookEventName : existing.hookEventName, + toolCallId: refreshDisplay ? next.toolCallId : existing.toolCallId, status: next.status, sizeBytes: mergeSizeBytes(existing, next), metadata: mergeMetadata(existing, next), @@ -1753,6 +1765,20 @@ function mergeArtifact( next.description = incoming.description; delete next.workspacePath; delete next.hideWorkspacePath; + } else if ( + existing.storage === 'workspace' && + incoming.storage === 'workspace' && + shouldRefreshWorkspaceDisplay(incoming, existing) + ) { + // Workspace re-records keep the same locator identity. Explicit + // record_artifact (or the same producer) may refresh the display + // name; write_file/hook auto-records must not clobber it. + next.title = incoming.title; + next.description = incoming.description ?? existing.description; + next.toolCallId = incoming.toolCallId; + next.toolName = incoming.toolName; + next.source = incoming.source; + next.hookEventName = incoming.hookEventName; } const changed = !publicArtifactsEqual( @@ -1778,6 +1804,23 @@ function shouldRecordEphemeralUnpin( ); } +function shouldRefreshWorkspaceDisplay( + incoming: Pick, + existing: Pick, +): boolean { + if (incoming.toolName === 'record_artifact' && incoming.source !== 'hook') { + return true; + } + if (!incoming.toolName) { + return true; + } + return ( + incoming.toolName === existing.toolName && + incoming.source === existing.source && + incoming.hookEventName === existing.hookEventName + ); +} + export function publicArtifactsEqual( a: DaemonSessionArtifact, b: DaemonSessionArtifact, diff --git a/packages/cli/src/commands/review/save-artifact.test.ts b/packages/cli/src/commands/review/save-artifact.test.ts index 32067fb1b8e..ce4b690f6b3 100644 --- a/packages/cli/src/commands/review/save-artifact.test.ts +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -467,8 +467,8 @@ describe('saveReviewArtifact', () => { }); expect(saved.path).toBe(join(root, '.qwen/reviews/review.json')); - // The registration value `record_artifact` wants, printed so the skill - // copies it verbatim. + // Canonical root-relative locator. record_artifact now prefers the + // absolute `path` as input and stores this form itself. expect(saved.workspacePath).toBe('.qwen/reviews/review.json'); expect(JSON.parse(readFileSync(saved.path, 'utf8'))).toMatchObject({ schemaVersion: 1, diff --git a/packages/cli/src/commands/review/save-artifact.ts b/packages/cli/src/commands/review/save-artifact.ts index 5c13b9b698d..a126f571895 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -53,9 +53,10 @@ export interface SavedReviewArtifact { /** Absolute path of the written document. */ path: string; /** - * The same path relative to the workspace root — the exact value - * `record_artifact` wants as `workspacePath`, so the skill copies it - * verbatim instead of re-deriving it from the absolute path. + * The same path relative to the workspace root. `record_artifact` now + * accepts the absolute `path` and stores this canonical form itself; + * keep emitting it so older runtimes and display surfaces can still + * use the root-relative locator. */ workspacePath: string; } 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 ef0f6044e6b..d11758ed4e9 100644 --- a/packages/cli/src/serve/routes/workspace-file-read.test.ts +++ b/packages/cli/src/serve/routes/workspace-file-read.test.ts @@ -14,6 +14,8 @@ import { buildRecordArtifactReminder, buildWorkspaceArtifactMetadata, Ignore, + makeFakeConfig, + RecordArtifactTool, type Config, } from '@qwen-code/qwen-code-core'; import { createServeApp } from '../server.js'; @@ -786,3 +788,141 @@ describe('artifact workspacePath contract (write_file ⇄ GET /file)', () => { } }); }); + +describe('artifact workspacePath contract (record_artifact ⇄ GET /file)', () => { + const ARTIFACT = 'name,value\norders,12\n'; + const signal = new AbortController().signal; + + async function recordedWorkspacePath( + sessionCwd: string, + workspacePath: string, + ): Promise { + const tool = new RecordArtifactTool( + makeFakeConfig({ targetDir: sessionCwd, cwd: sessionCwd }), + ); + const result = await tool + .build({ + title: 'Recorded report', + workspacePath, + }) + .execute(signal); + const recorded = result.artifacts?.[0]?.workspacePath; + if (!recorded) { + throw new Error( + `record_artifact did not return workspacePath: ${String(result.llmContent)}`, + ); + } + return recorded; + } + + it('round-trips a cwd-relative file recorded in an ordinary session', async () => { + const h = await makeHarness(); + try { + await fsp.writeFile(path.join(h.workspace, 'report.csv'), ARTIFACT); + + const workspacePath = await recordedWorkspacePath( + h.workspace, + 'report.csv', + ); + expect(workspacePath).toBe('report.csv'); + + const res = await request(h.app) + .get('/file') + .query({ path: workspacePath }) + .set('Host', loopbackHost()); + expect(res.status).toBe(200); + expect(res.body.content).toBe(ARTIFACT); + } finally { + await teardown(h); + } + }); + + it('round-trips a file recorded inside a worktree session', async () => { + const h = await makeHarness(); + try { + const sessionCwd = path.join( + h.workspace, + '.qwen', + 'worktrees', + 'my-feature', + ); + await fsp.mkdir(sessionCwd, { recursive: true }); + await fsp.writeFile(path.join(sessionCwd, 'report.csv'), ARTIFACT); + + const workspacePath = await recordedWorkspacePath( + sessionCwd, + 'report.csv', + ); + expect(workspacePath).toBe('.qwen/worktrees/my-feature/report.csv'); + + const res = await request(h.app) + .get('/file') + .query({ path: workspacePath }) + .set('Host', loopbackHost()); + expect(res.status).toBe(200); + expect(res.body.content).toBe(ARTIFACT); + } finally { + await teardown(h); + } + }); + + it('round-trips a workspace-absolute file recorded from a worktree session', async () => { + const h = await makeHarness(); + try { + await fsp.mkdir(path.join(h.workspace, 'docs'), { recursive: true }); + const abs = path.join(h.workspace, 'docs/review.md'); + await fsp.writeFile(abs, ARTIFACT); + const sessionCwd = path.join( + h.workspace, + '.qwen', + 'worktrees', + 'my-feature', + ); + await fsp.mkdir(sessionCwd, { recursive: true }); + + const workspacePath = await recordedWorkspacePath(sessionCwd, abs); + expect(workspacePath).toBe('docs/review.md'); + + const res = await request(h.app) + .get('/file') + .query({ path: workspacePath }) + .set('Host', loopbackHost()); + expect(res.status).toBe(200); + expect(res.body.content).toBe(ARTIFACT); + } finally { + await teardown(h); + } + }); + + it('does not let a worktree recording open a same-named file at the workspace root', async () => { + const h = await makeHarness(); + try { + await fsp.writeFile( + path.join(h.workspace, 'report.csv'), + 'name,value\nUNRELATED,1\n', + ); + const sessionCwd = path.join( + h.workspace, + '.qwen', + 'worktrees', + 'my-feature', + ); + await fsp.mkdir(sessionCwd, { recursive: true }); + await fsp.writeFile(path.join(sessionCwd, 'report.csv'), ARTIFACT); + + const workspacePath = await recordedWorkspacePath( + sessionCwd, + 'report.csv', + ); + const res = await request(h.app) + .get('/file') + .query({ path: workspacePath }) + .set('Host', loopbackHost()); + expect(res.status).toBe(200); + expect(res.body.content).toBe(ARTIFACT); + expect(res.body.content).not.toContain('UNRELATED'); + } finally { + await teardown(h); + } + }); +}); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index a77aff93194..977cbf04be3 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -8641,7 +8641,7 @@ export class Config { const { RecordArtifactTool } = await import( '../tools/record-artifact.js' ); - return new RecordArtifactTool(); + return new RecordArtifactTool(this); }); } if (this.isLspEnabled() && this.getLspClient()) { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index f40a7d6556a..924b7e41f42 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -205,14 +205,16 @@ export { buildRecordArtifactReminder, buildWorkspaceArtifactMetadata, } from './tools/write-file.js'; +export { + resolveBoundWorkspaceRoot, + toCanonicalWorkspaceArtifactPath, +} from './utils/workspace-artifact-path.js'; export type { ArtifactTool, ArtifactToolParams, } from './tools/artifact/artifact-tool.js'; -export type { - RecordArtifactTool, - RecordArtifactParams, -} from './tools/record-artifact.js'; +export { RecordArtifactTool } from './tools/record-artifact.js'; +export type { RecordArtifactParams } from './tools/record-artifact.js'; export type { ArtifactPublisher, PublishArtifactInput, diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index aaeace141c7..3e573a70812 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -1247,14 +1247,14 @@ After the Markdown report exists, create and register the structured review arti `save-artifact` resolves relative paths and its containment root against `--workspace-root` — **pass the main project directory explicitly, as the block above does**; without the flag it falls back to its own working directory. The flag is not decoration: the root anchors the containment checks (`isWithin` and the symlink walk), and an ambient-cwd root is only as trustworthy as wherever the command happened to run — from inside the untrusted PR worktree it would be the PR's own tree, the exact threat `comment-status`'s run-from-the-main-checkout rule exists to prevent. It used to prefer `QWEN_CODE_PROJECT_DIR`, which does not name the main checkout in any environment — the harness exports it as the session-storage directory under the runtime base — and every measured CI run burned minutes rediscovering that before improvising a workaround (measured; DESIGN.md — The artifact root that pointed at qwen-home). -For PR worktree mode, the findings and composed inputs were created inside `worktreePath`, while the durable report and output belong to the main project directory. Pass absolute paths for all four: resolve `--findings` and `--composed` against `worktreePath`, and resolve `--report` and `--out` against the main project directory. The worktree lives under the main project's `.qwen/tmp/`, so all four remain inside the session workspace accepted by the helper. `save-artifact` prints one JSON object on stdout — `{"path": "", "workspacePath": ""}`. Then call `record_artifact` in the current session with exactly this registration shape, copying `workspacePath` from that stdout object verbatim (do not re-derive it from the absolute path): +For PR worktree mode, the findings and composed inputs were created inside `worktreePath`, while the durable report and output belong to the main project directory. Pass absolute paths for all four: resolve `--findings` and `--composed` against `worktreePath`, and resolve `--report` and `--out` against the main project directory. The worktree lives under the main project's `.qwen/tmp/`, so all four remain inside the session workspace accepted by the helper. `save-artifact` prints one JSON object on stdout — `{"path": "", "workspacePath": ""}`. Then call `record_artifact` in the current session with exactly this registration shape, copying the absolute `path` into `workspacePath`. The tool verifies the file and stores the canonical workspace-root-relative form. Do not invent a different relative path, and do not use the old `path` tool parameter: ```json { "title": "Code review result", "kind": "other", "storage": "workspace", - "workspacePath": ".qwen/reviews/.json", + "workspacePath": "", "mimeType": "application/vnd.qwen.code-review+json", "metadata": { "artifactType": "code_review", diff --git a/packages/core/src/tools/record-artifact.test.ts b/packages/core/src/tools/record-artifact.test.ts index d8be93fca69..7e1f99ccd34 100644 --- a/packages/core/src/tools/record-artifact.test.ts +++ b/packages/core/src/tools/record-artifact.test.ts @@ -4,14 +4,70 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import { + chmod, + mkdir, + mkdtemp, + realpath, + rm, + symlink, + writeFile, +} from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { makeFakeConfig } from '../test-utils/config.js'; +import { ToolErrorType } from './tool-error.js'; import { RecordArtifactTool } from './record-artifact.js'; const signal = new AbortController().signal; +function makeTool(targetDir = '/') { + return new RecordArtifactTool(makeFakeConfig({ targetDir, cwd: targetDir })); +} + +async function createWorkspace(subdir?: string) { + const root = await realpath( + await mkdtemp(path.join(os.tmpdir(), 'record-artifact-')), + ); + const cwd = subdir ? path.join(root, subdir) : root; + if (subdir) { + await mkdir(cwd, { recursive: true }); + } + return { + root, + cwd, + tool: makeTool(cwd), + async write(rel: string, content = 'artifact-bytes') { + const abs = path.join(cwd, rel); + await mkdir(path.dirname(abs), { recursive: true }); + await writeFile(abs, content); + return abs; + }, + async cleanup() { + await rm(root, { recursive: true, force: true }); + }, + }; +} + describe('RecordArtifactTool', () => { + const workspaces: Array<{ cleanup: () => Promise }> = []; + + afterEach(async () => { + await Promise.all( + workspaces.splice(0).map((workspace) => workspace.cleanup()), + ); + }); + + async function workspace(subdir?: string) { + const created = await createWorkspace(subdir); + workspaces.push(created); + return created; + } + it('records a link artifact without touching the resource', async () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); const result = await tool .build({ title: 'Table details', @@ -31,25 +87,8 @@ describe('RecordArtifactTool', () => { ]); }); - it('records workspace and managed artifacts with inferred storage', async () => { - const tool = new RecordArtifactTool(); - - await expect( - tool - .build({ - title: 'Workspace report', - workspacePath: 'reports/summary.html', - }) - .execute(signal), - ).resolves.toMatchObject({ - artifacts: [ - { - title: 'Workspace report', - storage: 'workspace', - workspacePath: 'reports/summary.html', - }, - ], - }); + it('records a managed artifact with inferred storage', async () => { + const tool = makeTool(); await expect( tool @@ -69,32 +108,352 @@ describe('RecordArtifactTool', () => { }); }); - it('rejects published storage', () => { - const tool = new RecordArtifactTool(); + it('records a cwd-relative workspace file as a root-relative canonical path', async () => { + const ws = await workspace(); + await ws.write('reports/summary.html', 'ok'); - expect(() => - tool.build({ - title: 'Forged', - storage: 'published' as never, - url: 'https://example.com/artifact', - }), - ).toThrow(/allowed values/); + const result = await ws.tool + .build({ + title: 'Workspace report', + workspacePath: 'reports/summary.html', + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts).toMatchObject([ + { + title: 'Workspace report', + storage: 'workspace', + workspacePath: 'reports/summary.html', + sizeBytes: 'ok'.length, + }, + ]); + expect(String(result.llmContent)).toContain('status: available'); + expect(String(result.llmContent)).toContain( + 'workspacePath: reports/summary.html', + ); + expect(String(result.llmContent)).toContain( + `resolvedPath: ${path.join(ws.cwd, 'reports/summary.html')}`, + ); }); - it('requires exactly one locator', () => { - const tool = new RecordArtifactTool(); + it('normalizes a cwd-absolute workspace path to the canonical relative path', async () => { + const ws = await workspace(); + const abs = await ws.write('report.csv', 'a,b\n1,2\n'); - expect(() => - tool.build({ - title: 'Ambiguous', - workspacePath: 'report.html', - url: 'https://example.com/report', - }), - ).toThrow(/exactly one/); + const result = await ws.tool + .build({ + title: 'CSV report', + workspacePath: abs, + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts?.[0]).toMatchObject({ + storage: 'workspace', + workspacePath: 'report.csv', + }); + expect(String(result.llmContent)).toContain('status: available'); + expect(String(result.llmContent)).toContain('workspacePath: report.csv'); }); - it('rejects workspace paths that escape the workspace', () => { - const tool = new RecordArtifactTool(); + it('accepts a POSIX double-slash absolute locator inside the workspace', async () => { + if (process.platform === 'win32') { + return; + } + const ws = await workspace(); + await ws.write('report.csv', 'a,b\n'); + const doubled = `/${path.join(ws.cwd, 'report.csv')}`; + + const result = await ws.tool + .build({ + title: 'Double slash', + workspacePath: doubled, + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts?.[0]).toMatchObject({ + workspacePath: 'report.csv', + }); + }); + + it('accepts a long absolute locator when the canonical path is short', async () => { + const deep = Array.from({ length: 50 }, () => 'dddddddddd').join(path.sep); + const ws = await workspace(deep); + const abs = await ws.write('a.csv', '1'); + expect(abs.length).toBeGreaterThan(500); + + const result = await ws.tool + .build({ + title: 'Deep', + workspacePath: abs, + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts?.[0]).toMatchObject({ + workspacePath: 'a.csv', + }); + }); + + it('records a POSIX filename that contains a literal backslash', async () => { + if (process.platform === 'win32') { + return; + } + const ws = await workspace(); + const literal = 'reports\\summary.csv'; + await writeFile(path.join(ws.cwd, literal), 'a,b\n'); + + const result = await ws.tool + .build({ + title: 'Literal backslash', + workspacePath: literal, + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts?.[0]).toMatchObject({ + workspacePath: 'reports\\summary.csv', + }); + }); + + it('normalizes Windows-style relative separators to posix', async () => { + const ws = await workspace(); + await ws.write('reports/summary.html', 'ok'); + + const result = await ws.tool + .build({ + title: 'Windows-style relative report', + workspacePath: 'reports\\summary.html', + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts?.[0]).toMatchObject({ + workspacePath: 'reports/summary.html', + }); + }); + + it('canonicalizes a worktree-relative path against the bound workspace root', async () => { + const ws = await workspace(path.join('.qwen', 'worktrees', 'my-feature')); + await ws.write('report.csv', 'a,b\n'); + + const result = await ws.tool + .build({ + title: 'Worktree report', + workspacePath: 'report.csv', + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts?.[0]).toMatchObject({ + workspacePath: '.qwen/worktrees/my-feature/report.csv', + }); + expect(String(result.llmContent)).toContain( + 'workspacePath: .qwen/worktrees/my-feature/report.csv', + ); + }); + + it('does not fall back to the workspace root when a relative path misses in the worktree cwd', async () => { + const ws = await workspace(path.join('.qwen', 'worktrees', 'my-feature')); + await mkdir(path.join(ws.root, 'docs'), { recursive: true }); + await writeFile(path.join(ws.root, 'docs/review.md'), '# review'); + + const result = await ws.tool + .build({ + title: 'Root review', + workspacePath: 'docs/review.md', + }) + .execute(signal); + + expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND); + expect(String(result.llmContent)).not.toContain('Recorded artifact'); + }); + + it('accepts an absolute path inside the bound workspace from a worktree session', async () => { + const ws = await workspace(path.join('.qwen', 'worktrees', 'my-feature')); + const abs = path.join(ws.root, 'docs/review.md'); + await mkdir(path.dirname(abs), { recursive: true }); + await writeFile(abs, '# review'); + + const result = await ws.tool + .build({ + title: 'Absolute review', + workspacePath: abs, + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts?.[0]).toMatchObject({ + workspacePath: 'docs/review.md', + }); + }); + + it('accepts an absolute path that names the workspace through a symlink prefix', async () => { + const ws = await workspace(); + await ws.write('report.csv', 'a,b\n'); + const aliasRoot = path.join( + os.tmpdir(), + `record-artifact-alias-${process.pid}-${Date.now()}`, + ); + await symlink(ws.root, aliasRoot); + workspaces.push({ + cleanup: async () => { + await rm(aliasRoot, { force: true }); + }, + }); + + const result = await ws.tool + .build({ + title: 'Symlink prefix', + workspacePath: path.join(aliasRoot, 'report.csv'), + }) + .execute(signal); + + expect(result.error).toBeUndefined(); + expect(result.artifacts?.[0]).toMatchObject({ + workspacePath: 'report.csv', + }); + }); + + it('reports missing instead of outside when a symlink-root absolute path has no parent', async () => { + const ws = await workspace(); + const aliasRoot = path.join( + os.tmpdir(), + `record-artifact-missing-${process.pid}-${Date.now()}`, + ); + await symlink(ws.root, aliasRoot); + workspaces.push({ + cleanup: async () => { + await rm(aliasRoot, { force: true }); + }, + }); + + const result = await ws.tool + .build({ + title: 'Missing parent', + workspacePath: path.join(aliasRoot, 'no-such-dir', 'a.csv'), + }) + .execute(signal); + + expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND); + expect(String(result.llmContent)).not.toContain('outside the workspace'); + }); + + it('rejects a canonical workspacePath that fails display safety checks', async () => { + const ws = await workspace(); + const nasty = path.join(ws.cwd, 'reports', 'actual\u202eforged.csv'); + await mkdir(path.dirname(nasty), { recursive: true }); + await writeFile(nasty, 'x'); + await symlink(nasty, path.join(ws.cwd, 'safe.csv')); + + const result = await ws.tool + .build({ + title: 'Safe link', + workspacePath: 'safe.csv', + }) + .execute(signal); + + expect(result.artifacts).toBeUndefined(); + expect(result.error?.type).toBe(ToolErrorType.INVALID_TOOL_PARAMS); + }); + + it('rejects a fifo workspacePath', async () => { + if (process.platform === 'win32') { + return; + } + const ws = await workspace(); + const fifo = path.join(ws.cwd, 'pipe.fifo'); + const created = spawnSync('mkfifo', [fifo]); + if (created.status !== 0) { + return; + } + + const result = await ws.tool + .build({ + title: 'Fifo', + workspacePath: 'pipe.fifo', + }) + .execute(signal); + + expect(result.artifacts).toBeUndefined(); + expect(result.error?.type).toBe(ToolErrorType.TARGET_NOT_REGULAR_FILE); + }); + + it('classifies an unreadable path as permission denied', async () => { + if (process.platform === 'win32' || process.getuid?.() === 0) { + return; + } + const ws = await workspace(); + const hidden = path.join(ws.cwd, 'hidden'); + await mkdir(hidden); + await writeFile(path.join(hidden, 'a.csv'), 'x'); + await chmod(hidden, 0); + try { + const result = await ws.tool + .build({ + title: 'Hidden', + workspacePath: 'hidden/a.csv', + }) + .execute(signal); + expect(result.error?.type).toBe(ToolErrorType.PERMISSION_DENIED); + expect(result.artifacts).toBeUndefined(); + } finally { + await chmod(hidden, 0o755); + } + }); + + it('rejects a wrong workspace-folder prefix instead of reporting success', async () => { + const ws = await workspace(); + await ws.write('report.csv', 'a,b\n'); + + const result = await ws.tool + .build({ + title: 'Wrong prefix', + workspacePath: 'w/agent/report.csv', + }) + .execute(signal); + + expect(result.artifacts).toBeUndefined(); + expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND); + expect(String(result.llmContent)).not.toContain('Recorded artifact'); + expect(String(result.llmContent)).toContain('file not found'); + expect(String(result.llmContent)).toContain('report.csv'); + expect(String(result.llmContent)).toContain('w/agent/'); + }); + + it('rejects a missing workspace file instead of reporting success', async () => { + const ws = await workspace(); + + const result = await ws.tool + .build({ + title: 'Missing', + workspacePath: 'missing.csv', + }) + .execute(signal); + + expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND); + expect(String(result.llmContent)).not.toContain('Recorded artifact'); + }); + + it('rejects a directory workspacePath', async () => { + const ws = await workspace(); + await mkdir(path.join(ws.cwd, 'reports')); + + const result = await ws.tool + .build({ + title: 'Directory', + workspacePath: 'reports', + }) + .execute(signal); + + expect(result.error?.type).toBe(ToolErrorType.TARGET_IS_DIRECTORY); + expect(String(result.llmContent)).not.toContain('Recorded artifact'); + }); + + it('rejects a workspace-relative path that escapes the execution directory', () => { + const tool = makeTool(); for (const workspacePath of [ '../secret.txt', @@ -102,6 +461,45 @@ describe('RecordArtifactTool', () => { '..\\..\\secret.txt', 'reports\\..\\..\\secret.txt', 'reports/..\\..\\secret.txt', + ]) { + expect(() => + tool.build({ + title: 'Escape', + workspacePath, + }), + ).toThrow(/workspacePath/); + } + }); + + it('rejects UNC locators before resolving them', () => { + const tool = makeTool(); + + const locators = [ + '\\\\attacker.example\\share\\report.csv', + '\\\\?\\UNC\\attacker.example\\share\\report.csv', + '\\??\\UNC\\attacker.example\\share\\report.csv', + '\\\\?\\GLOBALROOT\\Device\\Mup\\attacker.example\\share\\report.csv', + ]; + if (process.platform === 'win32') { + locators.push('//attacker.example/share/report.csv'); + } + for (const workspacePath of locators) { + expect(() => + tool.build({ + title: 'UNC', + workspacePath, + }), + ).toThrow(/workspacePath/); + } + }); + + it('rejects Windows drive and UNC locators on POSIX', () => { + if (process.platform === 'win32') { + return; + } + const tool = makeTool(); + + for (const workspacePath of [ 'C:\\tmp\\report.html', 'C:/tmp/report.html', 'C:tmp\\report.html', @@ -117,34 +515,168 @@ describe('RecordArtifactTool', () => { } }); - it('accepts safe workspace-relative artifact paths', async () => { - const tool = new RecordArtifactTool(); - - await expect( - tool - .build({ - title: 'Safe report', - workspacePath: 'reports/summary.html', - }) - .execute(signal), - ).resolves.toMatchObject({ - artifacts: [{ workspacePath: 'reports/summary.html' }], + it('rejects an absolute path outside the execution directory', async () => { + const ws = await workspace(); + const outside = await realpath( + await mkdtemp(path.join(os.tmpdir(), 'record-artifact-outside-')), + ); + const outsideFile = path.join(outside, 'secret.csv'); + await writeFile(outsideFile, 'secret'); + workspaces.push({ + cleanup: async () => { + await rm(outside, { recursive: true, force: true }); + }, }); - await expect( - tool - .build({ - title: 'Windows-style relative report', - workspacePath: 'reports\\summary.html', - }) - .execute(signal), - ).resolves.toMatchObject({ - artifacts: [{ workspacePath: 'reports\\summary.html' }], + expect(() => + ws.tool.build({ + title: 'Outside', + workspacePath: outsideFile, + }), + ).toThrow(/workspace/); + }); + + it('rejects a symlink that escapes the execution directory', async () => { + const ws = await workspace(); + const outside = await realpath( + await mkdtemp(path.join(os.tmpdir(), 'record-artifact-link-')), + ); + const secret = path.join(outside, 'secret.csv'); + await writeFile(secret, 'secret'); + await symlink(secret, path.join(ws.cwd, 'escape.csv')); + workspaces.push({ + cleanup: async () => { + await rm(outside, { recursive: true, force: true }); + }, }); + + const result = await ws.tool + .build({ + title: 'Escape link', + workspacePath: 'escape.csv', + }) + .execute(signal); + + expect(result.error?.type).toBe(ToolErrorType.PATH_NOT_IN_WORKSPACE); + expect(String(result.llmContent)).not.toContain('Recorded artifact'); + }); + + it('rejects a workspace symlink whose target is a UNC path', async () => { + const ws = await workspace(); + try { + await symlink( + '\\\\attacker.example\\share\\report.csv', + path.join(ws.cwd, 'report.csv'), + ); + } catch { + return; + } + + const result = await ws.tool + .build({ + title: 'UNC link', + workspacePath: 'report.csv', + }) + .execute(signal); + + expect(result.artifacts).toBeUndefined(); + expect(result.error?.type).toBe(ToolErrorType.PATH_NOT_IN_WORKSPACE); + expect(String(result.llmContent)).not.toContain('Recorded artifact'); + }); + + it('rejects a UNC target reached through an intermediate directory symlink', async () => { + const ws = await workspace(); + try { + await symlink('\\\\attacker.example\\share', path.join(ws.cwd, 'docs')); + } catch { + return; + } + + const result = await ws.tool + .build({ + title: 'UNC dir', + workspacePath: 'docs/q3.csv', + }) + .execute(signal); + + expect(result.artifacts).toBeUndefined(); + expect(result.error?.type).toBe(ToolErrorType.PATH_NOT_IN_WORKSPACE); + expect(String(result.llmContent)).not.toContain('Recorded artifact'); + }); + + it('rejects a two-hop symlink chain that ends at a UNC path', async () => { + const ws = await workspace(); + try { + await symlink( + '\\\\attacker.example\\share\\x.csv', + path.join(ws.cwd, 'b.csv'), + ); + await symlink('b.csv', path.join(ws.cwd, 'a.csv')); + } catch { + return; + } + + const result = await ws.tool + .build({ + title: 'UNC chain', + workspacePath: 'a.csv', + }) + .execute(signal); + + expect(result.artifacts).toBeUndefined(); + expect(result.error?.type).toBe(ToolErrorType.PATH_NOT_IN_WORKSPACE); + expect(String(result.llmContent)).not.toContain('Recorded artifact'); + }); + + it('names the legacy path field instead of asking for a locator', () => { + const tool = makeTool(); + + expect(() => + tool.build({ + title: 'Legacy path', + path: 'report.csv', + } as never), + ).toThrow(/"path" is not supported.*workspacePath/); + }); + + it('rejects unknown fields such as artifactType', () => { + const tool = makeTool(); + + expect(() => + tool.build({ + title: 'Unknown field', + url: 'https://example.com/resource', + artifactType: 'csv', + } as never), + ).toThrow(/additional properties/); + }); + + it('rejects published storage', () => { + const tool = makeTool(); + + expect(() => + tool.build({ + title: 'Forged', + storage: 'published' as never, + url: 'https://example.com/artifact', + }), + ).toThrow(/allowed values/); + }); + + it('requires exactly one locator', () => { + const tool = makeTool(); + + expect(() => + tool.build({ + title: 'Ambiguous', + workspacePath: 'report.html', + url: 'https://example.com/report', + }), + ).toThrow(/exactly one/); }); it('rejects unsafe urls before reporting success', () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); expect(() => tool.build({ @@ -162,7 +694,7 @@ describe('RecordArtifactTool', () => { }); it('rejects path-like managed ids before reporting success', () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); for (const managedId of ['../secret', 'folder/item', 'folder\\item']) { expect(() => @@ -175,7 +707,7 @@ describe('RecordArtifactTool', () => { }); it('rejects storage values that do not match the locator', () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); expect(() => tool.build({ @@ -187,7 +719,7 @@ describe('RecordArtifactTool', () => { }); it('rejects artifact metadata that the daemon store would drop', () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); expect(() => tool.build({ @@ -209,7 +741,7 @@ describe('RecordArtifactTool', () => { }); it('rejects invalid artifact sizes before reporting success', () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); for (const sizeBytes of [-1, 1.5, Number.MAX_SAFE_INTEGER + 1]) { expect(() => @@ -223,7 +755,7 @@ describe('RecordArtifactTool', () => { }); it('rejects unsafe display markup before reporting success', () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); expect(() => tool.build({ @@ -304,7 +836,7 @@ describe('RecordArtifactTool', () => { }); it('allows benign words ending with on before equals signs', () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); expect(() => tool.build({ @@ -316,7 +848,7 @@ describe('RecordArtifactTool', () => { }); it('rejects Unicode control characters before reporting success', () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); expect(() => tool.build({ @@ -349,7 +881,7 @@ describe('RecordArtifactTool', () => { }); it('accepts line whitespace in descriptions but not titles', async () => { - const tool = new RecordArtifactTool(); + const tool = makeTool(); await expect( tool diff --git a/packages/core/src/tools/record-artifact.ts b/packages/core/src/tools/record-artifact.ts index 5cd3af1a628..7bb464d9f63 100644 --- a/packages/core/src/tools/record-artifact.ts +++ b/packages/core/src/tools/record-artifact.ts @@ -4,7 +4,16 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { lstatSync, readlinkSync, realpathSync } from 'node:fs'; +import fs from 'node:fs/promises'; import path from 'node:path'; +import type { Config } from '../config/config.js'; +import { isNodeError } from '../utils/errors.js'; +import { isWithinRoot } from '../utils/fileUtils.js'; +import { + resolveBoundWorkspaceRoot, + toCanonicalWorkspaceArtifactPath, +} from '../utils/workspace-artifact-path.js'; import type { ToolArtifact, ToolArtifactKind, @@ -13,6 +22,7 @@ import type { ToolResult, } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; +import { ToolErrorType } from './tool-error.js'; import { ToolDisplayNames, ToolNames } from './tool-names.js'; export interface RecordArtifactParams { @@ -28,28 +38,88 @@ 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, 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. When the session creates a remote resource, such as a pull request, issue, or comment submitted via gh, record its URL with kind "link" and the url locator so the user can reopen it 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, notebook, CSV, and Excel 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, JSON, and plain text, and for files produced outside write_file. When the session creates a remote resource, such as a pull request, issue, or comment submitted via gh, record its URL with kind "link" and the url locator so the user can reopen it later. -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.`; +Provide exactly one locator: workspacePath, managedId, or url. Do not use the old "path" field. Use the Artifact tool, not record_artifact, for published interactive HTML artifacts. + +For workspace files, workspacePath must be relative to the current execution directory (for example "report.csv" or "reports/summary.html") or an absolute path inside the bound workspace. Do not add workspace folder prefixes such as "w/agent/", and do not use ".." to walk up from a worktree. This tool resolves the file, verifies it exists as a regular file inside the workspace, then stores a workspace-root-relative canonical workspacePath. A successful result includes status=available, the canonical workspacePath, and resolvedPath. If verification fails, the tool returns an error — do not tell the user the artifact can be opened or downloaded.`; export const ARTIFACT_TITLE_MAX_LENGTH = 200; export const ARTIFACT_WORKSPACE_PATH_MAX_LENGTH = 500; +const WORKSPACE_PATH_HINT = + '"workspacePath" must be relative to the current execution directory (for example "report.csv") or an absolute path inside the workspace. Do not add workspace folder prefixes such as "w/agent/".'; + +type WorkspaceLocatorSuccess = { + ok: true; + workspacePath: string; + resolvedPath: string; + sizeBytes: number; +}; + +type WorkspaceLocatorFailure = { + ok: false; + message: string; + type: ToolErrorType; +}; + +type WorkspaceLocatorResult = WorkspaceLocatorSuccess | WorkspaceLocatorFailure; + class RecordArtifactInvocation extends BaseToolInvocation< RecordArtifactParams, ToolResult > { + constructor( + params: RecordArtifactParams, + private readonly config: Config, + ) { + super(params); + } + override getDescription(): string { return `Recording artifact ${this.params.title}`; } - execute(_signal: AbortSignal): Promise { + async execute(_signal: AbortSignal): Promise { + const workspacePathInput = trimOptional(this.params.workspacePath); + if (workspacePathInput) { + const locator = await resolveWorkspaceArtifactLocator( + workspacePathInput, + this.config, + ); + if (!locator.ok) { + return { + llmContent: locator.message, + returnDisplay: locator.message, + error: { + message: locator.message, + type: locator.type, + }, + }; + } + + const artifact: ToolArtifact = { + title: this.params.title.trim(), + kind: this.params.kind, + storage: 'workspace', + description: trimOptional(this.params.description), + workspacePath: locator.workspacePath, + mimeType: trimOptional(this.params.mimeType), + sizeBytes: this.params.sizeBytes ?? locator.sizeBytes, + metadata: this.params.metadata, + }; + return { + llmContent: formatWorkspaceSuccess(artifact.title, locator), + returnDisplay: formatWorkspaceSuccess(artifact.title, locator), + artifacts: [artifact], + }; + } + const artifact: ToolArtifact = { title: this.params.title.trim(), kind: this.params.kind, storage: this.params.storage ?? inferStorage(this.params), description: trimOptional(this.params.description), - workspacePath: trimOptional(this.params.workspacePath), managedId: trimOptional(this.params.managedId), url: trimOptional(this.params.url), mimeType: trimOptional(this.params.mimeType), @@ -57,11 +127,11 @@ class RecordArtifactInvocation extends BaseToolInvocation< metadata: this.params.metadata, }; - return Promise.resolve({ + return { llmContent: `Recorded artifact "${artifact.title}".`, returnDisplay: `Recorded artifact **${artifact.title}**.`, artifacts: [artifact], - }); + }; } } @@ -71,7 +141,7 @@ export class RecordArtifactTool extends BaseDeclarativeTool< > { static readonly Name: string = ToolNames.RECORD_ARTIFACT; - constructor() { + constructor(private readonly config: Config) { super( RecordArtifactTool.Name, ToolDisplayNames.RECORD_ARTIFACT, @@ -79,6 +149,7 @@ export class RecordArtifactTool extends BaseDeclarativeTool< Kind.Other, { type: 'object', + additionalProperties: false, properties: { title: { type: 'string', @@ -112,7 +183,7 @@ export class RecordArtifactTool extends BaseDeclarativeTool< workspacePath: { type: 'string', description: - 'Workspace-relative path for a file produced in the current workspace.', + 'Path relative to the current execution directory, or an absolute path inside the bound workspace. The tool verifies the file and stores a workspace-root-relative canonical path.', }, managedId: { type: 'string', @@ -157,6 +228,16 @@ export class RecordArtifactTool extends BaseDeclarativeTool< ); } + override validateToolParams(params: RecordArtifactParams): string | null { + if (hasLegacyPathField(params)) { + return ( + '"path" is not supported; use "workspacePath" for a file in the current execution directory ' + + '(relative to that directory, or an absolute path inside the workspace). Example: "report.csv".' + ); + } + return super.validateToolParams(params); + } + protected override validateToolParamValues( params: RecordArtifactParams, ): string | null { @@ -214,7 +295,10 @@ export class RecordArtifactTool extends BaseDeclarativeTool< } if (params.workspacePath) { - const workspacePathError = validateWorkspacePath(params.workspacePath); + const workspacePathError = validateWorkspacePath( + params.workspacePath, + this.config.getTargetDir(), + ); if (workspacePathError) { return workspacePathError; } @@ -255,7 +339,7 @@ export class RecordArtifactTool extends BaseDeclarativeTool< protected createInvocation( params: RecordArtifactParams, ): ToolInvocation { - return new RecordArtifactInvocation(params); + return new RecordArtifactInvocation(params, this.config); } } @@ -362,23 +446,34 @@ export function hasUnsafeDisplayPayload(value: string): boolean { ); } -function validateWorkspacePath(value: string): string | null { +function validateWorkspacePath(value: string, cwd: string): string | null { const trimmed = value.trim(); - const stringError = validateString( - trimmed, - 'workspacePath', - ARTIFACT_WORKSPACE_PATH_MAX_LENGTH, - true, - ); - if (stringError) { - return stringError; + if (!trimmed) { + return 'Missing or empty "workspacePath"'; } - if ( - path.isAbsolute(trimmed) || - path.win32.isAbsolute(trimmed) || - /^[A-Za-z]:/.test(trimmed) - ) { - return '"workspacePath" must be relative to the workspace'; + if (hasControlCharacter(trimmed) || hasUnsafeDisplayPayload(trimmed)) { + return hasControlCharacter(trimmed) + ? '"workspacePath" contains control characters' + : '"workspacePath" contains unsafe markup'; + } + if (isRedirectorRoutedPath(trimmed) || isForeignWindowsAbsolute(trimmed)) { + return WORKSPACE_PATH_HINT; + } + if (isAbsoluteWorkspaceInput(trimmed)) { + if (trimmed.length > 4096) { + return '"workspacePath" exceeds 4096 characters'; + } + const root = resolveBoundWorkspaceRoot( + tryResolveForContainment(path.resolve(cwd)) ?? path.resolve(cwd), + ); + const comparable = tryResolveForContainment(path.resolve(trimmed)); + if (comparable && !isWithinRoot(comparable, root)) { + return '"workspacePath" must stay inside the workspace'; + } + return null; + } + if (trimmed.length > ARTIFACT_WORKSPACE_PATH_MAX_LENGTH) { + return `"workspacePath" exceeds ${ARTIFACT_WORKSPACE_PATH_MAX_LENGTH} characters`; } const portableNormalized = path.posix.normalize(trimmed.replace(/\\/g, '/')); if ( @@ -386,7 +481,7 @@ function validateWorkspacePath(value: string): string | null { portableNormalized.startsWith('../') || path.posix.isAbsolute(portableNormalized) ) { - return '"workspacePath" must stay inside the workspace'; + return '"workspacePath" must stay inside the current execution directory'; } return null; } @@ -451,3 +546,359 @@ function isArtifactKind(kind: string): kind is ToolArtifactKind { kind === 'other' ); } + +function hasLegacyPathField(params: RecordArtifactParams): boolean { + const raw = params as RecordArtifactParams & { path?: unknown }; + return ( + Object.prototype.hasOwnProperty.call(raw, 'path') && + raw.path != null && + raw.path !== '' + ); +} + +function isRedirectorRoutedPath(value: string): boolean { + // NT junctions often readlink as `\??\UNC\host\share`. Treat that as `\\?\`. + const slashes = value.replace(/\//g, '\\').replace(/^\\\?\?\\/, '\\\\?\\'); + if (/\\Device\\Mup\\/i.test(slashes)) { + return true; + } + // On POSIX, `//repo/file` is a local absolute path, not SMB. + const posixDoubleSlash = + process.platform !== 'win32' && + !value.includes('\\') && + /^\/\//.test(value); + if ( + !posixDoubleSlash && + (/^\\\\[^\\?]+(?:\\|$)/.test(slashes) || + /^\\\\\?\\[Uu][Nn][Cc]\\/.test(slashes)) + ) { + return true; + } + // `\\?\C:\...` is a local drive; every other `\\?\` form (UNC, Mup, + // GLOBALROOT, Volume GUID) can leave the machine via the redirector. + return /^\\\\\?\\/.test(slashes) && !/^\\\\\?\\[A-Za-z]:\\/.test(slashes); +} + +function isForeignWindowsAbsolute(value: string): boolean { + if (process.platform === 'win32') { + return false; + } + return /^[A-Za-z]:/.test(value) || value.startsWith('\\'); +} + +function isAbsoluteWorkspaceInput(value: string): boolean { + return ( + path.isAbsolute(value) || + (process.platform === 'win32' && path.win32.isAbsolute(value)) + ); +} + +function formatWorkspaceSuccess( + title: string, + locator: WorkspaceLocatorSuccess, +): string { + return [ + `Recorded artifact "${title}".`, + 'status: available', + `workspacePath: ${locator.workspacePath}`, + `resolvedPath: ${locator.resolvedPath}`, + ].join('\n'); +} + +function locatorFailure( + type: ToolErrorType, + message: string, +): WorkspaceLocatorFailure { + return { ok: false, type, message }; +} + +async function resolveExistingDir(dir: string): Promise { + const resolved = path.resolve(dir); + try { + return await fs.realpath(resolved); + } catch { + return resolved; + } +} + +/** + * Compare absolute locators against a realpath'd workspace root. `path.resolve` + * alone keeps macOS `/var` vs `/private/var` (and similar symlink roots) in + * different namespaces and false-rejects files that are inside the workspace. + * Returns undefined when neither the path nor its parent can be realpath'd, so + * callers do not mix namespaces and call a missing-but-inside path "outside". + */ +function tryResolveForContainment(absolutePath: string): string | undefined { + const resolved = path.resolve(absolutePath); + if (pathHasRedirectorHop(resolved)) { + return undefined; + } + try { + return realpathSync(resolved); + } catch { + try { + const parent = path.dirname(resolved); + if (pathHasRedirectorHop(parent)) { + return undefined; + } + return path.join(realpathSync(parent), path.basename(resolved)); + } catch { + return undefined; + } + } +} + +const REDIRECTOR_HOP_LIMIT = 8; + +function pathHasRedirectorHop(absolutePath: string): boolean { + if (isRedirectorRoutedPath(absolutePath)) { + return true; + } + const resolved = path.resolve(absolutePath); + const root = path.parse(resolved).root; + const relative = path.relative(root, resolved); + if (!relative || relative.startsWith('..')) { + return symlinkChainHitsRedirector(resolved); + } + let acc = root; + for (const segment of relative.split(path.sep).filter(Boolean)) { + acc = path.join(acc, segment); + if (isRedirectorRoutedPath(acc) || symlinkChainHitsRedirector(acc)) { + return true; + } + } + return false; +} + +function symlinkChainHitsRedirector(absolutePath: string): boolean { + let current = absolutePath; + for (let hop = 0; hop < REDIRECTOR_HOP_LIMIT; hop++) { + let lst; + try { + lst = lstatSync(current); + } catch { + return false; + } + if (!lst.isSymbolicLink()) { + return false; + } + let target: string; + try { + target = readlinkSync(current); + } catch { + return false; + } + if (isRedirectorRoutedPath(target)) { + return true; + } + const next = path.isAbsolute(target) + ? target + : path.resolve(path.dirname(current), target); + if (isRedirectorRoutedPath(next)) { + return true; + } + current = next; + } + return false; +} + +async function resolveWorkspaceArtifactLocator( + rawPath: string, + config: Config, +): Promise { + const cwd = await resolveExistingDir(config.getTargetDir()); + const root = await resolveExistingDir(resolveBoundWorkspaceRoot(cwd)); + const first = workspacePathCandidate(rawPath, cwd, root, true); + if (!first.ok) { + return first; + } + const inspected = await inspectWorkspaceCandidate( + first.path, + rawPath, + cwd, + root, + ); + if ( + inspected.ok || + inspected.type !== ToolErrorType.FILE_NOT_FOUND || + process.platform === 'win32' || + !rawPath.includes('\\') + ) { + return inspected; + } + const fallback = workspacePathCandidate(rawPath, cwd, root, false); + if (!fallback.ok || fallback.path === first.path) { + return inspected; + } + return inspectWorkspaceCandidate(fallback.path, rawPath, cwd, root); +} + +function workspacePathCandidate( + locator: string, + cwd: string, + root: string, + preservePosixBackslash: boolean, +): { ok: true; path: string } | WorkspaceLocatorFailure { + if (isRedirectorRoutedPath(locator) || isForeignWindowsAbsolute(locator)) { + return locatorFailure( + ToolErrorType.INVALID_TOOL_PARAMS, + WORKSPACE_PATH_HINT, + ); + } + if (isAbsoluteWorkspaceInput(locator)) { + const absolute = path.resolve(locator); + const comparable = tryResolveForContainment(absolute); + if (comparable && !isWithinRoot(comparable, root)) { + return locatorFailure( + ToolErrorType.PATH_NOT_IN_WORKSPACE, + `Failed to record artifact: "${locator}" is outside the workspace.\n${WORKSPACE_PATH_HINT}`, + ); + } + return { ok: true, path: absolute }; + } + + const relative = + preservePosixBackslash && process.platform !== 'win32' + ? locator + : locator.replace(/\\/g, '/'); + return { ok: true, path: path.resolve(cwd, relative) }; +} + +async function inspectWorkspaceCandidate( + candidate: string, + rawPath: string, + cwd: string, + root: string, +): Promise { + if (pathHasRedirectorHop(candidate)) { + return locatorFailure( + ToolErrorType.PATH_NOT_IN_WORKSPACE, + `Failed to record artifact: "${rawPath}" resolves outside the workspace.\n${WORKSPACE_PATH_HINT}`, + ); + } + + let lst; + try { + lst = await fs.lstat(candidate); + } catch (error) { + return pathInspectFailure( + error, + candidate, + rawPath, + `Failed to record artifact: could not inspect "${candidate}" (${error instanceof Error ? error.message : String(error)}).`, + ); + } + + if (lst.isDirectory()) { + return locatorFailure( + ToolErrorType.TARGET_IS_DIRECTORY, + `Failed to record artifact: "${candidate}" is a directory, not a file.\n${WORKSPACE_PATH_HINT}`, + ); + } + + let resolved: string; + try { + resolved = await fs.realpath(candidate); + } catch (error) { + return pathInspectFailure( + error, + candidate, + rawPath, + `Failed to record artifact: could not resolve "${rawPath}" (${error instanceof Error ? error.message : String(error)}).`, + ); + } + + if (!isWithinRoot(resolved, root)) { + return locatorFailure( + ToolErrorType.PATH_NOT_IN_WORKSPACE, + `Failed to record artifact: "${rawPath}" resolves outside the workspace.\n${WORKSPACE_PATH_HINT}`, + ); + } + + let st; + try { + st = await fs.stat(resolved); + } catch (error) { + return pathInspectFailure( + error, + resolved, + rawPath, + `Failed to record artifact: ${error instanceof Error ? error.message : String(error)}`, + ); + } + if (!st.isFile()) { + return locatorFailure( + st.isDirectory() + ? ToolErrorType.TARGET_IS_DIRECTORY + : ToolErrorType.TARGET_NOT_REGULAR_FILE, + `Failed to record artifact: "${resolved}" is not a regular file.\n${WORKSPACE_PATH_HINT}`, + ); + } + + const workspacePath = toCanonicalWorkspaceArtifactPath(resolved, cwd); + if (!workspacePath) { + return locatorFailure( + ToolErrorType.PATH_NOT_IN_WORKSPACE, + `Failed to record artifact: "${rawPath}" could not be converted to a workspace-root-relative path.\n${WORKSPACE_PATH_HINT}`, + ); + } + const canonicalError = validateString( + workspacePath, + 'workspacePath', + ARTIFACT_WORKSPACE_PATH_MAX_LENGTH, + true, + ); + if (canonicalError) { + return locatorFailure( + ToolErrorType.INVALID_TOOL_PARAMS, + canonicalError.includes('exceeds') + ? `Failed to record artifact: the stored workspace-root-relative path "${workspacePath}" exceeds ${ARTIFACT_WORKSPACE_PATH_MAX_LENGTH} characters.` + : canonicalError, + ); + } + + return { + ok: true, + workspacePath, + resolvedPath: resolved, + sizeBytes: st.size, + }; +} + +function classifyPathError(error: unknown): ToolErrorType { + if (!isNodeError(error)) { + return ToolErrorType.EXECUTION_FAILED; + } + if (error.code === 'ENOENT' || error.code === 'ENOTDIR') { + return ToolErrorType.FILE_NOT_FOUND; + } + if (error.code === 'EACCES' || error.code === 'EPERM') { + return ToolErrorType.PERMISSION_DENIED; + } + return ToolErrorType.EXECUTION_FAILED; +} + +function pathInspectFailure( + error: unknown, + candidate: string, + rawPath: string, + fallbackMessage: string, +): WorkspaceLocatorFailure { + const type = classifyPathError(error); + if (type === ToolErrorType.FILE_NOT_FOUND) { + return locatorFailure( + type, + [ + `Failed to record artifact: file not found at "${candidate}".`, + WORKSPACE_PATH_HINT, + ].join('\n'), + ); + } + if (type === ToolErrorType.PERMISSION_DENIED) { + return locatorFailure( + type, + `Failed to record artifact: permission denied for "${rawPath}".\n${WORKSPACE_PATH_HINT}`, + ); + } + return locatorFailure(type, fallbackMessage); +} diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 8bd98716326..036ea699cc1 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -530,6 +530,8 @@ describe('WriteFileTool', () => { ['photo.jpg', 'image'], ['diagram.svg', 'image'], ['photo.webp', 'image'], + ['table.csv', 'file'], + ['table.xlsx', 'file'], ])('infers artifact kind for %s as %s', async (fileName, expectedKind) => { mockConfigInternal.isRecordArtifactEnabled.mockReturnValue(true); const filePath = path.join(rootDir, 'reports', fileName); @@ -1720,6 +1722,71 @@ describe('workspace artifact metadata guard', () => { expect(buildWorkspaceArtifactMetadata(mockConfig, filePath)).toBeNull(); }); + it('derives auto-record identity from the realpath target', () => { + fs.mkdirSync(path.join(rootDir, 'data'), { recursive: true }); + const target = path.join(rootDir, 'data', 'payload.csv'); + const link = path.join(rootDir, 'report.csv'); + fs.writeFileSync(target, 'a,b\n'); + fs.symlinkSync(target, link); + try { + expect(buildWorkspaceArtifactMetadata(mockConfig, link)).toMatchObject({ + title: 'payload.csv', + kind: 'file', + workspacePath: 'data/payload.csv', + }); + } finally { + fs.rmSync(link, { force: true }); + fs.rmSync(path.join(rootDir, 'data'), { recursive: true, force: true }); + } + }); + + it('infers kind from the realpath target, not the link name', () => { + fs.mkdirSync(path.join(rootDir, 'data'), { recursive: true }); + const target = path.join(rootDir, 'data', 'payload.csv'); + const link = path.join(rootDir, 'preview.png'); + fs.writeFileSync(target, 'a,b\n'); + fs.symlinkSync(target, link); + try { + expect(buildWorkspaceArtifactMetadata(mockConfig, link)).toMatchObject({ + title: 'payload.csv', + kind: 'file', + workspacePath: 'data/payload.csv', + }); + } finally { + fs.rmSync(link, { force: true }); + fs.rmSync(path.join(rootDir, 'data'), { recursive: true, force: true }); + } + }); + + it('skips auto-record when the realpath target is not a whitelisted kind', () => { + const target = path.join(rootDir, 'dropped.bin'); + const link = path.join(rootDir, 'report.csv'); + fs.mkdirSync(rootDir, { recursive: true }); + fs.writeFileSync(target, 'bin'); + fs.symlinkSync(target, link); + try { + expect(buildWorkspaceArtifactMetadata(mockConfig, link)).toBeNull(); + } finally { + fs.rmSync(link, { force: true }); + fs.rmSync(target, { force: true }); + } + }); + + it('does not auto-record a file whose realpath is outside the workspace', () => { + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'write-file-out-')); + const linkDir = path.join(rootDir, 'output'); + fs.mkdirSync(rootDir, { recursive: true }); + fs.symlinkSync(outside, linkDir); + const filePath = path.join(linkDir, 'report.csv'); + fs.writeFileSync(filePath, 'a,b\n'); + try { + expect(buildWorkspaceArtifactMetadata(mockConfig, filePath)).toBeNull(); + } finally { + fs.rmSync(linkDir, { force: true }); + fs.rmSync(outside, { recursive: true, force: true }); + } + }); + 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 diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 9ef1b475baf..2d765733ec1 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -59,9 +59,11 @@ import { hasControlCharacter, hasUnsafeDisplayPayload, } from './record-artifact.js'; +import { toCanonicalWorkspaceArtifactPath } from '../utils/workspace-artifact-path.js'; const debugLogger = createDebugLogger('WRITE_FILE'); const ARTIFACT_KIND_BY_EXTENSION = new Map([ + ['.csv', 'file'], ['.htm', 'html'], ['.html', 'html'], ['.ipynb', 'notebook'], @@ -71,6 +73,7 @@ const ARTIFACT_KIND_BY_EXTENSION = new Map([ ['.png', 'image'], ['.svg', 'image'], ['.webp', 'image'], + ['.xlsx', 'file'], ]); type WorkspaceToolArtifact = ToolArtifact & { @@ -689,11 +692,11 @@ export function buildWorkspaceArtifactMetadata( filePath: string, sizeBytes?: number, ): WorkspaceToolArtifact | null { - const workspacePath = getRecordArtifactWorkspacePath(config, filePath); - if (!workspacePath) { + const recorded = resolveRecordedWorkspaceFile(config, filePath); + if (!recorded) { return null; } - const title = path.basename(filePath); + const title = path.basename(recorded.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. @@ -701,9 +704,9 @@ export function buildWorkspaceArtifactMetadata( title.length > ARTIFACT_TITLE_MAX_LENGTH || hasControlCharacter(title) || hasUnsafeDisplayPayload(title) || - workspacePath.length > ARTIFACT_WORKSPACE_PATH_MAX_LENGTH || - hasControlCharacter(workspacePath) || - hasUnsafeDisplayPayload(workspacePath) + recorded.workspacePath.length > ARTIFACT_WORKSPACE_PATH_MAX_LENGTH || + hasControlCharacter(recorded.workspacePath) || + hasUnsafeDisplayPayload(recorded.workspacePath) ) { debugLogger.debug('workspace artifact skipped (safety checks)', { path: filePath, @@ -712,48 +715,46 @@ export function buildWorkspaceArtifactMetadata( } return { title, - kind: inferWorkspaceArtifactKind(filePath), + kind: inferWorkspaceArtifactKind(recorded.filePath), storage: 'workspace', - workspacePath, + workspacePath: recorded.workspacePath, mimeType: - getSpecificMimeType(filePath) ?? - (filePath.toLowerCase().endsWith('.ipynb') + getSpecificMimeType(recorded.filePath) ?? + (recorded.filePath.toLowerCase().endsWith('.ipynb') ? 'application/x-ipynb+json' : undefined), sizeBytes, }; } -function getRecordArtifactWorkspacePath( +function resolveRecordedWorkspaceFile( config: Config, filePath: string, -): string | null { +): { filePath: string; workspacePath: string } | null { if (!config.isRecordArtifactEnabled()) { return null; } - if (!ARTIFACT_KIND_BY_EXTENSION.has(path.extname(filePath).toLowerCase())) { - return null; + let resolvedFile = filePath; + let resolvedRoot = config.getTargetDir(); + try { + resolvedFile = fs.realpathSync(filePath); + resolvedRoot = fs.realpathSync(resolvedRoot); + } catch { + // Keep the lexical path when the file or root cannot be realpath'd yet. } - // The daemon's file-read route resolves workspacePath against the - // original workspace root, not the session cwd. When the session - // runs inside a worktree (/.qwen/worktrees/), anchor - // the relative path at the workspace root so artifact previews - // resolve correctly. - const targetDir = config.getTargetDir(); - const wtMatch = targetDir.match( - /^(.+)[\\/]\.qwen[\\/]worktrees[\\/][^\\/]+$/, - ); - const baseDir = wtMatch ? wtMatch[1] : targetDir; - const relativePath = path.relative(baseDir, filePath); if ( - !relativePath || - relativePath === '..' || - relativePath.startsWith(`..${path.sep}`) || - path.isAbsolute(relativePath) + !ARTIFACT_KIND_BY_EXTENSION.has(path.extname(resolvedFile).toLowerCase()) ) { return null; } - return relativePath.split(path.sep).join('/'); + const workspacePath = toCanonicalWorkspaceArtifactPath( + resolvedFile, + resolvedRoot, + ); + if (!workspacePath) { + return null; + } + return { filePath: resolvedFile, workspacePath }; } function inferWorkspaceArtifactKind(filePath: string): ToolArtifactKind { diff --git a/packages/core/src/utils/workspace-artifact-path.test.ts b/packages/core/src/utils/workspace-artifact-path.test.ts new file mode 100644 index 00000000000..4529857167a --- /dev/null +++ b/packages/core/src/utils/workspace-artifact-path.test.ts @@ -0,0 +1,81 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import path from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { + resolveBoundWorkspaceRoot, + toCanonicalWorkspaceArtifactPath, +} from './workspace-artifact-path.js'; + +describe('resolveBoundWorkspaceRoot', () => { + it('returns the directory unchanged for an ordinary session cwd', () => { + expect(resolveBoundWorkspaceRoot('/mnt/workspace/w/agent')).toBe( + path.resolve('/mnt/workspace/w/agent'), + ); + }); + + it('strips a .qwen/worktrees/ suffix', () => { + expect( + resolveBoundWorkspaceRoot( + '/mnt/workspace/w/agent/.qwen/worktrees/my-feature', + ), + ).toBe(path.resolve('/mnt/workspace/w/agent')); + }); + + it('does not strip a nested path under the worktree', () => { + const nested = '/mnt/workspace/w/agent/.qwen/worktrees/my-feature/reports'; + expect(resolveBoundWorkspaceRoot(nested)).toBe(path.resolve(nested)); + }); + + it('treats a worktree whose bound workspace is the filesystem root', () => { + const worktree = path.join( + path.parse(process.cwd()).root, + '.qwen', + 'worktrees', + 'feature', + ); + expect(resolveBoundWorkspaceRoot(worktree)).toBe(path.parse(worktree).root); + }); +}); + +describe('toCanonicalWorkspaceArtifactPath', () => { + it('returns a posix path relative to an ordinary session root', () => { + expect( + toCanonicalWorkspaceArtifactPath( + '/mnt/workspace/w/agent/reports/summary.csv', + '/mnt/workspace/w/agent', + ), + ).toBe('reports/summary.csv'); + }); + + it('anchors a worktree file at the bound workspace root', () => { + expect( + toCanonicalWorkspaceArtifactPath( + '/mnt/workspace/w/agent/.qwen/worktrees/my-feature/report.csv', + '/mnt/workspace/w/agent/.qwen/worktrees/my-feature', + ), + ).toBe('.qwen/worktrees/my-feature/report.csv'); + }); + + it('returns null when the file is outside the bound workspace', () => { + expect( + toCanonicalWorkspaceArtifactPath( + '/tmp/outside.csv', + '/mnt/workspace/w/agent', + ), + ).toBeNull(); + }); + + it('returns null for the workspace root itself', () => { + expect( + toCanonicalWorkspaceArtifactPath( + '/mnt/workspace/w/agent', + '/mnt/workspace/w/agent', + ), + ).toBeNull(); + }); +}); diff --git a/packages/core/src/utils/workspace-artifact-path.ts b/packages/core/src/utils/workspace-artifact-path.ts new file mode 100644 index 00000000000..ad0493e3f05 --- /dev/null +++ b/packages/core/src/utils/workspace-artifact-path.ts @@ -0,0 +1,60 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import path from 'node:path'; + +const WORKTREE_DIR_RE = /^(.*)[\\/]\.qwen[\\/]worktrees[\\/][^\\/]+$/; + +/** + * Session cwd may be a worktree (`/.qwen/worktrees/`). Artifact + * consumers (`GET /file`, SessionArtifactStore) always resolve workspacePath + * against the bound workspace root, so producers must strip that suffix. + * + * Both sides of a comparison should use the same path namespace: either both + * realpath'd or both unresolved. Mixing them on macOS (`/var` vs `/private/var`) + * makes a valid file look like it escaped the workspace. + */ +export function resolveBoundWorkspaceRoot(targetDir: string): string { + const resolved = path.resolve(targetDir); + const match = resolved.match(WORKTREE_DIR_RE); + if (!match) { + return resolved; + } + const base = match[1]; + if (!base) { + return path.parse(resolved).root; + } + // `C:\.qwen\worktrees\x` captures `C:`, which is drive-relative, not `C:\`. + if (/^[A-Za-z]:$/.test(base)) { + return `${base}${path.sep}`; + } + return base; +} + +/** + * Convert an absolute file path into the root-relative posix workspacePath + * that the daemon store and `GET /file` understand. Returns null when the + * file is outside the bound workspace root (including the root itself). + * + * This helper does not apply write_file's extension whitelist. Callers that + * only auto-record selected kinds must filter first. + */ +export function toCanonicalWorkspaceArtifactPath( + absoluteFilePath: string, + targetDir: string, +): string | null { + const baseDir = resolveBoundWorkspaceRoot(targetDir); + const relativePath = path.relative(baseDir, path.resolve(absoluteFilePath)); + if ( + !relativePath || + relativePath === '..' || + relativePath.startsWith(`..${path.sep}`) || + path.isAbsolute(relativePath) + ) { + return null; + } + return relativePath.split(path.sep).join('/'); +} diff --git a/packages/web-shell/client/components/artifacts/TurnOutputs.dom.test.tsx b/packages/web-shell/client/components/artifacts/TurnOutputs.dom.test.tsx index cdf81721688..3767c44cf51 100644 --- a/packages/web-shell/client/components/artifacts/TurnOutputs.dom.test.tsx +++ b/packages/web-shell/client/components/artifacts/TurnOutputs.dom.test.tsx @@ -594,4 +594,50 @@ describe('TurnOutputs artifact downloads', () => { expect(click).not.toHaveBeenCalled(); }); + + it('disables Open for a missing workspace artifact and shows the recorded path', () => { + const onOpenArtifact = vi.fn(); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + + act(() => { + root.render( + + {}} + onOpenArtifact={onOpenArtifact} + onOpenScheduledTask={() => {}} + /> + , + ); + }); + + const open = Array.from(container.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Open', + ); + expect(open?.disabled).toBe(true); + expect(container.textContent).toContain( + 'File not found in the workspace · w/agent/report.csv', + ); + + act(() => open?.click()); + expect(onOpenArtifact).not.toHaveBeenCalled(); + + act(() => root.unmount()); + }); }); diff --git a/packages/web-shell/client/components/artifacts/TurnOutputs.test.ts b/packages/web-shell/client/components/artifacts/TurnOutputs.test.ts index 20ea5549e70..4e0e0d953c5 100644 --- a/packages/web-shell/client/components/artifacts/TurnOutputs.test.ts +++ b/packages/web-shell/client/components/artifacts/TurnOutputs.test.ts @@ -1,9 +1,11 @@ import { describe, expect, it } from 'vitest'; import type { DaemonSessionArtifact } from '@qwen-code/sdk/daemon'; import { + canOpenWorkspaceArtifact, getArtifactFormatIcon, getArtifactPreviewContent, getFileChangePreviewContent, + getWorkspaceArtifactOpenBlockReason, isDownloadableReviewFilePath, isRenderedFilePath, type TurnOutputFileChange, @@ -120,4 +122,57 @@ describe('TurnOutputs helpers', () => { expect(getArtifactFormatIcon('other')).toBeUndefined(); expect(getArtifactFormatIcon('future-format')).toBeUndefined(); }); + + it('disables opening missing workspace artifacts and names the recorded path', () => { + const missing = { + id: 'missing-1', + kind: 'file', + storage: 'workspace', + status: 'missing', + title: 'Missing report', + workspacePath: 'w/agent/report.csv', + } as DaemonSessionArtifact; + const available = { + ...missing, + id: 'available-1', + status: 'available', + workspacePath: 'report.csv', + } as DaemonSessionArtifact; + const t = (key: string, vars?: Record) => + key === 'turnOutputs.artifactUnavailable' && vars?.path + ? `File not found in the workspace · ${vars.path}` + : key; + + expect(canOpenWorkspaceArtifact(missing)).toBe(false); + expect(canOpenWorkspaceArtifact(available)).toBe(true); + expect( + canOpenWorkspaceArtifact({ + ...missing, + status: 'blocked', + } as DaemonSessionArtifact), + ).toBe(false); + expect(getWorkspaceArtifactOpenBlockReason(missing, t)).toBe( + 'File not found in the workspace · w/agent/report.csv', + ); + expect(getWorkspaceArtifactOpenBlockReason(available, t)).toBeUndefined(); + }); + + it('names a missing workspace artifact even without a recorded path', () => { + const missing = { + id: 'missing-2', + kind: 'file', + storage: 'workspace', + status: 'missing', + title: 'Legacy missing', + } as DaemonSessionArtifact; + const t = (key: string) => + key === 'turnOutputs.artifactMissing' + ? 'File not found in the workspace' + : key; + + expect(canOpenWorkspaceArtifact(missing)).toBe(false); + expect(getWorkspaceArtifactOpenBlockReason(missing, t)).toBe( + 'File not found in the workspace', + ); + }); }); diff --git a/packages/web-shell/client/components/artifacts/TurnOutputs.tsx b/packages/web-shell/client/components/artifacts/TurnOutputs.tsx index 5e00d79a47d..913e3940a08 100644 --- a/packages/web-shell/client/components/artifacts/TurnOutputs.tsx +++ b/packages/web-shell/client/components/artifacts/TurnOutputs.tsx @@ -344,7 +344,11 @@ function TurnOutputsComponent({ openArtifact(artifact)} + onOpen={ + canOpenWorkspaceArtifact(artifact) + ? () => openArtifact(artifact) + : undefined + } onError={onError} onDownload={ canDownloadArtifact(artifact) && workspaceActions @@ -379,7 +383,7 @@ function ArtifactCard({ onError, }: { artifact: DaemonSessionArtifact; - onOpen: () => void; + onOpen?: () => void; onDownload?: (isCancelled: () => boolean) => Promise; onError?: (error: unknown, fallback: string) => void; }) { @@ -396,6 +400,7 @@ function ArtifactCard({ }, []); const size = formatArtifactSize(artifact.sizeBytes); const FormatIcon = getArtifactFormatIcon(artifact.kind); + const blockedReason = getWorkspaceArtifactOpenBlockReason(artifact, t); const downloadName = (artifact.workspacePath && normalizePath(artifact.workspacePath).split('/').at(-1)) || @@ -429,7 +434,9 @@ function ArtifactCard({
{artifact.title}
- {[getArtifactTypeLabel(artifact), size].filter(Boolean).join(' · ')} + {[getArtifactTypeLabel(artifact), size, blockedReason] + .filter(Boolean) + .join(' · ')}
@@ -449,7 +456,8 @@ function ArtifactCard({ type="button" className={styles.reviewButton} onClick={onOpen} - title={artifact.title} + title={blockedReason ?? artifact.title} + disabled={!onOpen} > {t('common.open')} @@ -658,6 +666,27 @@ function canDownloadArtifact( ); } +export function canOpenWorkspaceArtifact( + artifact: DaemonSessionArtifact, +): boolean { + if (artifact.storage !== 'workspace') { + return true; + } + return artifact.status === 'available' || artifact.status === 'changed'; +} + +export function getWorkspaceArtifactOpenBlockReason( + artifact: DaemonSessionArtifact, + t: (key: string, vars?: Record) => string, +): string | undefined { + if (canOpenWorkspaceArtifact(artifact)) { + return undefined; + } + return artifact.workspacePath + ? t('turnOutputs.artifactUnavailable', { path: artifact.workspacePath }) + : t('turnOutputs.artifactMissing'); +} + export function displayPath(path: string, workspaceCwd?: string) { return stripWorkspacePath(path, workspaceCwd); } diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index df05dba917f..6fff1a10f1b 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -1225,6 +1225,11 @@ const EN: Messages = { 'turnOutputs.fileCount': (v) => `${v?.count ?? 0} files`, 'turnOutputs.openFileTree': 'Open file tree', 'turnOutputs.closeFileTree': 'Close file tree', + 'turnOutputs.artifactMissing': 'File not found in the workspace', + 'turnOutputs.artifactUnavailable': (v) => + v?.path + ? `File not found in the workspace · ${v.path}` + : 'File not found in the workspace', 'sidebar.label': 'Workspace sidebar', 'sidebar.toggleMenu': 'Toggle menu', 'sidebar.newChat': 'New chat', @@ -4182,6 +4187,11 @@ const ZH: Messages = { 'turnOutputs.fileCount': (v) => `${v?.count ?? 0} 个文件`, 'turnOutputs.openFileTree': '打开文件树', 'turnOutputs.closeFileTree': '关闭文件树', + 'turnOutputs.artifactMissing': '工作区中未找到该文件', + 'turnOutputs.artifactUnavailable': (v) => + v?.path + ? `工作区中未找到该文件 · ${v.path}` + : '工作区中未找到该文件', 'sidebar.label': '工作区侧边栏', 'sidebar.toggleMenu': '切换菜单', 'sidebar.newChat': '新对话',