From 71b6a543ea950b10400ff26d13d2ba095c9213ba Mon Sep 17 00:00:00 2001 From: dvd233 <111864431+dvd233@users.noreply.github.com> Date: Fri, 9 Oct 2026 10:07:58 +0800 Subject: [PATCH] fix: report Agent file mutation failures --- .../src/lib/__tests__/fileTools.test.ts | 78 ++++++++++++ apps/webuiapps/src/lib/diskStorage.ts | 27 ++-- apps/webuiapps/src/lib/fileTools.ts | 9 +- e2e/file-tools.spec.ts | 115 ++++++++++++++++++ 4 files changed, 218 insertions(+), 11 deletions(-) create mode 100644 apps/webuiapps/src/lib/__tests__/fileTools.test.ts create mode 100644 e2e/file-tools.spec.ts diff --git a/apps/webuiapps/src/lib/__tests__/fileTools.test.ts b/apps/webuiapps/src/lib/__tests__/fileTools.test.ts new file mode 100644 index 00000000..f7f90b03 --- /dev/null +++ b/apps/webuiapps/src/lib/__tests__/fileTools.test.ts @@ -0,0 +1,78 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { deleteFilesByPaths, putTextFilesByJSON } from '../diskStorage'; +import { executeFileTool } from '../fileTools'; +import { setSessionPath } from '../sessionPath'; + +const filePath = 'apps/diary/data/entries/test.json'; +const content = '{"text":"Synthetic entry"}'; +const fetchMock = vi.fn(); + +beforeEach(() => { + fetchMock.mockReset(); + vi.stubGlobal('fetch', fetchMock); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + setSessionPath('test-character/test-mod'); +}); + +afterEach(() => { + setSessionPath(''); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe.each([ + { tool: 'file_write', method: 'POST' }, + { tool: 'file_delete', method: 'DELETE' }, +])('$tool storage results', ({ tool, method }) => { + it.each([400, 403, 500])('reports HTTP %s as an error without retrying', async (status) => { + fetchMock.mockResolvedValue(new Response('synthetic failure', { status })); + + const result = await executeFileTool(tool, { file_path: filePath, content }); + + expect(result).toMatch(/^error: /); + expect(result).toContain(String(status)); + expect(fetchMock).toHaveBeenCalledOnce(); + }); + + it('reports network failures without retrying', async () => { + fetchMock.mockRejectedValue(new Error('synthetic offline')); + + const result = await executeFileTool(tool, { file_path: filePath, content }); + + expect(result).toMatch(/^error: /); + expect(result).toContain('synthetic offline'); + expect(fetchMock).toHaveBeenCalledOnce(); + }); + + it.each([200, 204])('preserves successful HTTP %s results and request data', async (status) => { + fetchMock.mockResolvedValue(new Response(null, { status })); + + expect(await executeFileTool(tool, { file_path: filePath, content })).toBe('success'); + expect(fetchMock).toHaveBeenCalledOnce(); + expect(fetchMock).toHaveBeenCalledWith( + `/api/session-data?path=${encodeURIComponent(`test-character/test-mod/${filePath}`)}`, + method === 'POST' + ? { method, headers: { 'Content-Type': 'text/plain' }, body: content } + : { method }, + ); + }); +}); + +describe('storage callers without strict error reporting', () => { + const write = () => putTextFilesByJSON({ files: [{ name: filePath, content }] }); + const remove = () => deleteFilesByPaths({ file_paths: [filePath] }); + + it.each([write, remove])('keeps HTTP failures best-effort', async (mutate) => { + fetchMock.mockResolvedValue(new Response(null, { status: 500 })); + + await expect(mutate()).resolves.toBeUndefined(); + expect(fetchMock).toHaveBeenCalledOnce(); + }); + + it.each([write, remove])('keeps network failures best-effort', async (mutate) => { + fetchMock.mockRejectedValue(new Error('synthetic offline')); + + await expect(mutate()).resolves.toBeUndefined(); + expect(fetchMock).toHaveBeenCalledOnce(); + }); +}); diff --git a/apps/webuiapps/src/lib/diskStorage.ts b/apps/webuiapps/src/lib/diskStorage.ts index fd880e01..926f94e0 100644 --- a/apps/webuiapps/src/lib/diskStorage.ts +++ b/apps/webuiapps/src/lib/diskStorage.ts @@ -70,20 +70,26 @@ export async function getFile(filePath: string): Promise { /** * Write files. Compatible with the old putTextFilesByJSON signature. * files: [{ path: "directory", name: "filename", content: "..." }] + * Callers that report mutation outcomes can opt into error propagation. */ -export async function putTextFilesByJSON(data: { - files: Array<{ path?: string; name?: string; content?: string }>; -}): Promise { +export async function putTextFilesByJSON( + data: { files: Array<{ path?: string; name?: string; content?: string }> }, + options: { throwOnError?: boolean } = {}, +): Promise { const promises = data.files.map(async (file) => { const fullPath = file.path ? `${file.path}/${file.name}` : file.name || ''; if (!fullPath) return; try { - await fetch(apiUrl(fullPath), { + const res = await fetch(apiUrl(fullPath), { method: 'POST', headers: { 'Content-Type': 'text/plain' }, body: file.content || '', }); + if (options.throwOnError && !res.ok) { + throw new Error(`File write failed (HTTP ${res.status})`); + } } catch (e) { + if (options.throwOnError) throw e; console.warn('[diskStorage] putTextFilesByJSON write failed:', e); } }); @@ -113,11 +119,18 @@ export async function putBinaryFile( }); } -export async function deleteFilesByPaths(data: { file_paths: string[] }): Promise { +export async function deleteFilesByPaths( + data: { file_paths: string[] }, + options: { throwOnError?: boolean } = {}, +): Promise { const promises = data.file_paths.map(async (filePath) => { try { - await fetch(apiUrl(filePath), { method: 'DELETE' }); - } catch { + const res = await fetch(apiUrl(filePath), { method: 'DELETE' }); + if (options.throwOnError && !res.ok) { + throw new Error(`File delete failed (HTTP ${res.status})`); + } + } catch (e) { + if (options.throwOnError) throw e; // silently ignore } }); diff --git a/apps/webuiapps/src/lib/fileTools.ts b/apps/webuiapps/src/lib/fileTools.ts index 49c251b2..e651d359 100644 --- a/apps/webuiapps/src/lib/fileTools.ts +++ b/apps/webuiapps/src/lib/fileTools.ts @@ -226,9 +226,10 @@ export async function executeFileTool( const parts = filePath.split('/'); const name = parts.pop()!; const dir = parts.join('/'); - await idb.putTextFilesByJSON({ - files: [{ path: dir || undefined, name, content }], - }); + await idb.putTextFilesByJSON( + { files: [{ path: dir || undefined, name, content }] }, + { throwOnError: true }, + ); return 'success'; } catch (e) { return `error: ${String(e)}`; @@ -254,7 +255,7 @@ export async function executeFileTool( const filePath = (params.file_path || '').replace(/^\/+/, ''); if (!filePath) return 'error: file_path is required'; try { - await idb.deleteFilesByPaths({ file_paths: [filePath] }); + await idb.deleteFilesByPaths({ file_paths: [filePath] }, { throwOnError: true }); return 'success'; } catch (e) { return `error: ${String(e)}`; diff --git a/e2e/file-tools.spec.ts b/e2e/file-tools.spec.ts new file mode 100644 index 00000000..e61b7a49 --- /dev/null +++ b/e2e/file-tools.spec.ts @@ -0,0 +1,115 @@ +import { expect, test } from '@playwright/test'; + +for (const tool of ['file_write', 'file_delete']) { + for (const outcome of ['http-error', 'network-error', 'success']) { + test(`${tool} forwards ${outcome} to the next model request`, async ({ page, baseURL }) => { + const filePath = 'apps/diary/data/entries/storage-result.json'; + const content = '{"text":"Synthetic entry"}'; + const toolResults: string[] = []; + let mutationRequests = 0; + let modelRequests = 0; + + await page.addInitScript(() => { + localStorage.setItem( + 'webuiapps-llm-config', + JSON.stringify({ + provider: 'openai', + baseUrl: 'https://example.invalid', + model: 'test-model', + apiKey: '', + }), + ); + }); + + // Keep every API operation synthetic, including startup and chat persistence. + await page.route('**/*', async (route) => { + const request = route.request(); + const url = new URL(request.url()); + if (url.origin !== new URL(baseURL!).origin) { + await route.abort(); + return; + } + if (!url.pathname.startsWith('/api/')) { + await route.continue(); + return; + } + + if (url.pathname === '/api/llm-proxy') { + modelRequests++; + const body = request.postDataJSON(); + if (modelRequests === 1) { + await route.fulfill({ + json: { + choices: [ + { + message: { + content: '', + tool_calls: [ + { + id: 'storage-call', + type: 'function', + function: { + name: tool, + arguments: JSON.stringify({ file_path: filePath, content }), + }, + }, + ], + }, + }, + ], + }, + }); + } else { + const result = body.messages.find( + (message: { role: string; tool_call_id?: string }) => + message.role === 'tool' && message.tool_call_id === 'storage-call', + ); + toolResults.push(result?.content ?? 'missing tool result'); + await route.fulfill({ + json: { choices: [{ message: { content: 'Storage result received.' } }] }, + }); + } + return; + } + + if ( + url.pathname === '/api/session-data' && + url.searchParams.get('path')?.endsWith(filePath) + ) { + mutationRequests++; + expect(request.method()).toBe(tool === 'file_write' ? 'POST' : 'DELETE'); + if (tool === 'file_write') expect(request.postData()).toBe(content); + if (outcome === 'network-error') { + await route.abort('failed'); + } else { + await route.fulfill({ status: outcome === 'http-error' ? 500 : 200, json: {} }); + } + return; + } + + await route.fulfill({ + json: url.searchParams.get('action') === 'list' ? { files: [], not_exists: true } : {}, + }); + }); + + await page.goto('/'); + const input = page.getByTestId('chat-input'); + await expect(input).toBeEnabled(); + await input.fill('Update the synthetic diary entry.'); + await page.getByTestId('send-btn').click(); + await expect(page.getByTestId('chat-messages')).toContainText('Storage result received.'); + + expect(modelRequests).toBe(2); + expect(mutationRequests).toBe(1); + expect(toolResults).toHaveLength(1); + if (outcome === 'success') { + expect(toolResults[0]).toBe('success'); + } else { + expect(toolResults[0]).toMatch(/^error: /); + if (outcome === 'http-error') expect(toolResults[0]).toContain('500'); + } + await expect(input).toHaveValue(''); + await expect(input).toBeEnabled(); + }); + } +}