fix(proxy): thread-anchor guard on threads responses + honest tool:response success

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
-  },
 }
 ```
 

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 : [];

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()
           });
 

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<Record<string, unknown>> {
+    return mockDependencies.logger.info.mock.calls
+      .filter((call: unknown[]) => call[0] === 'tool:response')
+      .map((call: unknown[]) => call[1] as Record<string, unknown>);
+  }
+
+  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 });
+  });
+});

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<typeof createInitialState> | 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<any>('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;