diff --git a/changelog.d/663.fixed.md b/changelog.d/663.fixed.md new file mode 100644 index 00000000..5902ff79 --- /dev/null +++ b/changelog.d/663.fixed.md @@ -0,0 +1 @@ +**A failed DAP request reports the adapter's reason instead of "Request failed"** — js-debug sends its user-facing errors as `{success: false, body: {error: {format: …}}}` with no top-level `message`, and the DAP client read only `message`, so a ReferenceError in `evaluate_expression`, a bad frame id, or any other js-debug `ProtocolError` reached the agent as a bare `Request failed` (the text was visible only in the adapter's telemetry line in the proxy log). The rejection, the proxy log's response line, and the `expose_session` mirror's forwarded error now fall back to `body.error.format` with its `{placeholders}` filled from `variables`, per the DAP spec's split between the raw short-form `message` and the user-facing `body.error` (#663) diff --git a/src/proxy/dap-mirror-server.ts b/src/proxy/dap-mirror-server.ts index b9194a91..a5f30366 100644 --- a/src/proxy/dap-mirror-server.ts +++ b/src/proxy/dap-mirror-server.ts @@ -17,6 +17,7 @@ import net from 'net'; import crypto from 'crypto'; import { DebugProtocol } from '@vscode/debugprotocol'; import { DapFrameDecoder, encodeDapMessage } from './dap-framing.js'; +import { dapResponseErrorText } from './dap-response-error.js'; import type { ILogger } from './dap-proxy-interfaces.js'; // ===== Host interface (implemented by DapProxyWorker) ===== @@ -286,7 +287,12 @@ export class MirrorClientConnection { if (FORWARDED_REQUESTS.has(command)) { try { const response = await this.host.forwardRequest(command, request.arguments); - this.sendResponse(request, response?.body ?? {}, response?.success !== false, response?.message); + this.sendResponse( + request, + response?.body ?? {}, + response?.success !== false, + response ? dapResponseErrorText(response) : undefined + ); } catch (error) { const message = error instanceof Error ? error.message : String(error); this.sendErrorResponse(request, MIRROR_ERROR_READ_ONLY, message, false); diff --git a/src/proxy/dap-response-error.ts b/src/proxy/dap-response-error.ts new file mode 100644 index 00000000..35fa7307 --- /dev/null +++ b/src/proxy/dap-response-error.ts @@ -0,0 +1,43 @@ +/** + * The human-readable reason a DAP response failed (issue #663). + * + * DAP's ErrorResponse carries two texts: `message`, "the raw error in short + * form", and `body.error`, a structured Message whose `format` (with `{name}` + * placeholders filled from `variables`) is what the user is meant to see. + * Adapters are free to set either. js-debug sends a ProtocolError with only + * `body.error` — `{success:false, body:{error:{id, format:"Uncaught + * ReferenceError: x is not defined"}}}` — so reading `message` alone turned + * every one of its user-facing evaluate/variables/frame errors into a bare + * "Request failed". + */ +import type { DebugProtocol } from '@vscode/debugprotocol'; + +export const DEFAULT_DAP_FAILURE_MESSAGE = 'Request failed'; + +/** Fill `{name}` placeholders in a DAP Message format from its variables. */ +export function formatDapMessage(message: DebugProtocol.Message): string { + const variables = message.variables ?? {}; + return message.format.replace(/\{([^{}]+)\}/g, (placeholder, name: string) => + Object.prototype.hasOwnProperty.call(variables, name) ? variables[name] : placeholder + ); +} + +/** + * `message` when the adapter set it, else the formatted `body.error`, else + * undefined. Callers add their own fallback. + */ +export function dapResponseErrorText(response: DebugProtocol.Response): string | undefined { + if (response.message) { + return response.message; + } + const error = (response.body as DebugProtocol.ErrorResponse['body'] | undefined)?.error; + if (error && typeof error.format === 'string' && error.format !== '') { + return formatDapMessage(error); + } + return undefined; +} + +/** The reason a failed response gives, or the generic fallback. */ +export function dapResponseErrorMessage(response: DebugProtocol.Response): string { + return dapResponseErrorText(response) ?? DEFAULT_DAP_FAILURE_MESSAGE; +} diff --git a/src/proxy/minimal-dap.ts b/src/proxy/minimal-dap.ts index 4f6ac882..f24d5d46 100644 --- a/src/proxy/minimal-dap.ts +++ b/src/proxy/minimal-dap.ts @@ -6,6 +6,7 @@ import net, { Socket } from 'net'; import { EventEmitter } from 'events'; import { DebugProtocol } from '@vscode/debugprotocol'; +import { DEFAULT_DAP_FAILURE_MESSAGE, dapResponseErrorMessage, dapResponseErrorText } from './dap-response-error.js'; import { createLogger } from '../utils/logger.js'; import fs from 'fs'; import path from 'path'; @@ -221,8 +222,11 @@ export class MinimalDapClient extends EventEmitter { const resp = message as DebugProtocol.Response; debugInfo.success = resp.success; debugInfo.request_seq = resp.request_seq; - if (resp.success === false && resp.message) { - debugInfo.errorMessage = resp.message; + if (resp.success === false) { + const errorMessage = dapResponseErrorText(resp); + if (errorMessage) { + debugInfo.errorMessage = errorMessage; + } } } @@ -263,7 +267,7 @@ export class MinimalDapClient extends EventEmitter { if (response.success) { pending.resolve(response); } else { - pending.reject(new Error(response.message || 'Request failed')); + pending.reject(new Error(dapResponseErrorMessage(response))); } } else { if (this.isDisconnectingOrDisconnected) { @@ -906,7 +910,7 @@ export class MinimalDapClient extends EventEmitter { request_seq: request.seq, command: request.command, success, - ...(success ? { body } : { message: errorMessage || 'Request failed' }) + ...(success ? { body } : { message: errorMessage || DEFAULT_DAP_FAILURE_MESSAGE }) }; this.writeMessage(response); } diff --git a/tests/proxy/dap-mirror-server.test.ts b/tests/proxy/dap-mirror-server.test.ts index 7f03b96e..3d89beab 100644 --- a/tests/proxy/dap-mirror-server.test.ts +++ b/tests/proxy/dap-mirror-server.test.ts @@ -448,6 +448,26 @@ describe('DapMirrorServer', () => { expect(response?.message).toContain('not available while running'); }); + it('forwards a failed response that carries only body.error.format with that text as message (issue #663)', async () => { + const h = await createHarness(); + h.host.forwardRequest.mockResolvedValueOnce({ + seq: 999, + type: 'response', + request_seq: 0, + command: 'evaluate', + success: false, + body: { error: { id: 2013, format: 'Uncaught ReferenceError: {name} is not defined', variables: { name: 'x' } } } + }); + const socket = h.connect(); + await join(h, socket); + send(socket, request('evaluate', { expression: 'x' })); + await flush(); + + const response = responseFor(socket, 'evaluate'); + expect(response?.success).toBe(false); + expect(response?.message).toBe('Uncaught ReferenceError: x is not defined'); + }); + it('correlates out-of-order completions to the right request_seq', async () => { const h = await createHarness(); const deferred: Array<(r: DebugProtocol.Response) => void> = []; diff --git a/tests/unit/proxy/dap-response-error.test.ts b/tests/unit/proxy/dap-response-error.test.ts new file mode 100644 index 00000000..75f9bb87 --- /dev/null +++ b/tests/unit/proxy/dap-response-error.test.ts @@ -0,0 +1,44 @@ +/** + * The reason a failed DAP response gives (issue #663): `message` when the + * adapter set it, else the formatted `body.error`, else the generic fallback. + */ +import { describe, expect, it } from 'vitest'; +import type { DebugProtocol } from '@vscode/debugprotocol'; +import { + DEFAULT_DAP_FAILURE_MESSAGE, + dapResponseErrorMessage, + dapResponseErrorText, + formatDapMessage +} from '../../../src/proxy/dap-response-error.js'; + +function failed(extra: Partial): DebugProtocol.Response { + return { seq: 1, type: 'response', request_seq: 1, command: 'evaluate', success: false, ...extra }; +} + +describe('dapResponseErrorMessage', () => { + it('prefers the raw message when the adapter set one', () => { + const response = failed({ message: 'short form', body: { error: { id: 1, format: 'long form' } } }); + expect(dapResponseErrorMessage(response)).toBe('short form'); + }); + + it('falls back to body.error.format (js-debug ProtocolError shape)', () => { + const response = failed({ body: { error: { id: 2013, format: 'Uncaught ReferenceError: x is not defined' } } }); + expect(dapResponseErrorMessage(response)).toBe('Uncaught ReferenceError: x is not defined'); + }); + + it('uses the generic fallback when neither text is present', () => { + expect(dapResponseErrorMessage(failed({}))).toBe(DEFAULT_DAP_FAILURE_MESSAGE); + expect(dapResponseErrorMessage(failed({ message: '' }))).toBe(DEFAULT_DAP_FAILURE_MESSAGE); + expect(dapResponseErrorMessage(failed({ body: { error: { id: 1, format: '' } } }))).toBe(DEFAULT_DAP_FAILURE_MESSAGE); + expect(dapResponseErrorMessage(failed({ body: { error: 'not a Message' } }))).toBe(DEFAULT_DAP_FAILURE_MESSAGE); + expect(dapResponseErrorText(failed({ body: {} }))).toBeUndefined(); + }); +}); + +describe('formatDapMessage', () => { + it('substitutes {name} placeholders from variables and leaves unknown ones alone', () => { + expect(formatDapMessage({ id: 1, format: 'Cannot set {name}: {reason} {unknown}', variables: { name: 'x', reason: 'read-only' } })) + .toBe('Cannot set x: read-only {unknown}'); + expect(formatDapMessage({ id: 1, format: 'plain {text}' })).toBe('plain {text}'); + }); +}); diff --git a/tests/unit/proxy/minimal-dap.test.ts b/tests/unit/proxy/minimal-dap.test.ts index e23acb60..57efa4d2 100644 --- a/tests/unit/proxy/minimal-dap.test.ts +++ b/tests/unit/proxy/minimal-dap.test.ts @@ -429,6 +429,44 @@ describe('MinimalDapClient', () => { await expect(requestPromise).rejects.toThrow('Failed to launch'); }); + it('rejects with body.error.format when the adapter sets no message (js-debug ProtocolError, issue #663)', async () => { + await client.connect(); + + const requestPromise = client.sendRequest('evaluate', { expression: 'httpSessions.size', frameId: 3 }); + + // js-debug: this._send({...response, success: false, body: { error: cause }}) — no `message`. + const errorResponse: DebugProtocol.Response = { + seq: 2, + type: 'response', + request_seq: 1, + command: 'evaluate', + success: false, + body: { error: { id: 2013, format: 'Uncaught ReferenceError: httpSessions is not defined', showUser: true } } + }; + + mockSocket.emit('data', createDapMessage(errorResponse)); + + await expect(requestPromise).rejects.toThrow('Uncaught ReferenceError: httpSessions is not defined'); + }); + + it('fills {placeholders} in body.error.format from its variables (issue #663)', async () => { + await client.connect(); + + const requestPromise = client.sendRequest('setVariable', { name: 'x', value: '?' }); + const errorResponse: DebugProtocol.Response = { + seq: 2, + type: 'response', + request_seq: 1, + command: 'setVariable', + success: false, + body: { error: { id: 1, format: 'Cannot set {name}: {reason}', variables: { name: 'x', reason: 'read-only' } } } + }; + + mockSocket.emit('data', createDapMessage(errorResponse)); + + await expect(requestPromise).rejects.toThrow('Cannot set x: read-only'); + }); + it('should handle concurrent requests', async () => { await client.connect(); @@ -2027,6 +2065,32 @@ describe('MinimalDapClient', () => { c.shutdown('test done'); }); + + it('logs body.error.format as errorMessage when the adapter sets no message (issue #663)', async () => { + const c = new MinimalDapClient('localhost', 5678); + loggerInstances.forEach((l) => l.info.mockClear()); + + await (c as any).handleProtocolMessage({ + seq: 12, + type: 'response', + request_seq: 9, + command: 'evaluate', + success: false, + body: { error: { id: 2013, format: 'Uncaught ReferenceError: httpSessions is not defined' } } + } satisfies DebugProtocol.Response); + + const responseLog = loggerInstances + .flatMap((l) => l.info.mock.calls) + .find(([msg]) => typeof msg === 'string' && msg.includes('DAP message: response')); + expect(responseLog?.[1]).toMatchObject({ + command: 'evaluate', + success: false, + request_seq: 9, + errorMessage: 'Uncaught ReferenceError: httpSessions is not defined' + }); + + c.shutdown('test done'); + }); }); describe('Trace connection labels (issue #518)', () => {