diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java index 5e9d1f1..979fc9d 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/EventHistory.java @@ -10,20 +10,26 @@ import java.util.concurrent.ConcurrentLinkedDeque; /** - * Bounded ring buffer of debug events captured from the JDI event queue. Records every interesting - * event for post-mortem inspection via the `jdwp_get_events` tool. + * Bounded ring buffer of debug events for post-mortem inspection via the `jdwp_get_events` tool. + * Most events originate from the JDI event queue drained by {@link JdiEventListener}, but it is NOT + * the sole producer: synchronous install-time diagnostics are recorded from MCP worker threads too + * (e.g. {@code BP_MULTI_LOCATION} from {@link JDWPTools}, {@code RECONNECT} from + * {@link JDIConnectionService}). *

* Behaviour: * - FIFO ring buffer with a fixed cap of {@link #MAX_EVENTS} entries; oldest entries are dropped first. - * - Thread-safe via {@link ConcurrentLinkedDeque}; the producer ({@link JdiEventListener}) and consumers - * (MCP tool calls on worker threads) never block each other. + * - Thread-safe via {@link ConcurrentLinkedDeque}: the listener thread and the MCP worker threads may + * record concurrently, and concurrent readers never block writers. Because there are multiple + * producers, no global ordering is guaranteed beyond each individual {@code record} call. * - Cleared on `jdwp_reset`, `jdwp_clear_events`, and {@link JDIConnectionService#cleanupSessionState}. *

- * Documented event type strings: `BREAKPOINT`, `STEP`, `EXCEPTION`, `EXCEPTION_LOG`, - * `EXCEPTION_LOG_ERROR`, `LOGPOINT`, `LOGPOINT_ERROR`, `FIELD_ACCESS`, `FIELD_MODIFICATION`, - * `FIELD_LOGPOINT`, `FIELD_LOGPOINT_ERROR`, `BREAKPOINT_SUPPRESSED`, `EXCEPTION_SUPPRESSED`, - * `FIELD_BREAKPOINT_SUPPRESSED`, `CHAIN_ARMED`, `CHAIN_DISARMED`, `CHAIN_BROKEN`, `VM_START`, - * `VM_DEATH`. These are the keys clients can grep on or filter by. + * Documented event type strings (exhaustive for production code — keep in sync; tests may record + * ad-hoc types that are intentionally not listed here): `BREAKPOINT`, `BREAKPOINT_SUPPRESSED`, `STEP`, `STEP_SUPPRESSED`, + * `EXCEPTION`, `EXCEPTION_SUPPRESSED`, `EXCEPTION_LOG`, `EXCEPTION_LOG_ERROR`, `LOGPOINT`, + * `LOGPOINT_ERROR`, `FIELD_ACCESS`, `FIELD_MODIFICATION`, `FIELD_BREAKPOINT_SUPPRESSED`, + * `FIELD_LOGPOINT`, `FIELD_LOGPOINT_ERROR`, `CHAIN_ARMED`, `CHAIN_DISARMED`, `CHAIN_BROKEN`, + * `CLASS_PREPARE`, `BP_MULTI_LOCATION`, `BP_PROMOTION_FAILED`, `RECONNECT`, `VM_START`, `VM_DEATH`. + * These are the keys clients can grep on or filter by. */ @Service public class EventHistory { @@ -36,7 +42,10 @@ public class EventHistory { /** * Appends an event and evicts the oldest entries until the buffer is at or below {@link #MAX_EVENTS}. - * Called exclusively from {@link JdiEventListener} on the JDI event listener thread; non-blocking. + * Non-blocking, and safe to call from any thread: most records come from the JDI event listener + * thread ({@link JdiEventListener}), but MCP worker threads also record install-time diagnostics + * (e.g. {@code BP_MULTI_LOCATION}, {@code RECONNECT}). The backing {@link ConcurrentLinkedDeque} + * tolerates these concurrent producers. */ public void record(DebugEvent event) { events.addLast(event); diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java index ccabf09..f22a6ae 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java @@ -806,10 +806,10 @@ private static boolean isDefaultObjectToString(String value) { } - @McpTool(description = "Evaluate a Java expression in the context of a suspended thread's stack frame. Returns '[VM_DEATH] ...' if the target VM disconnects mid-call.") + @McpTool(description = "Evaluate a Java expression in the context of a suspended thread's stack frame. Two input modes: (1) single expression — `order.getTotal()`, `x + y`. (2) block mode — wrap the input in `{ ... }` to send a statement body with try/catch, intermediate locals, or early `return`; the user writes explicit `return X;` statements to yield a value. Returns '[VM_DEATH] ...' if the target VM disconnects mid-call.") public String jdwp_evaluate_expression( @McpToolParam(description = "Thread unique ID") long threadId, - @McpToolParam(description = "Java expression to evaluate (e.g., 'order.getTotal()', 'x + y', 'name.length()')") String expression, + @McpToolParam(description = "Java expression OR `{ ... }` block to evaluate. Examples: `order.getTotal()`, `x + y`, `{ try { return risky(); } catch (Exception e) { return e.getMessage(); } }`") String expression, @McpToolParam(required = false, description = "Frame index (0 = current frame, default: 0)") @Nullable Integer frameIndex) { try { final VirtualMachine vm = jdiService.getVM(); @@ -839,9 +839,9 @@ public String jdwp_evaluate_expression( } } - @McpTool(description = "Evaluate a Java expression and compare its result against an expected value. Returns 'OK' on match, 'MISMATCH' with actual vs expected on failure, or '[VM_DEATH] ...' if the target VM disconnects mid-call. Comparison is string-based against the same formatting jdwp_evaluate_expression uses (so primitives auto-unbox, strings strip surrounding quotes).") + @McpTool(description = "Evaluate a Java expression and compare its result against an expected value. Returns 'OK' on match, 'MISMATCH' with actual vs expected on failure, or '[VM_DEATH] ...' if the target VM disconnects mid-call. Comparison is string-based against the same formatting jdwp_evaluate_expression uses (so primitives auto-unbox, strings strip surrounding quotes). Supports `{ block }` syntax for multi-statement.") public String jdwp_assert_expression( - @McpToolParam(description = "Java expression (e.g., 'order.getTotal()', 'session.getRole()', 'list.size() == 5')") String expression, + @McpToolParam(description = "Java expression (e.g., 'order.getTotal()', 'session.getRole()', 'list.size() == 5') — supports `{ ...; return X; }` block syntax") String expression, @McpToolParam(description = "Expected value (string-compared against the formatted expression result)") String expected, @McpToolParam(required = false, description = "Thread ID — defaults to the last breakpoint thread") @Nullable Long threadId, @McpToolParam(required = false, description = "Frame index (default: 0)") @Nullable Integer frameIndex) { @@ -1956,12 +1956,12 @@ private String doStep(@Nullable Long threadId, int stepDepth, String label) { } } - @McpTool(description = "Set a breakpoint at a specific line in a class. Supports conditional breakpoints. By default the breakpoint is registered passively: if the class is not yet loaded, the BP is deferred via a ClassPrepareRequest and activates when the JVM loads the class on its own — same behaviour as IntelliJ / Eclipse / jdb. Pass forceLoad=true only when you need the breakpoint to bind immediately and accept that loading the class will run its `` (early static init, eager dependency loads, masked lazy-load diagnostics).") + @McpTool(description = "Set a breakpoint at a specific line in a class. Supports conditional breakpoints — conditions support `{ block }` syntax for multi-statement. By default the breakpoint is registered passively: if the class is not yet loaded, the BP is deferred via a ClassPrepareRequest and activates when the JVM loads the class on its own — same behaviour as IntelliJ / Eclipse / jdb. Pass forceLoad=true only when you need the breakpoint to bind immediately and accept that loading the class will run its `` (early static init, eager dependency loads, masked lazy-load diagnostics).") public String jdwp_set_breakpoint( @McpToolParam(description = "Fully qualified class name (e.g. 'com.example.MyClass')") String className, @McpToolParam(description = "Line number") int lineNumber, @McpToolParam(required = false, description = "Suspend policy: 'all' (default), 'thread', 'none'") @Nullable String suspendPolicy, - @McpToolParam(required = false, description = "Optional condition — only suspend when this evaluates to true (e.g., 'i > 100')") @Nullable String condition, + @McpToolParam(required = false, description = "Optional condition — only suspend when this evaluates to true (e.g., 'i > 100') — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this BP stays disarmed until the trigger fires. Sticky by default: once armed it stays so unless re-disarmed via jdwp_disarm_until_trigger or oneShot=true.") @Nullable Integer triggerBreakpointId, @McpToolParam(required = false, description = "If true, re-disarm this BP after each hit so the next trigger fire re-arms it (IntelliJ-style). Default: false (sticky).") @Nullable Boolean oneShot, @McpToolParam(required = false, description = "If true, force-load the class via Class.forName in the target VM when it isn't already loaded. Default: false (passive — defer via ClassPrepareRequest). Force-load triggers ``, cascades dependent loads, and can mask lazy-init / classloader-leak diagnostics — use sparingly.") @Nullable Boolean forceLoad) { @@ -2037,21 +2037,34 @@ public String jdwp_set_breakpoint( // request while it is still being wired up. JDI delivers events the instant // a request is enabled, so a chained BP enabled before its chain edge is // registered could fire once unchained on a hot code path. - final BreakpointRequest bpRequest = erm.createBreakpointRequest(locations.get(0)); + final Location boundLocation = locations.get(0); + final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); if (!breakpointTracker.promotePendingToActive(pendingId, bpRequest)) { - // Another path (listener or safety-net) won the promotion race; tear - // down the loser request so no duplicate fires on the target VM. + // Another path (the ClassPrepare listener or the safety-net promoter) won + // the promotion race; tear down the loser request so no duplicate fires on + // the target VM. Crucially, do NOT emit the multi-location diagnostic here: + // the winning path owns the binding and (for the listener) already recorded + // its own BP_MULTI_LOCATION — emitting again would double-count. try { erm.deleteEventRequest(bpRequest); } catch (Exception ignored) { // VM may already be disconnected — best-effort cleanup. } - } else if (triggerBreakpointId == null) { + return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s) " + + "(bound by a concurrent activation path)", + className, lineNumber, pendingId, policyLabel, conditionInfo, chainInfo); + } + if (triggerBreakpointId == null) { bpRequest.setEnabled(true); } - return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s)", - className, lineNumber, pendingId, policyLabel, conditionInfo, chainInfo); + // Won the promotion — emit the multi-location diagnostic for THIS bind. It must + // apply to the race-guard path too, not just the eager path, otherwise + // lambda-bearing lines silently miss depending on whether the class loaded + // before or after BP registration. + return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s%s)", + className, lineNumber, pendingId, policyLabel, conditionInfo, chainInfo, + multiLocationDiagnostic(pendingId, className, lineNumber, locations, boundLocation, "BP")); } } @@ -2071,7 +2084,8 @@ public String jdwp_set_breakpoint( // BP opens a window where the BP can fire once before its trigger ever does. JDI // request creation already returns a disabled request; postpone the enable to the very // last step (and only when no chain edge applies). - final BreakpointRequest bpRequest = erm.createBreakpointRequest(locations.get(0)); + final Location boundLocation = locations.get(0); + final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); final int breakpointId = breakpointTracker.registerBreakpoint(bpRequest); @@ -2086,8 +2100,12 @@ public String jdwp_set_breakpoint( bpRequest.setEnabled(true); } - return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s)", - className, lineNumber, breakpointId, policyLabel, conditionInfo, chainInfo); + // Diagnostic — see JdiEventListener.handleClassPrepareEvent for the rationale. A + // line with multiple Locations (typical for lambdas) only gets a BP at the first + // one; the other code paths through the same line silently miss. + return String.format("Breakpoint set at %s:%d (ID: %d, suspend: %s%s%s%s)", + className, lineNumber, breakpointId, policyLabel, conditionInfo, chainInfo, + multiLocationDiagnostic(breakpointId, className, lineNumber, locations, boundLocation, "BP")); } catch (AbsentInformationException e) { cleanupOrphanPendingBreakpoint(pendingIdForCleanup); return "Error: No line number information available for this class. Compile with debug info (-g)."; @@ -2097,12 +2115,12 @@ public String jdwp_set_breakpoint( } } - @McpTool(description = "Set a logpoint (non-stopping breakpoint) that evaluates an expression and logs the result without pausing execution. Supports an optional condition — the expression is only logged when the condition evaluates to true. By default the logpoint is registered passively: if the class is not yet loaded, it is deferred via a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to force the class load (runs ``, may mask lazy-init diagnostics).") + @McpTool(description = "Set a logpoint (non-stopping breakpoint) that evaluates an expression and logs the result without pausing execution. Supports an optional condition — the expression is only logged when the condition evaluates to true. Both expression and condition support `{ block }` syntax for multi-statement. By default the logpoint is registered passively: if the class is not yet loaded, it is deferred via a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to force the class load (runs ``, may mask lazy-init diagnostics).") public String jdwp_set_logpoint( @McpToolParam(description = "Fully qualified class name") String className, @McpToolParam(description = "Line number") int lineNumber, - @McpToolParam(description = "Java expression to evaluate and log (e.g., '\"x=\" + x', 'order.getTotal()')") String expression, - @McpToolParam(required = false, description = "Optional condition — only log when this evaluates to true (e.g., 'i > 100')") @Nullable String condition, + @McpToolParam(description = "Java expression to evaluate and log (e.g., '\"x=\" + x', 'order.getTotal()') — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition — only log when this evaluates to true (e.g., 'i > 100') — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "If true, force-load the class via Class.forName in the target VM when it isn't already loaded. Default: false (passive — defer via ClassPrepareRequest). Force-load triggers `` and can mask lazy-init / classloader-leak diagnostics — use sparingly.") @Nullable Boolean forceLoad) { // Track the pending ID outside the try so the catch can clean it up if locationsOfLine // (or any later step) throws after the pending entry has already been registered. @@ -2144,20 +2162,29 @@ public String jdwp_set_logpoint( final ReferenceType refType = recheck.get(0); final List locations = refType.locationsOfLine(lineNumber); if (!locations.isEmpty()) { - final BreakpointRequest bpRequest = erm.createBreakpointRequest(locations.get(0)); + final Location boundLocation = locations.get(0); + final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); bpRequest.enable(); if (!breakpointTracker.promotePendingToActive(pendingId, bpRequest)) { - // Another path (listener or safety-net) won the promotion race; tear - // down the loser request so no duplicate fires on the target VM. + // Another path (the ClassPrepare listener or the safety-net promoter) won + // the promotion race; tear down the loser request so no duplicate fires on + // the target VM. Do NOT emit the multi-location diagnostic here — the + // winning path owns the binding and its diagnostics. try { erm.deleteEventRequest(bpRequest); } catch (Exception ignored) { // VM may already be disconnected — best-effort cleanup. } + return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s) " + + "(bound by a concurrent activation path)", + className, lineNumber, pendingId, expression, conditionInfo); } - return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s)", - className, lineNumber, pendingId, expression, conditionInfo); + // Won the promotion — emit the multi-location diagnostic for THIS bind. Same + // rationale as jdwp_set_breakpoint. + return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s%s)", + className, lineNumber, pendingId, expression, conditionInfo, + multiLocationDiagnostic(pendingId, className, lineNumber, locations, boundLocation, "LP")); } } @@ -2172,7 +2199,8 @@ public String jdwp_set_logpoint( return String.format("Error: No executable code at line %d in class %s", lineNumber, className); } - final BreakpointRequest bpRequest = erm.createBreakpointRequest(locations.get(0)); + final Location boundLocation = locations.get(0); + final BreakpointRequest bpRequest = erm.createBreakpointRequest(boundLocation); bpRequest.setSuspendPolicy(jdiPolicy); bpRequest.enable(); @@ -2182,8 +2210,10 @@ public String jdwp_set_logpoint( breakpointTracker.setCondition(breakpointId, condition); } - return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s)", - className, lineNumber, breakpointId, expression, conditionInfo); + // Diagnostic — see jdwp_set_breakpoint for the rationale. + return String.format("Logpoint set at %s:%d (ID: %d, expression: %s%s%s)", + className, lineNumber, breakpointId, expression, conditionInfo, + multiLocationDiagnostic(breakpointId, className, lineNumber, locations, boundLocation, "LP")); } catch (AbsentInformationException e) { cleanupOrphanPendingBreakpoint(pendingIdForCleanup); return "Error: No line number information available. Compile with debug info (-g)."; @@ -2205,6 +2235,42 @@ private void cleanupOrphanPendingBreakpoint(@Nullable Integer pendingId) { } } + /** + * Emits the multi-location diagnostic for an eager (or race-guard) BP / LP bind and returns the + * WARNING suffix appended to the install response. When the source line maps to more than one + * bytecode {@link Location}, this: + *

+ * so {@code jdwp_get_events} surfaces the diagnostic uniformly whether the bind happened + * immediately or on a later ClassPrepare. Returns an empty string when there is only one + * Location, so the caller can splat the value into a format string unconditionally. {@code kind} + * is the short label ("BP" / "LP") used in the warning text. + */ + private String multiLocationDiagnostic(int id, String className, int lineNumber, + List locations, Location boundLocation, String kind) { + if (locations.size() <= 1) { + return ""; + } + final String kindLower = "LP".equals(kind) ? "logpoint" : "breakpoint"; + final String boundDesc = JdiEventListener.describeLocation(boundLocation); + log.warn("[JDI] {} {} bound at {}:{} — line has {} Locations; bound only to {} (other paths will MISS this {})", + kindLower, id, className, lineNumber, locations.size(), boundDesc, kind); + eventHistory.record(new EventHistory.DebugEvent("BP_MULTI_LOCATION", + String.format("%s #%d at %s:%d — line has %d bytecode locations; bound only to %s (other paths will MISS)", + kind, id, className, lineNumber, locations.size(), boundDesc), + Map.of("breakpointId", String.valueOf(id), + "kind", kindLower, + "class", className, + "line", String.valueOf(lineNumber), + "locationCount", String.valueOf(locations.size()), + "boundLocation", boundDesc))); + return String.format(", WARNING: line has %d Locations; bound only to %s — other paths will MISS this %s", + locations.size(), boundDesc, kind); + } + @McpTool(description = "Clear a breakpoint by its synthetic ID (from jdwp_overview). Routes by kind: line, exception, and field breakpoints share one ID space.") public String jdwp_clear_breakpoint(@McpToolParam(description = "Breakpoint ID to clear") int breakpointId) { try { @@ -2464,7 +2530,8 @@ public String jdwp_set_exception_breakpoint( "throw of the given exception type. The expression is evaluated against the throwing frame with " + "$exception bound to the thrown object (e.g., \"$exception.getMessage()\"); its result is attached " + "to the event. The optional condition is evaluated with the same $exception binding — the log is " + - "recorded only when the condition is true. Use this for tracing exception flows in long-running " + + "recorded only when the condition is true. Both expression and condition support `{ block }` syntax " + + "for multi-statement. Use this for tracing exception flows in long-running " + "services without halting traffic. By default the logpoint is registered passively: if the exception " + "class is not yet loaded the logpoint is deferred via a ClassPrepareRequest and activates on natural " + "load. Pass forceLoad=true when targeting an exception class the application hasn't referenced yet " + @@ -2472,8 +2539,8 @@ public String jdwp_set_exception_breakpoint( "running `` in the target VM.") public String jdwp_set_exception_logpoint( @McpToolParam(description = "Exception class name (e.g., 'java.sql.SQLException')") String exceptionClass, - @McpToolParam(description = "Java expression evaluated on each hit; $exception is bound to the thrown object") String expression, - @McpToolParam(required = false, description = "Optional condition with $exception bound — only log when this evaluates to true") @Nullable String condition, + @McpToolParam(description = "Java expression evaluated on each hit; $exception is bound to the thrown object — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition with $exception bound — only log when this evaluates to true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Log caught exceptions (default: true)") @Nullable Boolean caught, @McpToolParam(required = false, description = "Log uncaught exceptions (default: true)") @Nullable Boolean uncaught, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this logpoint stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -2600,15 +2667,16 @@ Exception breakpoint deferred (ID: %d) @McpTool(description = "Set a field watchpoint that suspends the firing thread when the field is read " + "(mode='access'), written (mode='modification'), or both. Conditions are evaluated against the firing " + "frame with $oldValue, $newValue (modification only), $object (null for static), $fieldName, and $mode " + - "bound. By default the BP is registered passively: if the class is not yet loaded it is deferred via " + - "a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to bind immediately and " + - "accept the side effects of running `` in the target VM. ERROR if the field is ambiguous on the " + - "class, missing, or static when objectFilterId is supplied.") + "bound; conditions support `{ block }` syntax for multi-statement. By default the BP is registered " + + "passively: if the class is not yet loaded it is deferred via a ClassPrepareRequest and activates on " + + "natural load. Pass forceLoad=true to bind immediately and accept the side effects of running " + + "`` in the target VM. ERROR if the field is ambiguous on the class, missing, or static when " + + "objectFilterId is supplied.") public String jdwp_set_field_breakpoint( @McpToolParam(description = "Fully-qualified class declaring the field (e.g., 'com.example.Order')") String className, @McpToolParam(description = "Field name (must be unambiguous on the class)") String fieldName, @McpToolParam(description = "Watch mode: 'access', 'modification', or 'both' (case-insensitive)") String mode, - @McpToolParam(required = false, description = "Optional condition with $oldValue, $newValue, $object, $fieldName, $mode bound — fire only when this evaluates to true") @Nullable String condition, + @McpToolParam(required = false, description = "Optional condition with $oldValue, $newValue, $object, $fieldName, $mode bound — fire only when this evaluates to true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional thread filter — only fire when this thread (uniqueID) hits the field") @Nullable Long threadFilterId, @McpToolParam(required = false, description = "Optional object filter — only fire on the given instance (object ID from jdwp_get_locals/jdwp_get_fields). Must be omitted for static fields.") @Nullable Long objectFilterId, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this field BP stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -2625,6 +2693,7 @@ public String jdwp_set_field_breakpoint( "access or modification of the field. The expression is evaluated against the firing frame with " + "$oldValue, $newValue (modification only), $object (null for static), $fieldName, and $mode bound; " + "its result is attached to the event. Optional condition gates whether the log is recorded. " + + "Both expression and condition support `{ block }` syntax for multi-statement. " + "Use this to trace state transitions in long-running services without halting traffic. By default the " + "logpoint is registered passively: if the class is not yet loaded it is deferred via a " + "ClassPrepareRequest and activates on natural load. Pass forceLoad=true to bind immediately and " + @@ -2633,8 +2702,8 @@ public String jdwp_set_field_logpoint( @McpToolParam(description = "Fully-qualified class declaring the field") String className, @McpToolParam(description = "Field name (must be unambiguous on the class)") String fieldName, @McpToolParam(description = "Watch mode: 'access', 'modification', or 'both' (case-insensitive)") String mode, - @McpToolParam(description = "Java expression evaluated on each hit (e.g., '$oldValue + \" -> \" + $newValue')") String expression, - @McpToolParam(required = false, description = "Optional condition with the same synthetic bindings — log only when true") @Nullable String condition, + @McpToolParam(description = "Java expression evaluated on each hit (e.g., '$oldValue + \" -> \" + $newValue') — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition with the same synthetic bindings — log only when true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional thread filter — only fire when this thread (uniqueID) hits the field") @Nullable Long threadFilterId, @McpToolParam(required = false, description = "Optional object filter — only fire on the given instance. Must be omitted for static fields.") @Nullable Long objectFilterId, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this logpoint stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -3682,11 +3751,11 @@ public String jdwp_get_current_thread() { } } - @McpTool(description = "Attach a watcher to a breakpoint to evaluate a Java expression when hit. Returns the watcher ID.") + @McpTool(description = "Attach a watcher to a breakpoint to evaluate a Java expression when hit. Returns the watcher ID. Supports `{ block }` syntax for multi-statement.") public String jdwp_attach_watcher( @McpToolParam(description = "Breakpoint request ID (from jdwp_overview)") int breakpointId, @McpToolParam(description = "Descriptive label for this watcher (e.g., 'Trace entity ID', 'Check user name')") String label, - @McpToolParam(description = "Java expression to evaluate (e.g., 'entity.id', 'user.name', 'items.size()')") String expression) { + @McpToolParam(description = "Java expression to evaluate (e.g., 'entity.id', 'user.name', 'items.size()') — supports `{ ...; return X; }` block syntax") String expression) { try { if (expression.trim().isEmpty()) { return "Error: No expression provided"; diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java index 964baf6..20393b3 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JdiEventListener.java @@ -168,6 +168,35 @@ private void applyChainEffectsAfterHit(int firingBpId, @Nullable EventRequest fi } } + /** + * Formats a JDI {@link Location} as a diagnostic string of the form + * {@code @bci=}. Used by BP-install paths to log exactly which bytecode + * Location we bound a breakpoint to — important for lambda-bearing lines where the same + * source line maps to multiple Locations (one in the enclosing method, one inside the + * synthetic {@code lambda$...$N} method) and {@code locationsOfLine(line).get(0)} only + * binds the first. + * + *

No-throw end-to-end. Both the structured form and the {@code toString()} fallback can + * raise JDI runtime exceptions (e.g. {@code VMDisconnectedException}, {@code ClassNotPreparedException}) + * during VM-tear-down races; on any failure we return a fixed placeholder so a single + * diagnostic call cannot abort the surrounding BP install path or tool response. + */ + static String describeLocation(Location location) { + try { + return location.method().name() + "@bci=" + location.codeIndex(); + } catch (Exception e) { + // method() / codeIndex() failed (e.g. pathological synthetic, VM mid-disconnect). + // Try the JDI toString() form as a softer fallback. + try { + return location.toString(); + } catch (Exception ignored) { + // Even toString() can throw on a half-torn-down Location. Return a fixed marker + // so the diagnostic emits SOMETHING and the caller keeps running. + return ""; + } + } + } + /** * Formats a JDI value for the human-readable logpoint output. Mirrors the format used by * {@link JDIConnectionService#formatFieldValue} but without its side effects (no object cache @@ -1017,12 +1046,38 @@ private void handleClassPrepareEvent(ClassPrepareEvent event) { final List> pendingFieldList = breakpointTracker.getPendingFieldBreakpointsForClass(className); + // No deferred work keyed to this class. We deliberately do NOT record a CLASS_PREPARE + // event here: EVERY loaded class fires a prepare, so recording unconditionally would + // bury the one actionable signal — "a class carrying deferred BPs/LPs just loaded" — + // under thousands of irrelevant entries. CLASS_PREPARE is recorded below only when the + // prepare actually activates pending work. if (pendingList.isEmpty() && pendingExList.isEmpty() && pendingFieldList.isEmpty()) { return; } - log.info("[JDI] ClassPrepareEvent for '{}', activating {} deferred breakpoint(s), {} deferred exception breakpoint(s), {} deferred field BP(s)", - className, pendingList.size(), pendingExList.size(), pendingFieldList.size()); + // The pending-line list mixes breakpoints and logpoints (logpoints ride the same + // deferred-line path), so split the counts — reporting everything as "line BP(s)" + // would misattribute deferred logpoints in the diagnostic. + final long lineLpCount = pendingList.stream() + .filter(e -> breakpointTracker.isLogpoint(e.getKey())) + .count(); + final long lineBpCount = pendingList.size() - lineLpCount; + + log.info("[JDI] ClassPrepareEvent for '{}', activating {} deferred line BP(s), {} deferred logpoint(s), {} deferred exception breakpoint(s), {} deferred field BP(s)", + className, lineBpCount, lineLpCount, pendingExList.size(), pendingFieldList.size()); + + // Surface the CPE to agents via jdwp_get_events. Without this, an agent that sets a + // deferred BP and waits on jdwp_resume_until_event sees only [VM_START, VM_DEATH] and + // cannot tell whether the class ever loaded — making it impossible to distinguish + // "class never loaded" from "class loaded but BP did not fire". + eventHistory.record(new EventHistory.DebugEvent("CLASS_PREPARE", + String.format("ClassPrepareEvent for %s, activating %d line BP(s), %d logpoint(s), %d exception BP(s), %d field BP(s)", + className, lineBpCount, lineLpCount, pendingExList.size(), pendingFieldList.size()), + Map.of("class", className, + "lineBpCount", String.valueOf(lineBpCount), + "lineLpCount", String.valueOf(lineLpCount), + "exceptionBpCount", String.valueOf(pendingExList.size()), + "fieldBpCount", String.valueOf(pendingFieldList.size())))); final VirtualMachine vm = event.virtualMachine(); final EventRequestManager erm = vm.eventRequestManager(); @@ -1060,7 +1115,37 @@ private void handleClassPrepareEvent(ClassPrepareEvent event) { log.debug("[JDI] Deferred breakpoint {} promotion lost the race to the opportunistic path; orphan request deleted", id); continue; } - log.info("[JDI] Deferred breakpoint {} activated at {}:{}", id, className, pending.getLineNumber()); + // Diagnostic — log how many Locations were available at this line and which one + // we bound to. A line with multiple Locations (typical for lambdas: one in the + // enclosing method, one in the synthetic `lambda$...$N`) means `.get(0)` only + // covers ONE of the code paths; if the user's code runs through the OTHER + // location, the BP silently misses. The bound-Location detail also helps + // confirm we bound where the user expected vs. an off-by-one source-map slip. + // Logpoints share this pending-line path, so distinguish the label in the log + // and event so an agent grepping for "logpoint" sees its hit. + final boolean isLogpoint = breakpointTracker.isLogpoint(id); + final String kindUpper = isLogpoint ? "LP" : "BP"; + final String kindLower = isLogpoint ? "logpoint" : "breakpoint"; + // describeLocation is best-effort and can change/throw during a VM tear-down + // race, so compute it ONCE and reuse — keeping the log, summary and details + // mutually consistent and avoiding repeated JDI round-trips. + final String boundLocationDesc = describeLocation(location); + if (locations.size() > 1) { + log.warn("[JDI] Deferred {} {} activated at {}:{} — line has {} Locations; bound only to {} (other paths will MISS this {})", + kindLower, id, className, pending.getLineNumber(), locations.size(), boundLocationDesc, kindUpper); + eventHistory.record(new EventHistory.DebugEvent("BP_MULTI_LOCATION", + String.format("%s #%d at %s:%d — line has %d bytecode locations; bound only to %s (other paths will MISS)", + kindUpper, id, className, pending.getLineNumber(), locations.size(), boundLocationDesc), + Map.of("breakpointId", String.valueOf(id), + "kind", kindLower, + "class", className, + "line", String.valueOf(pending.getLineNumber()), + "locationCount", String.valueOf(locations.size()), + "boundLocation", boundLocationDesc))); + } else { + log.info("[JDI] Deferred {} {} activated at {}:{} ({})", + kindLower, id, className, pending.getLineNumber(), boundLocationDesc); + } } catch (AbsentInformationException e) { recordPromotionFailure(id, className, pending.getLineNumber(), diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java index 1a87acb..827ac7a 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java @@ -376,10 +376,24 @@ private static String resolveLocalVarType(LocalVariable var, String wrapperPacka /** * Generates a wrapper class with a static `evaluate()` method that accepts the context - * variables as parameters and returns the result of the user expression. The user expression - * is wrapped in `(Object)(...)` so any value type — including primitives via autoboxing — - * can be returned. The bare `this` keyword in the expression is tokenizer-rewritten to `_this` - * because the wrapper class doesn't have a `this` reference (it's a static method). + * variables as parameters and returns the result of the user input. + * + *

Two input modes, picked by {@link #isBlockMode}: + *

    + *
  • Expression mode (default). The input is treated as a single Java expression and + * wrapped as {@code return (Object) ();} so any value type — including primitives + * via autoboxing — flows back as the method's return value.
  • + *
  • Block mode. The trimmed input starts with '{' and ends with the matching '}'; + * the inside is spliced into the method body verbatim, guarded by a runtime-only branch + * so that the trailing fallthrough {@code return null;} stays reachable even when the + * user body ends with an explicit {@code return X;}. The user is responsible for writing + * {@code return X;} statements to yield a value — block mode supports {@code try/catch}, + * intermediate locals, early {@code return}, and other statement-level constructs that + * the single-expression mode could not express.
  • + *
+ * + *

In both modes, the bare {@code this} keyword is tokenizer-rewritten to {@code _this} + * because the wrapper is a static method. * *

The package portion of {@code className} drives the wrapper's {@code package} declaration. * When the caller routes a non-public {@code this} type into its own package via @@ -400,17 +414,147 @@ private static String generateSourceCode(String className, EvaluationContext con // literals are NOT rewritten — see {@link #rewriteThisKeyword(String)}. final String safeExpression = rewriteThisKeyword(expression); + final String methodBody; + if (isBlockMode(safeExpression)) { + // Extract the body inside the outermost braces — already balanced because + // isBlockMode verified the structure (start with `{`, balanced end with `}`, + // string/char/text-block/comment regions skipped). + // + // The user body is wrapped in `if (__mcpFallthroughGuard) { ... }` where the guard + // is a non-final local set to true. The compiler can't treat it as a compile-time + // constant (JLS §15.29 — only `final` variables initialised with constant + // expressions qualify), so it does NOT prove the post-if `return null;` unreachable. + // That keeps the wrapper compiling whether or not the user's body ends with an + // explicit `return X;`. At runtime the guard is always true → user body always runs; + // `return null;` is the safety net for blocks that fall through without returning. + final String trimmed = safeExpression.trim(); + final String userBody = trimmed.substring(1, trimmed.length() - 1); + methodBody = + " // User block:\n" + + " boolean __mcpFallthroughGuard = true;\n" + + " if (__mcpFallthroughGuard) {\n" + + userBody + '\n' + + " }\n" + + " // Fallthrough guard — only reached when the user's block doesn't return.\n" + + " return null;\n"; + } else { + methodBody = + " // User expression:\n" + + " return (Object) (" + safeExpression + ");\n"; + } + final String packageDecl = packageName.isEmpty() ? "" : "package " + packageName + ";\n\n"; return packageDecl + "// Automatically generated class for JDI expression evaluation\n" + "public class " + simpleClassName + " {\n" + " public static Object " + EVALUATION_METHOD_NAME + '(' + methodParameters + ") {\n" + - " // User expression:\n" + - " return (Object) (" + safeExpression + ");\n" + + methodBody + " }\n" + "}\n"; } + /** + * Returns {@code true} when the user input is in block mode — i.e. its trimmed form starts + * with '{' and ends with the matching '}'. Tokenizer-aware: braces inside string / char / + * text-block literals and inside line ({@code //…}) or block ({@code /*…*}{@code /}) comments + * are ignored, so inputs like {@code "{x}".length()} (a method call on a string literal that + * happens to start with '{') stay in expression mode, and a comment containing '}' inside an + * otherwise-block input does not prematurely close the outer block. + * + *

Static + package-private so it can be unit-tested without a real {@link StackFrame}. + */ + static boolean isBlockMode(String expression) { + final String trimmed = expression.trim(); + if (trimmed.length() < 2 || trimmed.charAt(0) != '{' || trimmed.charAt(trimmed.length() - 1) != '}') { + return false; + } + // Walk the inside and confirm brace balance returns to zero at exactly the closing `}`. + // Tokenizer-aware so braces inside string / char / text-block literals and Java comments + // are ignored. + final int n = trimmed.length() - 1; + int depth = 1; + int i = 1; + while (i < n) { + final char c = trimmed.charAt(i); + if (c == '"') { + i = skipStringLiteral(trimmed, i); + continue; + } + if (c == '\'') { + i = skipCharLiteral(trimmed, i); + continue; + } + if (c == '/' && i + 1 < n) { + final char next = trimmed.charAt(i + 1); + if (next == '/') { + i = skipLineComment(trimmed, i); + // A line comment that runs to end-of-string would have swallowed the + // trailing `}` (no newline before it) — there is no real closer, so the + // input is not block-mode. + if (i > n) { + return false; + } + continue; + } + if (next == '*') { + i = skipBlockComment(trimmed, i); + // Unterminated block comment swallows the trailing `}` too. Same rationale. + if (i > n) { + return false; + } + continue; + } + } + if (c == '{') { + depth++; + } else if (c == '}') { + depth--; + if (depth == 0) { + // Hit a top-level `}` before the trailing one — input is something like + // `{stmt;} + foo` so the trailing `}` of the trimmed form is a different + // closer and block-mode does not apply. + return false; + } + } + i++; + } + // Trailing `}` at index n closes the outer brace. + return depth == 1; + } + + /** + * Returns the index just past the end of a {@code //…} line comment that starts at + * {@code start}. Terminator is the next newline; if no newline is found, returns the end of + * the string (best-effort tolerance — never throws). + */ + private static int skipLineComment(String s, int start) { + final int n = s.length(); + int i = start + 2; + while (i < n && s.charAt(i) != '\n') { + i++; + } + // Caller resumes scanning AT the newline (or end). The newline itself is not part of the + // comment, so we don't advance past it here. + return i; + } + + /** + * Returns the index just past the end of a {@code /*…*}{@code /} block comment that starts at + * {@code start}. If the closing {@code *}{@code /} is missing, returns the end of the string + * (best-effort tolerance — never throws). + */ + private static int skipBlockComment(String s, int start) { + final int n = s.length(); + int i = start + 2; + while (i + 1 < n) { + if (s.charAt(i) == '*' && s.charAt(i + 1) == '/') { + return i + 2; + } + i++; + } + return n; + } + /** * Get a name suitable to use in the generated wrapper class for the given runtime type. * Handles three cases the wrapper compiler cannot otherwise express: @@ -759,7 +903,9 @@ private static ClassLoaderReference findClassLoader(StackFrame frame) throws Jdi * In the latter case package-private and protected fields also become candidates — subject to * {@link #isFieldAccessibleFromWrapper}'s per-field accessibility check. (Explicit {@code this} * and {@code this.field} are handled separately and unconditionally by - * {@link #rewriteThisKeyword}.) + * {@link #rewriteThisKeyword}.) The bare-identifier rewrite is SKIPPED entirely in block mode + * (see {@link #isBlockMode}), where a statement body may declare locals the identifier-level + * rewriter would mistake for field references. * *

Synthetic bindings (e.g. {@code "$exception" -> ObjectReference}) appear in the wrapper's * parameter list after the real locals and can be referenced by name in the expression. This is @@ -853,6 +999,12 @@ private static ClassLoaderReference findClassLoader(StackFrame frame) throws Jdi // Auto-rewrite bare references to fields of `this` so users can write // `sessions.containsKey(session)` instead of `_this.sessions.containsKey(session)`. + // SKIPPED in block mode — the rewriter is identifier-level and cannot distinguish + // a field reference from a local-variable declaration. Rewriting a statement-body + // input like `int count = 1; ...` would corrupt the declaration into + // `int _this.count = 1; ...`. Block-mode users are expected to use explicit + // `this.field` / `_this.field` references (the keyword rewrite in `generateSourceCode` + // still handles `this.field` → `_this.field`). // Each candidate field is filtered for accessibility from the wrapper class: // - When the wrapper lives in the default isolated package, only public fields on a // public declaring type are reachable. @@ -863,7 +1015,8 @@ private static ClassLoaderReference findClassLoader(StackFrame frame) throws Jdi // package — so we'd produce a misleading "not visible" error if we rewrote it. // - Private fields are never rewritten. final ObjectReference thisObject = frame.thisObject(); - if (thisObject != null && thisObject.referenceType() instanceof ClassType thisClass) { + if (!isBlockMode(expression) + && thisObject != null && thisObject.referenceType() instanceof ClassType thisClass) { final boolean sharingPackage = !wrapperPackage.equals(DEFAULT_EVALUATION_PACKAGE) && wrapperPackage.equals(packageOf(thisClass.name())); if (thisClass.isPublic() || sharingPackage) { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java index 65ab456..88416e8 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsSetBreakpointTest.java @@ -18,7 +18,11 @@ import java.util.List; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -36,14 +40,17 @@ class JDWPToolsSetBreakpointTest { private JDWPTools tools; private VirtualMachine vm; private EventRequestManager erm; + private EventHistory eventHistory; @BeforeEach void setUp() throws Exception { jdiService = mock(JDIConnectionService.class); - tracker = new BreakpointTracker(); + // Spy (delegates to the real impl) so the race-guard tests can force promotePendingToActive + // to win/lose the promotion race deterministically; all other tests see real behaviour. + tracker = spy(new BreakpointTracker()); final WatcherManager watcherManager = new WatcherManager(); final JdiExpressionEvaluator evaluator = mock(JdiExpressionEvaluator.class); - final EventHistory eventHistory = new EventHistory(); + eventHistory = new EventHistory(); tools = JDWPToolsTestSupport.newTools( jdiService, tracker, watcherManager, evaluator, eventHistory, new EvaluationGuard(), new JvmDiscoveryService()); @@ -230,4 +237,119 @@ void shouldIncludeConditionInResponseWhenProvided() throws Exception { assertThat(result).contains("condition: i > 0"); } } + + @Nested + @DisplayName("multi-location diagnostic") + class MultiLocationDiagnostic { + + @Test + @DisplayName("eager bind to a line with >1 Location warns in the response AND records BP_MULTI_LOCATION") + void shouldWarnAndRecordEventForMultiLocationEagerBind() throws Exception { + final ReferenceType refType = mock(ReferenceType.class); + final Location bound = mock(Location.class); + final Location lambda = mock(Location.class); + // describeLocation reads method().name() + codeIndex() off the bound Location. + final com.sun.jdi.Method method = mock(com.sun.jdi.Method.class); + when(method.name()).thenReturn("doWork"); + when(bound.method()).thenReturn(method); + when(bound.codeIndex()).thenReturn(7L); + when(jdiService.findLoadedClass("com.example.WithLambda")).thenReturn(refType); + when(refType.locationsOfLine(99)).thenReturn(List.of(bound, lambda)); + when(erm.createBreakpointRequest(bound)).thenReturn(mock(BreakpointRequest.class)); + + final String result = tools.jdwp_set_breakpoint("com.example.WithLambda", 99, "all", null, null, null, null); + + assertThat(result) + .startsWith("Breakpoint set at com.example.WithLambda:99") + .contains("WARNING: line has 2 Locations") + .contains("other paths will MISS this BP"); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION"); + assertThat(e.summary()).startsWith("BP "); + assertThat(e.summary()).contains("com.example.WithLambda:99"); + assertThat(e.summary()).contains("2 bytecode locations"); + assertThat(e.details()).containsEntry("kind", "breakpoint"); + assertThat(e.details()).containsEntry("locationCount", "2"); + }); + } + + @Test + @DisplayName("single-Location bind records NO BP_MULTI_LOCATION event and adds no warning") + void shouldNotWarnOrRecordForSingleLocationBind() throws Exception { + wireEagerSet("com.example.Foo", 10); + + final String result = tools.jdwp_set_breakpoint("com.example.Foo", 10, "all", null, null, null, null); + + assertThat(result).doesNotContain("WARNING"); + assertThat(eventHistory.getRecent(10)) + .noneSatisfy(e -> assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION")); + } + } + + @Nested + @DisplayName("race-guard multi-location diagnostic") + class RaceGuardMultiLocationDiagnostic { + + /** + * Drives the race-guard recheck path: the class is NOT loaded eagerly, so a pending entry + + * ClassPrepareRequest are registered, but {@code vm.classesByName} then finds it (it loaded + * between the two checks), and the tool binds inline. + */ + private ReferenceType wireRaceGuard(String className, int line, Location... locations) throws Exception { + final ReferenceType refType = mock(ReferenceType.class); + when(jdiService.findLoadedClass(className)).thenReturn(null); + when(erm.createClassPrepareRequest()).thenReturn(mock(ClassPrepareRequest.class)); + when(vm.classesByName(className)).thenReturn(List.of(refType)); + when(refType.locationsOfLine(line)).thenReturn(List.of(locations)); + when(erm.createBreakpointRequest(locations[0])).thenReturn(mock(BreakpointRequest.class)); + return refType; + } + + @Test + @DisplayName("losing the promotion race emits NO BP_MULTI_LOCATION and notes the concurrent path") + void shouldNotRecordMultiLocationWhenPromotionLost() throws Exception { + final Location bound = mock(Location.class); + final Location lambda = mock(Location.class); + wireRaceGuard("com.example.Lam", 50, bound, lambda); + // Another path already promoted this id — our inline bind loses the race. + doReturn(false).when(tracker).promotePendingToActive(anyInt(), any()); + + final String result = tools.jdwp_set_breakpoint("com.example.Lam", 50, "all", null, null, null, null); + + assertThat(result) + .startsWith("Breakpoint set at com.example.Lam:50") + .contains("bound by a concurrent activation path") + .doesNotContain("WARNING"); + // The winning path owns the diagnostic; we must not double-record it. + assertThat(eventHistory.getRecent(10)) + .noneSatisfy(e -> assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION")); + } + + @Test + @DisplayName("winning the promotion race records BP_MULTI_LOCATION and warns") + void shouldRecordMultiLocationWhenPromotionWon() throws Exception { + final Location bound = mock(Location.class); + final Location lambda = mock(Location.class); + final com.sun.jdi.Method method = mock(com.sun.jdi.Method.class); + when(method.name()).thenReturn("doWork"); + when(bound.method()).thenReturn(method); + when(bound.codeIndex()).thenReturn(3L); + wireRaceGuard("com.example.Lam", 50, bound, lambda); + doReturn(true).when(tracker).promotePendingToActive(anyInt(), any()); + + final String result = tools.jdwp_set_breakpoint("com.example.Lam", 50, "all", null, null, null, null); + + assertThat(result) + .startsWith("Breakpoint set at com.example.Lam:50") + .contains("WARNING: line has 2 Locations") + .doesNotContain("concurrent activation path"); + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION"); + assertThat(e.details()).containsEntry("locationCount", "2"); + }); + } + } } diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java index d37e105..8d4b24d 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JdiEventListenerClassPrepareTest.java @@ -323,6 +323,152 @@ void shouldRecordFailureWhenCreateExceptionRequestThrowsGenericException() throw assertThat(pending.getFailureReason()).contains("ERM rejected the exception request"); } + // ── Diagnostic event emissions (CLASS_PREPARE, BP_MULTI_LOCATION) ──────── + + @Test + @DisplayName("CLASS_PREPARE event recorded in EventHistory when a CPE activates pending items") + void shouldRecordClassPrepareEventInHistory() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + Location loc = mock(Location.class); + when(refType.name()).thenReturn("com.example.Foo"); + when(refType.locationsOfLine(42)).thenReturn(List.of(loc)); + + BreakpointRequest createdBp = mock(BreakpointRequest.class); + EventRequestManager erm = mock(EventRequestManager.class); + when(erm.createBreakpointRequest(loc)).thenReturn(createdBp); + + tracker.registerPendingBreakpoint("com.example.Foo", 42, 2, "ALL"); + + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("CLASS_PREPARE"); + assertThat(e.summary()).contains("com.example.Foo"); + assertThat(e.summary()).contains("1 line BP"); + }); + } + + @Test + @DisplayName("CLASS_PREPARE NOT recorded when the CPE has no pending items for the class") + void shouldNotRecordClassPrepareWhenNothingPending() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + when(refType.name()).thenReturn("com.example.Unrelated"); + EventRequestManager erm = mock(EventRequestManager.class); + + // No pending entries for "com.example.Unrelated". + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + // The handler short-circuits at the "all pending lists empty" check before recording. + assertThat(eventHistory.getRecent(10)) + .noneSatisfy(e -> assertThat(e.type()).isEqualTo("CLASS_PREPARE")); + } + + @Test + @DisplayName("BP_MULTI_LOCATION recorded when locationsOfLine returns more than one Location (lambda case)") + void shouldRecordMultiLocationEventForLambdaLine() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + Location enclosingLoc = mock(Location.class); + Location lambdaLoc = mock(Location.class); + // Mock describeLocation's path so the summary is readable. The exact contents aren't + // what the test asserts on, but a sane shape helps diagnosis if the assertion fails. + com.sun.jdi.Method enclosingMethod = mock(com.sun.jdi.Method.class); + when(enclosingMethod.name()).thenReturn("doWork"); + when(enclosingLoc.method()).thenReturn(enclosingMethod); + when(enclosingLoc.codeIndex()).thenReturn(12L); + + when(refType.name()).thenReturn("com.example.WithLambda"); + when(refType.locationsOfLine(99)).thenReturn(List.of(enclosingLoc, lambdaLoc)); + + BreakpointRequest createdBp = mock(BreakpointRequest.class); + EventRequestManager erm = mock(EventRequestManager.class); + when(erm.createBreakpointRequest(enclosingLoc)).thenReturn(createdBp); + + int pendingId = tracker.registerPendingBreakpoint("com.example.WithLambda", 99, 2, "ALL"); + + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION"); + assertThat(e.summary()).contains("#" + pendingId); + assertThat(e.summary()).contains("com.example.WithLambda:99"); + assertThat(e.summary()).contains("2 bytecode locations"); + assertThat(e.summary()).contains("other paths will MISS"); + // Per the new logpoint-vs-breakpoint distinction, this is a plain BP. + assertThat(e.summary()).startsWith("BP "); + }); + } + + @Test + @DisplayName("BP_MULTI_LOCATION label is `LP` when the activated pending entry is a logpoint") + void shouldLabelMultiLocationAsLogpointWhenLogpointMetadataPresent() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + Location enclosingLoc = mock(Location.class); + Location lambdaLoc = mock(Location.class); + com.sun.jdi.Method enclosingMethod = mock(com.sun.jdi.Method.class); + when(enclosingMethod.name()).thenReturn("doWork"); + when(enclosingLoc.method()).thenReturn(enclosingMethod); + when(enclosingLoc.codeIndex()).thenReturn(12L); + + when(refType.name()).thenReturn("com.example.WithLambda"); + when(refType.locationsOfLine(99)).thenReturn(List.of(enclosingLoc, lambdaLoc)); + + BreakpointRequest createdBp = mock(BreakpointRequest.class); + EventRequestManager erm = mock(EventRequestManager.class); + when(erm.createBreakpointRequest(enclosingLoc)).thenReturn(createdBp); + + int pendingId = tracker.registerPendingBreakpoint("com.example.WithLambda", 99, 2, "ALL"); + // Mark this pending entry as a logpoint by attaching an expression. + tracker.setLogpointExpression(pendingId, "x + 1"); + + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("BP_MULTI_LOCATION"); + // The diagnostic should use "LP" rather than "BP" because this entry is a logpoint. + assertThat(e.summary()).startsWith("LP "); + }); + } + + @Test + @DisplayName("CLASS_PREPARE summary splits the line-pending count into BP(s) vs logpoint(s)") + void shouldSplitLineBpAndLogpointCountsInClassPrepareSummary() throws Exception { + ReferenceType refType = mock(ReferenceType.class); + Location bpLoc = mock(Location.class); + Location lpLoc = mock(Location.class); + when(refType.name()).thenReturn("com.example.Mixed"); + when(refType.locationsOfLine(10)).thenReturn(List.of(bpLoc)); + when(refType.locationsOfLine(20)).thenReturn(List.of(lpLoc)); + + EventRequestManager erm = mock(EventRequestManager.class); + when(erm.createBreakpointRequest(bpLoc)).thenReturn(mock(BreakpointRequest.class)); + when(erm.createBreakpointRequest(lpLoc)).thenReturn(mock(BreakpointRequest.class)); + + // One plain breakpoint and one logpoint deferred against the same class — both ride the + // pending-line path, so the summary must attribute them to separate buckets. + tracker.registerPendingBreakpoint("com.example.Mixed", 10, 2, "ALL"); + int lpId = tracker.registerPendingBreakpoint("com.example.Mixed", 20, 2, "ALL"); + tracker.setLogpointExpression(lpId, "x + 1"); + + ClassPrepareEvent event = mockClassPrepareEvent(refType, erm); + runListenerWith(listener, mockEventSet(event)); + + assertThat(eventHistory.getRecent(10)) + .anySatisfy(e -> { + assertThat(e.type()).isEqualTo("CLASS_PREPARE"); + assertThat(e.summary()).contains("1 line BP(s)"); + assertThat(e.summary()).contains("1 logpoint(s)"); + assertThat(e.details()).containsEntry("lineBpCount", "1"); + assertThat(e.details()).containsEntry("lineLpCount", "1"); + }); + } + // ── Test-specific event factory ────────────────────────────────────────── private static ClassPrepareEvent mockClassPrepareEvent(ReferenceType refType, EventRequestManager erm) { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java index 7dad664..348fbe7 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java @@ -383,4 +383,100 @@ void shouldTolerateUnterminatedStringInThisKeyword() { // The bare `this` before the literal should still be rewritten assertThat(result).isEqualTo("_this + \"unterminated"); } + + // ── isBlockMode: detect `{ ... }` block input vs expression input ──────────────────── + + @Test + @DisplayName("isBlockMode: plain expression is NOT a block") + void shouldNotDetectBlockForPlainExpression() { + assertThat(JdiExpressionEvaluator.isBlockMode("foo.bar() + baz")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: `{ stmt; }` IS a block") + void shouldDetectSingleStatementBlock() { + assertThat(JdiExpressionEvaluator.isBlockMode("{ return x; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: leading/trailing whitespace is allowed") + void shouldDetectBlockWithSurroundingWhitespace() { + assertThat(JdiExpressionEvaluator.isBlockMode(" \n { return x; }\n ")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: nested braces are matched correctly") + void shouldDetectBlockWithNestedBraces() { + assertThat(JdiExpressionEvaluator.isBlockMode( + "{ if (x > 0) { return x; } else { return 0; } }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: braces inside a string literal do NOT terminate the outer block") + void shouldNotConfuseBracesInsideStringLiteral() { + // The `}` inside the string would mislead a naive matcher; the tokenizer must skip it. + assertThat(JdiExpressionEvaluator.isBlockMode("{ return \"}\"; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `{x}.foo()` is NOT a block (trailing `}` is not the outer closer)") + void shouldNotDetectBlockWhenClosingBraceIsMidExpression() { + // `{x}` opens-and-closes before `.foo()`, so the trailing closer of the trimmed input + // is `)` — block mode should not apply. + assertThat(JdiExpressionEvaluator.isBlockMode("{x}.foo()")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: brace-balanced internals followed by expression text are NOT a block") + void shouldNotDetectBlockWhenInternalBraceClosesEarly() { + // `{a;}+b;}` — input both starts and ends with a brace (so it clears the cheap + // first/last-char guard), but the FIRST `}` at i=3 drops depth to 0 before the trailing + // `}` at i=7. That trailing brace is therefore a separate closer, not the match for the + // opening one, so this exercises the early `depth == 0` rejection branch — not the + // end-char guard. Block detection must reject it. + assertThat(JdiExpressionEvaluator.isBlockMode("{a;}+b;}")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: an empty input is NOT a block") + void shouldNotDetectBlockForEmptyInput() { + assertThat(JdiExpressionEvaluator.isBlockMode("")).isFalse(); + assertThat(JdiExpressionEvaluator.isBlockMode(" ")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: a lone `{}` is a (trivial, empty) block") + void shouldDetectEmptyBlock() { + assertThat(JdiExpressionEvaluator.isBlockMode("{}")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `}` inside a `/* ... */` block comment does NOT close the outer block") + void shouldNotConfuseBracesInsideBlockComment() { + // Without comment-skip, the `}` inside the comment would drop depth to 0 and the + // outer block would be rejected as non-block input. + assertThat(JdiExpressionEvaluator.isBlockMode("{ /* close: } */ return 1; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `}` inside a `//` line comment does NOT close the outer block") + void shouldNotConfuseBracesInsideLineComment() { + assertThat(JdiExpressionEvaluator.isBlockMode("{ // }\n return 1; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `/* nested } */` followed by real `}` still balances") + void shouldHandleBlockCommentWithBraceAtVariousDepths() { + // Nested braces in code, plus a `}` hidden inside a comment, plus the real closer. + assertThat(JdiExpressionEvaluator.isBlockMode( + "{ if (x) { /* not a closer: } */ return 1; } return 0; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: unterminated block comment is tolerated, treats remainder as comment") + void shouldTolerateUnterminatedBlockComment() { + // `/* …` without a closing `*/` swallows the rest of the input including the trailing `}`. + // The outer brace then never closes → not block-mode. Must not throw. + assertThat(JdiExpressionEvaluator.isBlockMode("{ /* unterminated }")).isFalse(); + } }