diff --git a/packages/shared/src/interfaces/adapter-policy-go.ts b/packages/shared/src/interfaces/adapter-policy-go.ts index 09019321..b4526c9a 100644 --- a/packages/shared/src/interfaces/adapter-policy-go.ts +++ b/packages/shared/src/interfaces/adapter-policy-go.ts @@ -25,6 +25,22 @@ export const GoAdapterPolicy: AdapterPolicy = { } return undefined; }, + /** + * A bare 'main' can only mean main.main (the language requires func main + * to live in package main), and Delve will never bind the bare form — so + * rewrite it instead of accepting a permanently-dead breakpoint (issue + * #467). Other bare names keep the advisory hint only: their package is + * not knowable here. + */ + normalizeFunctionBreakpointName: (name: string) => { + if (name === 'main') { + return { + name: 'main.main', + note: "Auto-qualified function breakpoint 'main' to 'main.main' — Go resolves function breakpoints against package-qualified runtime names, and a bare 'main' never binds" + }; + } + return undefined; + }, supportsReverseStartDebugging: false, childSessionStrategy: 'none', buildChildStartArgs: () => { diff --git a/packages/shared/src/interfaces/adapter-policy.ts b/packages/shared/src/interfaces/adapter-policy.ts index 59da5382..2af65c53 100644 --- a/packages/shared/src/interfaces/adapter-policy.ts +++ b/packages/shared/src/interfaces/adapter-policy.ts @@ -117,6 +117,16 @@ export interface AdapterPolicy { */ functionBreakpointNameHint?(name: string): string | undefined; + /** + * Policy-certain rewrite of a function-breakpoint name the adapter can + * never bind as given (issue #467) — e.g. Go's bare 'main' is always + * 'main.main' (func main must live in package main). Applied at + * set_breakpoint time; the response warning says the rewrite happened. + * Only return a value when the corrected form is certain — an uncertain + * name should get a functionBreakpointNameHint instead. + */ + normalizeFunctionBreakpointName?(name: string): { name: string; note: string } | undefined; + /** * True when the adapter binds function breakpoints lazily by design * (js-debug: CDP re-resolve at pauses for late-loaded modules; Java: diff --git a/src/server.ts b/src/server.ts index c4dbf85b..6bd92137 100644 --- a/src/server.ts +++ b/src/server.ts @@ -458,6 +458,21 @@ export class DebugMcpServer { } } + /** + * Policy-certain function-breakpoint name rewrite (issue #467). Swallows + * policy-lookup failures — normalization must never break the set path. + */ + private normalizeFunctionBreakpointName( + sessionId: string, + functionName: string + ): { name: string; note: string } | undefined { + try { + return this.sessionManager.getSessionPolicy(sessionId).normalizeFunctionBreakpointName?.(functionName); + } catch { + return undefined; + } + } + /** * Shared catch for the breakpoint management tools: session-lifecycle * failures become {success: false} results (same contract as @@ -1404,13 +1419,21 @@ export class DebugMcpServer { try { const fnGate = this.validateFunctionBreakpointSupport(args.sessionId); + // Policy-certain rewrite (issue #467): a name the adapter can + // never bind as given (go bare 'main') is corrected instead + // of stored as a permanently-dead breakpoint; the warning + // says the rewrite happened. + const normalized = this.normalizeFunctionBreakpointName(args.sessionId, args.function!); + const effectiveName = normalized?.name ?? args.function!; // Per-adapter name advisory (issues #303/#308): warn at set // time about names the adapter is known to mis-resolve // (rust bare 'main' -> CRT entry) or never bind (go bare // identifiers). Advisory only — the breakpoint is still set. - const nameHint = this.getFunctionBreakpointNameHint(args.sessionId, args.function!); + const nameHint = normalized + ? undefined + : this.getFunctionBreakpointNameHint(args.sessionId, effectiveName); const { breakpoint, warning: syncWarning } = await this.setFunctionBreakpoint( - args.sessionId, args.function!, args.condition + args.sessionId, effectiveName, args.condition ); this.logger.info('debug:breakpoint', { @@ -1423,10 +1446,11 @@ export class DebugMcpServer { timestamp: Date.now() }); - const warnings = [breakpoint.message, fnGate.warning, nameHint, syncWarning].filter(Boolean); + const warnings = [breakpoint.message, fnGate.warning, normalized?.note, nameHint, syncWarning].filter(Boolean); result = { content: [{ type: 'text', text: JSON.stringify({ success: true, breakpointId: breakpoint.id, + ...(normalized ? { requestedName: args.function } : {}), functionName: breakpoint.functionName, condition: breakpoint.condition, verified: breakpoint.verified, diff --git a/src/session/session-manager-operations.ts b/src/session/session-manager-operations.ts index 8598f6b6..7f58e75f 100644 --- a/src/session/session-manager-operations.ts +++ b/src/session/session-manager-operations.ts @@ -26,6 +26,7 @@ import { MIRROR_EXPOSE_COMMAND, MIRROR_UNEXPOSE_COMMAND } from '../proxy/dap-pro import { ErrorMessages } from '../utils/error-messages.js'; import { checkLaunchToolchain } from '../utils/language-availability.js'; import { resolveStatement } from '../utils/breakpoint-resolver.js'; +import { normalizeBreakpointMessage } from '../utils/breakpoint-message.js'; import { SessionManagerData } from './session-manager-data.js'; import { CustomLaunchRequestArguments, DebugResult } from './session-manager-core.js'; import { @@ -885,6 +886,15 @@ export abstract class SessionManagerOperations extends SessionManagerData { // unverified-at-launch is the designed deferral path. const fnBpWarning = this.buildFunctionBreakpointLaunchWarning(finalSession); + // Ran-to-completion with breakpoints that never bound (issue #467): + // state "stopped" where the caller expected "paused" is only + // explainable via list_breakpoints today — surface the stored + // per-breakpoint diagnostics right here where the caller is looking. + const unboundAtExitWarning = + finalState === SessionState.STOPPED + ? this.buildUnboundBreakpointExitWarning(finalSession) + : undefined; + // Logpoint-downgrade verdict (issue #469): the deferred set_breakpoint // warning promised a launch-time answer — deliver it on this response. const logpointWarning = this.buildLogpointDowngradeLaunchWarning(finalSession); @@ -894,7 +904,7 @@ export abstract class SessionManagerOperations extends SessionManagerData { // arriving after this return still lands in the output buffer as an // attributed [mcp-debugger] Warning entry. const launchWarning = - [fnBpWarning, logpointWarning, ...(finalSession.adapterNotices ?? [])] + [fnBpWarning, logpointWarning, unboundAtExitWarning, ...(finalSession.adapterNotices ?? [])] .filter(Boolean) .join('; ') || undefined; @@ -1511,6 +1521,32 @@ export abstract class SessionManagerOperations extends SessionManagerData { * undefined for bind-late policies (js/java) — unverified-at-launch is * their designed deferral, not a failure. */ + /** + * Ran-to-completion unbound-breakpoint warning (issue #467). Built only + * when the launch ends in STOPPED: at that point an unverified breakpoint + * never bound and never will, for bind-late adapters too — so this is a + * zero-false-positive moment to surface the per-breakpoint diagnostics the + * store already holds (e.g. the path-remap suggestion CodeLLDB puts in + * `message`). + */ + protected buildUnboundBreakpointExitWarning(session: ManagedSession): string | undefined { + const unbound = Array.from(session.breakpoints.values()).filter(bp => !bp.verified); + if (unbound.length === 0) { + return undefined; + } + const parts = unbound.map(bp => { + // Some stamp paths store the raw js-debug l10n key — translate it + // rather than showing 'breakpoint.provisionalBreakpoint' (issue #471). + const message = normalizeBreakpointMessage(bp.message, bp.verified); + return `${path.basename(bp.file)}:${bp.line}${message ? ` (${message})` : ''}`; + }); + return ( + `${unbound.length} breakpoint(s) never bound during this run: ${parts.join('; ')}. ` + + `The program ran to completion without stopping there — check the file path and line, ` + + `or list_breakpoints for the full per-breakpoint state` + ); + } + /** * Launch-time logpoint-downgrade warning (issue #469). A logpoint accepted * pre-launch under unknown policy support ("it will be validated against diff --git a/tests/core/unit/server/server-function-breakpoint-gating.test.ts b/tests/core/unit/server/server-function-breakpoint-gating.test.ts index 623cf065..3747a963 100644 --- a/tests/core/unit/server/server-function-breakpoint-gating.test.ts +++ b/tests/core/unit/server/server-function-breakpoint-gating.test.ts @@ -207,3 +207,90 @@ describe('set_breakpoint function gating (#271 phase 3)', () => { expect(tool.inputSchema.required).toEqual(['sessionId']); }); }); + +describe('set_breakpoint function-name normalization (issue #467)', () => { + let mockServer: any; + let mockSessionManager: any; + let callToolHandler: any; + + beforeEach(() => { + const 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; + mockSessionManager.getSession.mockReturnValue({ id: 'test-session', sessionLifecycle: 'active' }); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + function callFn(name: string) { + return callToolHandler({ + method: 'tools/call', + params: { + name: 'set_breakpoint', + arguments: { sessionId: 'test-session', function: name } + } + }); + } + + it('rewrites a policy-certain never-binding name and says so', async () => { + mockSessionManager.getSessionPolicy.mockReturnValue({ + name: 'go', + supportsFunctionBreakpoints: true, + normalizeFunctionBreakpointName: (name: string) => + name === 'main' ? { name: 'main.main', note: 'Auto-qualified to main.main' } : undefined + }); + mockSessionManager.setFunctionBreakpoint.mockResolvedValue({ + breakpoint: { id: 'fbp-2', functionName: 'main.main', verified: false } + }); + + const result = await callFn('main'); + const content = JSON.parse(result.content[0].text); + + expect(mockSessionManager.setFunctionBreakpoint).toHaveBeenCalledWith( + 'test-session', { functionName: 'main.main', condition: undefined } + ); + expect(content.functionName).toBe('main.main'); + expect(content.requestedName).toBe('main'); + expect(content.warning).toContain('Auto-qualified'); + }); + + it('keeps the advisory hint for names without a certain rewrite', async () => { + mockSessionManager.getSessionPolicy.mockReturnValue({ + name: 'go', + supportsFunctionBreakpoints: true, + normalizeFunctionBreakpointName: () => undefined, + functionBreakpointNameHint: (name: string) => + name.includes('.') ? undefined : `bare '${name}' may never bind` + }); + mockSessionManager.setFunctionBreakpoint.mockResolvedValue({ + breakpoint: { id: 'fbp-3', functionName: 'helper', verified: false } + }); + + const result = await callFn('helper'); + const content = JSON.parse(result.content[0].text); + + expect(mockSessionManager.setFunctionBreakpoint).toHaveBeenCalledWith( + 'test-session', { functionName: 'helper', condition: undefined } + ); + expect(content.requestedName).toBeUndefined(); + expect(content.warning).toContain("bare 'helper' may never bind"); + }); +}); + +describe('GoAdapterPolicy.normalizeFunctionBreakpointName (issue #467)', () => { + it("rewrites bare 'main' to 'main.main' and leaves other names alone", async () => { + const { GoAdapterPolicy } = await import('@debugmcp/shared'); + expect(GoAdapterPolicy.normalizeFunctionBreakpointName?.('main')?.name).toBe('main.main'); + expect(GoAdapterPolicy.normalizeFunctionBreakpointName?.('helper')).toBeUndefined(); + expect(GoAdapterPolicy.normalizeFunctionBreakpointName?.('main.main')).toBeUndefined(); + }); +}); diff --git a/tests/core/unit/session/session-manager-unbound-exit-warning.test.ts b/tests/core/unit/session/session-manager-unbound-exit-warning.test.ts new file mode 100644 index 00000000..80c8a7be --- /dev/null +++ b/tests/core/unit/session/session-manager-unbound-exit-warning.test.ts @@ -0,0 +1,65 @@ +/** + * buildUnboundBreakpointExitWarning (issue #467): a launch that ran to + * completion with breakpoints that never bound must say so in the + * start_debugging response, surfacing the per-breakpoint diagnostics the + * store already holds. + */ +import { describe, it, expect } from 'vitest'; +import { SessionManager } from '../../../../src/session/session-manager.js'; + +type BuilderSession = { + breakpoints: Map; +}; + +function build(session: BuilderSession): string | undefined { + return (SessionManager.prototype as unknown as { + buildUnboundBreakpointExitWarning(s: BuilderSession): string | undefined; + }).buildUnboundBreakpointExitWarning.call({}, session); +} + +describe('buildUnboundBreakpointExitWarning', () => { + it('names each unbound breakpoint with its stored diagnostic', () => { + const warning = build({ + breakpoints: new Map([ + ['a', { + file: '/workspace/examples/rust/hello_world/src/main.rs', + line: 27, + verified: false, + message: 'could not be resolved, but a valid location was found at /workspace/rust/hello_world/src/main.rs:27' + }] + ]) + }); + expect(warning).toMatch(/1 breakpoint\(s\) never bound/); + expect(warning).toMatch(/main\.rs:27/); + expect(warning).toMatch(/valid location was found/); + expect(warning).toMatch(/list_breakpoints/); + }); + + it('stays silent when every breakpoint bound', () => { + expect( + build({ + breakpoints: new Map([ + ['a', { file: '/p/x.rs', line: 3, verified: true }] + ]) + }) + ).toBeUndefined(); + }); + + it('stays silent with no breakpoints', () => { + expect(build({ breakpoints: new Map() })).toBeUndefined(); + }); + + it('counts multiple unbound breakpoints', () => { + const warning = build({ + breakpoints: new Map([ + ['a', { file: '/p/x.rs', line: 3, verified: false }], + ['b', { file: '/p/y.rs', line: 9, verified: false }], + ['c', { file: '/p/z.rs', line: 1, verified: true }] + ]) + }); + expect(warning).toMatch(/2 breakpoint\(s\) never bound/); + expect(warning).toMatch(/x\.rs:3/); + expect(warning).toMatch(/y\.rs:9/); + expect(warning).not.toMatch(/z\.rs/); + }); +});