diff --git a/README.md b/README.md index 6f844cf..360c6ea 100644 --- a/README.md +++ b/README.md @@ -248,6 +248,40 @@ Then point your MCP client at `http://localhost:8080/mcp` using the same header/ | `AMPLITUDE_API_KEY` | No | — | Amplitude project API key. Sends MCP usage analytics — SDK lifecycle events plus our own tool/skill events | | `MCP_COMPLIANCE_MODE` | No | unset (full surface) | Serve the reduced, directory-compliant surface. Fails closed: any set value except `false`/`0`/`no`/`off` enables it | +### Failure diagnostics + +`MCP Tool Request` retains `analytics_version=2`, the existing coarse +`error_category`, `status_code`, timing and tool-specific properties. These +additive diagnostic fields are failure-only; a successful retry has none of them. + +| Property | Meaning | +| ---------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `error_reason` | `selector_miss`, `invalid_params`, `unknown_method`, `script_error`, `unauthorized`, `forbidden`, `not_found`, `server_error`, `session_lost`, `navigation_failed`, `timeout`, or `unknown`. Agent errors retain their existing detailed classification; local URL/batch validation is `invalid_params`. | +| `error_source` | `validation`, `script`, `target_website`, `api`, `transport`, or `unknown`. This identifies the observed failure boundary, not blame. A 403 alone does not identify its source. | +| `failed_command_index` | Zero-based index in the invocation's command batch, not the session-wide command counter. Omitted when setup/validation fails before a command starts. | +| `failed_method` | The failed command's recognized typed method name. Unrecognized free-form method names are omitted to avoid emitting arbitrary input; the index still identifies the command. | +| `error_code` | Allowlisted structured codes: the uppercase reason names above, `SELECTOR_NOT_FOUND`, `BROWSER_CRASHED`, `ECONNRESET`, `ECONNREFUSED`, `ENOTFOUND`, `EAI_AGAIN`, `ETIMEDOUT`. Opaque/unrecognized codes are omitted, not copied into messages. | +| `error_status_code` | An integer HTTP status (100–599) carried by structured error metadata. Never extracted from error prose. | +| `error_status_origin` | `api` for an observed API response/upgrade, `target_website` for a failed navigation result, otherwise `unknown`. Omitted when no structured status is available. | +| `error_message` | A synthesized summary capped at 500 characters. Raw error messages, response bodies, HTML, scripts, selectors, credentials, cookies, authorization headers and URLs are never copied into this field. | + +`status_code` keeps its original tool-specific meaning; the new status fields +do not replace it or turn successful target-page HTTP responses into failures. +HTTP failures retain API response status even when thrown. Codes are retained +when already available in structured errors or the JSON body read by the existing +4xx error handler; diagnostics do not read additional bodies on 5xx failures. + +An unsuccessful search without structured evidence reports `error_reason=unknown` +and `error_message="Unclassified search failure."`. Its legacy `user_error` +category remains for chart compatibility, not as evidence of caller fault. + +Example breakdowns: filter `success=false` and group by `tool → error_reason`; +for agent calls, group by `failed_method → error_reason`; for HTTP failures, +group by `error_status_origin → error_status_code`. Missing fields in older +events mean unavailable instrumentation, not an `unknown` failure. There is no +historical backfill. Verify representative received events after deployment +before treating these properties as available in production. + ## MCP Resources | Resource URI | Description | diff --git a/src/lib/api-client.ts b/src/lib/api-client.ts index cb3d7f7..ffdbfab 100644 --- a/src/lib/api-client.ts +++ b/src/lib/api-client.ts @@ -113,7 +113,16 @@ async function defaultHandleResponse(res: Response): Promise { if (!res.ok) { const errorBody = await res.text().catch(() => res.statusText); const message = errorBody.trim() || res.statusText; - throw new Error(`Server error ${res.status}: ${message}`); + let code: unknown; + try { + const body = JSON.parse(errorBody); + code = body?.code ?? body?.error?.code; + } catch { + /* Non-JSON errors retain their existing message. */ + } + throw Object.assign(new Error(`Server error ${res.status}: ${message}`), { + apiCode: code, + }); } return (await res.json()) as T; } @@ -146,8 +155,15 @@ function apiFetch( ); try { const res = await fetch(url, { ...init, signal: controller.signal }); - await throwIfProfileMissing(res, opts.profile); - return await handle(res); + try { + await throwIfProfileMissing(res, opts.profile); + return await handle(res); + } catch (error) { + if (error instanceof Error && !res.ok) { + Object.assign(error, { apiStatus: res.status }); + } + throw error; + } } finally { clearTimeout(timeoutId); } diff --git a/src/lib/define-tool.ts b/src/lib/define-tool.ts index d8ac1a4..cc9aa86 100644 --- a/src/lib/define-tool.ts +++ b/src/lib/define-tool.ts @@ -11,6 +11,7 @@ import { ResponseCache } from './cache.js'; import { AnalyticsHelper } from './analytics.js'; import { setAmplitudeToolContext } from './amplitude-analytics.js'; import { categorizeThrown, categoryFromStatus } from './error-classifier.js'; +import { failureDetails, failureFields } from './failure-details.js'; import type { ApiClient, BrowserlessSession, @@ -187,11 +188,23 @@ export function defineTool( let fired = false; // Held so a `format` that throws still reports the run's `ok`/`status_code`. let resultProps: Record | undefined; + let validating = true; const enrich = (props: Record) => { const success = normalizeSuccess(props); + const cleanProps = { ...props }; + if (success) { + for (const field of failureFields) delete cleanProps[field]; + delete cleanProps.error_category; + } else { + for (const [field, value] of Object.entries( + failureDetails(undefined), + )) { + if (cleanProps[field] === undefined) cleanProps[field] = value; + } + } return { - ...props, + ...cleanProps, success, duration_ms: Date.now() - startedAt, analytics_version: ANALYTICS_VERSION, @@ -237,6 +250,7 @@ export function defineTool( apiUrl = s.apiUrl; } def.validateUrl?.(params); + validating = false; await reportProgress({ progress: 0, total: 100 }); @@ -284,6 +298,12 @@ export function defineTool( if (!fired) { emit({ + ...failureDetails( + err, + validating + ? { category: 'INVALID_PARAMS', source: 'validation' } + : {}, + ), ...resultProps, success: false, error_category: diff --git a/src/lib/failure-details.ts b/src/lib/failure-details.ts new file mode 100644 index 0000000..99ed5c1 --- /dev/null +++ b/src/lib/failure-details.ts @@ -0,0 +1,119 @@ +import type { ErrorCategory } from '../@types/types.js'; + +type Source = + 'validation' | 'script' | 'target_website' | 'api' | 'transport' | 'unknown'; + +const reasons = { + SELECTOR_MISS: 'selector_miss', + INVALID_PARAMS: 'invalid_params', + UNKNOWN_METHOD: 'unknown_method', + SCRIPT_ERROR: 'script_error', + UNAUTHORIZED: 'unauthorized', + FORBIDDEN: 'forbidden', + NOT_FOUND: 'not_found', + SERVER_ERROR: 'server_error', + SESSION_LOST: 'session_lost', + NAVIGATION_FAILED: 'navigation_failed', + TIMEOUT: 'timeout', + UNKNOWN: 'unknown', +} satisfies Record; + +// Codes are untrusted text too. Only documented categories and transport codes +// are safe to publish; an opaque provider code could itself contain a secret. +const safeCodes = new Set([ + ...Object.keys(reasons), + 'SELECTOR_NOT_FOUND', + 'BROWSER_CRASHED', + 'ECONNRESET', + 'ECONNREFUSED', + 'ENOTFOUND', + 'EAI_AGAIN', + 'ETIMEDOUT', +]); + +export const failureFields = [ + 'error_reason', + 'error_source', + 'error_code', + 'error_message', + 'error_status_code', + 'error_status_origin', + 'failed_method', + 'failed_command_index', +] as const; + +/** Build analytics only from structured evidence, never arbitrary error prose. */ +export function failureDetails( + error: unknown, + options: { + category?: ErrorCategory; + source?: Source; + } = {}, +): Record { + const err = + error && typeof error === 'object' + ? (error as { + code?: unknown; + status?: unknown; + statusCode?: unknown; + apiStatus?: unknown; + apiCode?: unknown; + }) + : {}; + const rawStatus = err.apiStatus ?? err.status ?? err.statusCode; + const status = + typeof rawStatus === 'number' && + Number.isInteger(rawStatus) && + rawStatus >= 100 && + rawStatus <= 599 + ? rawStatus + : undefined; + const rawCode = err.apiCode ?? err.code; + const code = + typeof rawCode === 'string' && safeCodes.has(rawCode) ? rawCode : undefined; + const category = + options.category ?? + (code === 'SELECTOR_NOT_FOUND' + ? 'SELECTOR_MISS' + : code === 'BROWSER_CRASHED' + ? 'SESSION_LOST' + : code && Object.hasOwn(reasons, code) + ? (code as ErrorCategory) + : status === 401 + ? 'UNAUTHORIZED' + : status === 403 + ? 'FORBIDDEN' + : status === 404 + ? 'NOT_FOUND' + : status !== undefined && status >= 500 + ? 'SERVER_ERROR' + : 'UNKNOWN'); + const reason = reasons[category]; + const source = + options.source ?? + (err.apiStatus !== undefined + ? 'api' + : code === 'INVALID_PARAMS' || code === 'UNKNOWN_METHOD' + ? 'validation' + : 'unknown'); + return { + error_reason: reason, + error_source: source, + // Synthesized summaries deliberately omit raw text rather than attempting + // best-effort regex redaction of arbitrary scripts, selectors, or bodies. + error_message: `Request failed: ${reason.replaceAll('_', ' ')}.`.slice( + 0, + 500, + ), + ...(code === undefined ? {} : { error_code: code }), + ...(status === undefined + ? {} + : { + error_status_code: status, + error_status_origin: + source === 'api' || source === 'target_website' + ? source + : 'unknown', + }), + }; +} diff --git a/src/tools/agent.ts b/src/tools/agent.ts index a39b500..c936d6c 100644 --- a/src/tools/agent.ts +++ b/src/tools/agent.ts @@ -17,6 +17,7 @@ import { closeSession, destroySession, isRetryableUpgradeError, + UpgradeError, } from '../lib/agent-client.js'; import type { AgentParams, @@ -32,6 +33,7 @@ import { toAnalyticsCategory, } from '../lib/error-classifier.js'; import { AnalyticsHelper } from '../lib/analytics.js'; +import { failureDetails } from '../lib/failure-details.js'; import { defineTool } from '../lib/define-tool.js'; import { markFired, @@ -686,13 +688,16 @@ export function registerAgentTools( .array(compliant ? CompliantAgentCommandSchema : AgentCommandSchema) .safeParse(params.commands); if (!commandContract.success) { - throw new UserError( - commandContract.error.issues - .map( - (i) => - (i.path.length ? `${i.path.join('.')}: ` : '') + i.message, - ) - .join('; '), + throw Object.assign( + new UserError( + commandContract.error.issues + .map( + (i) => + (i.path.length ? `${i.path.join('.')}: ` : '') + i.message, + ) + .join('; '), + ), + { code: 'INVALID_PARAMS' }, ); } // Forward the parsed batch, not the raw one: the per-command schemas @@ -748,6 +753,7 @@ export function registerAgentTools( } let lastCategory: ErrorCategory | undefined; + let lastFailure: Record | undefined; const sendAnalytics = (success: boolean, err?: unknown) => { analytics?.fireToolRequest(token, 'browserless_agent', { @@ -760,6 +766,7 @@ export function registerAgentTools( ...(success ? {} : { + ...(lastFailure ?? failureDetails(err)), error_category: lastCategory ? toAnalyticsCategory(lastCategory) : categorizeThrown(err), @@ -781,6 +788,10 @@ export function registerAgentTools( const proxyCmd = commands.find((c) => c.method === 'proxy'); if (proxyCmd) { lastCategory = 'INVALID_PARAMS'; + lastFailure = failureDetails(undefined, { + category: lastCategory, + source: 'validation', + }); sendAnalytics(false); throw new UserError( 'Invalid command: "proxy" is not a BQL mutation. Proxy config is a top-level tool argument (proxy, proxyCountry, proxyState, proxyCity, proxySticky, proxyLocaleMatch, proxyPreset, externalProxyServer) and is read once at session creation. ' + @@ -829,6 +840,9 @@ export function registerAgentTools( record, ); } catch (connErr: unknown) { + lastFailure = failureDetails(connErr, { + source: connErr instanceof UpgradeError ? 'api' : 'unknown', + }); sendAnalytics(false, connErr); throw new UserError(formatConnectError(connErr)); } @@ -843,6 +857,8 @@ export function registerAgentTools( } const runCommands = async (isRetry: boolean): Promise => { + lastFailure = undefined; + lastCategory = undefined; let agentSession; try { agentSession = await getOrCreateSession( @@ -867,6 +883,9 @@ export function registerAgentTools( // with the same (bad token / wrong profile / unsupported params) // will just produce the same response and waste time. if (isRetry || !isRetryableUpgradeError(connErr)) { + lastFailure = failureDetails(connErr, { + source: connErr instanceof UpgradeError ? 'api' : 'unknown', + }); throw new UserError(formatConnectError(connErr)); } destroySession( @@ -891,7 +910,26 @@ export function registerAgentTools( // still detects the A→snapshot cross-origin transition. let crossOriginBaseline: string | undefined = agentSession.lastUrl; let promptSent = false; - for (const cmd of commands) { + for (const [commandIndex, cmd] of commands.entries()) { + const commandFailure = (err: unknown, category: ErrorCategory) => ({ + ...failureDetails(err, { + category, + source: + category === 'SCRIPT_ERROR' + ? 'script' + : category === 'NAVIGATION_FAILED' + ? 'target_website' + : category === 'SESSION_LOST' + ? 'transport' + : 'unknown', + }), + failed_command_index: commandIndex, + ...(AgentCommandSchema.options[0].options.some( + (schema) => schema.shape.method.safeParse(cmd.method).success, + ) + ? { failed_method: cmd.method } + : {}), + }); if (cmd.method === 'close') { closeSession( mcpSessionId, @@ -965,6 +1003,7 @@ export function registerAgentTools( cmd, }); lastCategory = classified.category; + lastFailure = commandFailure(sendErr, classified.category); throw new UserError( formatErrorMessage({ category: classified.category, @@ -997,6 +1036,7 @@ export function registerAgentTools( const classified = classifyAgentError({ err, cmd }); lastCategory = classified.category; + lastFailure = commandFailure(err, classified.category); const prefix = commands.length > 1 @@ -1047,6 +1087,7 @@ export function registerAgentTools( const navFailure = classifyNavigationResult(cmd.method, resp.result); if (navFailure) { lastCategory = navFailure.category; + lastFailure = commandFailure(resp.result, navFailure.category); throw new UserError( [ formatErrorMessage({ diff --git a/src/tools/crawl.ts b/src/tools/crawl.ts index 2d886f1..403424d 100644 --- a/src/tools/crawl.ts +++ b/src/tools/crawl.ts @@ -2,6 +2,7 @@ import { FastMCP, UserError } from 'fastmcp'; import type { Content } from 'fastmcp'; import { z } from 'zod'; import { defineTool, validateHttpUrl } from '../lib/define-tool.js'; +import { failureDetails } from '../lib/failure-details.js'; import { profileField } from './schemas.js'; import { AnalyticsHelper } from '../lib/analytics.js'; import type { @@ -321,6 +322,7 @@ export function registerCrawlTool( ...analyticsBase, success: false, error_category: 'timeout', + ...failureDetails(undefined, { category: 'TIMEOUT' }), crawl_id: crawlId, timeout: true, }); diff --git a/src/tools/search.ts b/src/tools/search.ts index 97840e2..10fa28b 100644 --- a/src/tools/search.ts +++ b/src/tools/search.ts @@ -4,6 +4,7 @@ import { z } from 'zod'; import { defineTool } from '../lib/define-tool.js'; import { isCompliant, COMPLIANT_SEARCH_DESCRIPTION } from './compliance.js'; import { AnalyticsHelper } from '../lib/analytics.js'; +import { failureDetails } from '../lib/failure-details.js'; import type { McpConfig, SearchParams, @@ -145,13 +146,26 @@ export function registerSearchTool( ); return response; }, - analyticsProps: (params, result) => ({ - query: params.query, - limit: params.limit ?? 10, - sources: (params.sources ?? ['web']).join(','), - success: result.success, - total_results: result.totalResults, - }), + analyticsProps: (params, result) => { + const details = failureDetails( + typeof result.error === 'object' ? result.error : result, + ); + return { + query: params.query, + limit: params.limit ?? 10, + sources: (params.sources ?? ['web']).join(','), + success: result.success, + total_results: result.totalResults, + ...(result.success + ? {} + : { + ...details, + ...(details.error_reason === 'unknown' + ? { error_message: 'Unclassified search failure.' } + : {}), + }), + }; + }, format: (response, params) => { if (!response.success) { throw new UserError( diff --git a/test/lib/define-tool.spec.ts b/test/lib/define-tool.spec.ts index 6483195..007f845 100644 --- a/test/lib/define-tool.spec.ts +++ b/test/lib/define-tool.spec.ts @@ -78,6 +78,88 @@ describe('defineTool analytics', () => { beforeEach(() => mockContext.reportProgress.resetHistory()); afterEach(() => sinon.restore()); + it('preserves structured thrown status without guessing its origin or leaking text', async () => { + const { execute, props } = register({ + run: async () => { + throw Object.assign( + new Error('password=secret ' + 'x'.repeat(600)), + { + status: 403, + code: 'FORBIDDEN', + }, + ); + }, + }); + await rejects(execute({}, mockContext as never)); + expect(props()).to.include({ + error_reason: 'forbidden', + error_source: 'unknown', + error_code: 'FORBIDDEN', + error_status_code: 403, + error_status_origin: 'unknown', + }); + expect(props()).not.to.have.property('status_code'); + expect(props().error_message).to.be.a('string').with.length.at.most(500); + expect(JSON.stringify(props())).not.to.match(/secret||xxx/); + }); + + it('omits stale diagnostic properties on success', async () => { + const { execute, props } = register({ + analyticsProps: () => ({ + success: true, + error_category: 'timeout', + error_reason: 'timeout', + error_source: 'transport', + error_code: 'ETIMEDOUT', + error_message: 'private', + failed_method: 'goto', + failed_command_index: 1, + error_status_code: 503, + error_status_origin: 'api', + }), + }); + await execute({}, mockContext as never); + expect( + Object.keys(props()).filter( + (k) => k.startsWith('error_') || k.startsWith('failed_'), + ), + ).to.deep.equal([]); + }); + + it('retains API status on a thrown HTTP response without emitting its body', async () => { + sinon.stub(globalThis, 'fetch').resolves( + new Response('{"code":"UNAUTHORIZED","message":"Bearer private"}', { + status: 401, + }), + ); + const { execute, props } = register({ + run: async ({ client }) => client.search({ query: 'test' }), + }); + await rejects(execute({}, mockContext as never)); + expect(props()).to.include({ + error_reason: 'unauthorized', + error_source: 'api', + error_status_code: 401, + error_status_origin: 'api', + error_code: 'UNAUTHORIZED', + }); + expect(JSON.stringify(props())).not.to.include('private'); + }); + + it('does not change coarse classification when retaining a new upstream code', async () => { + sinon + .stub(globalThis, 'fetch') + .resolves(new Response('{"code":"BROWSER_CRASHED"}', { status: 403 })); + const { execute, props } = register({ + run: async ({ client }) => client.search({ query: 'test' }), + }); + await rejects(execute({}, mockContext as never)); + expect(props()).to.include({ + error_code: 'BROWSER_CRASHED', + error_category: 'user_error', + }); + }); + it('emits session attribution without leaking authentication credentials', async () => { const { execute, fire, skill, props } = register({ run: async ({ analytics, token, mcpSource }) => { @@ -220,6 +302,11 @@ describe('defineTool analytics', () => { await rejects(execute({ url: 'ftp://x' }, mockContext as never)); expect(fire.calledOnce).to.be.true; expect(props().error_category).to.equal('user_error'); + expect(props()).to.include({ + error_reason: 'invalid_params', + error_source: 'validation', + }); + expect(props()).not.to.have.property('failed_command_index'); }); it('classifies network failures', async () => { diff --git a/test/tools/agent.spec.ts b/test/tools/agent.spec.ts index c35b352..62f6de9 100644 --- a/test/tools/agent.spec.ts +++ b/test/tools/agent.spec.ts @@ -1153,6 +1153,225 @@ describe('browserless_agent _prompt capture', () => { return { added, execute: added.execute, fire }; }; + it('attributes failure to the second command and excludes diagnostic input text', async () => { + const srv = await makeRespondingServer((method) => + method === 'click' + ? new AgentErrorFrame({ + code: 'SELECTOR_NOT_FOUND', + message: 'selector #private password=secret ' + 'x'.repeat(700), + }) + : { url: 'https://example.com', status: 200 }, + ); + try { + const { execute, fire } = registerWithAnalytics({ + ...mockConfig, + browserlessApiUrl: srv.url, + }); + try { + await execute( + { + commands: [ + { method: 'goto', params: { url: 'https://example.com' } }, + { method: 'click', params: { selector: '#private' } }, + ], + }, + { ...mockContext, sessionId: 'diagnostic-batch' }, + ); + } catch { + /* assert event below */ + } + expect(fire.calledOnce).to.equal(true); + const props = fire.firstCall.args[2]; + expect(props).to.include({ + error_reason: 'selector_miss', + error_code: 'SELECTOR_NOT_FOUND', + failed_method: 'click', + failed_command_index: 1, + error_category: 'user_error', + }); + expect(JSON.stringify(props)).not.to.match(/#private|secret|xxx/); + } finally { + await srv.close(); + } + }); + + it('retains setup HTTP status without a command index', async () => { + const srv = await makeRejectingServer(403, 'private'); + try { + const { execute, fire } = registerWithAnalytics({ + ...mockConfig, + browserlessApiUrl: srv.url, + }); + try { + await execute( + { method: 'goto', params: { url: 'https://example.com' } }, + { ...mockContext, sessionId: 'diagnostic-connect' }, + ); + } catch { + /* assert event below */ + } + expect(fire.calledOnce).to.equal(true); + expect(fire.firstCall.args[2]).to.include({ + error_reason: 'forbidden', + error_source: 'api', + error_status_code: 403, + error_status_origin: 'api', + }); + expect(fire.firstCall.args[2]).not.to.have.property('failed_method'); + expect(fire.firstCall.args[2]).not.to.have.property( + 'failed_command_index', + ); + } finally { + await srv.close(); + } + }); + + it('reports invalid batches before commands start', async () => { + const { execute, fire } = registerWithAnalytics(mockConfig); + try { + await execute( + { commands: [{ method: 'click', params: {} }] }, + { ...mockContext, sessionId: 'diagnostic-invalid-batch' }, + ); + } catch { + /* assert event */ + } + expect(fire.calledOnce).to.equal(true); + expect(fire.firstCall.args[2]).to.include({ + error_reason: 'invalid_params', + error_source: 'validation', + }); + expect(fire.firstCall.args[2]).not.to.have.property('failed_command_index'); + }); + + it('does not label a failed connection without an HTTP response as an API response', async () => { + const srv = await makeRejectingServer(403, 'unused'); + await srv.close(); + const { execute, fire } = registerWithAnalytics({ + ...mockConfig, + browserlessApiUrl: srv.url, + }); + try { + await execute( + { method: 'goto', params: { url: 'https://example.com' } }, + { ...mockContext, sessionId: 'diagnostic-refused' }, + ); + } catch { + /* assert event */ + } + expect(fire.calledOnce).to.equal(true); + expect(fire.firstCall.args[2].error_source).not.to.equal('api'); + expect(fire.firstCall.args[2]).not.to.have.property('error_status_code'); + expect(fire.firstCall.args[2]).not.to.have.property('failed_command_index'); + }); + + it('keeps a target navigation status separate from API response status', async () => { + const srv = await makeRespondingServer(() => ({ + url: 'chrome-error://chromewebdata/', + status: 503, + })); + try { + const { execute, fire } = registerWithAnalytics({ + ...mockConfig, + browserlessApiUrl: srv.url, + }); + try { + await execute( + { method: 'goto', params: { url: 'https://example.com' } }, + { ...mockContext, sessionId: 'diagnostic-target' }, + ); + } catch { + /* assert event */ + } + expect(fire.calledOnce).to.equal(true); + expect(fire.firstCall.args[2]).to.include({ + error_reason: 'navigation_failed', + error_source: 'target_website', + error_status_code: 503, + error_status_origin: 'target_website', + }); + expect(fire.firstCall.args[2]).not.to.have.property('status_code'); + } finally { + await srv.close(); + } + }); + + for (const [code, method, message, reason] of [ + ['INVALID_PARAMS', 'click', 'bad parameters', 'invalid_params'], + ['UNKNOWN_METHOD', 'notAMethod', 'unknown', 'unknown_method'], + ['BROWSER_CRASHED', 'snapshot', 'crashed', 'session_lost'], + ['', 'evaluate', 'private script threw', 'script_error'], + ['', 'goto', 'net::ERR_NAME_NOT_RESOLVED', 'navigation_failed'], + ['', 'waitForSelector', 'Timed out', 'timeout'], + ['', 'goto', 'HTTP 401', 'unauthorized'], + ['', 'goto', 'HTTP 403', 'forbidden'], + ['', 'goto', 'HTTP 404', 'not_found'], + ['', 'goto', 'HTTP 503', 'server_error'], + ['opaque-private-code', 'snapshot', 'unrecognized', 'unknown'], + ]) { + it(`preserves ${reason} as a detailed agent reason`, async () => { + const srv = await makeRespondingServer( + () => new AgentErrorFrame({ code: code || undefined, message }), + ); + try { + const { execute, fire } = registerWithAnalytics({ + ...mockConfig, + browserlessApiUrl: srv.url, + }); + try { + await execute( + { method, params: {} }, + { ...mockContext, sessionId: `diagnostic-${reason}` }, + ); + } catch { + /* assert event */ + } + expect(fire.calledOnce).to.equal(true); + expect(fire.firstCall.args[2]).to.include({ + error_reason: reason, + failed_command_index: 0, + }); + // Numbers in prose cannot establish HTTP status provenance. + expect(fire.firstCall.args[2]).not.to.have.property( + 'error_status_code', + ); + if (reason === 'unknown') + expect(fire.firstCall.args[2]).not.to.have.property('error_code'); + } finally { + await srv.close(); + } + }); + } + + it('clears failure metadata when a fatal command succeeds on retry', async () => { + let calls = 0; + const srv = await makeRespondingServer(() => + ++calls === 1 + ? new AgentErrorFrame({ code: 'BROWSER_CRASHED', message: 'crashed' }) + : { url: 'https://example.com', status: 200 }, + ); + try { + const { execute, fire } = registerWithAnalytics({ + ...mockConfig, + browserlessApiUrl: srv.url, + }); + await execute( + { method: 'goto', params: { url: 'https://example.com' } }, + { ...mockContext, sessionId: 'diagnostic-retry' }, + ); + expect(srv.hits()).to.equal(2); + expect(fire.calledOnce).to.equal(true); + expect(fire.firstCall.args[2].success).to.equal(true); + expect( + Object.keys(fire.firstCall.args[2]).filter( + (k) => k.startsWith('error_') || k.startsWith('failed_'), + ), + ).to.deep.equal([]); + } finally { + await srv.close(); + } + }); + it('injects _prompt into the schema and logs it redacted', async () => { const { added, execute, fire } = registerWithAnalytics(mockConfig); expect((added.parameters as any).shape).to.have.property('_prompt'); @@ -1219,8 +1438,11 @@ describe('browserless_agent _prompt capture', () => { expect(fire.firstCall.args[2]).to.include({ success: false, error_category: 'user_error', + error_reason: 'invalid_params', + error_source: 'validation', analytics_version: 2, }); + expect(fire.firstCall.args[2]).not.to.have.property('failed_command_index'); }); it('does NOT inject _prompt on the compliant surface', () => { diff --git a/test/tools/crawl.spec.ts b/test/tools/crawl.spec.ts index bc4b87e..2075c57 100644 --- a/test/tools/crawl.spec.ts +++ b/test/tools/crawl.spec.ts @@ -64,6 +64,48 @@ describe('browserless_crawl tool', () => { expect(() => registerCrawlTool(server, mockConfig)).to.not.throw(); }); + it('reports a polling timeout once with a detailed timeout reason', async () => { + const clock = sinon.useFakeTimers({ toFake: ['Date'] }); + fetchStub + .onCall(0) + .resolves(Response.json({ success: true, id: 'crawl-timeout' })); + fetchStub.onCall(1).callsFake(async () => { + clock.tick(1001); + return Response.json({ + status: 'in-progress', + total: 1, + completed: 0, + failed: 0, + data: [], + }); + }); + const server = new FastMCP({ name: 'test', version: '0.1.0' }); + const addTool = sinon.spy(server, 'addTool'); + const analytics = new AnalyticsHelper(false); + const fire = sinon.stub(analytics, 'fireToolRequest'); + registerCrawlTool(server, mockConfig, analytics); + try { + await addTool.firstCall.args[0].execute( + { url: 'https://example.com', maxWaitTime: 1000 }, + mockContext, + ); + expect.fail('expected timeout'); + } catch (error) { + expect(error).to.be.instanceOf(UserError); + expect((error as Error).message).to.include('exceeded max wait time'); + } + expect(fire.callCount).to.equal(1); + expect(fire.firstCall.args[2]).to.include({ + success: false, + error_category: 'timeout', + error_reason: 'timeout', + error_source: 'unknown', + error_message: 'Request failed: timeout.', + timeout: true, + }); + expect(fire.firstCall.args[2]).to.not.have.property('failed_command_index'); + }); + it('starts a crawl and waits for completion', async () => { // First call: POST /crawl to start fetchStub.onCall(0).resolves( diff --git a/test/tools/search.spec.ts b/test/tools/search.spec.ts index c11aa38..ce18a4f 100644 --- a/test/tools/search.spec.ts +++ b/test/tools/search.spec.ts @@ -3,6 +3,7 @@ import sinon from 'sinon'; import { FastMCP, UserError } from 'fastmcp'; import type { Content } from 'fastmcp'; import { registerSearchTool } from '../../src/tools/search.js'; +import { AnalyticsHelper } from '../../src/lib/analytics.js'; import type { McpConfig } from '../../src/@types/types.js'; const mockConfig: McpConfig = { @@ -63,6 +64,71 @@ describe('browserless_search tool', () => { expect(() => registerSearchTool(server, mockConfig)).to.not.throw(); }); + it('reports generic search failure as unclassified without changing its coarse category', async () => { + fetchStub.resolves( + new Response( + JSON.stringify({ + success: false, + totalResults: 0, + data: {}, + error: 'private query cookie=secret ' + 'x'.repeat(600), + }), + ), + ); + const server = new FastMCP({ name: 'test', version: '0.1.0' }); + const spy = sinon.spy(server, 'addTool'); + const analytics = new AnalyticsHelper(false); + const fire = sinon.stub(analytics, 'fireToolRequest'); + registerSearchTool(server, mockConfig, analytics); + try { + await spy.firstCall.args[0].execute({ query: 'test' }, mockContext); + } catch { + /* assert event */ + } + expect(fire.calledOnce).to.equal(true); + const props = fire.firstCall.args[2]; + expect(props).to.include({ + success: false, + error_reason: 'unknown', + error_source: 'unknown', + error_message: 'Unclassified search failure.', + error_category: 'user_error', + }); + expect(JSON.stringify(props)).not.to.match(/secret|private|xxx/); + }); + + it('preserves safe structured search diagnostics without inventing status provenance', async () => { + fetchStub.resolves( + new Response( + JSON.stringify({ + success: false, + totalResults: 0, + data: {}, + error: { code: 'FORBIDDEN', status: 403, message: 'private' }, + }), + ), + ); + const server = new FastMCP({ name: 'test', version: '0.1.0' }); + const spy = sinon.spy(server, 'addTool'); + const analytics = new AnalyticsHelper(false); + const fire = sinon.stub(analytics, 'fireToolRequest'); + registerSearchTool(server, mockConfig, analytics); + try { + await spy.firstCall.args[0].execute({ query: 'test' }, mockContext); + } catch { + /* assert event */ + } + expect(fire.calledOnce).to.equal(true); + expect(fire.firstCall.args[2]).to.include({ + error_reason: 'forbidden', + error_source: 'unknown', + error_code: 'FORBIDDEN', + error_status_code: 403, + error_status_origin: 'unknown', + }); + expect(fire.firstCall.args[2].error_message).not.to.include('Unclassified'); + }); + it('returns web search results', async () => { fetchStub.resolves( new Response(