From ad9a67509c1edbdd278c3edb858b9636fa9012ba Mon Sep 17 00:00:00 2001 From: JF Date: Fri, 21 Aug 2026 21:31:45 -0400 Subject: [PATCH] fix(proxy): thread-anchor guard on threads responses + honest tool:response success - ProxyManager only captures threads[0] as currentThreadId when no thread is anchored yet, so list_threads and internal thread polls no longer retarget stackTrace/scopes/evaluate away from the stopped thread (#396) - The tool:response log line mirrors the tool payload's own success boolean instead of hardcoding true (#397); logging-format spec synced Co-Authored-By: Claude Fable 5 --- docs/logging-format-specification.md | 12 +- src/proxy/proxy-manager.ts | 8 +- src/server.ts | 26 ++++- .../server-tool-response-logging.test.ts | 105 ++++++++++++++++++ tests/unit/proxy/proxy-manager.start.test.ts | 50 +++++++++ 5 files changed, 190 insertions(+), 11 deletions(-) create mode 100644 tests/core/unit/server/server-tool-response-logging.test.ts diff --git a/docs/logging-format-specification.md b/docs/logging-format-specification.md index 6c2221ed..3dc1fc84 100644 --- a/docs/logging-format-specification.md +++ b/docs/logging-format-specification.md @@ -29,7 +29,11 @@ Logged when an MCP tool is invoked. ``` #### tool:response -Logged when a tool completes successfully. +Logged when a tool handler completes without throwing. `success` mirrors the +`success` boolean inside the tool's own response payload, so a handler that +returns `{ "success": false }` (e.g. a failed `attach_to_process`) is logged +with `success: false`. Payloads that carry no boolean `success` field are +logged as `success: true`. ```json { @@ -41,12 +45,6 @@ Logged when a tool completes successfully. "sessionId": "abc-123-def-456", "sessionName": "My Debug Session", "success": true, - "response": { - "breakpointId": "bp-1", - "verified": true, - "file": "path/to/file.py", - "line": 42 - }, } ``` diff --git a/src/proxy/proxy-manager.ts b/src/proxy/proxy-manager.ts index 80115420..b7214178 100644 --- a/src/proxy/proxy-manager.ts +++ b/src/proxy/proxy-manager.ts @@ -981,9 +981,13 @@ export class ProxyManager extends EventEmitter implements IProxyManager { } if (message.success) { - // If this was a 'threads' response, opportunistically capture a usable thread id + // If this was a 'threads' response and no thread is anchored yet, capture a + // usable thread id as a fallback for adapters that omit threadId from + // 'stopped' events. Never overwrite an existing anchor: a list_threads call + // (or internal threads poll) while paused on a non-first thread must not + // retarget stackTrace/scopes/evaluate to threads[0] (issue #396). try { - if (pending.command === 'threads') { + if (pending.command === 'threads' && this.currentThreadId == null) { const resp = (message.response || message.body) as DebugProtocol.ThreadsResponse | undefined; // eslint-disable-next-line @typescript-eslint/no-explicit-any const threads = (resp && (resp as any).body && Array.isArray((resp as any).body.threads)) ? (resp as any).body.threads : []; diff --git a/src/server.ts b/src/server.ts index 2066f257..504f3d44 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1002,6 +1002,28 @@ export class DebugMcpServer { return sanitized; } + /** + * Derive the success flag for the tool:response log line from the tool's own + * payload. Handlers report failures as { success: false } inside the JSON + * text content without throwing; the log line must agree with the payload + * rather than meaning merely "the handler didn't throw" (issue #397). + */ + private extractPayloadSuccess(result: ServerResult): boolean { + try { + const content = (result as { content?: Array<{ type?: string; text?: string }> }).content; + const first = content?.[0]; + if (first?.type === 'text' && typeof first.text === 'string') { + const payload = JSON.parse(first.text) as unknown; + if (payload && typeof payload === 'object' && typeof (payload as { success?: unknown }).success === 'boolean') { + return (payload as { success: boolean }).success; + } + } + } catch { + // Non-JSON payloads carry no success flag; treat handler completion as success. + } + return true; + } + /** * Get session name for logging */ @@ -2151,12 +2173,12 @@ export class DebugMcpServer { throw new McpError(McpErrorCode.MethodNotFound, `Unknown tool: ${toolName}`); } - // Log successful tool response + // Log tool response; success mirrors the payload's own success flag (issue #397) this.logger.info('tool:response', { tool: toolName, sessionId: args.sessionId, sessionName: args.sessionId ? this.getSessionName(args.sessionId) : undefined, - success: true, + success: this.extractPayloadSuccess(result), timestamp: Date.now() }); diff --git a/tests/core/unit/server/server-tool-response-logging.test.ts b/tests/core/unit/server/server-tool-response-logging.test.ts new file mode 100644 index 00000000..823a7a56 --- /dev/null +++ b/tests/core/unit/server/server-tool-response-logging.test.ts @@ -0,0 +1,105 @@ +/** + * Tests for the structured tool:response log line (issue #397). + * + * The `success` field must reflect the tool payload's own `success` boolean — + * a handler that returns { success: false } without throwing is a failed tool + * call and must not be logged as success: true. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { Server } from '@modelcontextprotocol/sdk/server/index.js'; +import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; +import { DebugMcpServer } from '../../../../src/server.js'; +import { SessionManager } from '../../../../src/session/session-manager.js'; +import { SessionState } from '@debugmcp/shared'; +import { createProductionDependencies } from '../../../../src/container/dependencies.js'; +import { + createMockDependencies, + createMockServer, + createMockSessionManager, + createMockStdioTransport, + getToolHandlers +} from './server-test-helpers.js'; + +vi.mock('@modelcontextprotocol/sdk/server/index.js'); +vi.mock('@modelcontextprotocol/sdk/server/stdio.js'); +vi.mock('../../../../src/session/session-manager.js'); +vi.mock('../../../../src/container/dependencies.js'); + +describe('tool:response logging (issue #397)', () => { + let mockServer: any; + let mockSessionManager: any; + let mockDependencies: any; + let callToolHandler: any; + + beforeEach(() => { + mockDependencies = createMockDependencies(); + vi.mocked(createProductionDependencies).mockReturnValue(mockDependencies); + + mockServer = createMockServer(); + vi.mocked(Server).mockImplementation(function () { return mockServer as any; }); + + const mockStdioTransport = createMockStdioTransport(); + vi.mocked(StdioServerTransport).mockImplementation(function () { return mockStdioTransport as any; }); + + mockSessionManager = createMockSessionManager(mockDependencies.adapterRegistry); + vi.mocked(SessionManager).mockImplementation(function () { return mockSessionManager as any; }); + + new DebugMcpServer(); + callToolHandler = getToolHandlers(mockServer).callToolHandler; + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + function toolResponseLogEntries(): Array> { + return mockDependencies.logger.info.mock.calls + .filter((call: unknown[]) => call[0] === 'tool:response') + .map((call: unknown[]) => call[1] as Record); + } + + it('logs success: false when the tool payload reports failure', async () => { + mockSessionManager.attachToProcess.mockResolvedValue({ + success: false, + state: SessionState.ERROR, + error: 'Attach did not become debuggable: no threads reported within 5000ms' + }); + + const result = await callToolHandler({ + method: 'tools/call', + params: { + name: 'attach_to_process', + arguments: { sessionId: 'sess-1', port: 5678 } + } + }); + + // Sanity: the payload itself reports failure without throwing + expect(JSON.parse(result.content[0].text).success).toBe(false); + + const entries = toolResponseLogEntries(); + expect(entries).toHaveLength(1); + expect(entries[0]).toMatchObject({ tool: 'attach_to_process', success: false }); + }); + + it('logs success: true when the tool payload reports success', async () => { + mockSessionManager.attachToProcess.mockResolvedValue({ + success: true, + state: SessionState.PAUSED, + data: { message: 'Attached' } + }); + + const result = await callToolHandler({ + method: 'tools/call', + params: { + name: 'attach_to_process', + arguments: { sessionId: 'sess-1', port: 5678 } + } + }); + + expect(JSON.parse(result.content[0].text).success).toBe(true); + + const entries = toolResponseLogEntries(); + expect(entries).toHaveLength(1); + expect(entries[0]).toMatchObject({ tool: 'attach_to_process', success: true }); + }); +}); diff --git a/tests/unit/proxy/proxy-manager.start.test.ts b/tests/unit/proxy/proxy-manager.start.test.ts index 3b9c345a..2920905c 100644 --- a/tests/unit/proxy/proxy-manager.start.test.ts +++ b/tests/unit/proxy/proxy-manager.start.test.ts @@ -1255,6 +1255,56 @@ describe('ProxyManager.start', () => { expect(pending.size).toBe(0); }); + it('does not clobber a stopped-event thread id with threads[0] from a threads response', async () => { + (proxyManager as unknown as { proxyProcess: IProxyProcess | null }).proxyProcess = fakeProcess; + (proxyManager as unknown as { isInitialized: boolean }).isInitialized = true; + (proxyManager as unknown as { sessionId: string | null }).sessionId = baseConfig.sessionId; + (proxyManager as unknown as { dapState: ReturnType | null }).dapState = + createInitialState(baseConfig.sessionId); + + // Breakpoint hits on worker thread 12 + (proxyManager as unknown as { + handleProxyMessage: (message: object) => void; + }).handleProxyMessage({ + type: 'dapEvent', + sessionId: baseConfig.sessionId, + event: 'stopped', + body: { threadId: 12, reason: 'breakpoint' } + }); + expect(proxyManager.getCurrentThreadId()).toBe(12); + // handleProxyMessage syncs isInitialized from the functional-core state, + // which this test primes as uninitialized — restore the flag. + (proxyManager as unknown as { isInitialized: boolean }).isInitialized = true; + + fakeProcess.sendCommand.mockImplementation((payload) => { + if (payload.cmd === 'dap') { + (proxyManager as unknown as { + handleProxyMessage: (message: object) => void; + }).handleProxyMessage({ + type: 'dapResponse', + sessionId: baseConfig.sessionId, + requestId: payload.requestId, + success: true, + response: { + type: 'response', + seq: 11, + request_seq: 6, + command: payload.dapCommand, + success: true, + body: { + threads: [{ id: 1, name: 'main' }, { id: 12, name: 'worker' }] + } + } + }); + } + }); + + // A list_threads-style lookup must not retarget the anchored thread + await proxyManager.sendDapRequest('threads'); + + expect(proxyManager.getCurrentThreadId()).toBe(12); + }); + it('rejects DAP requests on proxy error', async () => { (proxyManager as unknown as { proxyProcess: IProxyProcess | null }).proxyProcess = fakeProcess; (proxyManager as unknown as { isInitialized: boolean }).isInitialized = true;