diff --git a/CHANGELOG.md b/CHANGELOG.md index 769806e71c..3d128221d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Improved the validation of the response of the `web_fetch` tool in the `FetchService` - Deprecated the `api/v1/admin/user`, `api/v1/user/:id` and `api/v1/user/:id/access-token` endpoints in favor of the `api/v1/users` endpoints - Upgraded `chart.js` from version `4.5.1` to `4.6.1` diff --git a/apps/api/src/helper/web-fetch.helper.spec.ts b/apps/api/src/helper/web-fetch.helper.spec.ts new file mode 100644 index 0000000000..0759b65005 --- /dev/null +++ b/apps/api/src/helper/web-fetch.helper.spec.ts @@ -0,0 +1,112 @@ +import { getWebFetchBody } from './web-fetch.helper'; + +const FAILURE_MESSAGE = + '{"url":"https://www.example.com/data.json","status":"failed","error":"HTTP 405: Method Not Allowed","_failure_reason":"http_error","http_status":405}'; + +const BODY = '{"items":[{"id":1,"name":"Example"}],"total":1}'; + +describe('getWebFetchBody', () => { + describe('with a JSON route', () => { + it.each([ + ['the raw body', BODY], + ['leading line breaks', `\n\n\n\n${BODY}`], + ['a code fence', `\n\n\`\`\`json\n${BODY}\n\`\`\``], + ['an intro text', `I'll fetch that URL for you.\n\n${BODY}`] + ])('should get the body from %s', (_, candidate) => { + expect(getWebFetchBody({ candidate, isJson: true })).toBe(BODY); + }); + + it('should get an array', () => { + expect( + getWebFetchBody({ candidate: 'Result:\n[{"a":1}]', isJson: true }) + ).toBe('[{"a":1}]'); + }); + + it.each([ + ['the raw message', FAILURE_MESSAGE], + [ + 'a text before the message', + `The fetch failed — no response body was returned. The tool reported:\n\n${FAILURE_MESSAGE}` + ], + [ + 'a text around the message', + `I'll fetch that URL for you.\n\nThe fetch did not succeed, so there is no response body to return. The exact tool result was:\n\n${FAILURE_MESSAGE}\n\nHTTP 405 (Method Not Allowed) means the server rejected the request method used by the fetch tool for this endpoint.` + ], + [ + 'a formatted message', + 'I\'ll fetch that URL for you.\n\n{\n "url": "https://www.example.com/data.json",\n "status": "failed",\n "error": "The engine returned no content for this URL.",\n "_failure_reason": "empty_result"\n}' + ] + ])('should reject a failure message: %s', (_, candidate) => { + expect(getWebFetchBody({ candidate, isJson: true })).toBeUndefined(); + }); + + it.each([ + ['an empty text', ' \n '], + ['a number', '42'], + ['a string', '"Example"'], + ['null', 'null'], + [ + 'a text without JSON', + '`AccessDenied`Access Denied\n\n\n\nAccessDenied\n\nAccess Denied\n\n...\n\n' + ], + [ + 'a failure in plain words', + 'The fetch did not succeed, so there is no response body to return.\n\n- Status: failed\n- HTTP status: 403 (Forbidden)' + ], + [ + 'a body with broken JSON', + '{"items":{"a":{"id":1,"value":0.1},"b\\":{\\"id\\":2,\\"value\\":0.2},"c":{"id":3,"value":0.3}}}' + ], + ['two JSON objects', `${BODY}\n\n${BODY}`], + ['a citation marker', 'The fetch failed with HTTP 403 [1].'], + ['an empty array after a text', 'No data was returned []'], + ['an object after a citation marker', 'Sources [1]: {"a":1}'], + ['an object after a text in brackets', `[Note] ${BODY}`], + [ + 'an array in a broken object', + '{"name":"bad\\q","items":[{"id":1},{"id":2}],"total":2}' + ], + ['an object in a broken array', '[{"a":1},]'], + [ + 'a false tool call', + '\n\n{"url":"https://www.example.com/data.json"}' + ] + ])('should reject %s', (_, candidate) => { + expect(getWebFetchBody({ candidate, isJson: true })).toBeUndefined(); + }); + + it('should not reject an object which has only some keys of a failure message', () => { + const candidate = '{"id":1,"status":"failed"}'; + + expect(getWebFetchBody({ candidate, isJson: true })).toBe(candidate); + }); + }); + + describe('with a route for other content', () => { + it.each([ + ['a text', 'Plain text'], + ['HTML with brackets', ''], + ['JSON', BODY] + ])('should get %s without a change', (_, candidate) => { + expect(getWebFetchBody({ candidate, isJson: false })).toBe(candidate); + }); + + it('should remove the white space around the body', () => { + expect(getWebFetchBody({ candidate: ' \nBody\n ', isJson: false })).toBe( + 'Body' + ); + }); + + it('should reject a failure message', () => { + expect( + getWebFetchBody({ candidate: FAILURE_MESSAGE, isJson: false }) + ).toBeUndefined(); + }); + + it('should reject an empty text', () => { + expect( + getWebFetchBody({ candidate: ' \n ', isJson: false }) + ).toBeUndefined(); + }); + }); +}); diff --git a/apps/api/src/helper/web-fetch.helper.ts b/apps/api/src/helper/web-fetch.helper.ts new file mode 100644 index 0000000000..530c41a5d9 --- /dev/null +++ b/apps/api/src/helper/web-fetch.helper.ts @@ -0,0 +1,70 @@ +/** + * Gets the response body from a candidate of the `web_fetch` tool. The model + * retypes the body, so that JSON can have code fences or text around it. Gives + * back `undefined` if the candidate has no usable body. + */ +export function getWebFetchBody({ + candidate, + isJson +}: { + candidate: string; + isJson: boolean; +}): string | undefined { + const body = candidate.trim(); + + if (!body) { + return undefined; + } + + const json = extractJson(body); + + if (isWebFetchFailureMessage(json?.value)) { + return undefined; + } + + if (!isJson) { + return body; + } + + return json?.isWhole ? json.text : undefined; +} + +/** + * Extracts the JSON object or array from a text. The JSON must start at the + * beginning of a line and goes to the last matching closing bracket, so that a + * bracket in a sentence gives no result. `isWhole` tells if only a closing code + * fence follows the JSON. + */ +function extractJson(text: string) { + const start = text.search(/^[[{]/m); + + if (start < 0) { + return undefined; + } + + const end = text.lastIndexOf(text[start] === '{' ? '}' : ']') + 1; + const jsonText = text.slice(start, end); + + try { + return { + isWhole: /^(```)?$/.test(text.slice(end).trim()), + text: jsonText, + value: JSON.parse(jsonText) as unknown + }; + } catch { + return undefined; + } +} + +/** + * Checks for the result of a failed fetch, which the model can give back as if + * it was the response body: + * { "url": "…", "status": "failed", "error": "HTTP 405: Method Not Allowed" } + */ +function isWebFetchFailureMessage(value: unknown) { + const { error, status, url } = (value ?? {}) as Record; + + return ( + status === 'failed' && typeof error === 'string' && typeof url === 'string' + ); +} diff --git a/apps/api/src/services/fetch/fetch.service.spec.ts b/apps/api/src/services/fetch/fetch.service.spec.ts new file mode 100644 index 0000000000..9659a038f0 --- /dev/null +++ b/apps/api/src/services/fetch/fetch.service.spec.ts @@ -0,0 +1,259 @@ +import { PropertyService } from '@ghostfolio/api/services/property/property.service'; +import { + PROPERTY_API_KEY_OPENROUTER, + PROPERTY_OPENROUTER_ENGINE_WEB_FETCH, + PROPERTY_OPENROUTER_MODEL_WEB_FETCH, + PROPERTY_WEB_FETCH_ROUTES +} from '@ghostfolio/common/config'; + +import { Logger } from '@nestjs/common'; +import { createOpenRouter } from '@openrouter/ai-sdk-provider'; +import { generateText } from 'ai'; + +import { FetchService } from './fetch.service'; + +// The packages are ECMAScript modules, which Jest cannot load +jest.mock('@openrouter/ai-sdk-provider', () => { + return { createOpenRouter: jest.fn() }; +}); +jest.mock('ai', () => { + return { + generateText: jest.fn(), + jsonSchema: jest.fn(), + tool: jest.fn((options) => { + return options; + }) + }; +}); + +const BODY = '{"items":[{"id":1,"name":"Example"}],"total":1}'; + +const FAILURE_MESSAGE = + '{"url":"https://www.example.com/data.json","status":"failed","error":"HTTP 405: Method Not Allowed","http_status":405}'; + +const URL_WITH_ROUTE = 'https://www.example.com/data.json'; + +describe('FetchService', () => { + let directFetch: jest.SpyInstance; + let fetchService: FetchService; + let properties: Record; + + const mockGenerateText = (result: { + finishReason?: string; + sources?: unknown[]; + text?: string; + }) => { + (generateText as jest.Mock).mockResolvedValue({ + finishReason: 'stop', + sources: [], + text: '', + ...result + }); + }; + + beforeEach(async () => { + jest.clearAllMocks(); + + (createOpenRouter as jest.Mock).mockReturnValue({ + chat: jest.fn((model: string) => { + return model; + }) + }); + + directFetch = jest + .spyOn(globalThis, 'fetch') + .mockResolvedValue(new Response('direct')); + + properties = { + [PROPERTY_API_KEY_OPENROUTER]: 'api-key', + [PROPERTY_OPENROUTER_ENGINE_WEB_FETCH]: 'parallel', + [PROPERTY_OPENROUTER_MODEL_WEB_FETCH]: 'model', + [PROPERTY_WEB_FETCH_ROUTES]: [ + { + domain: 'example.com', + responseContentType: 'application/json' + }, + { domain: 'example.org' } + ] + }; + + fetchService = new FetchService({ + getByKey: jest.fn((key: string) => { + return Promise.resolve(properties[key]); + }) + } as unknown as PropertyService); + + await fetchService.onModuleInit(); + }); + + afterEach(() => { + directFetch.mockRestore(); + }); + + it('should get the body from the text of the model', async () => { + mockGenerateText({ text: `\`\`\`json\n${BODY}\n\`\`\`` }); + + const response = await fetchService.fetch(URL_WITH_ROUTE); + + await expect(response.text()).resolves.toBe(BODY); + + expect(response.headers.get('content-type')).toBe('application/json'); + expect(directFetch).not.toHaveBeenCalled(); + }); + + it('should get the body from the content of a source first', async () => { + mockGenerateText({ + sources: [{ providerMetadata: { openrouter: { content: BODY } } }], + text: '{"text":true}' + }); + + const response = await fetchService.fetch(URL_WITH_ROUTE); + + await expect(response.text()).resolves.toBe(BODY); + }); + + it('should use the text of the model after a source with a failure message', async () => { + mockGenerateText({ + sources: [ + { providerMetadata: { openrouter: { content: FAILURE_MESSAGE } } } + ], + text: BODY + }); + + const response = await fetchService.fetch(URL_WITH_ROUTE); + + await expect(response.text()).resolves.toBe(BODY); + }); + + it('should get the body from the content of a source on a truncated text', async () => { + mockGenerateText({ + finishReason: 'length', + sources: [{ providerMetadata: { openrouter: { content: BODY } } }], + text: '{"items":[' + }); + + const response = await fetchService.fetch(URL_WITH_ROUTE); + + await expect(response.text()).resolves.toBe(BODY); + + expect(directFetch).not.toHaveBeenCalled(); + }); + + it('should log the failure message of a source after an empty text', async () => { + const debug = jest.spyOn(Logger.prototype, 'debug'); + + mockGenerateText({ + sources: [ + { providerMetadata: { openrouter: { content: FAILURE_MESSAGE } } } + ], + text: '' + }); + + await fetchService.fetch(URL_WITH_ROUTE); + + expect(debug.mock.calls.join()).toContain('HTTP 405'); + + debug.mockRestore(); + }); + + it('should redact the API keys in the log of a rejected body', async () => { + const debug = jest.spyOn(Logger.prototype, 'debug'); + + mockGenerateText({ + text: '{"url":"https://www.example.com/data.json?apikey=secret&api_token=secret","status":"failed","error":"HTTP 405"}' + }); + + await fetchService.fetch(URL_WITH_ROUTE); + + const logs = debug.mock.calls.join(); + + expect(logs).toContain('apikey=*******'); + expect(logs).toContain('api_token=*******'); + expect(logs).not.toContain('secret'); + + debug.mockRestore(); + }); + + it('should call the model with temperature 0 and the configured engine', async () => { + mockGenerateText({ text: BODY }); + + await fetchService.fetch(URL_WITH_ROUTE); + + expect(generateText).toHaveBeenCalledWith( + expect.objectContaining({ + model: 'model', + temperature: 0, + tools: { + web_fetch: expect.objectContaining({ + args: { parameters: { engine: 'parallel' } } + }) + } + }) + ); + }); + + it.each([ + ['a failure message', { text: FAILURE_MESSAGE }], + ['a text without JSON', { text: 'The fetch did not succeed.' }], + ['an empty text', { text: '' }], + ['a truncated body', { finishReason: 'length', text: BODY }], + ['a content filter', { finishReason: 'content-filter', text: BODY }] + ])('should use the direct request on %s', async (_, result) => { + mockGenerateText(result); + + const response = await fetchService.fetch(URL_WITH_ROUTE); + + await expect(response.text()).resolves.toBe('direct'); + + expect(directFetch).toHaveBeenCalledTimes(1); + }); + + it('should use the direct request on an error of the model', async () => { + (generateText as jest.Mock).mockRejectedValue(new Error('Timeout')); + + const response = await fetchService.fetch(URL_WITH_ROUTE); + + await expect(response.text()).resolves.toBe('direct'); + }); + + it('should get a body which is not JSON for a route without a content type', async () => { + mockGenerateText({ text: 'Plain text' }); + + const response = await fetchService.fetch('https://example.org/page'); + + await expect(response.text()).resolves.toBe('Plain text'); + + expect(directFetch).not.toHaveBeenCalled(); + }); + + it('should reject a failure message for a route without a content type', async () => { + mockGenerateText({ text: FAILURE_MESSAGE }); + + const response = await fetchService.fetch('https://example.org/page'); + + await expect(response.text()).resolves.toBe('direct'); + }); + + it('should not use the model without an API key', async () => { + delete properties[PROPERTY_API_KEY_OPENROUTER]; + + await fetchService.fetch(URL_WITH_ROUTE); + + expect(generateText).not.toHaveBeenCalled(); + expect(directFetch).toHaveBeenCalledTimes(1); + }); + + it('should not use the model for a request other than GET', async () => { + await fetchService.fetch(URL_WITH_ROUTE, { method: 'POST' }); + + expect(generateText).not.toHaveBeenCalled(); + expect(directFetch).toHaveBeenCalledTimes(1); + }); + + it('should not use the model for a domain without a route', async () => { + await fetchService.fetch('https://www.ghostfol.io'); + + expect(generateText).not.toHaveBeenCalled(); + expect(directFetch).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/api/src/services/fetch/fetch.service.ts b/apps/api/src/services/fetch/fetch.service.ts index 7b7cf4d534..d8648154e5 100644 --- a/apps/api/src/services/fetch/fetch.service.ts +++ b/apps/api/src/services/fetch/fetch.service.ts @@ -1,4 +1,5 @@ import { redactPaths } from '@ghostfolio/api/helper/object.helper'; +import { getWebFetchBody } from '@ghostfolio/api/helper/web-fetch.helper'; import { PropertyService } from '@ghostfolio/api/services/property/property.service'; import { DEFAULT_OPENROUTER_ENGINE_WEB_FETCH, @@ -119,13 +120,14 @@ export class FetchService implements OnModuleInit { try { const openRouterService = createOpenRouter({ apiKey: openRouterApiKey }); - const { sources, text } = await generateText({ + const { finishReason, sources, text } = await generateText({ model: openRouterService.chat(model), prompt: [ 'You have access to a web_fetch tool. You MUST call it to retrieve the URL below, do not answer from prior knowledge.', 'Return the fetched response body exactly as received: raw body only, no commentary, no Markdown, and no code fences.', `URL: ${url}` ].join('\n'), + temperature: 0, timeout: FetchService.WEB_FETCH_TIMEOUT, tools: { // Provider-executed tool: lets OpenRouter perform the actual web @@ -149,13 +151,23 @@ export class FetchService implements OnModuleInit { } }); + if (finishReason !== 'stop') { + this.logger.warn( + `Web fetch tool stopped with the reason "${finishReason}" for ${this.redactUrl(url)}` + ); + } + const candidates = [ ...(sources ?? []).map((source) => { return source.providerMetadata?.openrouter?.content; }), - text + ...(finishReason === 'stop' ? [text] : []) ]; + const isJson = + webFetchRoute.responseContentType?.includes('application/json') ?? + false; + let rejectedBody: string | undefined; for (const candidate of candidates) { @@ -163,20 +175,12 @@ export class FetchService implements OnModuleInit { continue; } - const body = candidate.trim(); + const body = getWebFetchBody({ candidate, isJson }); if (!body) { - continue; - } + rejectedBody = candidate.trim() || rejectedBody; - if (webFetchRoute.responseContentType?.includes('application/json')) { - try { - JSON.parse(body); - } catch { - rejectedBody = body; - - continue; - } + continue; } this.logger.debug(`Routed ${this.redactUrl(url)} via web fetch tool`); @@ -264,7 +268,16 @@ export class FetchService implements OnModuleInit { return '(empty)'; } - const singleLineBody = body.replace(/\s+/g, ' ').trim(); + const singleLineBody = body + .replace( + new RegExp( + `(${FetchService.REDACTED_QUERY_PARAM_NAMES.join('|')})=[^&\\s"']+`, + 'gi' + ), + '$1=*******' + ) + .replace(/\s+/g, ' ') + .trim(); return singleLineBody.length > FetchService.BODY_PREVIEW_LENGTH ? `${singleLineBody.slice(0, FetchService.BODY_PREVIEW_LENGTH)}…`